Last Comment Bug 804784 - Crash with range, normalize
: Crash with range, normalize
Status: RESOLVED FIXED
: assertion, crash, testcase
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- critical (vote)
: mozilla20
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks: 336383 815065
  Show dependency treegraph
 
Reported: 2012-10-23 14:55 PDT by Jesse Ruderman
Modified: 2013-04-04 13:53 PDT (History)
8 users (show)
mats: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (crashes Firefox when loaded) (497 bytes, text/html)
2012-10-23 14:55 PDT, Jesse Ruderman
no flags Details
fix + test (7.29 KB, patch)
2012-11-07 09:12 PST, Mats Palmgren (:mats)
bugs: review-
Details | Diff | Splinter Review
fix + test (8.10 KB, patch)
2012-11-11 23:44 PST, Mats Palmgren (:mats)
bugs: review+
Details | Diff | Splinter Review

Description Jesse Ruderman 2012-10-23 14:55:00 PDT
Created attachment 674394 [details]
testcase (crashes Firefox when loaded)

###!!! ASSERTION: ContentIterator stack underflow: '!aIndexes || !aIndexes->IsEmpty()', file content/base/src/nsContentIterator.cpp, line 715

Crash [@ nsContentIterator::NextNode]

This seems like the opposite of bug 803924.
Comment 1 Scoobidiver (away) 2012-10-23 23:17:26 PDT
On Windows: bp-9056da4d-fd84-47c8-a7f3-83fd92121024.
Comment 2 Mats Palmgren (:mats) 2012-11-07 09:12:36 PST
Created attachment 679185 [details] [diff] [review]
fix + test

Green on Try:
https://tbpl.mozilla.org/?tree=Try&rev=3f92287de016
Comment 3 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-11-09 00:02:42 PST
Comment on attachment 679185 [details] [diff] [review]
fix + test

Could you explain this a bit.
I'm trying to figure out why mStartOffset > 0 and not >= 0.

Could you add some test where textnode is empty.
Comment 4 Mats Palmgren (:mats) 2012-11-11 23:44:05 PST
Created attachment 680543 [details] [diff] [review]
fix + test

(In reply to Olli Pettay [:smaug] from comment #3)
> Could you explain this a bit.
> I'm trying to figure out why mStartOffset > 0 and not >= 0.

Because a boundary point before the first child cannot possibly
be affected by a normalize(), so it's an optimization --
"removed == parentNode->GetChildAt(mStartOffset)" is always false
for mStartOffset == 0.

(I realized I could do the same in bug 803924, thanks!)

https://tbpl.mozilla.org/?tree=Try&rev=591739349e83
Comment 5 Mats Palmgren (:mats) 2012-11-11 23:44:55 PST
Aryeh, there's a similar bug in the spec for 'normalize()':
http://dom.spec.whatwg.org/#dom-node-normalize
After 7.2 you need to add something like:
* For each range whose start node is /context object/ and start offset
  equals the index of /current node/ in /context object/, set start node
  to /node/ and start offset to /length/
* ditto with s/start/end/
Comment 6 :Aryeh Gregor (working until September 2) 2012-11-12 04:01:42 PST
(In reply to Mats Palmgren [:mats] from comment #5)
> Aryeh, there's a similar bug in the spec for 'normalize()':
> http://dom.spec.whatwg.org/#dom-node-normalize
> After 7.2 you need to add something like:
> * For each range whose start node is /context object/ and start offset
>   equals the index of /current node/ in /context object/, set start node
>   to /node/ and start offset to /length/
> * ditto with s/start/end/

Ah, good point.  It's not quite right, because /current node/ doesn't have to be a child of /context object/, but it's true that we need to account for boundary points in the parent.  Sigh, ranges are way too complicated.  Filed a spec bug:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=19946

If Anne doesn't handle it soon, I will when I have the time.
Comment 7 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-11-12 07:19:02 PST
Anne has valid concerns in the W3 bug.
Comment 8 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-11-14 04:10:14 PST
Comment on attachment 680543 [details] [diff] [review]
fix + test

uh, this is tricky. I hope I got this right.
Could you add a comment why > 0 is ok.
Comment 9 :Aryeh Gregor (working until September 2) 2012-11-15 04:32:56 PST
(In reply to Olli Pettay [:smaug] from comment #7)
> Anne has valid concerns in the W3 bug.

I don't understand them, as I said there.  Maybe you could explain why they're valid?
Comment 10 Olli Pettay [:smaug] (vacation Aug 25-28) 2012-11-15 04:43:31 PST
Yeah, ok, perhaps those concerns don't apply here after all. This normalize case is pretty unique.
Comment 11 Mats Palmgren (:mats) 2012-11-20 12:16:51 PST
(In reply to Olli Pettay [:smaug] from comment #8)
> Could you add a comment why > 0 is ok.

Added comment and some more tests:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ee5de769c4a
Comment 12 Ryan VanderMeulen [:RyanVM] 2012-11-20 18:57:41 PST
https://hg.mozilla.org/mozilla-central/rev/6ee5de769c4a

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