1407669 - Custom element creation hides exceptions from the constructor

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

In DoCustomElementCreate in nsHTMLContentSink.cpp we have:

  RefPtr<Element> element =
    aConstructor->Construct("Custom Element Create", aRv);
  if (aRv.Failed() || !element->IsHTMLElement()) {
    aRv.ThrowTypeError<MSG_THIS_DOES_NOT_IMPLEMENT_INTERFACE>(NS_LITERAL_STRING("HTMLElement"));
    return;
  }

So if the constructor throws, we will go ahead and clobber that that exception with a MSG_THIS_DOES_NOT_IMPLEMENT_INTERFACE TypeError.  This is not terribly helpful.

Oh, and nothing will have reported the JS exception before now, because CustomElementConstructor::Construct uses eRethrowExceptions on its CallSetup.

Why are we hiding these exceptions?  It makes debugging super-painful.  For example, if I have this:

  class Foo extends HTMLElement {
    constructor() {
      super()
      someUnknownFunction();
    }
  }

  customElements.define("test-custom-element", Foo);
  document.createElement("test-custom-element");

the error we report in the console is:

  TypeError: "this" object does not implement interface HTMLElement.

pointing to the createElement line.  Chrome reports the much more useful:

  Uncaught ReferenceError: someUnknownFunction is not defined

While we're here, by the way, the code in CustomElementConstructor::Construct is also wrong.  It does this:

  if (!JS::Construct(cx, constructor, JS::HandleValueArray::empty(), &result)) {
    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
    return nullptr;
  }

while it should do aRv.NoteJSContextException(cx) instead.  As things stand, this code will suppress uncatchable exceptions and turn them into catchable ones, which seems wrong.

Of course even if we fix that, we'll still report-and-suppress the exception in NS_NewHTMLElement.  That's arguably wrong; if the exception is uncatchable we should consider propagating it on out, maybe... Hard to say.

Comment 1

[Adrian Aichner [:anaran]]

What a coincidence! I think I reported a test case for this in Bug 1407576 earlier today.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Yes, that looks like exactly the same issue.

Comment 3

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #0)
> In DoCustomElementCreate in nsHTMLContentSink.cpp we have:
> 
>   RefPtr<Element> element =
>     aConstructor->Construct("Custom Element Create", aRv);
>   if (aRv.Failed() || !element->IsHTMLElement()) {
>    
> aRv.
> ThrowTypeError<MSG_THIS_DOES_NOT_IMPLEMENT_INTERFACE>(NS_LITERAL_STRING("HTML
> Element"));
>     return;
>   }
> 
> So if the constructor throws, we will go ahead and clobber that that
> exception with a MSG_THIS_DOES_NOT_IMPLEMENT_INTERFACE TypeError.  This is
> not terribly helpful.
> 
Yes, you are right. I am fixing this issue at Bug 1406297.

> Oh, and nothing will have reported the JS exception before now, because
> CustomElementConstructor::Construct uses eRethrowExceptions on its CallSetup.
> 
> Why are we hiding these exceptions?  It makes debugging super-painful.  For
> example, if I have this:
> 
>   class Foo extends HTMLElement {
>     constructor() {
>       super()
>       someUnknownFunction();
>     }
>   }
> 
>   customElements.define("test-custom-element", Foo);
>   document.createElement("test-custom-element");
> 
> the error we report in the console is:
> 
>   TypeError: "this" object does not implement interface HTMLElement.
> 
> pointing to the createElement line.  Chrome reports the much more useful:
> 
>   Uncaught ReferenceError: someUnknownFunction is not defined
> 
> While we're here, by the way, the code in
> CustomElementConstructor::Construct is also wrong.  It does this:
> 
>   if (!JS::Construct(cx, constructor, JS::HandleValueArray::empty(),
> &result)) {
>     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>     return nullptr;
>   }
> 
> while it should do aRv.NoteJSContextException(cx) instead.  As things stand,
> this code will suppress uncatchable exceptions and turn them into catchable
> ones, which seems wrong.
> 
aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR); should be removed. I am not sure it should do aRv.NoteJSContextException(cx) instead, because CallSetup() dtor will handle exception for us. 

> Of course even if we fix that, we'll still report-and-suppress the exception
> in NS_NewHTMLElement.  That's arguably wrong; if the exception is
> uncatchable we should consider propagating it on out, maybe... Hard to say.
It'll also be fixed at bug 1406297. Thank you.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> Yes, you are right. I am fixing this issue at Bug 1406297.

Yes, looks good.

> I am not sure it should do aRv.NoteJSContextException(cx) instead,
> because CallSetup() dtor will handle exception for us. 

Not really.

Look, there are three possible outcomes to making a fallible SpiderMonkey API call:

1)  Everything is fine, press on.
2)  Exception is thrown on the JSContext, false/null is returned.  In this case you
    need to make sure to stop whatever you're doing and propagate the exception
    out, or report it and press on (corresponding to uncaught and
    caught+reported exceptions).
