shadowDoc: Return promise from close method #25094

mattwomple · 2019-10-17T04:52:11Z

The existing shadowDoc.close() method is not fully synchronous. Notably,
the pass that occurs on a visibility state change may not complete
before additional tasks are performed, like removing the ShadowRoot host
completely. Cleanup is incomplete and there are some potentially
valuable unload tasks that were not given the chance to run. Return a promise
from shadowDoc.close() that resolves when the next visibility state change
completes after requesting a change to INACTIVE.

The existing shadowDoc.close() method is not fully synchronous. Notably, the 'pass' that occurs on a visibility state change may not complete before additional tasks are performed, like removing the ShadowRoot host completely. Cleanup is incomplete and there are some potentially valuable unload tasks that were not given the chance to run. Expose shadowDoc.closeAsync() method that waits for the next visibility state change pass to complete before resolving.

mattwomple · 2019-10-17T05:02:45Z

This commit is most easily tested by combining with PR #25014.

jridgewell · 2019-10-17T19:00:39Z

/cc @dvoytenko

jridgewell

@dvoytenko: Would it be so bad to add a forced sync unlayoutCallback to resources? That way everyone always gets the correct behavior, not just the people who call closeAsync.

src/service/timer-impl.js

src/runtime.js

dvoytenko · 2019-10-17T20:07:02Z

@dvoytenko: Would it be so bad to add a forced sync unlayoutCallback to resources? That way everyone always gets the correct behavior, not just the people who call closeAsync.

I think we just need close() that returns a promise and let the clients decide where or not they want to wait for the promise to complete. As far as unlayoutCallback - possible, but I think the most important part done via visibility state is to let analytics callbacks to complete.

jridgewell · 2019-10-17T21:13:06Z

As far as unlayoutCallback - possible, but I think the most important part done via visibility state is to let analytics callbacks to complete.

What we found in #25014 (comment) is that calling close then immediately calling removeChild on the host element will stop unlayoutCallback from firing. If there are any event listeners that get unregistered during unlayoutCallback, they'll never be done. Eg, the amp-img's https://github.com/ampproject/amphtml/pull/25014/files#diff-ba655557663735d2bb090f707f986bccR274-R281

mattwomple · 2019-10-18T15:49:36Z

Gathering comments, here are the paths forward I see. I'm interested in hearing which option (or another if you see more options) is preferred.

Status

closeShadowRoot_() is partly synchronous and partly asynchronous (callbacks to visibility state change occur asynchronously).
closeShadowRootAsync_() is fully asynchronous but currently not designed to be used as a Promise.

Documentation for shadowDoc.close() (which calls closeShadowRoot_()) makes no mention of asynchronous behavior. attachShadowDoc() includes the following check before it runs that may be a bit race condition-y with visibility state change requests that occur after it.

if (shadowRoot.AMP) {
  user().warn(TAG, "Shadow doc wasn't previously closed");
  this.closeShadowRoot_(shadowRoot);
}

Option 1

Revise closeShadowRoot_() so that it returns a promise that resolves when callbacks to the visibility state change (to VisibilityState.INACTIVE) have completed. Existing consumers of the current Shadow-doc API would not be adversely affected. New consumers, as well as existing consumers who revisit the documentation, will see that they now have a safer option to use for cleanup.

The safety check in attachShadowDoc() (mentioned above) would still be a problem. It's probably not appropriate to fail completely if shadowRoot.AMP already exists, so the cleanup attempt is a good idea, but attachShadowDoc() should really be rewritten as a promise. We could leave attachShadowDoc() in its current form for existing consumers and introduce attachShadowDocAsync() for new consumers. Documentation would list both, but tag attachShadowDoc() as deprecated.

Option 2

Introduce an optional, fully synchronous path for handling the visibility state transition to VisibilityState.INACTIVE. No changes to the current Shadow-doc API would be required.

jridgewell · 2019-10-18T18:56:30Z

I don't think Option 2 is feasible, because it would impact the swipe-to-next-doc performance of the SERP carousel. I'd rather the closeShadowRoot_ told Resources to perform a synchronous unlayout for all elements.

dvoytenko · 2019-10-18T21:16:59Z

As far as unlayoutCallback - possible, but I think the most important part done via visibility state is to let analytics callbacks to complete.

What we found in #25014 (comment) is that calling close then immediately calling removeChild on the host element will stop unlayoutCallback from firing. If there are any event listeners that get unregistered during unlayoutCallback, they'll never be done. Eg, the amp-img's https://github.com/ampproject/amphtml/pull/25014/files#diff-ba655557663735d2bb090f707f986bccR274-R281

@jridgewell that's the user code behavior, right? I think it's ok for many, and otherwise our recommendation should be:

amp.close().then(removeFromDom);

Would that work?

jridgewell · 2019-10-18T21:27:44Z

that's the user code behavior, right? I think it's ok for many, and otherwise our recommendation should be… Would that work?

Yes, that'll work. My preference would still be do to the bug-free thing by default, but I'm fine with just returning a promise.

mattwomple · 2019-10-18T22:10:39Z

I'm not completely sure I know where you two landed. Do your expectations match everything I wrote in Option 1 above? Namely, is introducing attachShadowDocAsync warranted? Or, should I focus on only revising closeShadowRoot_() to return a Promise?

mattwomple · 2019-10-31T15:24:43Z

@jridgewell @dvoytenko I didn't hear back from you guys on my last comment, so I made revisions to the best of my understanding of your requests. Please let me know if the current version of this PR matches your expected behavior. Thanks!

codecov · 2019-10-31T16:43:53Z

Codecov Report

Merging #25094 into master will decrease coverage by 0.05%.
The diff coverage is 71.42%.

