804784 - Crash with range, normalize

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

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

On Windows: bp-9056da4d-fd84-47c8-a7f3-83fd92121024.

Comment 2

[Mats Palmgren (inactive)]

Attachment: fix + test

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

Comment 3

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

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

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

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 (:ayg) (no longer with Mozilla)]

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

Anne has valid concerns in the W3 bug.

Comment 8

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

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 (:ayg) (no longer with Mozilla)]

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

Yeah, ok, perhaps those concerns don't apply here after all. This normalize case is pretty unique.

Comment 11

[Mats Palmgren (inactive)]

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

https://hg.mozilla.org/mozilla-central/rev/6ee5de769c4a