1397463 - Assertion failure: !rv.Failed() at html/HTMLTableElement.cpp:727

Status: NEW

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

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev 20170905-3ecda4678c49.

Comment 1

[Jason Kratzer [:jkratzer]]

Attachment: stacktrace.txt

Comment 2

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Jessica, could you please take a look ?

Comment 3

[Jessica Jong [:jessica] (inactive)]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> Hi Jessica, could you please take a look ?

Sure.

Comment 4

[Jessica Jong [:jessica] (inactive)]

This happens because we fire DOMSubtreeModified before actually removing the node (tfoot) [1], and we end up with an infinite loop.

[1] http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/dom/base/nsINode.cpp#594,604

Comment 5

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

So DOMNodeRemoved might cause the same issue.

DOMSubtreeModified should fire right after firing the other mutation event, and DOMNodeRemoved is fired before actually removing the node.
https://www.w3.org/TR/DOM-Level-2-Events/events.html

Comment 6

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] (pto Sep 14-15) from comment #5)
> So DOMNodeRemoved might cause the same issue.

Yes, it has the same issue if we do o1.deleteTFoot() right after o1.createTFoot().

> 
> DOMSubtreeModified should fire right after firing the other mutation event,
> and DOMNodeRemoved is fired before actually removing the node.
> https://www.w3.org/TR/DOM-Level-2-Events/events.html

Does that mean we're doing the right thing?
(Chrome seems to fire DOMNodeRemoved, then remove the child, and then fire DOMSubtreeModified)

Comment 7

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

I'm not aware of any spec hinting Chrome's behavior would be the right thing.
MutationEvents are of course deprecated and all, and that is why they aren't spec'ed in the current DOM
https://dom.spec.whatwg.org/#goals

Comment 8

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Sorry I misunderstood the comments. This is a known long-standing problem [1]. We did the right thing per spec and not impact release, it's not a P2. 

If we do want to avoid the assertion on debug build, removing it could be an option, but I am not sure that's meaningful... We'd probably live with it and make it wontfix? 


[1] https://developer.mozilla.org/zh-TW/docs/Web/Events/DOMSubtreeModified

Comment 9

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

So why is the assertion failing? Because of http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/dom/base/nsINode.cpp#600 ?
That is a valid reason to throw exception, and if that is the case, then the assertion in HTMLTableElement is wrong.

But a question is that whether deleteTFoot and similar methods should throw too.
What do other browsers do here? Or do they not support DOMNodeRemoved (anymore), which certainly should happen before removal?

Comment 10

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] (pto Sep 14-15) from comment #9)
> So why is the assertion failing? Because of
> http://searchfox.org/mozilla-central/rev/
> 6326724982c66aaeaf70bb7c7ee170f7a38ca226/dom/base/nsINode.cpp#600 ?
> That is a valid reason to throw exception, and if that is the case, then the
> assertion in HTMLTableElement is wrong.

Yes, that's why the assertion is failing, and we assert if nsINode::RemoveChild fails here: http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/dom/html/HTMLTableElement.cpp#727

> 
> But a question is that whether deleteTFoot and similar methods should throw
> too.
> What do other browsers do here? Or do they not support DOMNodeRemoved
> (anymore), which certainly should happen before removal?

I just tested on Edge, and it behaves like Chrome, that is, works normailly with DOMSubtreeModified, but falls into infinite loop with DOMNodeRemoved.
Per the current spec, .deleteTFoot() does not throw.

Comment 11

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #10)
> (In reply to Olli Pettay [:smaug] (pto Sep 14-15) from comment #9)
> > But a question is that whether deleteTFoot and similar methods should throw
> > too.
> > What do other browsers do here? Or do they not support DOMNodeRemoved
> > (anymore), which certainly should happen before removal?
> 
> I just tested on Edge, and it behaves like Chrome, that is, works normailly
> with DOMSubtreeModified, but falls into infinite loop with DOMNodeRemoved.

And got the same result with Safari.

> Per the current spec, .deleteTFoot() does not throw.

Comment 12

[Andrew Overholt [:overholt]]

Do we know if this is a regression?

Comment 13

[Hsin-Yi Tsai (she/her) [:hsinyi]]

This looks a long standing issue.

Comment 14

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals