803924 - Crash with range, splitText

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

Comment 1

[Jesse Ruderman]

Attachment: stack

Nightly: bp-a307650f-eb88-40a1-a041-bce862121021

Comment 2

[Alex Vincent [:WeirdAl]]

Confirmed on FF 17 beta:
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Firefox/17.0

Comment 3

[Scoobidiver (away)]

On Windows: bp-aa288a5c-f12e-40bb-aff7-010252121021

Comment 4

[(no longer active)]

Is this a regression?

Comment 5

[Jesse Ruderman]

Likely related: bug 804784

Comment 6

[Alice0775 White]

Regression window(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/630e28e90986
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110816 Firefox/8.0a1 ID:20110816031314
Crash:
http://hg.mozilla.org/mozilla-central/rev/35657230a209
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110816 Firefox/8.0a1 ID:20110816035854
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=630e28e90986&tochange=35657230a209



Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/01ce7e95d7b7
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110815 Firefox/8.0a1 ID:20110815161956
Crash:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4c6dfeb5dc3a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110815 Firefox/8.0a1 ID:20110815175900
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=01ce7e95d7b7&tochange=4c6dfeb5dc3a

Suspected: Bug 191864

Comment 7

[Mats Palmgren (inactive)]

Attachment: wip

Splitting a text node implies a new node, so if the range start/end
is that text node's parent the offset may need to be adjusted.

Something like this should fix it...

It doesn't fix bug 804784, which I suspect is the opposite case -
when merging text nodes we may need to decrement...

Amazing, I think we have more than a hundred tests for split/merge
and yet these simple cases were never tested for some reason.

Fuzz tools: 1  --  Humans: 0

Comment 8

[Mats Palmgren (inactive)]

It was more complicated than I first thought but I think I have a fix now...

Comment 9

[Mats Palmgren (inactive)]

Attachment: fix + test

This is in essence what the "wip" patch suggested, but we can't just
increment the range offsets because that will trigger another increment
in the ContentInserted/Appended notification that follows when the new
text node is inserted.

Green on Try:
https://tbpl.mozilla.org/?tree=Try&rev=3f92287de016

Comment 10

[Lukas Blakk [:lsblakk] use ?needinfo]

Unfortunately we're too late to land speculative fixes to beta 17, but please nominate for uplift to aurora when this has had bake time on trunk so we'll get it for 18.

Comment 11

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

Comment on attachment 679184 [details] [diff] [review]
fix + test

Uh, this is a bit scary, or error prone :/

Could you add some more debug code to make sure the next ContentAppended/Inserted
is actually the call we're expecting and not something else.
So, perhaps in #ifdef DEBUG store aInfo->mDetails->mNextSibling and
its index and assert that they are the right ones in ContentAppended/Inserted
...or something like that.
Also, perhaps assert that when mIgnoreNextInsertOrAppend is true we don't
get any unexpected notifications, I mean non-ContentAppended/Inserted

What is the change to test_Range-mutations.html.json ?
I thought dom/imptests/failures/webapps/DOMCore/tests/approved is something
from W3C/WhatWG testsuite. If we change it, we should change also to original copy.

Would like to see a new patch with some more assertions.

Comment 12

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Olli Pettay [:smaug] from comment #11)
> What is the change to test_Range-mutations.html.json ?
> I thought dom/imptests/failures/webapps/DOMCore/tests/approved is something
> from W3C/WhatWG testsuite. If we change it, we should change also to
> original copy.

It means that before this patch, the upstream file test_Range-mutations.html passed all tests.  This adds new expected fails in file test_Range-mutations.html.json.  (Notice how the added file is under the directory "dom/imptests/failures/", since it's an expected failures file.)  Obviously, these fails constitute a regression unless it can be demonstrated that the spec is wrong.

Comment 13

[Mats Palmgren (inactive)]

Attachment: fix + test

(In reply to Olli Pettay [:smaug] from comment #11)
> Could you add some more debug code to make sure the next
> ContentAppended/Inserted
> is actually the call we're expecting and not something else.

Added assertions and some additional tests with empty text nodes.

Also, as an optimization, skip boundary points positioned before the
first child since they can't possibly be affected by a splitText().

https://tbpl.mozilla.org/?tree=Try&rev=591739349e83

Comment 14

[Mats Palmgren (inactive)]

(In reply to :Aryeh Gregor from comment #12)
> Obviously, these fails constitute a regression unless it can be demonstrated
> that the spec is wrong.

Yes, the spec has a bug.  If you follow the steps in 5.10 for the reported
testcase you will end up with an invalid range.  Specifically, in step
7.1 the callout to Insert will not affect the range boundaries; in step
7.2 the condition "start node is node and start offset is greater than offset"
is true so we "set its start node to new node..." -- this means we move the
start boundary *after* the end boundary and none of the remaining steps
adjusts the end boundary.
http://dom.spec.whatwg.org/#interface-text

The spec should add:
7.4 For each range whose start node is /parent/ and start offset is equal to
    the index of /node/ + 1, set its start offset to its current value + 1.
7.5 ditto with s/start/end/

Comment 15

[Mats Palmgren (inactive)]

Attachment: additional tests (that does NOT crash)

Here are a few more tests (that does not crash) to demonstrate the bug.
The desired result is that the content covered by the range is the
same after the splitText() call as it was before.  This is after all
one of the two *general principles* laid out in the original spec:
http://www.w3.org/TR/DOM-Level-2-Traversal-Range/ranges.html#Level-2-Range-Mutation
"[...] all Ranges will select the same portion of the document after
any mutation operation"

IE10 pass these tests.  So will we with the attached patch.
(Opera and Chrome fails)

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Mats Palmgren [:mats] from comment #14)
> Yes, the spec has a bug.  If you follow the steps in 5.10 for the reported
> testcase you will end up with an invalid range.  Specifically, in step
> 7.1 the callout to Insert will not affect the range boundaries;

Why not?  The algorithm for insert says:

"For each range whose end node is parent and end offset is greater than child's index, increase its end offset by count."
http://dom.spec.whatwg.org/#concept-node-insert

Isn't that the case here?  Also, some of the tests you're making into failures are passed by all of IE, Gecko, WebKit, and Opera:

http://w3c-test.org/webapps/DOMCore/tests/approved/Range-mutations.html

If all browsers pass a test and you're changing Gecko to not pass the test, it would be very surprising if you're not regressing Gecko.  (IE and WebKit fail some of the tests in question for other reasons.)

> in step
> 7.2 the condition "start node is node and start offset is greater than
> offset"
> is true so we "set its start node to new node..." -- this means we move the
> start boundary *after* the end boundary and none of the remaining steps
> adjusts the end boundary.
> http://dom.spec.whatwg.org/#interface-text
> 
> The spec should add:
> 7.4 For each range whose start node is /parent/ and start offset is equal to
>     the index of /node/ + 1, set its start offset to its current value + 1.
> 7.5 ditto with s/start/end/

It already says that in the "insert" algorithm (although it's not just node's index + 1, it's anything greater than node's index).

Comment 17

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

Mats, could you answer to Aryeh? We can't change behavior to be different than what
everybody else have.

Comment 18

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

Comment on attachment 680539 [details] [diff] [review]
fix + test

r+ for the assertions, but need to figure out the behavior we want here and which
is compatible with other browsers.

Comment 19

[Mats Palmgren (inactive)]

Allow me to illustrate why the spec has a bug for splitText() by using
an example:
<div>"abc""def"</div>
(where "..." denotes text nodes)

Using the range "abc"/2 .. div/1 (i.e. startContainer="abc"
startOffset=2, endContainer=div endOffset=1)

Now let's see what the spec says about "abc".splitText(1):

In 5.10 http://dom.spec.whatwg.org/#interface-text -
 1+2+3+4+5+6, creates a new text node, /new node/, with content "bc"
 7.1 call out to Insert to insert /new node/ in the div before /node/'s
     next sibling (which is "def").

In 5.2.1, Insert - http://dom.spec.whatwg.org/#concept-node-insert -
   Just to be clear, here /node/ is our new node "bc", /parent/ is the
   div and /child/ is "def".
   1. ...
   2. For each range whose start node is /parent/ ...
      => this condition is false, no change
   3. For each range whose end node is /parent/ (true) and end offset is
      greater than /child/'s index, increase its end offset by count.
      => "def"'s child index is 1, the range end offset is 1, thus the
         latter part of the condition is false => no change
   4. /nodes/ is "bc"
   5+6 does not apply beacuse "bc" is not a DocumentFragment node
   7+8+9 insert "bc" before "def"

Thus 5.2.1 did not affect the range; back to 5.10:
   (reminder: /node/ is "abc" and /offset/ is 1 here)
   7.2 For each range whose start node is /node/ (true) and start offset is
       greater than /offset/ (true), set its start node to /new node/ and
       decrease its start offset by /offset/.
       => the condition is true: set the start boundary to "bc"/1
   7.3 For each range whose end node is /node/ (false) ... => no change
   8. replace data ... => the first text node becomes "a"
   9. n/a
   10. return /new node/

So, the resulting content is:
<div>"a""bc""def"</div>

and our range is:
"bc"/1 .. div/1

=> the start boundary is *after* the end boundary

Comment 20

[Mats Palmgren (inactive)]

Attachment: The example in comment 19

Here's the example I used above.  Note that I'm using *verbatim* copies
of the function testSplitText() from test_Range-mutations.html and
indexOf() from common.js in the DOM test suite.  This demonstrates that
the test suite expects an *invalid* range for this test (matching the
spec).

Comment 21

[Mats Palmgren (inactive)]

(In reply to :Aryeh Gregor from comment #16)
> Why not?  The algorithm for insert says:
> 
> "For each range whose end node is parent and end offset is greater than
> child's index, increase its end offset by count."
> http://dom.spec.whatwg.org/#concept-node-insert
> 
> Isn't that the case here?

No, see above.

> Also, some of the tests you're making into
> failures are passed by all of IE, Gecko, WebKit, and Opera:

I tried IE9 on Win7 and IE10 on Win8, both hangs when I run
the original test_Range-mutations.html test (apparently inside
indexOf() because the range.startContainer is a text node that
isn't a child of the parent of the text node we split - so IE
does not follow the spec)

> > The spec should add:
> > 7.4 For each range whose start node is /parent/ and start offset is equal to
> >     the index of /node/ + 1, set its start offset to its current value + 1.
> > 7.5 ditto with s/start/end/
> 
> It already says that in the "insert" algorithm (although it's not just
> node's index + 1, it's anything greater than node's index).

The reason I say "is equal to" and not "is equal to or greater than" is
precisely because the "Insert" algorithm takes care of the "greater than"
part -- we don't want to increment those twice.

Comment 22

[Mats Palmgren (inactive)]

Attachment: Standalone "failures" test

This is a standalone version of the five tests that I marked as failures
(from test_Range-mutations.html).
If you run this in IE you will see that the selection matches Gecko with
the fix.

Comment 23

[Mats Palmgren (inactive)]

To further motivate why we should change the spec in the way that
I suggest and not in some other way, let's look at a few more examples
and apply the current spec:

1. <div>"abc""def"</div>, with range div/1 .. div/2
   "abc".splitText(1) leads to the result:
   <div>"a""bc""def"</div>, with range div/1 .. div/3
   That is, "bc" is now part of the range, but wasn't before.

2. <div>"abc""def"</div>, with range div/0 .. div/1
   "abc".splitText(1) leads to the result:
   <div>"a""bc""def"</div>, with range div/0 .. div/1
   That is, "bc" is not part of the range anymore, but was before.

3. <div>"abc""def"</div>, with range div/1 .. div/1
   "abc".splitText(1) leads to the result:
   <div>"a""bc""def"</div>, with range div/1 .. div/1
   Imagine that this content is editable and that the collapsed range
   represents the caret position.  If the user now types a character it
   will be inserted between a and b, not between c and d where the caret
   originally was before the splitText mutation.

All these results violates the general principle that "all Ranges will
select the same portion of the document after any mutation operation".
Changing the spec the way that I suggest will make splitText() mutations
adhere to this principle and work as expected (POLA).  I don't see why
splitText() (and normalize()) is special and should be destructive when
we work so hard to adjust ranges for other types of mutations.

Comment 24

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Yes, now I see what you're saying.  This is definitely a spec bug.  It's symptomatic of a general issue with insertions -- if a range has a boundary point at the point where something is inserted, we can't really know if the boundary point would make more sense winding up before or after the newly-inserted thing.  The spec goes with before by default, but in this case it should be after, so we do indeed need a special case as you suggest.

I've filed a W3C bug, and will handle it if Anne doesn't: https://www.w3.org/Bugs/Public/show_bug.cgi?id=19968  I'll also update the upstream tests when the spec is fixed.

Comment 25

[Mats Palmgren (inactive)]

As noted in the W3C bug, the latest IE10 (preview version for Win7) has actually
implemented the same fix as I suggest here.

Comment 26

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

Ok. Should I then perhaps review the patch? If so, ask a review :)

Comment 27

[Mats Palmgren (inactive)]

Comment on attachment 680539 [details] [diff] [review]
fix + test

Yes, please.

Comment 28

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

So the spec talks now about 
"... offset is equal to the index of node + 1", but the patch
is about < or <=.
Makes hard to follow this all.

Also, could we have tests for the case when either one of range end point points to an
empty text node and split text is called on that one.
Test also the case when range is collapsed to an empty text node and split is called.

Comment 29

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

There are some tests for empty text node, but I don't see all the cases covered...
or am I missing something.

Comment 30

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

Ah, the spec handles index of node + 1 in one place and 
* offset is greater than child's index in another place.

Any reason the implementation couldn't do the same? I think it would make the code easier to read.

Comment 31

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

Comment on attachment 680539 [details] [diff] [review]
fix + test

I think that should be doable. Only handle the same special case in
CharacterDataChange as what the spec has in
http://dom.spec.whatwg.org/#concept-text-split  7.4/5

(Sorry being annoying with this review but getting this code as easy to
understand as possible is quite important, IMO.)

Comment 32

[Mats Palmgren (inactive)]

Attachment: fix + test

Sure, it's doable.  We still need to prevent incrementing the offset
again in Insert though (since incrementing it in CharacterDataChange
makes the new value test true there).

Added more tests with empty text nodes, and collapsed ranges.

https://tbpl.mozilla.org/?tree=Try&rev=531171ad3f2a
https://tbpl.mozilla.org/?tree=Try&rev=dbf5bcb67685

Comment 33

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

Comment on attachment 683642 [details] [diff] [review]
fix + test

Perhaps you could assert that 
mStartOffsetWasIncremented and mEndOffsetWasIncremented are false
in ContentRemoved and ParentChainChanged

Comment 34

[Mats Palmgren (inactive)]

Ok, did so:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88bad356f5a4

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/88bad356f5a4

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(PTO: 9/19-9/27)]

Attachment: Bug 803924 - Make `HTMLTextAreaElement` handle the mutation changes after all ranges handle them r=smaug!

A mutation caused by a call of Text::SplitText is handled by 2 method calls,
CharacterDataChanged and ContentInserted, in nsRange. Therefore,
nsRange stores some nodes for the later one, but
HTMLTextAreaElement::ContentInserted is called before it and that causes
another mutation which causes calling nsRange::CharacterDataChanged again.
Therefore, the assertion detects the recursive call.

For avoiding this issue, HTMLTextAreaElement needs to wait that all ranges
handle the mutation first. Fortunately, ContentInserted is called with a
script blocker (*1). Therefore, HTMLTextAreaElement can use script runner
to reset the anonymous subtree.

1. https://searchfox.org/mozilla-central/rev/f1dc2743777711c821d43f9911ee7c4447d60c8e/dom/base/nsINode.cpp#1566,1610

Comment 37

[BugBot [:suhaib / :marco]]

A patch has been attached on this bug, which was already closed. Filing a separate bug will ensure better tracking. If this was not by mistake and further action is needed, please alert the appropriate party. (Or: if the patch doesn't change behavior -- e.g. landing a test case, or fixing a typo -- then feel free to disregard this message)

Comment 38

[Phabricator Automation]

Comment on attachment 9313946 [details]
Bug 803924 - Make HTMLTextAreaElement handle the mutation changes after all ranges handle them r=smaug!

Revision D167766 was moved to bug 822734. Setting attachment 9313946 [details] to obsolete.