Last Comment Bug 682463 - "ASSERTION: unexpected disconnected nodes" with DOM range, splitText
: "ASSERTION: unexpected disconnected nodes" with DOM range, splitText
Status: VERIFIED FIXED
[qa!]
: assertion, regression, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: Mats Palmgren (:mats)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 688996
Blocks: 326633 336383 191864
  Show dependency treegraph
 
Reported: 2011-08-26 16:20 PDT by Jesse Ruderman
Modified: 2013-12-27 14:21 PST (History)
10 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
+


Attachments
testcase (247 bytes, text/html)
2011-08-26 16:20 PDT, Jesse Ruderman
no flags Details
stack trace (2.84 KB, text/plain)
2011-08-26 16:21 PDT, Jesse Ruderman
no flags Details
WIP (2.12 KB, patch)
2011-08-29 15:00 PDT, Olli Pettay [:smaug]
no flags Details | Diff | Splinter Review
fix (6.30 KB, patch)
2011-09-04 23:18 PDT, Mats Palmgren (:mats)
bugs: review-
Details | Diff | Splinter Review
regression test (1011 bytes, patch)
2011-09-04 23:20 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
fix v2 (6.74 KB, patch)
2011-09-15 17:07 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
regression tests, v2 (6.21 KB, patch)
2011-09-15 17:08 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
fix v3 (6.64 KB, patch)
2011-09-16 10:19 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
fix v4 (7.52 KB, patch)
2011-09-16 14:11 PDT, Mats Palmgren (:mats)
bugs: review+
Details | Diff | Splinter Review
fix v5 (7.57 KB, patch)
2011-09-16 14:45 PDT, Mats Palmgren (:mats)
bugs: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2011-08-26 16:20:59 PDT
Created attachment 556170 [details]
testcase

###!!! ASSERTION: unexpected disconnected nodes: 'aDisconnected', file content/base/src/nsContentUtils.cpp, line 1617
Comment 1 Jesse Ruderman 2011-08-26 16:21:29 PDT
Created attachment 556171 [details]
stack trace
Comment 2 Olli Pettay [:smaug] 2011-08-26 16:27:52 PDT
Is this a recent regression?
Comment 3 Olli Pettay [:smaug] 2011-08-26 16:28:35 PDT
Bug 191864 landed recently.
Comment 4 Olli Pettay [:smaug] 2011-08-29 13:51:08 PDT
Mats, could you look at this?
Comment 5 Olli Pettay [:smaug] 2011-08-29 14:07:31 PDT
If I read https://bug191864.bugzilla.mozilla.org/attachment.cgi?id=550898 correctly, 
it doesn't update mRoot when it should.
Comment 6 Olli Pettay [:smaug] 2011-08-29 14:13:24 PDT
Actually, since I started to look at this, I'll fix this.
Comment 7 Olli Pettay [:smaug] 2011-08-29 14:23:18 PDT
Actually, I don't even know what behavior is expected here.
Comment 8 Olli Pettay [:smaug] 2011-08-29 15:00:08 PDT
Created attachment 556681 [details] [diff] [review]
WIP

Actually, this could be what we need.
Comment 9 Mats Palmgren (:mats) 2011-08-29 21:17:42 PDT
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.
Comment 10 Asa Dotzler [:asa] 2011-09-01 14:55:51 PDT
Does this have likely fallout on the wild web or is it just a correctness issue?
Comment 11 Mats Palmgren (:mats) 2011-09-04 23:18:59 PDT
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.
Comment 12 Mats Palmgren (:mats) 2011-09-04 23:20:24 PDT
Created attachment 558232 [details] [diff] [review]
regression test
Comment 13 Mats Palmgren (:mats) 2011-09-04 23:43:27 PDT
(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. :-(
Comment 14 Olli Pettay [:smaug] 2011-09-05 10:33:01 PDT
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 15 Olli Pettay [:smaug] 2011-09-05 11:14:50 PDT
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
Comment 16 Olli Pettay [:smaug] 2011-09-08 03:08:50 PDT
The spec is now clearer http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-text-split
Comment 17 Mats Palmgren (:mats) 2011-09-15 17:07:12 PDT
Created attachment 560492 [details] [diff] [review]
fix v2

Good catch, thanks.
Comment 18 Mats Palmgren (:mats) 2011-09-15 17:08:56 PDT
Created attachment 560493 [details] [diff] [review]
regression tests, v2

More tests...
Comment 19 Mats Palmgren (:mats) 2011-09-16 10:19:18 PDT
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...
Comment 20 Mats Palmgren (:mats) 2011-09-16 14:11:47 PDT
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...
Comment 21 Olli Pettay [:smaug] 2011-09-16 14:45:20 PDT
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.
Comment 22 Mats Palmgren (:mats) 2011-09-16 14:45:41 PDT
Created attachment 560631 [details] [diff] [review]
fix v5

As v4, with compile fix for Opt builds.
Comment 25 Ioana (away) 2011-10-04 07:17:47 PDT
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)

Note You need to log in before you can comment on or make changes to this bug.