@@            Coverage Diff             @@
##           master   #25094      +/-   ##
==========================================
- Coverage   79.79%   79.74%   -0.06%     
==========================================
  Files         855      853       -2     
  Lines       52162    51905     -257     
==========================================
- Hits        41624    41392     -232     
+ Misses      10538    10513      -25

Flag	Coverage Δ
#integration_tests	`31.51% <14.28%> (-0.16%)`	⬇️
#unit_tests	`79.17% <71.42%> (+0.01%)`	⬆️

Impacted Files	Coverage Δ
src/runtime.js	`87.93% <70%> (-0.67%)`	⬇️
src/service/timer-impl.js	`91.37% <75%> (-1.48%)`	⬇️
.../amp-viewer-integration/0.1/messaging/messaging.js	`65.43% <0%> (-12.91%)`	⬇️
extensions/amp-accordion/0.1/amp-accordion.js	`83.65% <0%> (-7.21%)`	⬇️
src/polyfills.js	`92.85% <0%> (-7.15%)`	⬇️
src/curve.js	`79.48% <0%> (-2.57%)`	⬇️
...bscriptions-google/0.1/amp-subscriptions-google.js	`81.25% <0%> (-1.86%)`	⬇️
extensions/amp-story/1.0/amp-story-consent.js	`95.58% <0%> (-1.48%)`	⬇️
extensions/amp-viewer-integration/0.1/findtext.js	`95.09% <0%> (-1.38%)`	⬇️
extensions/amp-mega-menu/0.1/amp-mega-menu.js	`91.86% <0%> (-1.33%)`	⬇️
... and 48 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8937adb...609d7bb. Read the comment docs.

jridgewell · 2019-11-01T18:31:58Z

Ping @dvoytenko

dvoytenko · 2019-11-01T18:53:15Z

LGTM

* shadowDoc: Expose async close method The existing shadowDoc.close() method is not fully synchronous. Notably, the 'pass' that occurs on a visibility state change may not complete before additional tasks are performed, like removing the ShadowRoot host completely. Cleanup is incomplete and there are some potentially valuable unload tasks that were not given the chance to run. Expose shadowDoc.closeAsync() method that waits for the next visibility state change pass to complete before resolving. * Add shadowDoc.closeAsync to documentation * Prefer service/timer promise race method * Avoid race between resources promise and cleanup * Revisions based on feedback * Backwards-compatible shadow close and update unit tests * Fix comment, remove examples * Another shot at fixing the comment

googlebot added the cla: yes label Oct 17, 2019

amp-owners-bot bot requested a review from dreamofabear October 17, 2019 04:52

mattwomple mentioned this pull request Oct 17, 2019

amp-img: Cancel pending images on unlayout #25014

Merged

mattwomple requested a review from jridgewell October 17, 2019 05:03

mattwomple added 3 commits October 16, 2019 22:06

Add shadowDoc.closeAsync to documentation

e70ebcf

Prefer service/timer promise race method

70afbaa

Avoid race between resources promise and cleanup

5e977d8

jridgewell reviewed Oct 17, 2019

View reviewed changes

dvoytenko suggested changes Oct 17, 2019

View reviewed changes

src/service/timer-impl.js Outdated Show resolved Hide resolved

src/runtime.js Outdated Show resolved Hide resolved

mattwomple added 2 commits October 31, 2019 08:15

Revisions based on feedback

8fa114a

Merge remote-tracking branch 'origin/master' into patch-8

f4b507a

Backwards-compatible shadow close and update unit tests

e84ad45

mattwomple added 2 commits October 31, 2019 09:51

Fix comment, remove examples

a1628b3

Another shot at fixing the comment

609d7bb

jridgewell approved these changes Oct 31, 2019

View reviewed changes

mattwomple changed the title ~~shadowDoc: Expose async close method~~ shadowDoc: Return promise from close method Nov 1, 2019

mattwomple requested a review from dvoytenko November 1, 2019 07:06

dvoytenko approved these changes Nov 1, 2019

View reviewed changes

jridgewell merged commit 1876279 into ampproject:master Nov 1, 2019

mattwomple deleted the patch-8 branch November 1, 2019 19:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shadowDoc: Return promise from close method #25094

shadowDoc: Return promise from close method #25094

mattwomple commented Oct 17, 2019 •

edited

mattwomple commented Oct 17, 2019

jridgewell commented Oct 17, 2019

jridgewell left a comment

dvoytenko commented Oct 17, 2019

jridgewell commented Oct 17, 2019

mattwomple commented Oct 18, 2019

jridgewell commented Oct 18, 2019

dvoytenko commented Oct 18, 2019

jridgewell commented Oct 18, 2019

mattwomple commented Oct 18, 2019

mattwomple commented Oct 31, 2019

codecov bot commented Oct 31, 2019 •

edited

jridgewell commented Nov 1, 2019

dvoytenko commented Nov 1, 2019

shadowDoc: Return promise from close method #25094

shadowDoc: Return promise from close method #25094

Conversation

mattwomple commented Oct 17, 2019 • edited

mattwomple commented Oct 17, 2019

jridgewell commented Oct 17, 2019

jridgewell left a comment

Choose a reason for hiding this comment

dvoytenko commented Oct 17, 2019

jridgewell commented Oct 17, 2019

mattwomple commented Oct 18, 2019

jridgewell commented Oct 18, 2019

dvoytenko commented Oct 18, 2019

jridgewell commented Oct 18, 2019

mattwomple commented Oct 18, 2019

mattwomple commented Oct 31, 2019

codecov bot commented Oct 31, 2019 • edited

Codecov Report

jridgewell commented Nov 1, 2019

dvoytenko commented Nov 1, 2019

mattwomple commented Oct 17, 2019 •

edited

codecov bot commented Oct 31, 2019 •

edited