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

NEW
Unassigned

Status

()

P3
normal
a year ago
10 months ago

People

(Reporter: jkratzer, Unassigned)

Tracking

(Blocks: 1 bug, {assertion, testcase})

unspecified
assertion, testcase
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox56 wontfix, firefox57 wontfix, firefox58 wontfix, firefox59 ?)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Created attachment 8905256 [details]
trigger.html

Testcase found while fuzzing mozilla-central rev 20170905-3ecda4678c49.
Flags: in-testsuite?
(Reporter)

Comment 1

a year ago
Created attachment 8905257 [details]
stacktrace.txt
Hi Jessica, could you please take a look ?
Flags: needinfo?(jjong)
Summary: Assertion failure: !rv.Failed() → Assertion failure: !rv.Failed() at html/HTMLTableElement.cpp:727
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #2)
> Hi Jessica, could you please take a look ?

Sure.
Assignee: nobody → jjong
Flags: needinfo?(jjong)
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
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
Priority: -- → P2
(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)
Flags: needinfo?(bugs)
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
Flags: needinfo?(bugs)
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
Priority: P2 → --
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?
(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.
(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.
Priority: -- → P2
Do we know if this is a regression?
Priority: P2 → --
This looks a long standing issue.
Priority: -- → P3
Has Regression Range: --- → no
status-firefox56: --- → wontfix
status-firefox57: --- → wontfix
status-firefox58: --- → fix-optional
status-firefox-esr52: --- → wontfix
Assignee: jessi3py → nobody
You need to log in before you can comment on or make changes to this bug.