Open Bug 655947 Opened 13 years ago Updated 2 years ago

nsContentUtils::ComparePoints assumes that anonymous content bound to a DOM node always precedes any of the node children

Categories

(Core :: DOM: Core & HTML, defect)

defect

Tracking

()

People

(Reporter: exclusion, Unassigned)

Details

Attachments

(2 files, 4 obsolete files)

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0.1) Gecko/20100101 Firefox/4.0.1

Relation between the node children and anonymous content is undefined and such compare operation should be considered as erroneous (and return value should be 0, but not -1 or 1).

(It's impossible to set up Range (nsIDOMRange) with a DOM node, when end of the range is within the anonymous content bound to it. Such a range would collapse cause ComparePoints always returns -1 when comparing any of the node children with the anonymous content.)

Reproducible: Always

Steps to Reproduce:
1. Try to set up DOMRange, when mStartNode is some DOM node, mStartOffset is 0, and mEndNode is within anonymous content that is bound to the node, mEndOffset is any:
  nsCOMPtr<nsIDOMRange> range = nsFind::CreateRange();
  range->SetStart(mStartNode, mStartOffset);
  range->SetEnd(mEndNode, mEndOffset);

Actual Results:  
Range collapses

Expected Results:  
Range should be not empty (as it would be when mStartNode is a parent of the node anonymous content is bound to).

Bug 263049 is a consequence of the issue.
What is the practical consequences of this? Note that we don't expose anonymous content to the web any more since we disabled XBL.
(In reply to comment #2)
> What is the practical consequences of this? Note that we don't expose
> anonymous content to the web any more since we disabled XBL.
I discovered this issue when investigating bug with Find Backward within XML content (Bug 263049).
And I found following explanation of it:

Find backward doesn't work because visible XML content generated as anonymous content and it's binded to root element of the document. And nsTypeAheadFind::GetSearchContainers() chooses root element, child 0 as the start of the search range, and some node inside anonymous content (current selection) as the end of the range. Then nsFindContentIterator::Reset is invoked, which in turn tries to set up the mOuterIterator range. But the range collapses (after range->SetEnd(mEndNode, mEndOffset)), because nsContentUtils::ComparePoints (content/base/src/nsContentUtils.cpp) assumes that anonymous content always precedes any of the node children. (Due to IndexOf for anonymous content returns -1 (so the content assumed as placed before the first child of the node)).

So proposed patch in fact fixes Bug 263049. (Search Backward starts to work just as Search Forward).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 531251 [details] [diff] [review]
Patched ComparePoints will not compare child indexes when one of the compared nodes is not a child

Jonas, this passes try.  Want to review?  Any obvious gotchas here?
Attachment #531251 - Flags: review?(jonas)
Shouldn't we treat these nodes as being disconnected? I.e. set *aDisconnected to true and return 1? At least the former seems like something we should do.

Andrew: Does searching backwards work if you set *aDisconnected to true and return 1?
Sorry! Meant Alexander, not Andrew.
Unfortunately, it wouldn't work. nsRange::SetEnd doesn't use or check aDisconnected parameter:

  // Collapse if not positioned yet, if positioned in another doc or
  // if the new end is before start.
  if (!mIsPositioned || newRoot != mRoot ||
      nsContentUtils::ComparePoints(mStartParent, mStartOffset,
                                    aParent, aOffset) == 1) {
    DoSetRange(aParent, aOffset, aParent, aOffset, newRoot);

    return NS_OK;
  }

So it would collapse the range anyway.
In my opinion, this case should not be treated as disconnected subtrees. For example, when start of the range set to HTML root and end of the range set inside of input or textarea content (presented as anonymous content), ComparePoints, SetRange (and Find) works just fine (start of the document precedes that input and it's anonymous content too). The problem arises only when the start of the range set to the node anonymous content is bound to.
I think that considering such nodes (regular and anonymous content) as disconnected should not depend on whether the regular node is the holder of anonymous content or it is it's parent or some preceding node. So it seems that most appropriate solution is to return 0 as error status or at least as not a statement that such nodes preceding one another.
Comment on attachment 531251 [details] [diff] [review]
Patched ComparePoints will not compare child indexes when one of the compared nodes is not a child

Review of attachment 531251 [details] [diff] [review]:
-----------------------------------------------------------------

I'll defer to Olli on this one.
Attachment #531251 - Flags: review?(jonas) → review?(bugs)
Um, the patch is ancient. Is it still valid?
Flags: needinfo?(bzbarsky)
I think so. Cause the bug https://bugzilla.mozilla.org/show_bug.cgi?id=263049 still not fixed.
I would expect the patch is still valid; might need to be merged to tip to make sure.
Flags: needinfo?(bzbarsky)
So the behavior of nsRange::CompareNodeToRange changes too. But that looks ok.

I'm more worried about nsIFrame::IsFrameSelected()
Comment on attachment 531251 [details] [diff] [review]
Patched ComparePoints will not compare child indexes when one of the compared nodes is not a child

Mats, can you think of problems this could cause to
range/selection?
Attachment #531251 - Flags: review?(matspal)
Attachment #531251 - Flags: review?(bugs)
Attachment #531251 - Flags: review+
Comment on attachment 531251 [details] [diff] [review]
Patched ComparePoints will not compare child indexes when one of the compared nodes is not a child

I don't see any obvious problems for range/selection code,
r=mats if it still fixes the problem and pass regression tests.

A couple of nits:
Please use single-assignment, i.e.
  const int32_t index1 = parent->IndexOf(child1);
etc

Wrap the new if-conditions in MOZ_UNLIKELY.
Attachment #531251 - Flags: review?(matspal) → review+
Alexander, are you willing to update the patch?  Have you run tests on it, or should I push it to try?
Flags: needinfo?(exclusion)
When I made the patch, I ran the tests and found no problems.
I have no building environment for the Firefox just now. It will take some time to setup it. If the issue can wait, I will update the patch and retest it within a day or two.
Flags: needinfo?(exclusion)
That would be great.  I doubt this has suddenly gotten life-or-death urgent just because Jonas finally stopped being reviewer of record... ;)
Assignee: nobody → exclusion
Attached patch Optimized patch (obsolete) — Splinter Review
Here comes another version. It still fixes the bug.
Please push to try it, as I have no hg account.
Thank you.
Attachment #531251 - Attachment is obsolete: true
There is a M4 failure across platforms:
11:27:48 INFO - 16082 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/forms/test/test_textarea_resize.html | Assertion count 1 is greater than expected range 0-0 assertions.

I've started a new run using an up-to-date m-c tree in case I got a bad parent
cset in the first run.
https://tbpl.mozilla.org/?tree=Try&rev=2d5deee758fb
Attached patch debug patchSplinter Review
The M4 failure is a real regression.  The assertion is unrelated but it
occurred because layout/forms/test/test_textarea_resize.html started
a drag operation where previously it didn't.

I've done some debugging, using this patch I get the following output
in a build that works:

 0:07.95 start: 0 / Text@0x2b2540dc2e00 flags=[0000008a] primaryframe=0x2b25411b1728 refcount=4<Text>
 0:07.95 end: 1 / div@0x2b2540dc2b00 class="anonymous-div wrap" state=[40000010005] flags=[0020019e] ranges:1 primaryframe=0x2b2541125c10 refcount=34<
 0:07.95 Text@0x2b2540dc2e00 flags=[0000008a] primaryframe=0x2b25411b1728 refcount=3<Text>
 0:07.95 br@0x2b25411b4000 _moz_dirty="" type="_moz" state=[40000020000] flags=[00200088] primaryframe=0x2b2541127cc0 refcount=2<>
 0:07.95 >
 0:07.95 isChromeShell=0
 0:07.95 DragDataProducer::GetDraggableSelectionData
 0:07.95 isCollapsed=0
 0:07.95 inRealTargetNode: div@0x2b2540dc2b00 class="anonymous-div wrap" state=[40000010005] flags=[0020019e] ranges:1 primaryframe=0x2b2541125c10 refcount=35<
 0:07.95 Text@0x2b2540dc2e00 flags=[0000008a] primaryframe=0x2b25411b1728 refcount=3<Text>
 0:07.95 br@0x2b25411b4000 _moz_dirty="" type="_moz" state=[40000020000] flags=[00200088] primaryframe=0x2b2541127cc0 refcount=2<>
 0:07.95 >
 0:07.95 selectionContainsTarget=0
 0:07.95 haveSelectedContent=0
 0:07.95 DetermineDragTarget: rv=0x0 canDrag=1 *aSelection=(nil)


And in a build that fails:

 0:07.90 start: 0 / Text@0x2af41db0b200 flags=[0000008a] primaryframe=0x2af4202bb728 refcount=4<Text>
 0:07.90 end: 1 / div@0x2af41db0a300 class="anonymous-div wrap" state=[40000010005] flags=[0020019e] ranges:1 primaryframe=0x2af420222c10 refcount=34<
 0:07.90 Text@0x2af41db0b200 flags=[0000008a] primaryframe=0x2af4202bb728 refcount=3<Text>
 0:07.90 br@0x2af41f0b9e80 _moz_dirty="" type="_moz" state=[40000020000] flags=[00200088] primaryframe=0x2af420224cc0 refcount=2<>
 0:07.90 >
 0:07.90 isChromeShell=0
 0:07.90 DragDataProducer::GetDraggableSelectionData
 0:07.90 isCollapsed=0
 0:07.90 inRealTargetNode: div@0x2af41db0a300 class="anonymous-div wrap" state=[40000010005] flags=[0020019e] ranges:1 primaryframe=0x2af420222c10 refcount=35<
 0:07.90 Text@0x2af41db0b200 flags=[0000008a] primaryframe=0x2af4202bb728 refcount=3<Text>
 0:07.90 br@0x2af41f0b9e80 _moz_dirty="" type="_moz" state=[40000020000] flags=[00200088] primaryframe=0x2af420224cc0 refcount=2<>
 0:07.90 >
 0:07.90 selectionContainsTarget=1
 0:07.90 haveSelectedContent=1
 0:07.90 DetermineDragTarget: rv=0x0 canDrag=1 *aSelection=0x2af420706240
 0:07.91 ###!!! ASSERTION: Wrong bounds: 'bounds.IsEqualInterior(aChildren.GetBounds(aBuilder))', file /home/mats/moz/mc/layout/base/FrameLayerBuilder.cpp, line 2978


The difference is in that nsContentAreaDragDrop::GetDragData gets a non-null
selection.  This is because "inSelection->ContainsNode()":
http://hg.mozilla.org/mozilla-central/annotate/631d57b31bb1/content/base/src/nsContentAreaDragDrop.cpp#l825
now return selectionContainsTarget=true, when it should be false.

IOW, a selection range where the start node is the parent of the end node,
and the end node is anonymous, selection.containsNode(parent,false) returns
true when it should return false.
It's easy to reproduce the bug without running mochitest:
1. load data:text/html,<textarea>
2. type some text into the textarea, then CTRL+A to select it
3. press the left mouse-button on the textarea background away from the text and
   drag it
=> this starts a drag operation of the selected text
Comment on attachment 728715 [details] [diff] [review]
Optimized patch

r- because of the bug described above.
Maybe it can be fixed by tweaking Selection::ContainsNode() ?
Attachment #728715 - Flags: review-
Attached patch Modified patch (obsolete) — Splinter Review
I think the problem is in the nsRange::CompareNodeToRange. Anonymous content should not be considered as contained in a range. So both outNodeBefore and outNodeAfter should be false.
Please push to try this version.
Attachment #728715 - Attachment is obsolete: true
(In reply to Alexander LAW from comment #24)

> I think the problem is in the nsRange::CompareNodeToRange. Anonymous content
> should not be considered as contained in a range. So both outNodeBefore and
> outNodeAfter should be false.

Sorry, I meant "both outNodeBefore and outNodeAfter should be true (not false)".
> Anonymous content should not be considered as contained in a range.

That doesn't seem right to me.

>   // is RANGE(start) <= NODE(start) ?
>   bool disconnected = false;
>-  *outNodeBefore = nsContentUtils::ComparePoints(rangeStartParent,
>+  if (MOZ_UNLIKELY(rangeStartParent == parent && rangeStartOffset == nodeStart)) {
>+    *outNodeBefore = false;

The code you added contradicts the comment and what this function is supposed
to do.  It seems to me the above applies to all content not just anonymous.

Compare nsContentUtils::ComparePoints for the case RANGE(start) == NODE(start):
http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentUtils.cpp#2019
which would return zero on line 2024, and set *outNodeBefore == true.

Pushed to Try:
https://tbpl.mozilla.org/?tree=Try&rev=3281ab480318
(In reply to Mats Palmgren [:mats] from comment #26)
> > Anonymous content should not be considered as contained in a range.
> 
> That doesn't seem right to me.
> 
> >   // is RANGE(start) <= NODE(start) ?
> >   bool disconnected = false;
> >-  *outNodeBefore = nsContentUtils::ComparePoints(rangeStartParent,
> >+  if (MOZ_UNLIKELY(rangeStartParent == parent && rangeStartOffset == nodeStart)) {
> >+    *outNodeBefore = false;
> 
> The code you added contradicts the comment and what this function is supposed
> to do.  It seems to me the above applies to all content not just anonymous.
Please look at the original code http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsRange.cpp#119
Here we have the same contradiction or not so clear wording. Top comment for the method says:
// If neither are true, the node is contained inside of the range.
So outNodeBefore should be false if (RANGE(start) <= NODE(start)). Maybe it's worth to change the comment but the logic is the same.

> Pushed to Try:
> https://tbpl.mozilla.org/?tree=Try&rev=3281ab480318
It seems that there are no other issues.
Are there any obstacles to get the latest patch applied? Should I edit the comments to make it more clear?
Flags: needinfo?(matspal)
(In reply to Alexander LAW from comment #27)
> Top comment for the method says:
> // If neither are true, the node is contained inside of the range.

Yeah, I think the comment at the top of nsRange::CompareNodeToRange
is correct.  The later comments are quite confusing since what the
respective ComparePoints() calls do is actually the opposite of what
the comment say.  Perhaps the comment is just trying to convey
which part of the condition in the top comment is tested?

Anyway, I agree they are confusing and not very helpful so we should
just remove them.

> It seems that there are no other issues.

I believe that's more a sign of our extremely poor test coverage of
anything involving anonymous content. ;-)

One remaining (minor) problem:
When I "Find in Page" in anon content, and click "Previous" until
it reaches the top, I then get for each click:
###!!! ASSERTION: Null current node in an iterator that's not done!: 'mCurNode',
file content/base/src/nsContentIterator.cpp, line 1069

Could you look into that and see how serious it looks?

Also, the arguments to ComparePoints() needs indentation.
Flags: needinfo?(matspal)
Comment on attachment 729523 [details] [diff] [review]
Modified patch

I don't think these changes are particularly "correct" but then again
neither is the original code for anonymous content, so I guess I can
live with this if it seems to work...

Olli, do you still think the patch looks OK with the additional
change in nsRange::CompareNodeToRange ?
Attachment #729523 - Flags: feedback?(bugs)
Comment on attachment 729523 [details] [diff] [review]
Modified patch

Ehsan, does Editor have any dependencies on nsRange::CompareNodeToRange
or nsContentUtils::ComparePoints for anonymous content that you know of?
Attachment #729523 - Flags: feedback?(ehsan)
(In reply to Mats Palmgren [:mats] from comment #31)
> Comment on attachment 729523 [details] [diff] [review]
> Modified patch
> 
> Ehsan, does Editor have any dependencies on nsRange::CompareNodeToRange
> or nsContentUtils::ComparePoints for anonymous content that you know of?

Not that I know of.  In the editor land, all point comparisons should either be between two points in the same anonymous subtree (for input/textarea) or between two points in the non-anonymous tree (for the html editor).  We have had bugs where the HTML editor would get confused and start looking into the anonymous subtree of an element, but most of those should have been fixed (bug 805668 is the only open one that I know of.)
Attachment #729523 - Flags: feedback?(bugs) → feedback+
Comment on attachment 729523 [details] [diff] [review]
Modified patch

Review of attachment 729523 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, missed the feedback? (but please do not consider this an r+!)
Attachment #729523 - Flags: feedback?(ehsan) → feedback+
Attached patch Corrected patch (obsolete) — Splinter Review
Ambiguous comments removed, indentation fixed.
Attachment #729523 - Attachment is obsolete: true
(In reply to Mats Palmgren [:mats] from comment #29)
> One remaining (minor) problem:
> When I "Find in Page" in anon content, and click "Previous" until
> it reaches the top, I then get for each click:
> ###!!! ASSERTION: Null current node in an iterator that's not done!:
> 'mCurNode',
> file content/base/src/nsContentIterator.cpp, line 1069
> 
> Could you look into that and see how serious it looks?
It looks like the problem is not related to the bug fix. It's just consequence of working find previous.
The root of the problem is in the nsContentIterator::GetPrevSibling. Here for XML content we have:
xml root is the parent of xml:div (defined at content/xml/document/resources/XMLPrettyPrint.xml), but xml:div is not a child of the xml root.
So (nsIContent* sib = parent->GetChildAt(indx)) == null and (indx = parent->IndexOf(aNode)) == -1.
I think it should be considered as another bug.
Attachment #738059 - Flags: feedback?(ehsan)
Comment on attachment 738059 [details] [diff] [review]
Corrected patch

Review of attachment 738059 [details] [diff] [review]:
-----------------------------------------------------------------

I already provided feedback on this patch.  Over to smaug for review, assuming this is ready for review.
Attachment #738059 - Flags: feedback?(ehsan) → review?(bugs)
Comment on attachment 738059 [details] [diff] [review]
Corrected patch

The nsRange code sure needs some comments
Attachment #738059 - Flags: review?(bugs)
Attachment #738059 - Attachment is obsolete: true
Attachment #8444956 - Flags: review?(bugs)
Comment on attachment 8444956 [details] [diff] [review]
Corrected patch with comments

I wonder... should we add another param to nsContentUtils::ComparePoints.
Since if setting aDisconnected doesn't really work, we need something 
to fix at least nsRange::ComparePoint and nsRange::CompareBoundaryPoints.

Do we have other interesting nsContentUtils::ComparePoints callers?
Need to go through all of them.
Attachment #8444956 - Flags: review?(bugs)

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: exclusion → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: