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

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM: Core & HTML
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Jesse Ruderman, Assigned: jessica)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
mozilla50
assertion, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8766029 [details]
testcase

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

Comment 1

2 years ago
Created attachment 8766030 [details]
stack
Jessica, could you please take a look?
Flags: needinfo?(jjong)
(Assignee)

Updated

2 years ago
Assignee: nobody → jjong
Flags: needinfo?(jjong)
(It would be nice to submit this test case to wpt, rather than just landing the crashtest.)
(Assignee)

Comment 4

2 years ago
Created attachment 8772678 [details] [diff] [review]
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.
(Assignee)

Comment 5

2 years ago
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
(Assignee)

Updated

2 years ago
Attachment #8772678 - Flags: review?(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.
Flags: needinfo?(jjong)
(Assignee)

Comment 7

2 years ago
(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
(Assignee)

Comment 8

2 years ago
(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
Flags: needinfo?(jjong)
(Assignee)

Comment 9

2 years ago
Created attachment 8773612 [details] [diff] [review]
patch, v2.

Propagating the error on setting HTMLTableElement.caption and added corresponding wpt.
Attachment #8772678 - Attachment is obsolete: true
Attachment #8772678 - Flags: review?(bzbarsky)
Attachment #8773612 - Flags: review?(bzbarsky)
Comment on attachment 8773612 [details] [diff] [review]
patch, v2.

r=me.  Thank you!
Attachment #8773612 - Flags: review?(bzbarsky) → review+
philipj, FYI:

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

Comment 12

2 years ago
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.
Note that the spec now says to throw.

Comment 14

2 years ago
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!
(Assignee)

Comment 15

2 years ago
(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 17

2 years ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b510c6b911a
Propagate exception on setting HTMLTableElement.caption. r=bz
Keywords: checkin-needed

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8b510c6b911a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.