1282894 - "Assertion failure: !Failed()" - mozilla::dom::HTMLTableElement::SetCaption

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: !Failed(), at dist/include/mozilla/ErrorResult.h:105

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

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

Jessica, could you please take a look?

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(It would be nice to submit this test case to wpt, rather than just landing the crashtest.)

Comment 4

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

This assertion was added in Bug 1224007 part 6.

In this case, it fails in SetCaption -> AppendChild because the caption is not allowed as child due to 'host-including inclusive ancestor'. Since ErrorResult is not used originally, I changed it to IgnoredErrorResult.

Comment 5

[Jessica Jong [:jessica] (inactive)]

I am not sure if we should add the test case to wpt, since it's an assertion that we added. The spec doesn't say much about failure on setting the caption. Please correct me if I am wrong.


[1] https://html.spec.whatwg.org/multipage/tables.html#dom-table-caption

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8772678 [details] [diff] [review]
patch, v1.

The spec should definitely be clearer here (please file a spec issue).  What do other browsers do?  Do they silently no-op, or do they throw?  Because rethrowing the append failure would make a _lot_ of sense.....

Either way, there should be a wpt test here: either this operation should throw or it should not, and either way the caption should NOT end up as the child of the table, and this all should be tested in wpt.

Comment 7

[Jessica Jong [:jessica] (inactive)]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Comment on attachment 8772678 [details] [diff] [review]
> patch, v1.
> 
> The spec should definitely be clearer here (please file a spec issue).  What
> do other browsers do?  Do they silently no-op, or do they throw?  Because
> rethrowing the append failure would make a _lot_ of sense.....

Tested Chrome, and it does throw, but interestingly, it has a comment in its IDL [1] saying:
// TODO(philipj): The caption, tHead and tFoot setters should never throw. 

In our case, we only throw a TypeError when the caption is not null nor a HTMLTableCaptionElement (webidl bindings check this).

I'll file a spec issue to clarify this a bit. Thanks.

> 
> Either way, there should be a wpt test here: either this operation should
> throw or it should not, and either way the caption should NOT end up as the
> child of the table, and this all should be tested in wpt.

I see, will add the test after clarifying whether we should throw or not.

[1] https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/Source/core/html/HTMLTableElement.idl#24

Comment 8

[Jessica Jong [:jessica] (inactive)]

(In reply to Jessica Jong [:jessica] from comment #7)
> (In reply to Boris Zbarsky [:bz] from comment #6)
> > Comment on attachment 8772678 [details] [diff] [review]
> > patch, v1.
> > 
> > The spec should definitely be clearer here (please file a spec issue).  What
> > do other browsers do?  Do they silently no-op, or do they throw?  Because
> > rethrowing the append failure would make a _lot_ of sense.....
> 
> Tested Chrome, and it does throw, but interestingly, it has a comment in its
> IDL [1] saying:
> // TODO(philipj): The caption, tHead and tFoot setters should never throw. 
> 
> In our case, we only throw a TypeError when the caption is not null nor a
> HTMLTableCaptionElement (webidl bindings check this).
> 
> I'll file a spec issue to clarify this a bit. Thanks.

Filed https://github.com/whatwg/html/issues/1582

> 
> > 
> > Either way, there should be a wpt test here: either this operation should
> > throw or it should not, and either way the caption should NOT end up as the
> > child of the table, and this all should be tested in wpt.
> 
> I see, will add the test after clarifying whether we should throw or not.
> 
> [1]
> https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/
> Source/core/html/HTMLTableElement.idl#24

Comment 9

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Propagating the error on setting HTMLTableElement.caption and added corresponding wpt.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8773612 [details] [diff] [review]
patch, v2.

r=me.  Thank you!

Comment 11

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

philipj, FYI:

(In reply to Jessica Jong [:jessica] from comment #7)
> // TODO(philipj): The caption, tHead and tFoot setters should never throw.

Comment 12

[Philip Jägenstedt [:foolip]]

Thanks for pointing this out, I just added the TODO because I noticed it wasn't per spec at the time, I never investigated what would really make sense.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Note that the spec now says to throw.

Comment 14

[Philip Jägenstedt [:foolip]]

It looks like the spec hasn't changed here since I added that TODO, so I was just wrong at the time, or maybe I just looked at the definition of the caption setter. Anyway, always rethrowing makes sense thanks for filing https://github.com/whatwg/html/issues/1582 Jessica!

Comment 15

[Jessica Jong [:jessica] (inactive)]

(In reply to Philip Jägenstedt from comment #14)
> It looks like the spec hasn't changed here since I added that TODO, so I was
> just wrong at the time, or maybe I just looked at the definition of the
> caption setter. Anyway, always rethrowing makes sense thanks for filing
> https://github.com/whatwg/html/issues/1582 Jessica!

No problem. :)

Comment 16

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=959d7dc042cc

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b510c6b911a
Propagate exception on setting HTMLTableElement.caption. r=bz

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b510c6b911a