409380 - [REGRESSION] range.selectNodeContents throws an INVALID_NODE_TYPE_ERR on elements created with document.createElement

Status: RESOLVED FIXED

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

Description

[Tobie Langel]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en) AppleWebKit/419.3 (KHTML, like Gecko) Safari/419.3
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2) Gecko/2007121014 Firefox/3.0b2

The following JS:

var element, range;
element = document.createElement('div');
range = element.ownerDocument.createRange();
range.selectNodeContents(element);
thows:

NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR: The container of an boundary-point of a range is being set to either a node of an invalid type or a node with an ancestor of an invalid type.([Exception... "The container of an boundary-point of a range is being set to either a node of an invalid type or a node with an ancestor of an invalid type." code: "2" nsresult: "0x805c0002 (NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR)"

at:

range.selectNodeContents(element);


Reproducible: Always

Steps to Reproduce:
1. run the above mentioned JS

Actual Results:  
throws an NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR error. 

Expected Results:  
This shouldn't throw an error. It's a regression issue in FF3b2.

Comment 1

[John Resig]

Attachment: Test Case

I've confirmed this issue - and it's a serious regression, it prevents all forms of DOM insertion from working in the popular Prototype JavaScript Library.

Additionally, the following test case also fails:

var element, range;
element = document.createElement('div');
range = document.createRange();
range.selectNodeContents(element);

I've attached a test case of the original submission to verify against.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Also Opera throws an exception when an element, which is not in a document, is 
tried to be used in a range. (This was changed in bug 336381)
For some reason bug 358106 changed the exception type from 
NS_ERROR_DOM_RANGE_BAD_BOUNDARYPOINTS_ERR to NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

And creating range using element which isn't in document or document fragment
isn't allowed per DOM 2 Range recommendation:
"The boundary-points of a Range must have a common ancestor container which is either a Document, DocumentFragment or Attr node."

Comment 4

[Tobie Langel]

"The boundary-points of a Range must have a common ancestor container which is
either a Document, DocumentFragment or Attr node."

that's the case here.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

What you mean?
If you create an element using document.createElement, the created element
doesn't have any ancestors before it is somehow attached to dom
(for example using insertBefore or appendChild).

Or do you just mean that "that's the case here, that is why this doesn't work anymore. FF3 implements the W3C recommendation more strictly and correctly than FF2." :)

Comment 6

[Tobie Langel]

The definition of common ancestor container is rather vague in the specs. I initially read it as meaning that it needed to share the same document - but I think you might be right and in saying that it must actually be appended to the document. 

Comment 7

[Tobie Langel]

The following (from http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Creating) seems to imply that sharing the same ownerDocument is enough:

"Like some objects created using methods in the Document interface (such as Nodes and DocumentFragments), Ranges created via a particular document instance can select only content associated with that Document, or with DocumentFragments and Attrs for which that Document is the ownerDocument."

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Well, this is quite clear
"the content of a Range must be entirely within the subtree rooted by a single Document, DocumentFragment or Attr Node."

Comment 9

[Tobie Langel]

The only thing that seems clear to me is that that setting ranges on newly created nodes wasn't thought about when the specs were designed:

- there's no relevant error for that particular case (BAD_BOUNDARYPOINTS_ERR doesn't seem appropriate, INVALID_NODE_TYPE_ERR is plain wrong), 

- as pointed above, there are some blatant contradictions in the specs themselves (is sharing the same ownerDocument sufficient, or must the selected node be appended to it?).

Furthermore, API-wise, I don't see any reason why a newly created node couldn't be targeted by document.selectNodeContents.

Previous implementations of FF understood the specs in the broad sense (i.e. sharing ownerDocument is enough). WebKit still does.

Restricting the scope of range.selectNodeContent to appended nodes only is a serious regression issue and will notably break websites using any current version of Prototype.

