🐛 Fix race condition with layout -> unlayout #30426

jridgewell · 2020-09-29T02:26:12Z

When an unlayout (eg, when carousel swipes away) occurs before an unlayout, we can get into a broken state where we think we're fully laid out but in reality we've performed the unlayout.

There's still race conditions in each component, since they need to track their own layout/unlayout state. We should quickly migrate to Bento where this isn't an issue.

Fixes #27036
Partial #13838

src/custom-element.js

dvoytenko · 2020-09-29T17:36:08Z

src/custom-element.js

@@ -1168,6 +1169,9 @@ function createBaseCustomElementClass(win) {

      return promise.then(
        () => {
+          if (abortSignal.aborted) {
+            return;


Two things:

Do we need to reset LOAD_START from above? And the same in the catch clause?

It looks like spec for AbortController says that a supporting promise should yield the AbortError rejection?

Do we need to reset LOAD_START from above? And the same in the catch clause?

That is taken care of when calling the unlayoutCallback. By the time we get here, it's already reset.

It looks like spec for AbortController says that a supporting promise should yield the AbortError rejection?

That's specific to fetch. We could reject with an error, but that I didn't want to polyfill AbortError, too. As long as it gets handled, it should be fine though either the success or failure cases.

I still have a concern here. The promise realistically fails by API and I think we should say so as well. We could even just use our cancellation() error? Or we can even stick code = 20 on it, which is how, I think, AbortError is usually tested for.

I switched to the cancellation error.

src/service/resource.js

dvoytenko · 2020-09-29T17:38:27Z

src/service/resource.js

+      // We hit a race condition, where `layoutCallback` -> `unlayoutCallback`
+      // was called in quick succession. Since the unlayout was called before
+      // the layout completed, we want to remain in the unlayout state.
+      const err = dev().createError('layoutComplete race');


Is it worth logging? This seems like a very expected condition.

It's going through expectedError, but I could remove if you'd like.

If temporarily - it's ok. But in the long term, I don't think we need it. Up to you. But if you keep it, I recommend we file an issue to cleanup.

src/service/resource.js

src/service/resources-impl.js

samouri · 2020-10-01T20:17:19Z

I'm confused about what the introduction of AbortController buys us

When an unlayout (eg, when carousel swipes away) occurs before an unlayout, we can get into a broken state where we _think_ we're fully laid out but in reality we've performed the unlayout. There's still race conditions in each component, since they need to track their own layout/unlayout state. We should quickly migrate to Bento where this isn't an issue. Fixes ampproject#27036 Partial ampproject#13838

dvoytenko

Just two more questions. Otherwise this looks good.

dvoytenko · 2020-10-02T23:33:45Z

src/custom-element.js

      assertNotTemplate(this);
      devAssert(this.isBuilt(), 'Must be built to receive viewport events');
+      if (signal.aborted) {
+        throw cancellation();


Can we instead to Promise.reject(cancellation())?

dvoytenko · 2020-10-02T23:35:45Z

src/service/resource.js

    ) {
      return;
    }
+    if (this.abortController_) {
+      this.abortController_.abort();


Do we also need to set this.abortController_ = null here or if layout completes successfuly?

Since we're clearing it out after layout completes, it won't exist when we eventually unlayout

kristoferbaxter · 2020-10-08T19:18:02Z

Small prettier change required, but otherwise looks good.

extensions/amp-analytics/0.1/test/test-amp-analytics.js
  232:11  error  Insert `;`  prettier/prettier

CLAassistant · 2020-10-08T22:08:35Z

All committers have signed the CLA.

* Fix race condition with layout -> unlayout When an unlayout (eg, when carousel swipes away) occurs before an unlayout, we can get into a broken state where we _think_ we're fully laid out but in reality we've performed the unlayout. There's still race conditions in each component, since they need to track their own layout/unlayout state. We should quickly migrate to Bento where this isn't an issue. Fixes ampproject#27036 Partial ampproject#13838 * Rename variable * Throw cancellation errors where appropriate * Review fixes * Fix dep requirement * Fix sourcemap check * Fix tests * Only allow missing signal param in test mode * Do not report missing abortController Since we're clearing it out after layout completes, it won't exist when we eventually unlayout * Toggle amp-analytics visibility _after_ layout * Fix method call * Fix test * lint * Fix test

jridgewell requested review from dvoytenko and samouri September 29, 2020 02:26

google-cla bot added the cla: yes label Sep 29, 2020

dvoytenko suggested changes Sep 29, 2020

View reviewed changes

samouri approved these changes Oct 2, 2020

View reviewed changes

jridgewell added 3 commits October 2, 2020 15:26

Rename variable

6a19ebf

Throw cancellation errors where appropriate

ab3cabb

jridgewell force-pushed the layout-race branch from a82715c to ab3cabb Compare October 2, 2020 19:26

dvoytenko approved these changes Oct 2, 2020

View reviewed changes

jridgewell added 2 commits October 2, 2020 20:08

Review fixes

3cc011b

Fix dep requirement

a480b9e

dvoytenko approved these changes Oct 3, 2020

View reviewed changes

jridgewell added 2 commits October 2, 2020 21:11

Fix sourcemap check

6320f34

Fix tests

46ac2e9

jridgewell force-pushed the layout-race branch from 36b04dd to 46ac2e9 Compare October 3, 2020 03:48

jridgewell added 3 commits October 3, 2020 02:15

Only allow missing signal param in test mode

0dec65a

Do not report missing abortController

66102dc

Since we're clearing it out after layout completes, it won't exist when we eventually unlayout

Toggle amp-analytics visibility _after_ layout

376c08e

jridgewell force-pushed the layout-race branch 2 times, most recently from 7986509 to 4cb6a5c Compare October 8, 2020 04:54

Fix method call

6154204

jridgewell force-pushed the layout-race branch 2 times, most recently from 8ed3304 to 6154204 Compare October 8, 2020 18:51

Fix test

4a74f02

kristoferbaxter approved these changes Oct 8, 2020

View reviewed changes

lint

4cfa60f

kristoferbaxter approved these changes Oct 9, 2020

View reviewed changes

jridgewell merged commit 93fdf01 into ampproject:master Oct 9, 2020

jridgewell deleted the layout-race branch October 9, 2020 22:21

rcebulko mentioned this pull request Oct 12, 2020

🚨 Error: AbortController is not defined ampproject/error-reporting#15

Closed

ampprojectbot added the PR use: In Beta / Experimental label Oct 14, 2020

jridgewell mentioned this pull request Oct 19, 2020

🔥 Only collapse sticky-ad when ad collapses #30730

Merged

powerivq mentioned this pull request Oct 19, 2020

🌸 Cherry-pick request for #30730 into #30741 (Approved) #30740

Closed

jridgewell mentioned this pull request Oct 28, 2020

wg-performance Status Update 2020-10-12 ampproject/wg-performance#50

Closed

ampprojectbot mentioned this pull request Dec 6, 2020

🚨 Error: null is not an object (evaluating 'this.o.signals') #31464

Closed

nickFalcone mentioned this pull request Jun 7, 2021

Amp-lightbox-gallery - Thumbnails showing the wrong image when clicked. #34556

Closed

This was referenced Oct 19, 2021

🚨 Error: startLayout called but not LAYOUT_SCHEDULED currently: 2 #36406

Closed

🚨 Error: startLayout called but not LAYOUT_SCHEDULED currently: 2 #36407

Closed

ampprojectbot mentioned this pull request Oct 26, 2021

🚨 Error: startLayout called but not LAYOUT_SCHEDULED currently: 2 #36566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Fix race condition with layout -> unlayout #30426

🐛 Fix race condition with layout -> unlayout #30426

jridgewell commented Sep 29, 2020

dvoytenko Sep 29, 2020

jridgewell Sep 29, 2020

dvoytenko Oct 1, 2020

jridgewell Oct 2, 2020

dvoytenko Sep 29, 2020

jridgewell Sep 29, 2020

dvoytenko Oct 1, 2020

samouri commented Oct 1, 2020

dvoytenko left a comment

dvoytenko Oct 2, 2020

jridgewell Oct 3, 2020

dvoytenko Oct 2, 2020

jridgewell Oct 3, 2020

kristoferbaxter commented Oct 8, 2020

CLAassistant commented Oct 8, 2020 •

edited

Loading

🐛 Fix race condition with layout -> unlayout #30426

🐛 Fix race condition with layout -> unlayout #30426

Conversation

jridgewell commented Sep 29, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samouri commented Oct 1, 2020

dvoytenko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoferbaxter commented Oct 8, 2020

CLAassistant commented Oct 8, 2020 • edited Loading

CLAassistant commented Oct 8, 2020 •

edited

Loading