1406297 - Document.createElement must report an exception which is user defined thrown by a custom element constructor

Status: RESOLVED FIXED

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

Description

[John Dai[:jdai]]

After fixed Document-createElement.html[1] error, I got a test failure[2] which is document.createElement must report an exception thrown by a custom element constructor. 

[1] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/testing/web-platform/tests/custom-elements/Document-createElement.html#11

[2] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/testing/web-platform/tests/custom-elements/Document-createElement.html#305-328

Comment 1

[John Dai[:jdai]]

(In reply to John Dai[:jdai] from comment #0)
> After fixed Document-createElement.html[1][3] error, I got a test failure[2]
> which is document.createElement must report an exception thrown by a custom
> element constructor. 
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/testing/web-platform/tests/custom-
> elements/Document-createElement.html#11
> 
web-platform-tests bug: https://github.com/w3c/web-platform-tests/issues/7600

> [2]
> http://searchfox.org/mozilla-central/rev/
> 8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/testing/web-platform/tests/custom-
> elements/Document-createElement.html#305-328

Per offline discussion with Edgar, we can either
1) Move UNWRAP_OBJECT[1] out of CustomElementConstructor::Construct, and make upgrade and createElement throw different error. Per spec, upgrade should throw "InvalidStateError"[2] and createElement should throw "TypeError"[3].
2) Make UNWRAP_OBJECT return nullptr, then check nullptr for upgrade or createElement in order to throw different error.

[1] http://searchfox.org/mozilla-central/rev/8efd128b48cdae3a6c6862795ce618aa1ca1c0b4/dom/base/CustomElementRegistry.cpp#118-122
[2] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades(step 7.2)
[3] https://dom.spec.whatwg.org/#concept-create-element(step 6.1.3)

Comment 2

[John Dai[:jdai]]

Attachment: patch, v1

Comment 3

[John Dai[:jdai]]

Attachment: patch, v2

- Make UNWRAP_OBJECT return nullptr, then check nullptr for upgrade or createElement in order to throw different error.
- Implement create an element[1] step 6.1.9.
- Update WPT.

[1] https://dom.spec.whatwg.org/#concept-create-element

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

Comment 4

[John Dai[:jdai]]

We still remains 3 errors at Document-createElement.html, the main cause comes when we adopt/append custom element into iframe[1]. Chrome and Safari pass this tests. The issue had been discussed at https://bugzilla.mozilla.org/show_bug.cgi?id=1081039#c9

[1] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/testing/web-platform/tests/custom-elements/Document-createElement.html#243,262,284

Comment 5

[Edgar Chen [:edgar]]

Comment on attachment 8917269 [details] [diff] [review]
patch, v2

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

::: dom/base/CustomElementRegistry.cpp
@@ -113,3 @@
>    JS::Rooted<JS::Value> constructor(cx, JS::ObjectValue(*mCallback));
>    if (!JS::Construct(cx, constructor, JS::HandleValueArray::empty(), &result)) {
>      aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);

This line is redundant, if there is an exception on JSContext, ~CallSetup will move the exception from the JSContext to aRv and overwrite aRv to NS_ERROR_INTERNAL_ERRORRESULT_JS_EXCEPTION.

::: testing/web-platform/meta/custom-elements/Document-createElement.html.ini
@@ +1,4 @@
> +[Document-createElement.html]
> +  type: testharness
> +  [document.createElement must report a NotSupportedError when the element is adopted into a the document of an iframe during construction]
> +    expected: FAIL

It seems to me that we don't need to update this .ini here, it should be handled when we merge the wpt upstream changes which include https://github.com/w3c/web-platform-tests/issues/7600 to our codebase.

Comment 6

[John Dai[:jdai]]

Attachment: patch, v3

- Address comment 5.
- Use nsAtom to compare local name.

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

Comment 7

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

Comment on attachment 8917325 [details] [diff] [review]
patch, v3

>     // Step 6.1.
>     if (synchronousCustomElements) {
>       DoCustomElementCreate(aResult, nodeInfo->GetDocument(),
>+                            nodeInfo->NameAtom(),
>                             definition->mConstructor, rv);
>       if (rv.MaybeSetPendingException(cx)) {
>         NS_IF_ADDREF(*aResult = NS_NewHTMLUnknownElement(nodeInfo.forget(), aFromParser));
>       }
>       return NS_OK;
>     }
So what happens to the exception here? Do we report the error per spec? Is there a test for this.
Assuming this works per spec[1], and that we have a test, r+


[1] https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception

Comment 8

[Edgar Chen [:edgar]]

(In reply to Olli Pettay [:smaug] from comment #7)
> So what happens to the exception here? Do we report the error per spec? Is
> there a test for this.

The exception here will be reported when AutoEntryScript [1] come off the stack.
There are some tests in http://searchfox.org/mozilla-central/source/testing/web-platform/tests/custom-elements/Document-createElement.html, but they are busted and fixed in upstream, see https://github.com/w3c/web-platform-tests/issues/7600.

[1] http://searchfox.org/mozilla-central/rev/1a4a26905f923458679a59a4be1e455ebc53c333/dom/html/nsHTMLContentSink.cpp#295

Comment 9

[Edgar Chen [:edgar]]

Comment on attachment 8917325 [details] [diff] [review]
patch, v3

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

::: dom/html/nsHTMLContentSink.cpp
@@ +321,1 @@
>          aes.ReportException();

Since we are here, per comment #7, manually calling ReportException() seems unnecessary.

Comment 10

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #9)
> Comment on attachment 8917325 [details] [diff] [review]
> patch, v3
> 
> Review of attachment 8917325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/html/nsHTMLContentSink.cpp
> @@ +321,1 @@
> >          aes.ReportException();
> 
> Since we are here, per comment #7, manually calling ReportException() seems
                         ^^^^^^^^^^
                    Err, comment #8
> unnecessary.

Comment 11

[John Dai[:jdai]]

Attachment: patch, v4

Address comment 10.

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

Comment 12

[John Dai[:jdai]]

(In reply to Edgar Chen [:edgar] from comment #9)
> Comment on attachment 8917325 [details] [diff] [review]
> patch, v3
> 
> Review of attachment 8917325 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/html/nsHTMLContentSink.cpp
> @@ +321,1 @@
> >          aes.ReportException();
> 
> Since we are here, per comment #7, manually calling ReportException() seems
> unnecessary.

It's necessary to keep aes.ReportException(), otherwise it'll have build error on try. We can't use MOZ_ALWAYS_TRUE(rv.MaybeSetPendingException(cx)), it'll break test_custom_element_clone_callbacks_extended.html[1] when creating custom element.


[1] http://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/dom/tests/mochitest/webcomponents/test_custom_element_clone_callbacks_extended.html#31

Comment 13

[John Dai[:jdai]]

Attachment: patch, v5

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

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b47e0b4d65a9
Fix Document.createElement must report an exception. r=smaug

Comment 15

[Boris Zbarsky [:bzbarsky]]

The pattern of "just ignore a falsy return value from SpiderMonkey" used in these patches is not great... I'll follow up in bug 1407669.

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b47e0b4d65a9