Given the lack of clarity of the specs in that area, and given the obvious use cases for selecting unappended nodes (I'm not going to pull the "don't break the web" argument here ;) ) it would seem unreasonable imho to dismiss this bug as being due to non spec-conforming code.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

To me it is very clear what the spec says but because of backward compatibility
supporting the old behavior is one option.

(If there is an exception, it should be BAD_BOUNDARYPOINTS_ERR.)

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #10)
> (If there is an exception, it should be BAD_BOUNDARYPOINTS_ERR.)
> 
Actually, INVALID_NODE_TYPE_ERR is possible too.

Comment 12

[Peter Van der Beken [:peterv]]

(In reply to comment #9)
> - as pointed above, there are some blatant contradictions in the specs
> themselves (is sharing the same ownerDocument sufficient, or must the selected
> node be appended to it?).

There's really no discussion about this, the spec is clear that the "boundary-points of a Range must have a common ancestor container which is either a Document, DocumentFragment or Attr node." A node that is not appended to anything does not have an ancestor.

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Agreed. Can we simply mark this as INVALID, or are we throwing the wrong exception?

Comment 14

[Tobie Langel]

(In reply to comment #13)
> Agreed. Can we simply mark this as INVALID, or are we throwing the wrong
> exception?
> 

Do you realize that making selectNodeContents throw an error is a regression and will break every website using Prototype ?

Comment 15

[John Resig]

Smaug, Jonas: Is there tangible benefits to leaving the code this way (throwing an exception when an element is outside of the document)? Or was this change made to become more spec compliant?

On one hand: If the change was strictly for compliance, then we should fight to keep more web sites working.

On the other hand: If this was done with real performance/operational benefits (and citing the fact that Opera already does this) then we might have to swallow the bullet.

Andrew, Tobie: Does the Prototype test suite not test for regressions to the DOM insertion code? Nothing was triggered by this change to selectNodeContents so that leads me to believe that it isn't in there (or, at least, our version of the suite isn't current).

Comment 16

[Mike Shaver (:shaver emeritus)]

What would be broken in our browser if we returned to the FF2 behaviour here?  Or, in other words, if we hadn't made this behavioural change, would we want it for FF3 on its own merits?  It looks to me that we chose this path in 2006 because it simplified a fix for a crash case, and believed that Opera's behaviour here would be sufficient to indicate compatibility.  Smaug expressed concerns back then about regressions, and I think they've come to pass: Prototype is quite widely used (though it might not have been in 2006, and their lack of support for Opera has protected the code in question from needing to cope with a strict interpretation of the standard until they started testing against FF3b2) and we shouldn't break those sites lightly.

It's unfortunate that the Prototype test suite in our ajax tests didn't pick this up, since it's apparently a critical issue for their toolkit; we should make sure that we're running the test suite correctly in our harness, since this is basically exactly the sort of case that our ajax test collection is meant to catch. :/

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #16)
> What would be broken in our browser if we returned to the FF2 behaviour here? 
Nothing

> Or, in other words, if we hadn't made this behavioural change, would we want it
> for FF3 on its own merits?
It wouldn't be a blocker, I guess, but a wanted-1.9. Similar
to firing capturing event listeners @target - doing something which the spec 
doesn't allow but changing implementation to follow the spec possibly breaks 
some sites.

> It looks to me that we chose this path in 2006
> because it simplified a fix for a crash case, and believed that Opera's
> behaviour here would be sufficient to indicate compatibility.
Right. What is surprising that the this bug was filed 1½ years after the change.
Maybe there aren't so many alpha users or maybe this feature isn't used
too often in Prototype.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Jonas, since you've made some quite big clean-ups to nsRange code, what do you
think about making it support the old (un-specified/un-defined) behavior? Would 
it be lots of work? Any guesses?

Comment 19

[Andrew Dupont]

This issue showed up in our unit tests for version 1.6 of Prototype; before then we didn't write any code around element creation, so we had no test coverage thereof. The Ajax tests were added before 1.6 was released.

So it sounds like the problem is keeping the test suite up to date. I'll talk to John Resig soon about how we can make this as easy as possible for you guys.

We test against Opera, albeit unofficially; the relevant test passes in Opera because we circumvent the DOM range approach altogether, instead relying on Opera's support for the non-standard IE API.

(In reply to comment #17)
> Maybe there aren't so many alpha users or maybe this feature isn't used
> too often in Prototype.

We've since determined that this won't break all versions of Prototype — only version 1.6. That's still a substantial portion of the Web, and this is a very common use case.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Btw, would be great to know what is the real use case for 
range.selectNodeContents(element) where element isn't attached to DOM tree.
Or any other similar case where element isn't in DOM tree.
Not that it matters much to whether this bug should be fixed or not, but just to understand why it is used in such way.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I wouldn't be entirely opposed to reverting this change no. I do recall being
nervous about it when the change was originally made.

My memory regarding all of this is quite hazy though, as it all happened quite
a long time ago.

However we should all be aware that if we revert this we would be choosing
website compatibility over spec compatibility. I have no faith at all that we
can get the spec changed. So there is no "fight to keep more web sites
working", we'd simply be giving up on following the spec.

We've had this argument with W3C multiple times, and their stance is basically
that the DOM is used for more things than the web so we can't change the spec
since we'd be making other things out there non-compliant.

There is one problem with reverting the fix. There is not really any logical
behavior to what happens to a range that lives inside an orphaned node, when
that node is inserted in a Document.

A range is always "rooted" such that if an endpoint lives in a node, and the
node is removed from the tree, the endpoint is moved up closer to the root of
that tree. In the spec the root of such a tree is always a node that can't be
inserted elsewhere. I.e. Documents, DocFragments and Attrs can't ever have
their parent set to be another node.

So what should happen if an orphaned element that contains a range is inserted
into a document and then removed from the document? What should the range span?
I'd argue that we should "reroot" the range when the element is inserted into
the document. This isn't ideal, but neither is any other solution.

Comment 22

[Hixie (not reading bugmail)]

There is a growing feeling that we should have a "Web DOM Core" spec, we could similarly have a "Web DOM Range" spec. Right now, while the spec does indeed require that ranges be rooted in documents or documentFragments (or attrs), it doesn't actually say what should happen if you selectNode() something that isn't in one of those. The spec needs to be fixed either way.

Comment 23

[John Resig]

This should really be reverted until a final decision is made, as it causes web content to break (anything that uses Prototype DOM Insertion, for example).

Comment 24

[John Resig]

Ok, so I just talked with Tobie some more and, apparently, this will only effect Prototype 1.6.0/1.6.0.1. If this fix is easy, then I think we should go for it, to avoid any potential problems.

Comment 25

[Johnny Stenback (:jst)]

Another web compatibility change that was apparently done primarily to match a spec, but breaks existing code. I vote for reverting this change. Smaug, any chance you could have a look a this?

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

I guess I need to look at this, though there is the problem Jonas described:

(In reply to comment #21)
> There is one problem with reverting the fix. There is not really any logical
> behavior to what happens to a range that lives inside an orphaned node, when
> that node is inserted in a Document.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #20)
> Btw, would be great to know what is the real use case for 
> range.selectNodeContents(element) where element isn't attached to DOM tree.
> Or any other similar case where element isn't in DOM tree.
Could any Prototype developer answer to this? I'd like to know what kind
of behavior is expected and how the not-in-dom range is used.
The problem is that in FF3 some range bugs have been fixed and if someone
is relying on the old, wrong, behavior (well, like this one), it may be hard to 
inject back those bugs without breaking the fixed things.

Comment 28

[John Resig]

(In reply to comment #27)
> Could any Prototype developer answer to this?

They told me, at least, that this was only an issue with the 1.6.0 release and was fixed in subsequent versions. As I mentioned above, it would be ideal if this could be resolved, if there were no ramifications, but that does not appear to be the case. They were fine with having it slip by if it was going to be a problem.

Comment 29

[Tobie Langel]

Smaug:

To make a long story short: range.selectNodeContents was used to insert HTML before/above/below/after an element in Prototype 1.5. In 1.6, we refactored that old code into a new API which handles both inserting content as a HTML string or a DOM node. People used it to populate newly created elements with pure text (instead of creating a text node and appending it - probably because the syntax is shorter).

We've fixed that issue in 1.6.0.2 (changeset is here: http://dev.rubyonrails.org/changeset/8537/spinoffs/prototype/trunk/src/dom.js) , which is a drop-in replacement for 1.6.0, so I think that's no longer an issue.

BTW, sorry if I initially overreacted, I thought the modifications in the insertion API were pre 1.6, so I was worried that a lot of users might be affected by it. Turns out it wasn't the case... and that inserting HTML *before* a detached element doesn't make much sense indeed.

I hope this clarifies the issue.

Thanks again for your help.

Comment 30

[John Resig]

They're cool, we're cool. Removing the block and closing the bug.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Well, are we sure this issue isn't affecting more things?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

I'm still trying to fix this, or as much as is possibly without regressing
other things.

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

FWIW, I think the old code mostly ended up behaving as if you did "reroot" a range to the new top node if the range was part of an orphaned tree that was inserted into another tree.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Attachment: something like this

Tried to keep changes pretty simple.
The change to nsContentUtils::ComparePoints isn't perhaps the nicest one, but 
that way the behavior in other cases doesn't change. And I *really* want to
try to avoid all regressions atm.
Jonas, comments?

Comment 35

[Alex Vincent [:WeirdAl]]

Comment on attachment 304524 [details] [diff] [review]
something like this

>+++ content/base/src/nsRange.cpp	20 Feb 2008 17:36:35 -0000
>+  if (disconnected) {
>+    return NS_ERROR_FAILURE;
>+  }

Can we come up with a better error code than this?  NS_ERROR_FAILURE is very generic.

>-  if (!nsContentUtils::ContentIsDescendantOf(parent, mRoot)) {
>+  if (!parent->HasSameOwnerDoc(mRoot)) {

I wonder about this change.  In particular, a document fragment from a document, compared against the same document.  (Then again, reading the bug summary, this might be the point.)

>+  return disconnected ? NS_ERROR_FAILURE : NS_OK;

Again, generic error code.

>@@ -1265,23 +1291,27 @@ nsRange::CompareBoundaryPoints(PRUint16 
>+  if (!mRoot->HasSameOwnerDoc(otherRoot))

Again, wondering about document fragments or detached nodes that may have the same owner document.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

You're right. I'll change those error codes to NS_ERROR_DOM_WRONG_DOCUMENT_ERR.
That is at least closest to what the spec says here:
"WRONG_DOCUMENT_ERR: Raised if the two Ranges are not in the same Document or DocumentFragment."
And so I can put back !nsContentUtils::ContentIsDescendantOfs.
Will re-run tests and upload the new patch.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Attachment: better error code

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 304540 [details] [diff] [review]
better error code

You shouldn't

>@@ -1517,18 +1518,22 @@ nsContentUtils::ComparePoints(nsINode* a
>   do {
>     parents2.AppendElement(node2);
>     node2 = node2->GetNodeParent();
>   } while (node2);
> 
>   PRUint32 pos1 = parents1.Length() - 1;
>   PRUint32 pos2 = parents2.Length() - 1;
> 
>-  NS_ASSERTION(parents1.ElementAt(pos1) == parents2.ElementAt(pos2),
>-               "disconnected nodes");
>+  if (aDisconnected) {
>+    *aDisconnected = (parents1.ElementAt(pos1) != parents2.ElementAt(pos2));
>+  } else {
>+    NS_ASSERTION(parents1.ElementAt(pos1) == parents2.ElementAt(pos2),
>+                 "disconnected nodes");
>+  }

This'll make the non-debug code look weird. Do

NS_ASSERTION(parents1.ElementAt(pos1) == parents2.ElementAt(pos2) ||
             aDisconnected);
if (aDisconnected) ...

Set *aDisconnected to PR_FALSE at the top to handle the early-returns.

>Index: content/base/src/nsRange.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsRange.cpp,v
>retrieving revision 1.226
>diff -u -8 -p -r1.226 nsRange.cpp
>--- content/base/src/nsRange.cpp	10 Feb 2008 13:04:19 -0000	1.226
>+++ content/base/src/nsRange.cpp	20 Feb 2008 19:11:57 -0000
>@@ -98,16 +98,17 @@ nsRange::CompareNodeToRange(nsIContent* 
>   return CompareNodeToRange(aNode, range, outNodeBefore, outNodeAfter);
> }
> 
> // static
> nsresult
> nsRange::CompareNodeToRange(nsIContent* aNode, nsIRange* aRange,
>                             PRBool *outNodeBefore, PRBool *outNodeAfter)
> {
>+  NS_ENSURE_STATE(aNode);
>   // create a pair of dom points that expresses location of node:
>   //     NODE(start), NODE(end)
>   // Let incoming range be:
>   //    {RANGE(start), RANGE(end)}
>   // if (RANGE(start) <= NODE(start))  and (RANGE(end) => NODE(end))
>   // then the Node is contained (completely) by the Range.
>   
>   nsresult rv;
>@@ -137,24 +138,33 @@ nsRange::CompareNodeToRange(nsIContent* 
>   }
> 
>   nsINode* rangeStartParent = range->GetStartParent();
>   nsINode* rangeEndParent = range->GetEndParent();
>   PRInt32 rangeStartOffset = range->StartOffset();
>   PRInt32 rangeEndOffset = range->EndOffset();
> 
>   // is RANGE(start) <= NODE(start) ?
>+  PRBool disconnected = PR_FALSE;
>   *outNodeBefore = nsContentUtils::ComparePoints(rangeStartParent,
>                                                  rangeStartOffset,
>-                                                 parent, nodeStart) > 0;
>+                                                 parent, nodeStart,
>+                                                 &disconnected) > 0;
>+  if (disconnected) {
>+    return NS_ERROR_DOM_WRONG_DOCUMENT_ERR;
>+  }
>+
>   // is RANGE(end) >= NODE(end) ?
>   *outNodeAfter = nsContentUtils::ComparePoints(rangeEndParent,
>                                                 rangeEndOffset,
>-                                                parent, nodeEnd) < 0;
>-
>+                                                parent, nodeEnd,
>+                                                &disconnected) < 0;
>+  if (disconnected) {
>+    return NS_ERROR_DOM_WRONG_DOCUMENT_ERR;
>+  }
>   return NS_OK;
> }
> 
> /******************************************************
>  * non members
>  ******************************************************/
> 
> nsresult
>@@ -330,16 +340,29 @@ nsRange::NodeWillBeDestroyed(const nsINo
>   NS_ASSERTION(mIsPositioned, "shouldn't be notified if not positioned");
> 
>   // No need to detach, but reset positions so that the endpoints don't
>   // end up disconnected from each other.
>   // An alternative solution would be to make mRoot a strong pointer.
>   DoSetRange(nsnull, 0, nsnull, 0, nsnull);
> }
> 
>+void
>+nsRange::ParentChainChanged(nsIContent *aContent)
>+{
>+  if (mRoot == aContent) {
>+    nsINode* newRoot = mRoot;
>+    nsINode* tmp;
>+    while ((tmp = newRoot->GetNodeParent())) {
>+      newRoot = tmp;
>+    }
>+    DoSetRange(mStartParent, mStartOffset, mEndParent, mEndOffset, newRoot);
>+  }
>+}
>+
> /********************************************************
>  * Utilities for comparing points: API from nsIDOMNSRange
>  ********************************************************/
> NS_IMETHODIMP
> nsRange::IsPointInRange(nsIDOMNode* aParent, PRInt32 aOffset, PRBool* aResult)
> {
>   PRInt16 compareResult = 0;
>   nsresult rv = ComparePoint(aParent, aOffset, &compareResult);
>@@ -367,32 +390,36 @@ nsRange::ComparePoint(nsIDOMNode* aParen
>     return NS_ERROR_NOT_INITIALIZED;
> 
>   nsCOMPtr<nsINode> parent = do_QueryInterface(aParent);
>   NS_ENSURE_TRUE(parent, NS_ERROR_DOM_HIERARCHY_REQUEST_ERR);
> 
>   if (!nsContentUtils::ContentIsDescendantOf(parent, mRoot)) {
>     return NS_ERROR_DOM_WRONG_DOCUMENT_ERR;
>   }
>-  
>+
>+  PRBool disconnected = PR_FALSE;
>   PRInt32 cmp;
>   if ((cmp = nsContentUtils::ComparePoints(parent, aOffset,
>-                                           mStartParent, mStartOffset)) <= 0) {
>+                                           mStartParent, mStartOffset,
>+                                           &disconnected)) <= 0) {

Don't you need to add |&& !disconnected| here?

>-  else if (nsContentUtils::ComparePoints(mEndParent, mEndOffset,
>-                                         parent, aOffset) == -1) {
>+  else if (!disconnected &&
>+           nsContentUtils::ComparePoints(mEndParent, mEndOffset,
>+                                         parent, aOffset,
>+                                         &disconnected) == -1) {

And technically you need to check for connected-ness here. It should be the same as from the first ComparePoints call. So leave it as null to enable the assertion.


>@@ -592,46 +621,41 @@ nsINode* nsRange::IsValidBoundary(nsINod
>   NS_ASSERTION(!root->IsNodeOfType(nsINode::eDOCUMENT),
>                "GetCurrentDoc should have returned a doc");
> 
>   if (root->IsNodeOfType(nsINode::eDOCUMENT_FRAGMENT) ||
>       root->IsNodeOfType(nsINode::eATTRIBUTE)) {
>     return root;
>   }
> 
>-#ifdef DEBUG_smaug
>-  nsCOMPtr<nsIContent> cont = do_QueryInterface(root);
>-  if (cont) {
>-    nsAutoString name;
>-    cont->Tag()->ToString(name);
>-    printf("nsRange::IsValidBoundary: node is not a valid boundary point [%s]\n",
>-           NS_ConvertUTF16toUTF8(name).get());
>-  }
>-#endif
>-
>-  return nsnull;
>+  NS_WARNING("Creating a DOM Range using root which isn't in DOM!");
>+  // But we allow this because of backward compatibility.
>+  return root;

No need to check for docfragment and attribute as only debug-only code differs. If you really do want to warn do something like

#ifdef DEBUG
  if (DOC_FRAGMENT || ATTR) {
     NS_WARNING
  }
#endif

Though I sort of think that if we're going to allow ranges like this we should embrace it fully and not even warn.


> nsresult nsRange::SetStart(nsIDOMNode* aParent, PRInt32 aOffset)
> {
>   VALIDATE_ACCESS(aParent);
> 
>   nsCOMPtr<nsINode> parent = do_QueryInterface(aParent);
>   nsINode* newRoot = IsValidBoundary(parent);
>   NS_ENSURE_TRUE(newRoot, NS_ERROR_DOM_RANGE_INVALID_NODE_TYPE_ERR);
> 
>   PRInt32 len = GetNodeLength(parent);
>   if (aOffset < 0 || aOffset > len)
>     return NS_ERROR_DOM_INDEX_SIZE_ERR;
> 
>   // Collapse if not positioned yet, if positioned in another doc or
>   // if the new start is after end.
>+  PRBool disconnected = PR_FALSE;
>   if (!mIsPositioned || newRoot != mRoot ||
>       nsContentUtils::ComparePoints(parent, aOffset,
>-                                    mEndParent, mEndOffset) == 1) {
>+                                    mEndParent, mEndOffset,
>+                                    &disconnected) == 1 ||
>+      disconnected) {

The nodes can't be disconnected here can they, since newRoot == mRoot. So leave as is to get the assertion.

Same in SetEnd and CompareBoundaryPoints.

Actually, come to think of it. Why do you need to add the disconnected check anywhere? There were disconnected subtrees before too, just always rooted in docfragments. So the old code should have had all necessary checks, no?

IsPointInRange checks nsContentUtils::ContentIsDescendantOf so that should be enough there. And the CompareNodeToRange callers should already be checking for disconnectedness.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

Ok, disconnectivity check isn't needed when there is root check, but I think
it is better to have the test in nsRange::CompareNodeToRange.

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 305308 [details] [diff] [review]
v2

>+nsRange::ParentChainChanged(nsIContent *aContent)
>+{
>+  if (mRoot == aContent) {

Just assert that the above is true. The only node we observe is mRoot so if we get notified for any other something is very wrong.

r/sr=me with that