Closed Bug 1419305 Opened 2 years ago Closed 2 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: edgar, Assigned: edgar)

References

()

Details

Attachments

(4 files, 2 obsolete files)

No description provided.
Assignee: nobody → echen
Priority: -- → P2
Attached image good.jpg
Attachment #8930388 - Attachment is obsolete: true
Attachment #8930389 - Attachment is obsolete: true
Attached image bad.jpg
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.
Summary: https://nest.com isn't displayed correctly with enabling custom elements pref → https://nest.com/cameras/nest-cam-iq-outdoor/overview/ sometimes isn't displayed correctly with enabling custom elements pref
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?
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.
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
With these patches, I couldn't reproduce the issue so far and didn't see long pending reactions in BackupQueue, either.
Attachment #8930853 - Flags: review?(bugs)
Attachment #8930854 - Flags: review?(bugs)
Comment on attachment 8930853 [details]
Bug 1419305 - Part 1: Use MicroTask on CustomElements correctly;

https://reviewboard.mozilla.org/r/201954/#review208114
Attachment #8930853 - Flags: review?(bugs) → review+
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
Attachment #8930854 - Flags: review?(bugs) → review+
Glad that the new microtask stuff worked.
Pushed by echen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/79c5abcb4abe
https://hg.mozilla.org/mozilla-central/rev/9b6030d735c7
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.