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

VERIFIED FIXED in mozilla9

Status

()

Core
DOM: Core & HTML
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: Jesse Ruderman, Assigned: Mats Palmgren (vacation - back in August))

Tracking

(Blocks: 2 bugs, {assertion, regression, testcase})

Trunk
mozilla9
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox8+, firefox9+)

Details

(Whiteboard: [qa!])

Attachments

(5 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 556170 [details]
testcase

###!!! ASSERTION: unexpected disconnected nodes: 'aDisconnected', file content/base/src/nsContentUtils.cpp, line 1617
(Reporter)

Comment 1

6 years ago
Created attachment 556171 [details]
stack trace

Comment 2

6 years ago
Is this a recent regression?

Comment 3

6 years ago
Bug 191864 landed recently.

Comment 4

6 years ago
Mats, could you look at this?
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?

Comment 5

6 years ago
If I read https://bug191864.bugzilla.mozilla.org/attachment.cgi?id=550898 correctly, 
it doesn't update mRoot when it should.

Comment 6

6 years ago
Actually, since I started to look at this, I'll fix this.
Assignee: nobody → Olli.Pettay

Comment 7

6 years ago
Actually, I don't even know what behavior is expected here.
Assignee: Olli.Pettay → nobody

Comment 8

6 years ago
Created attachment 556681 [details] [diff] [review]
WIP

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

Comment 10

6 years ago
Does this have likely fallout on the wild web or is it just a correctness issue?
tracking-firefox8: ? → +
tracking-firefox9: ? → +
Keywords: regression
Blocks: 191864
No longer depends on: 679459
OS: Mac OS X → All
Hardware: x86_64 → All
Created attachment 558231 [details] [diff] [review]
fix

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)
Created attachment 558232 [details] [diff] [review]
regression test
(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+

Updated

6 years ago
Attachment #558231 - Flags: review+ → review-
The spec is now clearer http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-text-split
Created attachment 560492 [details] [diff] [review]
fix v2

Good catch, thanks.
Attachment #558231 - Attachment is obsolete: true
Attachment #560492 - Flags: review?(Olli.Pettay)
Created attachment 560493 [details] [diff] [review]
regression tests, v2

More tests...
Attachment #558232 - Attachment is obsolete: true
Created attachment 560567 [details] [diff] [review]
fix v3

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)
Created attachment 560622 [details] [diff] [review]
fix v4

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+
Created attachment 560631 [details] [diff] [review]
fix v5

As v4, with compile fix for Opt builds.
Attachment #560622 - Attachment is obsolete: true
Attachment #560631 - Flags: review?(Olli.Pettay)

Updated

6 years ago
Attachment #560631 - Flags: review?(Olli.Pettay) → review+

Updated

6 years ago
Attachment #560567 - Flags: review?(Olli.Pettay)
https://hg.mozilla.org/integration/mozilla-inbound/rev/21c337884f31
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0213fd6698
Flags: in-testsuite+
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/8e0213fd6698
https://hg.mozilla.org/mozilla-central/rev/21c337884f31
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla9

Comment 25

6 years ago
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
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.