3)  Exception _not_ thrown on the JSContext, false/null is returned.  This is what happens
    when the user does the "stop script" thing on infinite-looping scripts, for example.
    Conceptually, this is an "uncatchable exception".  In this situation, in my opinion,
    you must make sure to stop whatever you're doing and unwind out past all page code.
    In other words, the exception is uncatchable, so "catch+report" is not the
    right thing to do.

The CallSetup destructor will handle case 2: it will see an exception on the JSContext and either propagate it out on the ErrorResult or report it, depending on what you told the CallSetup constructor.

The CallSetup destructor will NOT HANDLE CASE 3, because it has no way to know that false was returned somewhere in there.  There is no ambient state that says an uncatchable exception was thrown.  Yes, this is a little dumb, and I just filed bug 1408013 on that, but that's how it works.

This is the situation NoteJSContextException() is meant to address.  It will check whether an exception was thrown and ensure that the ErrorResult is placed in the right error state, including an indication that an uncatchable exception was thrown, if needed.

So in practice, there are two possible API contracts for functions that have a JSContext they are working with:

1)  The SpiderMonkey contract: Use passed-in JSContext, return false/null to indicate failure.  Caller can tell whether the failure was catchable or not by examining the JSContext and act accordingly.

2)  The DOM contract: Use local or passed-in JSContext, use an ErrorResult to indicate failure.  Caller can examine the ErrorResult to see what's going on.

A function in bucket 2 that calls a function in bucket 1 _MUST_ call NoteJSContextException() to make sure that information is not lost, and that in particular the return value information is properly propagated to the ErrorResult.

> It'll also be fixed at bug 1406297. 

No, it won't.  The code checked in for that bug totally loses uncatchable exceptions, converting them into NS_ERROR_DOM_INVALID_STATE_ERR DOMExceptions in DoUpgrade and into MSG_THIS_DOES_NOT_IMPLEMENT_INTERFACE TypeErrors (why the inconsistency?) in DoCustomElementCreate, because Construct() doesn't set an error state on aRv if JS::Construct fails.

Comment 5

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> > Yes, you are right. I am fixing this issue at Bug 1406297.
> 
> Yes, looks good.
> 
> > I am not sure it should do aRv.NoteJSContextException(cx) instead,
> > because CallSetup() dtor will handle exception for us. 
> 
> Not really.
> 
> Look, there are three possible outcomes to making a fallible SpiderMonkey
> API call:
> 
> 1)  Everything is fine, press on.
> 2)  Exception is thrown on the JSContext, false/null is returned.  In this
> case you
>     need to make sure to stop whatever you're doing and propagate the
> exception
>     out, or report it and press on (corresponding to uncaught and
>     caught+reported exceptions).
> 3)  Exception _not_ thrown on the JSContext, false/null is returned.  This
> is what happens
>     when the user does the "stop script" thing on infinite-looping scripts,
> for example.
>     Conceptually, this is an "uncatchable exception".  In this situation, in
> my opinion,
>     you must make sure to stop whatever you're doing and unwind out past all
> page code.
>     In other words, the exception is uncatchable, so "catch+report" is not
> the
>     right thing to do.
> 
> The CallSetup destructor will handle case 2: it will see an exception on the
> JSContext and either propagate it out on the ErrorResult or report it,
> depending on what you told the CallSetup constructor.
> 
> The CallSetup destructor will NOT HANDLE CASE 3, because it has no way to
> know that false was returned somewhere in there.  There is no ambient state
> that says an uncatchable exception was thrown.  Yes, this is a little dumb,
> and I just filed bug 1408013 on that, but that's how it works.
> 
> This is the situation NoteJSContextException() is meant to address.  It will
> check whether an exception was thrown and ensure that the ErrorResult is
> placed in the right error state, including an indication that an uncatchable
> exception was thrown, if needed.
> 
Will do.

