Last Comment Bug 803924 - Crash with range, splitText
: Crash with range, splitText
Status: RESOLVED FIXED
: crash, regression, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: 9 Branch
: All All
: -- critical (vote)
: mozilla20
Assigned To: Mats Palmgren (:mats)
:
:
Mentors:
Depends on: 813919
Blocks: 336383 191864
  Show dependency treegraph
 
Reported: 2012-10-20 18:07 PDT by Jesse Ruderman
Modified: 2013-04-04 13:53 PDT (History)
12 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
wontfix


Attachments
testcase (crashes Firefox when loaded) (318 bytes, text/html)
2012-10-20 18:07 PDT, Jesse Ruderman
no flags Details
stack (12.00 KB, text/plain)
2012-10-20 18:08 PDT, Jesse Ruderman
no flags Details
wip (1.62 KB, patch)
2012-10-24 19:54 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
fix + test (13.66 KB, patch)
2012-11-07 09:11 PST, Mats Palmgren (:mats)
bugs: review-
Details | Diff | Splinter Review
fix + test (17.32 KB, patch)
2012-11-11 23:35 PST, Mats Palmgren (:mats)
bugs: review-
Details | Diff | Splinter Review
additional tests (that does NOT crash) (1.85 KB, text/html)
2012-11-11 23:39 PST, Mats Palmgren (:mats)
no flags Details
The example in comment 19 (4.18 KB, text/html)
2012-11-14 06:05 PST, Mats Palmgren (:mats)
no flags Details
Standalone "failures" test (1.61 KB, text/html)
2012-11-14 06:09 PST, Mats Palmgren (:mats)
no flags Details
fix + test (18.20 KB, patch)
2012-11-20 09:50 PST, Mats Palmgren (:mats)
bugs: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-10-20 18:07:14 PDT
Created attachment 673646 [details]
testcase (crashes Firefox when loaded)
Comment 1 Jesse Ruderman 2012-10-20 18:08:12 PDT
Created attachment 673647 [details]
stack

Nightly: bp-a307650f-eb88-40a1-a041-bce862121021
Comment 2 Alex Vincent [:WeirdAl] 2012-10-20 18:18:41 PDT
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) 2012-10-21 04:44:13 PDT
On Windows: bp-aa288a5c-f12e-40bb-aff7-010252121021
Comment 4 :Ehsan Akhgari 2012-10-22 11:18:09 PDT
Is this a regression?
Comment 5 Jesse Ruderman 2012-10-23 14:55:28 PDT
Likely related: bug 804784
Comment 6 Alice0775 White 2012-10-24 11:24:43 PDT
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 (:mats) 2012-10-24 19:54:32 PDT
Created attachment 674951 [details] [diff] [review]
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 (:mats) 2012-11-04 21:45:49 PST
It was more complicated than I first thought but I think I have a fix now...
Comment 9 Mats Palmgren (:mats) 2012-11-07 09:11:24 PST
Created attachment 679184 [details] [diff] [review]
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 2012-11-07 15:49:28 PST
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] 2012-11-08 23:28:49 PST
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) (away until October 25) 2012-11-10 10:29:43 PST
(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 (:mats) 2012-11-11 23:35:37 PST
Created attachment 680539 [details] [diff] [review]
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 (:mats) 2012-11-11 23:36:27 PST
(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 (:mats) 2012-11-11 23:39:12 PST
Created attachment 680540 [details]
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) (away until October 25) 2012-11-12 03:48:18 PST
(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] 2012-11-12 07:06:48 PST
Mats, could you answer to Aryeh? We can't change behavior to be different than what
everybody else have.
Comment 18 Olli Pettay [:smaug] 2012-11-12 07:09:23 PST
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 (:mats) 2012-11-14 06:03:30 PST
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 (:mats) 2012-11-14 06:05:08 PST
Created attachment 681461 [details]
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 (:mats) 2012-11-14 06:07:03 PST
(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 (:mats) 2012-11-14 06:09:03 PST
Created attachment 681463 [details]
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 (:mats) 2012-11-14 06:12:32 PST
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) (away until October 25) 2012-11-15 04:52:47 PST
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 (:mats) 2012-11-16 03:58:01 PST
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] 2012-11-16 04:01:11 PST
Ok. Should I then perhaps review the patch? If so, ask a review :)
Comment 27 Mats Palmgren (:mats) 2012-11-17 00:44:08 PST
Comment on attachment 680539 [details] [diff] [review]
fix + test

Yes, please.
Comment 28 Olli Pettay [:smaug] 2012-11-18 12:42:38 PST
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] 2012-11-18 12:54:25 PST
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] 2012-11-18 14:16:01 PST
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] 2012-11-18 14:29:19 PST
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 (:mats) 2012-11-20 09:50:35 PST
Created attachment 683642 [details] [diff] [review]
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] 2012-11-20 10:40:13 PST
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 (:mats) 2012-11-20 12:18:29 PST
Ok, did so:
https://hg.mozilla.org/integration/mozilla-inbound/rev/88bad356f5a4
Comment 35 Ryan VanderMeulen [:RyanVM] 2012-11-20 19:01:33 PST
https://hg.mozilla.org/mozilla-central/rev/88bad356f5a4

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