Bug 183586

Summary:

connectedCallback is invoked during the removal of the element inside another element's connectedCallback

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

DOM

Assignee:

Ryosuke Niwa <rniwa>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

cdumez, cmarcelo, dbates, dkadu, esprehn+autocc, ews-watchlist, fred.wang, gyuyoung.kim, kangil.han, rbuis, rniwa, webkit-bug-importer, youennf

Priority:

Keywords:

InRadar

Version:

Safari Technology Preview

Hardware:

Unspecified

OS:

Unspecified

See Also:

https://github.com/w3c/web-platform-tests/pull/11131
https://bugs.webkit.org/show_bug.cgi?id=184307

Bug Depends on:

187317, 187806, 188327

Bug Blocks:

154907

Attachments:

Description	Flags
Initial repro case	none
Patch	none
Archive of layout-test-results from ews116 for mac-sierra	none
Patch	none
Patch	none
Archive of layout-test-results from ews200 for win-future	none
Patch	none
Patch	none
Patch	rniwa: review-, ews-watchlist: commit-queue-
Archive of layout-test-results from ews202 for win-future	none
Patch	none
Another repro case	none
Repro case (xhtml)	none
Only repro that produces different results in Safari vs. Chrome/Firefox	none
Aligns WebKit's behavior with Chrome/Firefox	fred.wang: review+

Kent Tamura

Reported 2018-03-12 19:28:53 PDT

Created attachment 335672 [details] Initial repro case 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