> So in practice, there are two possible API contracts for functions that have
> a JSContext they are working with:
> 
> 1)  The SpiderMonkey contract: Use passed-in JSContext, return false/null to
> indicate failure.  Caller can tell whether the failure was catchable or not
> by examining the JSContext and act accordingly.
> 
> 2)  The DOM contract: Use local or passed-in JSContext, use an ErrorResult
> to indicate failure.  Caller can examine the ErrorResult to see what's going
> on.
> 
> A function in bucket 2 that calls a function in bucket 1 _MUST_ call
> NoteJSContextException() to make sure that information is not lost, and that
> in particular the return value information is properly propagated to the
> ErrorResult.
> 
> > It'll also be fixed at bug 1406297. 
> 
> No, it won't.  The code checked in for that bug totally loses uncatchable
> exceptions, converting them into NS_ERROR_DOM_INVALID_STATE_ERR
> DOMExceptions in DoUpgrade and into MSG_THIS_DOES_NOT_IMPLEMENT_INTERFACE
> TypeErrors (why the inconsistency?) 
Per spec, upgrade should throw "InvalidStateError"[1], and createElement should throw "TypeError"[2]. I guess it is because createElement is a web IDL API and it has defined exceptions[3], but upgrade is part of internal steps, hence, it throws InvalidStateError.

[1] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades (step 8.2)
[2] https://dom.spec.whatwg.org/#concept-create-element (step 6.1.3)
[3] https://heycam.github.io/webidl/#idl-exceptions

> in DoCustomElementCreate, because
> Construct() doesn't set an error state on aRv if JS::Construct fails.

Comment 6

[John Dai[:jdai]]

Attachment: Bug 1407669 - Fix custom element creation hides uncatchable exceptions from the constructor. v1

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f4f7b2a6cffa0549f1c4bc605b58f7b0b0b9265&filter-tier=1&group_state=expanded

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8928946 [details] [diff] [review]
Bug 1407669 - Fix custom element creation hides uncatchable exceptions from the constructor. v1

Review of attachment 8928946 [details] [diff] [review]:
-----------------------------------------------------------------

Could you also add some test cases? Thank you.

Comment 8

[John Dai[:jdai]]

Attachment: Bug 1407669 - Fix custom element creation hides uncatchable exceptions from the constructor. v2

Add testcase.

Comment 9

[John Dai[:jdai]]

Attachment: Bug 1407669 - Fix custom element creation hides uncatchable exceptions from the constructor. v3

Revise testcase.

Comment 10

[Edgar Chen [:edgar]]

Comment on attachment 8930370 [details] [diff] [review]
Bug 1407669 - Fix custom element creation hides uncatchable exceptions from the constructor. v3

Review of attachment 8930370 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/tests/mochitest/webcomponents/mochitest.ini
@@ +24,5 @@
>  [test_custom_element_upgrade.html]
>  support-files =
>    test_upgrade_page.html
>    upgrade_tests.js
> +[test_custom_element_runtime_exception.html]

s/runtime_exception/uncatchable_exception

::: dom/tests/mochitest/webcomponents/test_custom_element_runtime_exception.html
@@ +20,5 @@
> +
> +  class Foo extends HTMLElement {
> +    constructor() {
> +      super()
> +      TestFunctions.throwUncatchableException();

TestFunctions is exposed only on debug build; you need to skip this test on the non-debug build in .ini.

@@ +25,5 @@
> +    }
> +  }
> +
> +  customElements.define("test-custom-element", Foo);
> +  document.createElement("test-custom-element");

Please add a check to ensure the created element should be a HTMLUnknownElement.

Comment 11

[John Dai[:jdai]]

Attachment: Bug 1407669 - Fix custom element creation hides uncatchable exceptions from the constructor. v4

Use NoteJSContextException() to handle uncatchable exceptions in constructor.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=57ec626f26275a144fea12a90df2d6336ef089c2&filter-tier=1&group_state=expanded&selectedJob=146670403

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8930796 [details] [diff] [review]
Bug 1407669 - Fix custom element creation hides uncatchable exceptions from the constructor. v4

r=me.  I assume the new test does fail before the patch?

Comment 13

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #12)
> Comment on attachment 8930796 [details] [diff] [review]
> Bug 1407669 - Fix custom element creation hides uncatchable exceptions from
> the constructor. v4
> 
> r=me.  I assume the new test does fail before the patch?

Yes, it'll show error message and fail before the patch.

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

Comment on attachment 8930796 [details] [diff] [review]
Bug 1407669 - Fix custom element creation hides uncatchable exceptions from the constructor. v4

Please update the patch, because there are conflicts when trying to land it.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e5b62d139a8
Fix custom element creation hides uncatchable exceptions from the constructor. f=echen, r=bz

Comment 16

[John Dai[:jdai]]

Attachment: Bug 1407669 - Fix custom element creation hides uncatchable exceptions from the constructor. f=echen, r=bz

Rebase.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bcb94e993ca50571038ca5ec4d5fe36ea38b738&group_state=expanded&filter-tier=1

Comment 17

[Edgar Chen [:edgar]]

The path has been merged per comment 15.

Comment 18

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4e5b62d139a8