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

Status: VERIFIED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

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

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

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

Is this a recent regression?

Comment 3

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

Bug 191864 landed recently.

Comment 4

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

Mats, could you look at this?

Comment 5

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

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][[e-mail]]]

Actually, since I started to look at this, I'll fix this.

Comment 7

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

Actually, I don't even know what behavior is expected here.

Comment 8

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

Attachment: WIP

Actually, this could be what we need.

Comment 9

[Mats Palmgren (inactive)]

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]]

Does this have likely fallout on the wild web or is it just a correctness issue?

Comment 11

[Mats Palmgren (inactive)]

Attachment: 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 (inactive)]

Attachment: regression test

Comment 13

[Mats Palmgren (inactive)]

(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][[e-mail]]]

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][[e-mail]]]

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][[e-mail]]]

The spec is now clearer http://dvcs.w3.org/hg/domcore/raw-file/tip/Overview.html#concept-text-split

Comment 17

[Mats Palmgren (inactive)]

Attachment: fix v2

Good catch, thanks.

Comment 18

[Mats Palmgren (inactive)]

Attachment: regression tests, v2

More tests...

Comment 19

[Mats Palmgren (inactive)]

Attachment: 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 (inactive)]

Attachment: 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][[e-mail]]]

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 (inactive)]

Attachment: fix v5

As v4, with compile fix for Opt builds.

Comment 23

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/21c337884f31
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e0213fd6698

Comment 24

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/8e0213fd6698
https://hg.mozilla.org/mozilla-central/rev/21c337884f31

Comment 25

[Ioana (away)]

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)