Bug 183586 - Custom Elements: connectedCallback is invoked at unexpected timing
Summary: Custom Elements: connectedCallback is invoked at unexpected timing
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 187317
Blocks: 154907
  Show dependency treegraph
 
Reported: 2018-03-12 19:28 PDT by Kent Tamura
Modified: 2018-07-04 12:34 PDT (History)
12 users (show)

See Also:


Attachments
Repro (970 bytes, text/html)
2018-03-12 19:28 PDT, Kent Tamura
no flags Details
Patch (5.39 KB, patch)
2018-05-22 05:44 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews116 for mac-sierra (3.80 MB, application/zip)
2018-05-22 07:39 PDT, Build Bot
no flags Details
Patch (8.40 KB, patch)
2018-05-22 12:25 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.08 KB, patch)
2018-05-23 12:38 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.85 MB, application/zip)
2018-05-23 15:10 PDT, Build Bot
no flags Details
Patch (8.38 KB, patch)
2018-05-24 06:28 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (8.42 KB, patch)
2018-05-24 11:32 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (9.00 KB, patch)
2018-05-25 05:27 PDT, Rob Buis
rniwa: review-
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.84 MB, application/zip)
2018-05-25 07:07 PDT, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2018-03-12 19:28:53 PDT
Created attachment 335672 [details]
Repro

Load the following HTML:

<body>
<script type="application/javascript">
class Parenter extends HTMLElement {
  connectedCallback() {
    console.group('Parenter.connectedCallback');
    const child = this.firstChild;
    this.removeChild(child);
    console.groupEnd('Parenter.connectedCallback');

    this.appendChild(child);    
  }
}
customElements.define('x-parenter', Parenter);

class Child extends HTMLElement {
  connectedCallback() {
    const connected = this.isConnected;
    console.log('x-child connected '+connected);

    if (!connected) {
      const h1 = document.createElement('h1');
      h1.textContent = 'connectedCallback called when not connected!';
      document.body.appendChild(h1)
    }
  }
}
customElements.define('x-child', Child);

document.body.innerHTML = '<x-parenter><x-child></x-child></x-parenter>';
</script>
</body>


Chrome and Firefox Nightly show nothing, Safari TP 50 shows "conenctedCallback called when not connected!"
It seems "this.removeChild(child);" in Parenter connectedCallback unexpectedly invokes x-child's reactions.


Originally reported to crbug.com: crbug.com/821195
Comment 1 Radar WebKit Bug Importer 2018-03-12 20:53:20 PDT
<rdar://problem/38403504>
Comment 2 Rob Buis 2018-05-22 05:44:24 PDT
Created attachment 340977 [details]
Patch
Comment 3 Build Bot 2018-05-22 07:39:49 PDT
Comment on attachment 340977 [details]
Patch

Attachment 340977 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7764796

New failing tests:
imported/w3c/web-platform-tests/custom-elements/reactions/HTMLTableElement.html
fast/custom-elements/connected-callback.html
imported/w3c/web-platform-tests/custom-elements/reactions/Element.html
imported/w3c/web-platform-tests/custom-elements/reactions/HTMLAnchorElement.html
imported/w3c/web-platform-tests/custom-elements/reactions/HTMLTableSectionElement.html
imported/w3c/web-platform-tests/custom-elements/reactions/Document.html
imported/w3c/web-platform-tests/custom-elements/reactions/HTMLTableRowElement.html
imported/w3c/web-platform-tests/custom-elements/reactions/ShadowRoot.html
imported/w3c/web-platform-tests/custom-elements/reactions/HTMLOutputElement.html
imported/w3c/web-platform-tests/custom-elements/parser/parser-uses-registry-of-owner-document.html
fast/custom-elements/backup-element-queue.html
Comment 4 Build Bot 2018-05-22 07:39:50 PDT
Created attachment 340981 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 5 Rob Buis 2018-05-22 12:25:37 PDT
Created attachment 341004 [details]
Patch
Comment 6 Ryosuke Niwa 2018-05-22 13:57:15 PDT
Comment on attachment 341004 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341004&action=review

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:204
> +            // 6.2.1. Set result to a new element that implements the HTMLElement interface, with no attributes, namespace set to the HTML namespace,

