Closed Bug 682463 Opened 8 years ago Closed 8 years ago

"ASSERTION: unexpected disconnected nodes" with DOM range, splitText

Categories

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

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla9
Tracking Status
firefox8 + ---
firefox9 + ---

People

(Reporter: jruderman, Assigned: mats)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, regression, testcase, Whiteboard: [qa!])

Attachments

(5 files, 5 obsolete files)

Attached file testcase
###!!! ASSERTION: unexpected disconnected nodes: 'aDisconnected', file content/base/src/nsContentUtils.cpp, line 1617
Attached file stack trace
Is this a recent regression?
Bug 191864 landed recently.
Mats, could you look at this?
If I read https://bug191864.bugzilla.mozilla.org/attachment.cgi?id=550898 correctly, 
it doesn't update mRoot when it should.
Actually, since I started to look at this, I'll fix this.
Assignee: nobody → Olli.Pettay
Actually, I don't even know what behavior is expected here.
Assignee: Olli.Pettay → nobody
Attached patch WIP (obsolete) — Splinter Review
Actually, this could be what we need.
I'll take a look at this.  I suspect it might be the same underlying problem as in
bug 679459 which was a regression from bug 191864.
Assignee: nobody → matspal
Depends on: 679459
Does this have likely fallout on the wild web or is it just a correctness issue?
Blocks: 191864
No longer depends on: 679459
OS: Mac OS X → All
Hardware: x86_64 → All
Attached patch fix (obsolete) — Splinter Review
As you said, the problem is that nsRange::CharacterDataChanged doesn't
update mRoot when it changes a boundary node.  (I naively assumed that
text nodes always had a root different from themselves but that isn't
the case when they haven't been added to the document yet.)

This patch checks if either boundary node was changed and if so calls
DoSetRange.  I check explicitly for "mRoot == boundary point" and
change the root in that case.  The call to DoSetRange takes care of
moving the observer etc.

The change to nsGenericDOMDataNode::SplitData moves the insertion of
the new text node into its parent before CharacterDataChanged is
called, otherwise looking up the new root will always result in the
text node itself, which is not what we want in the case the original
node is in the document.  It's possible to fix 
nsRange::CharacterDataChanged without this change, but it seems like
the right thing to do.
Attachment #556681 - Attachment is obsolete: true
Attachment #558231 - Flags: review?(Olli.Pettay)
Attached patch regression test (obsolete) — Splinter Review
(In reply to Asa Dotzler [:asa] from comment #10)
> Does this have likely fallout on the wild web or is it just a correctness
> issue?

It's hard to predict the fallout from this and the other regression in
bug 679459, so I filed bug 684661 to back out the change from aurora.
Sorry for the regressions. :-(
The spec is hard to read, but I believe the special case for splittext applies only
when text node has parent. In other cases range should be modified in a normal way after DOM mutation.
Comment on attachment 558231 [details] [diff] [review]
fix


>+  if (newStartNode || newEndNode) {
>+    if (!newStartNode) {
>+      newStartNode = mStartParent;
>+      newStartOffset = mStartOffset;
>+    }
>+    if (!newEndNode) {
>+      newEndNode = mEndParent;
>+      newEndOffset = mEndOffset;
>+    }
>+    DoSetRange(newStartNode, newStartOffset, newEndNode, newEndOffset,
>+               newRoot ? newRoot : mRoot.get());
>+  }

So I don't see how does this handle the case when text node doesn't have parent
and text node is "foobar" and range selects the whole text and then splittext splits the
node between o and b.
Wouldn't newStartNode point to the original textnode and newEndNode to the newly created node?

This needs more tests.

I filed http://www.w3.org/Bugs/Public/show_bug.cgi?id=14035 to clarify the spec
http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#range-behavior-under-document-mutation
Attachment #558231 - Flags: review?(Olli.Pettay) → review+
Attachment #558231 - Flags: review+ → review-
Attached patch fix v2 (obsolete) — Splinter Review
Good catch, thanks.
Attachment #558231 - Attachment is obsolete: true
Attachment #560492 - Flags: review?(Olli.Pettay)
More tests...
Attachment #558232 - Attachment is obsolete: true
Attached patch fix v3Splinter Review
I realized this part became redundant with the latest change:

  if (NS_UNLIKELY(aContent == mRoot)) {
    newRoot = IsValidBoundary(newEndNode);
  }

so I removed that.  Try server results pending...
Attachment #560492 - Attachment is obsolete: true
Attachment #560492 - Flags: review?(Olli.Pettay)
Attachment #560567 - Flags: review?(Olli.Pettay)
Attached patch fix v4 (obsolete) — Splinter Review
Here's an alternative patch that doesn't change the order of
CharacterDataChanged and inserting the new node. It requires
suppressing an assertion in DoSetRange since the new text node
isn't inserted yet...
Attachment #560622 - Flags: review?(Olli.Pettay)
Comment on attachment 560622 [details] [diff] [review]
fix v4


>+    DoSetRange(newStartNode, newStartOffset, newEndNode, newEndOffset,
>+               newRoot ? newRoot : mRoot.get(), !newEndNode->GetParent());
ifdef DEBUG for the last parameter.
Attachment #560622 - Flags: review?(Olli.Pettay) → review+
Attached patch fix v5Splinter Review
As v4, with compile fix for Opt builds.
Attachment #560622 - Attachment is obsolete: true
Attachment #560631 - Flags: review?(Olli.Pettay)
Attachment #560631 - Flags: review?(Olli.Pettay) → review+
Attachment #560567 - Flags: review?(Olli.Pettay)
https://hg.mozilla.org/mozilla-central/rev/8e0213fd6698
https://hg.mozilla.org/mozilla-central/rev/21c337884f31
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Verified fixed on:
Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 5.1; rv:9.0a2) Gecko/20111002 Firefox/9.0a2
Mozilla/5.0 (Windows NT 5.1; rv:10.0a1) Gecko/20111003 Firefox/10.0a1

Mozilla/5.0 (Windows NT 6.1; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Windows NT 6.1; rv:9.0a2) Gecko/20111003 Firefox/9.0a2
Mozilla/5.0 (Windows NT 6.1; rv:10.0a1) Gecko/20111003 Firefox/10.0a1

Mozilla/5.0 (X11; Linux i686; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (X11; Linux i686; rv:9.0a2) Gecko/20111003 Firefox/9.0a2
Mozilla/5.0 (X11; Linux i686; rv:10.0a1) Gecko/20111003 Firefox/10.0a1

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:8.0) Gecko/20100101 Firefox/8.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0a2) Gecko/20110929 Firefox/9.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:10.0a1) Gecko/20110927 Firefox/10.0a1

I used the test case attached to this bug to verify it and there were no issues (unexpected assertions, warnings, errors)
Status: RESOLVED → VERIFIED
Whiteboard: [qa!]
Depends on: 688996
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.