Attachments
Initial repro case (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, EWS Watchlist	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, EWS Watchlist	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-watchlist: 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, EWS Watchlist	no flags	Details
Patch (7.83 KB, patch) 2018-07-23 07:27 PDT, Frédéric Wang (:fredw)	no flags	Details Formatted Diff Diff
Another repro case (1.32 KB, text/html) 2018-07-26 01:05 PDT, Frédéric Wang (:fredw)	no flags	Details
Repro case (xhtml) (1.52 KB, application/xhtml+xml) 2018-08-05 22:40 PDT, Frédéric Wang (:fredw)	no flags	Details
Only repro that produces different results in Safari vs. Chrome/Firefox (1.42 KB, text/html) 2018-08-14 19:40 PDT, Ryosuke Niwa	no flags	Details
Aligns WebKit's behavior with Chrome/Firefox (14.93 KB, patch) 2018-12-10 20:32 PST, Ryosuke Niwa	fred.wang: review+	Details Formatted Diff Diff
Show Obsolete (12) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2018-03-12 20:53:20 PDT

<rdar://problem/38403504>

Rob Buis

Comment 2 2018-05-22 05:44:24 PDT

Created attachment 340977 [details] Patch

EWS Watchlist

Comment 3 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

EWS Watchlist

Comment 4 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

Rob Buis

Comment 5 2018-05-22 12:25:37 PDT

Created attachment 341004 [details] Patch

Ryosuke Niwa

Comment 6 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.

Chris Dumez

Comment 7 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.

Frédéric Wang (:fredw)

Comment 8 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?

youenn fablet

Comment 9 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.

youenn fablet

Comment 10 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.

Rob Buis

Comment 11 2018-05-23 12:38:33 PDT

Created attachment 341117 [details] Patch

Rob Buis

Comment 12 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

EWS Watchlist

Comment 13 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

EWS Watchlist

Comment 14 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

Rob Buis

Comment 15 2018-05-24 06:28:34 PDT

Created attachment 341186 [details] Patch

Rob Buis

Comment 16 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.

Rob Buis

Comment 17 2018-05-24 11:32:14 PDT

Created attachment 341211 [details] Patch

Rob Buis

Comment 18 2018-05-25 05:27:23 PDT

Created attachment 341272 [details] Patch

Daniel Bates

Comment 19 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.

EWS Watchlist

Comment 20 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

EWS Watchlist

Comment 21 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

Ryosuke Niwa

Comment 22 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.

Rob Buis

Comment 23 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!

Ryosuke Niwa

Comment 24 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.

Frédéric Wang (:fredw)

Comment 25 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.

Frédéric Wang (:fredw)

Comment 26 2018-07-23 07:27:21 PDT

Created attachment 345570 [details] Patch Just rebasing Rob's patch...

Frédéric Wang (:fredw)

Comment 27 2018-07-23 09:15:10 PDT

(In reply to Frédéric Wang (:fredw) from comment #25) > 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? So replying to myself, I don't think we are properly following these steps (see bug 187907 and bug 187319). However, these steps are only executed for custom elements outside HTML fragment parsing, so they won't be executed for attachment 335672 [details] anyway... Now I wonder if the the use of document.body.innerHTML = '<x-parenter><x-child></x-child></x-parenter>'; is involved in the issue (bug 184307).

Frédéric Wang (:fredw)

Comment 28 2018-07-26 01:05:22 PDT

Created attachment 345834 [details] Another repro case This is the same as attachment 335672 [details] but using DOMParser() instead of innerHTML in order to parse the content, as in Justin's original test case (bug 183583). Rob's patch (attachment 341272 [details]) does not fix the bug for this use case. I'm not sure why, but I suspect that in general the custom element reactions are not called consistently depending on how the html fragment is parsed and inserted into the DOM tree (e.g. see bug 184307 comment 2 for using innerHTML VS using DOM operations).

Ryosuke Niwa

Comment 29 2018-08-03 01:13:32 PDT

Comment on attachment 345570 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=345570&action=review > Source/WebCore/html/parser/HTMLDocumentParser.cpp:214 > + if (isParsingFragment()) { > + ASSERT(Document::validateCustomElementName(constructionData->name) == CustomElementNameValidationStatus::Valid); This is not the right place to check this condition. We need to override the behavior of HTMLConstructionSite::createHTMLElementOrFindCustomElementInterface instead. Will upload a patch shortly.

Ryosuke Niwa

Comment 30 2018-08-03 01:31:35 PDT

Oh, interesting. There is a subtle bug in the element queue implementation here. Will look into it tomorrow.

Frédéric Wang (:fredw)

Comment 31 2018-08-03 02:05:47 PDT

(In reply to Ryosuke Niwa from comment #30) > Oh, interesting. There is a subtle bug in the element queue implementation > here. Will look into it tomorrow. Thank you! (In reply to Ryosuke Niwa from comment #29) > Comment on attachment 345570 [details] > This is not the right place to check this condition. We need to override the > behavior of > HTMLConstructionSite::createHTMLElementOrFindCustomElementInterface instead. > Will upload a patch shortly. BTW, I suspect this bug is also going to happen for XML parsing but the code path is different (it uses Document::createElement instead of constructElementWithFallback to create the custom element). You might want to take a look at bug 187802 once you've addressed that one.

Ryosuke Niwa

Comment 32 2018-08-03 19:45:38 PDT

My patch in https://bugs.webkit.org/show_bug.cgi?id=188327 fixes the original repro posted by tkent but not the second test case you found. We'd have to investigate it separately.

Frédéric Wang (:fredw)

Comment 33 2018-08-04 00:10:11 PDT

(In reply to Ryosuke Niwa from comment #32) > My patch in https://bugs.webkit.org/show_bug.cgi?id=188327 fixes the > original repro posted by tkent but not the second test case you found. We'd > have to investigate it separately. Great, thanks! Yes, Justin's case is more complicated with DOMParser and importing node from another document, so that needs more investigation.

Frédéric Wang (:fredw)

Comment 34 2018-08-05 22:40:47 PDT

Created attachment 346614 [details] Repro case (xhtml) (In reply to Frédéric Wang (:fredw) from comment #31) > BTW, I suspect this bug is also going to happen for XML parsing but the code > path is different (it uses Document::createElement instead of > constructElementWithFallback to create the custom element). You might want > to take a look at bug 187802 once you've addressed that one. I stand corrected, setting via innerHTML in XHTML documents works as expected in release (and trunk) builds. See attached testcase.

Ryosuke Niwa

Comment 35 2018-08-06 00:57:48 PDT

Yeah, XML parser just uses the upgrade path for now so it just "works" in innerHTML setter since it's marked as CEReactions. We need to eventually fix the XML parser but we should probably track that as a separate bug as that's much less important compared to getting the HTML parser into the spec compliant state.

Ryosuke Niwa

Comment 36 2018-08-08 13:51:58 PDT

It looks like the second test case (https://bugs.webkit.org/attachment.cgi?id=345834) still fails.

Ryosuke Niwa

Comment 37 2018-08-14 19:40:52 PDT

Created attachment 347145 [details] Only repro that produces different results in Safari vs. Chrome/Firefox Here's the reduction of https://bug-183586-attachments.webkit.org/attachment.cgi?id=345834 which only shows a difference between Chrome/Firefox and trunk WebKit. This is an interesting one. If I added disconnectedCallback, Chrome and Firefox would behave like trunk WebKit.

Ryosuke Niwa

Comment 38 2018-08-14 20:29:37 PDT

Filed https://github.com/w3c/webcomponents/issues/760 to see if we want WebKit's behavior here or not. I'd argue that WebKit's current behavior is probably better but we'll see.

Frédéric Wang (:fredw)

Comment 39 2018-11-15 02:30:14 PST

Hi Ryosuke. So trying to summarize the status of this: - WebKit now behaves the same as Chrome/Firefox for attachment 335672 [details] - For attachment 347145 [details], the TPAC conclusion is that Chrome/Firefox should adopt WebKit's behavior. We still need to write a WPT test for this. Is that correct? So the only work that remains on our side is the WPT test and maybe the XML equivalent if we really want but they can be handled in separate bugs...

Ryosuke Niwa

Comment 40 2018-11-15 11:41:36 PST

(In reply to Frédéric Wang (:fredw) from comment #39) > Hi Ryosuke. So trying to summarize the status of this: > > - WebKit now behaves the same as Chrome/Firefox for attachment 335672 [details] > [details] > - For attachment 347145 [details], the TPAC conclusion is that > Chrome/Firefox should adopt WebKit's behavior. We still need to write a WPT > test for this. > > Is that correct? So the only work that remains on our side is the WPT test > and maybe the XML equivalent if we really want but they can be handled in > separate bugs... Yes, that's my understanding although we may still need to fix attribute change to be more permissive in enqueuing each element.

Ryosuke Niwa

Comment 41 2018-12-10 20:31:33 PST

Actually, the latest observation I made makes me think the status quo might be better so I'm gonna just align WebKit's behavior here.

Ryosuke Niwa

Comment 42 2018-12-10 20:32:02 PST

Created attachment 357032 [details] Aligns WebKit's behavior with Chrome/Firefox

Frédéric Wang (:fredw)

Comment 43 2018-12-11 01:29:52 PST

Comment on attachment 357032 [details] Aligns WebKit's behavior with Chrome/Firefox Great, thanks! (I guess a picky reviewer could say that the test does not cover all the changes in this patch...)

Ryosuke Niwa

Comment 44 2018-12-11 19:32:09 PST

(In reply to Frédéric Wang (:fredw) from comment #43) > Comment on attachment 357032 [details] > Aligns WebKit's behavior with Chrome/Firefox > > Great, thanks! (I guess a picky reviewer could say that the test does not > cover all the changes in this patch...) Oops, I forgot to add more test cases. Will land with more test cases.

Ryosuke Niwa

Comment 45 2018-12-11 19:58:43 PST

Committed r239096: <https://trac.webkit.org/changeset/239096>

darshan

Comment 46 2019-01-04 11:15:21 PST

crated pull request https://github.com/web-platform-tests/wpt/pull/14721 it's passing on chromium, chrome, Mozilla

darshan

Comment 47 2019-01-05 06:57:44 PST

(In reply to darshan from comment #46) > crated pull request https://github.com/web-platform-tests/wpt/pull/14721 > it's passing on chromium, chrome, Mozilla This test is also passing on mini-browser

Note You need to log in before you can comment on or make changes to this bug.