No need to have these comments. The above link to the spec is sufficient.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:205
> +            // namespace prefix set to prefix, local name set to localName, custom element state set to "undefined", and node document set to document.

Please remove.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:206
> +            auto newElement = HTMLElement::create(QualifiedName(nullAtom(), constructionData->name, HTMLNames::xhtmlNamespaceURI), *document());

Please assert Document::validateCustomElementName(constructionData->name) here.

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:207
> +            // 6.2.2. Enqueue a custom element upgrade reaction given result and definition.

Ditto.

> LayoutTests/imported/w3c/ChangeLog:8
> +        * web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing.html: Added.

Don't add a test to LayoutTests/imported/w3c. Add it to fast/custom-elements instead.
r- because adding a test test to LayoutTests/imported/ is forbidden.
Comment 7 Chris Dumez 2018-05-22 14:03:09 PDT
Comment on attachment 341004 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341004&action=review

>> LayoutTests/imported/w3c/ChangeLog:8
>> +        * web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing.html: Added.
> 
> Don't add a test to LayoutTests/imported/w3c. Add it to fast/custom-elements instead.
> r- because adding a test test to LayoutTests/imported/ is forbidden.

+Youenn because I thought that's how we upstreamed WPT tests now.
Comment 8 Frédéric Wang (:fredw) 2018-05-22 14:06:07 PDT
(In reply to Ryosuke Niwa from comment #6)
> > LayoutTests/imported/w3c/ChangeLog:8
> > +        * web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing.html: Added.
> 
> Don't add a test to LayoutTests/imported/w3c. Add it to fast/custom-elements
> instead.
> r- because adding a test test to LayoutTests/imported/ is forbidden.

@Youenn: In the past you mentioned you were working on a script to allow porting to the WPT repo new tests from patches reviewed in WebKit. Can you explain the status of this and the actual process? Or should we still first submit tests to the WPT repo before landing patches into WebKit?
Comment 9 youenn fablet 2018-05-22 14:06:36 PDT
(In reply to Chris Dumez from comment #7)
> Comment on attachment 341004 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=341004&action=review
> 
> >> LayoutTests/imported/w3c/ChangeLog:8
> >> +        * web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing.html: Added.
> > 
> > Don't add a test to LayoutTests/imported/w3c. Add it to fast/custom-elements instead.
> > r- because adding a test test to LayoutTests/imported/ is forbidden.
> 
> +Youenn because I thought that's how we upstreamed WPT tests now.

We have done the following workflow:
- Do a patch which add or update WPT tests
- Make a corresponding PR on WPT GitHub and add a link to it in bugzilla
- Make the usual review process in WebKit (and/or WPT)
- Merge the WPT GitHub PR
- Land the WebKit patch

The alternative is to make the GitHub PR first and then reimport the WPT tests as part of the WebKit patch or as a prepunch.
Comment 10 youenn fablet 2018-05-22 14:08:13 PDT
@fwang, Tools/Scripts/export-w3c-test-changes export the changes to a WPT clone on GitHub. It can also create the PR and add the link in bugzilla.
Comment 11 Rob Buis 2018-05-23 12:38:33 PDT
Created attachment 341117 [details]
Patch
Comment 12 Rob Buis 2018-05-23 12:40:00 PDT
The latest uploaded patch tries to address the review comments.
I am trying to follow Youenn's steps, so I created a GitHub PR:
https://github.com/w3c/web-platform-tests/pull/11131
Comment 13 Build Bot 2018-05-23 15:10:45 PDT
Comment on attachment 341117 [details]
Patch

Attachment 341117 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7781015

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 14 Build Bot 2018-05-23 15:10:56 PDT
Created attachment 341135 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 15 Rob Buis 2018-05-24 06:28:34 PDT
Created attachment 341186 [details]
Patch
Comment 16 Rob Buis 2018-05-24 06:39:27 PDT
I updated the test in the mentioned PR and also put the same test in the latest Patch.
Comment 17 Rob Buis 2018-05-24 11:32:14 PDT
Created attachment 341211 [details]
Patch
Comment 18 Rob Buis 2018-05-25 05:27:23 PDT
Created attachment 341272 [details]
Patch
Comment 19 Daniel Bates 2018-05-25 06:37:00 PDT
Comment on attachment 341272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341272&action=review

Looks sane to me.

r=me

> Source/WebCore/html/parser/HTMLDocumentParser.cpp:210
> +            m_treeBuilder->didCreateCustomOrCallbackElement(WTFMove(newElement), *constructionData);

This patch is ok as-is. We always execute this line. I suggest we move this line to be above the return (line 212) instead of duplicating it in both branches.
Comment 20 Build Bot 2018-05-25 07:07:25 PDT
Comment on attachment 341272 [details]
Patch

Attachment 341272 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7799201

New failing tests:
http/tests/security/local-video-source-from-remote.html
Comment 21 Build Bot 2018-05-25 07:07:36 PDT
Created attachment 341278 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 22 Ryosuke Niwa 2018-05-25 20:11:05 PDT
Comment on attachment 341272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341272&action=review

> Source/WebCore/dom/Element.cpp:1981
> +    if (!data.customElementReactionQueue()) {
> +        data.setCustomElementReactionQueue(std::make_unique<CustomElementReactionQueue>(elementInterface));

r- because of this change.
Comment 23 Rob Buis 2018-05-30 08:05:42 PDT
> r- because of this change.

Can you explain a bit more? What would be the better approach? TIA!
Comment 24 Ryosuke Niwa 2018-05-30 13:57:59 PDT
(In reply to Rob Buis from comment #23)
> > r- because of this change.
> 
> Can you explain a bit more? What would be the better approach? TIA!

We should avoid calling enqueueToUpgrade more than once on an element. That's an important invariant.
Comment 25 Frédéric Wang (:fredw) 2018-07-04 12:34:38 PDT
Comment on attachment 341272 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341272&action=review

> Source/WebCore/ChangeLog:11
> +        follow step 6.2.

Maybe we should mention that "Particularly in HTML fragment parsing mode synchronous custom elements flag should be unset" refers to steps 5 and 7 of [1].

I also wonder about steps 6 and 9 of [1], especially pushing element queues into the custom element reactions stack and then popping + invoking custom element reactions. It's not obvious to me whether we are already managing such custom element reactions stack of element queues and I wonder how skipping these steps in HTML fragment parsing mode will affect the present bug. @Ryosuke: maybe you have any opinion on this?

>> Source/WebCore/dom/Element.cpp:1981
>> +        data.setCustomElementReactionQueue(std::make_unique<CustomElementReactionQueue>(elementInterface));
> 
> r- because of this change.

@Rob: It seems the reason for this hack is because Element::enqueueToUpgrade is called later by CustomElementReactionQueue::enqueueElementUpgradeIfDefined, right? However by 6.2 of [2], the HTMLElement is supposed to have state "undefined" so there is indeed something wrong...

>> Source/WebCore/html/parser/HTMLDocumentParser.cpp:210
>> +            m_treeBuilder->didCreateCustomOrCallbackElement(WTFMove(newElement), *constructionData);
> 
> This patch is ok as-is. We always execute this line. I suggest we move this line to be above the return (line 212) instead of duplicating it in both branches.

I think the fact that newElement is a Ref might not make this convenient, maybe we need a new helper function to retrieve newElement. Anyway, I suspect the final code will not look like this as the spec's algorithm seems a bit more complex.

> LayoutTests/imported/w3c/ChangeLog:8
> +        * web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing.html: Added.

Reminder: update it at the next patch iteration since this file has changed a bit in upstream WPT.