1419305 - https://nest.com/cameras/nest-cam-iq-outdoor/overview/ sometimes isn't displayed correctly with enabling custom elements pref

Status: RESOLVED FIXED

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

Comment 1

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: good.png

Comment 2

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: bad.png

Comment 3

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Revision: dd08f8b19cc3

Steps:
 1. set dom.webcomponents.customelements.enabled to true.
 2. open https://nest.com/

Expect result:
 attachment 8930388 [details]

Actual result:
 attachment 8930389 [details]

Comment 4

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: good.jpg

Comment 5

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: bad.jpg

Comment 6

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Please ignore comment #3, I tested multiple different revisions, and I mixed up the revisions and steps...

Revision: dd08f8b19cc3

Steps:
 1. set dom.webcomponents.customelements.enabled to true.
 2. enter https://nest.com/ in the url bar, wait page loaded.
 3. navigate to "Nest Cam IQ outdoor security camera" page by clicking the product picture below
 4. if the issue does not happen, go to step 2.

Expect result:
 attachment 8930406 [details]

Actual result:
 attachment 8930407 [details]

Note:
 1. Once we hit the issue, reload the page will 100% reproduce.
 2. Entering https://nest.com/cameras/nest-cam-iq-outdoor/overview/ in url bar directly seems not able to reproduce the issue.

Comment 7

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

I thought this probably because of bug 1331334, but it is not the case actually.

After comparing the normal case and broken case, I found some custom elements are not being created when the issue happens, I have no idea why yet. 

(In reply to Edgar Chen [:edgar] from comment #6)
>  1. Once we hit the issue, reload the page will 100% reproduce.
It seems like we are in a weird state so that it becomes 100% reproducible?

Comment 8

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Okay, I saw some wrong behaviors, but I am not super sure if they are the root cause.

When appending node into tree happens from the parser, the connectedCallback is enqueued to BackupQueue, but the BackupQueue is not consumed correctly in some case, probably because we don't use microtask correctly. If this is the root cause, it could also explain why the issue becomes 100% reproducible once we hit the issue: we use the same CustomElementReactionStack whenever we reload the page, and CustomElementReactionStack has long pending reactions in BackupQueue.

Comment 9

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Another issue is we stop invoking reactions if there is an element in ElementQueue is unlinked [1], we should continue process remaining reactions instead.

[1] https://searchfox.org/mozilla-central/rev/797c93d81fe446f78babf20894f0729f15f71ee6/dom/base/CustomElementRegistry.cpp#1062

Comment 10

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Bug 1419305 - Part 1: Use MicroTask on CustomElements correctly;

Review commit: https://reviewboard.mozilla.org/r/201954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201954/

Comment 11

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Attachment: Bug 1419305 - Part 2: Keep processing remaining elements in ElementQueue even if some of elements are already unlinked;

Review commit: https://reviewboard.mozilla.org/r/201956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201956/

Comment 12

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

With these patches, I couldn't reproduce the issue so far and didn't see long pending reactions in BackupQueue, either.

Comment 13

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

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

Comment 14

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

Comment on attachment 8930854 [details]
Bug 1419305 - Part 2: Keep processing remaining elements in ElementQueue even if some of elements are already unlinked;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201956/diff/1-2/

Comment 15

[Edgar Chen [:edgar] (Parental leave until Oct. 09.)]

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

Comment 16

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

Comment on attachment 8930853 [details]
Bug 1419305 - Part 1: Use MicroTask on CustomElements correctly;

https://reviewboard.mozilla.org/r/201954/#review208114

Comment 17

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

Comment on attachment 8930854 [details]
Bug 1419305 - Part 2: Keep processing remaining elements in ElementQueue even if some of elements are already unlinked;

https://reviewboard.mozilla.org/r/201956/#review208116

Comment 18

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

Glad that the new microtask stuff worked.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/79c5abcb4abe
Part 1: Use MicroTask on CustomElements correctly; r=smaug
https://hg.mozilla.org/integration/autoland/rev/9b6030d735c7
Part 2: Keep processing remaining elements in ElementQueue even if some of elements are already unlinked; r=smaug

Comment 20

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/79c5abcb4abe
https://hg.mozilla.org/mozilla-central/rev/9b6030d735c7