Closed Bug 1406297 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jdai, Assigned: jdai)

References

Details

(Whiteboard: dom-ce-m2)

Attachments

(1 file, 4 obsolete files)

(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)
Attached patch patch, v1 (obsolete) — Splinter Review
Priority: -- → P2
Whiteboard: dom-ce-m2
Attached patch patch, v2 (obsolete) — Splinter Review
- 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
Attachment #8916546 - Attachment is obsolete: true
Attachment #8917269 - Flags: feedback?(echen)
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 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.
Attachment #8917269 - Flags: feedback?(echen) → feedback+
Attached patch patch, v3 (obsolete) — Splinter Review
- 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
Attachment #8917269 - Attachment is obsolete: true
Attachment #8917325 - Flags: review?(bugs)
Attachment #8917325 - Flags: feedback+
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
Attachment #8917325 - Flags: review?(bugs) → review+
(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 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.
(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.
Keywords: checkin-needed
Keywords: checkin-needed
(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
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b47e0b4d65a9
Fix Document.createElement must report an exception. r=smaug
Keywords: checkin-needed
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.
https://hg.mozilla.org/mozilla-central/rev/b47e0b4d65a9
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.