🐛 Fix race condition with layout -> unlayout (ampproject#30426)

* 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

build-system/tasks/check-sourcemaps.js

@@ -31,8 +31,8 @@ const sourcemapUrlMatcher =
   'https://raw.githubusercontent.com/ampproject/amphtml/\\d{13}/';
 
 // Mapping related constants
-const expectedFirstLineFile = 'src/polyfills/array-includes.js'; // First file that is compiled into v0.js.
-const expectedFirstLineCode = 'function includes(value, opt_fromIndex) {'; // First line of code in that file.
+const expectedFirstLineFile = 'src/polyfills/abort-controller.js'; // First file that is compiled into v0.js.
+const expectedFirstLineCode = 'class AbortController {'; // First line of code in that file.
 const expectedFirstLineColumn = "'use strict;'".length; // Column at which the first line is found (after an initial 'use strict;')
 
 /**

build-system/test-configs/dep-check-config.js

@@ -431,6 +431,7 @@ exports.rules = [
       '3p/polyfills.js->src/polyfills/object-assign.js',
       '3p/polyfills.js->src/polyfills/object-values.js',
       '3p/polyfills.js->src/polyfills/promise.js',
+      'src/polyfills.js->src/polyfills/abort-controller.js',
       'src/polyfills.js->src/polyfills/domtokenlist.js',
       'src/polyfills.js->src/polyfills/document-contains.js',
       'src/polyfills.js->src/polyfills/fetch.js',

extensions/amp-analytics/0.1/amp-analytics.js

@@ -44,7 +44,6 @@ import {isAnalyticsChunksExperimentOn} from './analytics-group';
 import {isArray, isEnumValue} from '../../../src/types';
 import {isIframed} from '../../../src/dom';
 import {isInFie} from '../../../src/iframe-helper';
-import {toggle} from '../../../src/style';
 
 const TAG = 'amp-analytics';
 
@@ -210,7 +209,6 @@ export class AmpAnalytics extends AMP.BaseElement {
     if (this.iniPromise_) {
       return this.iniPromise_;
     }
-    toggle(this.element, false);
 
     this.iniPromise_ = this.getAmpDoc()
       .whenFirstVisible()
@@ -253,6 +251,9 @@ export class AmpAnalytics extends AMP.BaseElement {
       })
       .then(this.registerTriggers_.bind(this))
       .then(this.initializeLinker_.bind(this));
+    this.iniPromise_.then(() => {
+      this./*OK*/ collapse();
+    });
     return this.iniPromise_;
   }
 

extensions/amp-analytics/0.1/test/test-amp-analytics.js

@@ -214,20 +214,22 @@ describes.realWin(
         el.textContent = config;
         const whenFirstVisibleStub = env.sandbox
           .stub(ampdoc, 'whenFirstVisible')
-          .callsFake(() => new Promise(function () {}));
+          .callsFake(() => Promise.resolve());
         doc.body.appendChild(el);
         const analytics = new AmpAnalytics(el);
         el.getAmpDoc = () => ampdoc;
         analytics.buildCallback();
         const iniPromise = analytics.iniPromise_;
         expect(iniPromise).to.be.ok;
-        expect(el).to.have.attribute('hidden');
         // Viewer.whenFirstVisible is the first blocking call to initialize.
         expect(whenFirstVisibleStub).to.be.calledOnce;
 
         // Repeated call, returns pre-created promise.
         expect(analytics.ensureInitialized_()).to.equal(iniPromise);
         expect(whenFirstVisibleStub).to.be.calledOnce;
+        return iniPromise.then(() => {
+          expect(el).to.have.attribute('hidden');
+        });
       });
 
       it('does not send a hit when multiple child tags exist', function () {

src/custom-element.js

@@ -28,7 +28,12 @@ import {LayoutDelayMeter} from './layout-delay-meter';
 import {ResourceState} from './service/resource';
 import {Services} from './services';
 import {Signals} from './utils/signals';
-import {blockedByConsentError, isBlockedByConsent, reportError} from './error';
+import {
+  blockedByConsentError,
+  cancellation,
+  isBlockedByConsent,
+  reportError,
+} from './error';
 import {createLoaderElement} from '../src/loader.js';
 import {dev, devAssert, rethrowAsync, user, userAssert} from './log';
 import {getIntersectionChangeEntry} from './utils/intersection-observer-3p-host';
@@ -1134,12 +1139,18 @@ function createBaseCustomElementClass(win) {
      *
      * Can only be called on a upgraded and built element.
      *
+     * @param {!AbortSignal} signal
      * @return {!Promise}
      * @package @final
      */
-    layoutCallback() {
+    layoutCallback(signal) {
       assertNotTemplate(this);
       devAssert(this.isBuilt(), 'Must be built to receive viewport events');
+      // A lot of tests call layoutCallback manually, and don't pass a signal.
+      if ((!getMode().test || signal) && signal.aborted) {
+        return Promise.reject(cancellation());
+      }
+
       this.dispatchCustomEventForTesting(AmpEvents.LOAD_START);
       const isLoadEvent = this.layoutCount_ == 0; // First layout is "load".
       this.signals_.reset(CommonSignals.UNLOAD);
@@ -1156,6 +1167,9 @@ function createBaseCustomElementClass(win) {
 
       return promise.then(
         () => {
+          if ((!getMode().test || signal) && signal.aborted) {
+            throw cancellation();
+          }
           if (isLoadEvent) {
             this.signals_.signal(CommonSignals.LOAD_END);
           }
@@ -1171,6 +1185,9 @@ function createBaseCustomElementClass(win) {
           }
         },
         (reason) => {
+          if ((!getMode().test || signal) && signal.aborted) {
+            throw cancellation();
+          }
           // add layoutCount_ by 1 despite load fails or not
           if (isLoadEvent) {
             this.signals_.rejectSignal(

src/polyfills.js

@@ -16,6 +16,7 @@
 
 /** @fileoverview */
 
+import {install as installAbortController} from './polyfills/abort-controller';
 import {install as installArrayIncludes} from './polyfills/array-includes';
 import {install as installCustomElements} from './polyfills/custom-elements';
 import {install as installDOMTokenList} from './polyfills/domtokenlist';
@@ -55,5 +56,6 @@ if (self.document) {
   if (!IS_SXG) {
     installCustomElements(self, class {});
     installIntersectionObserver(self);
+    installAbortController(self);
   }
 }

src/polyfills/abort-controller.js

@@ -0,0 +1,71 @@
+/**
+ * Copyright 2020 The AMP HTML Authors. All Rights Reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS-IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class AbortController {
+  /** */
+  constructor() {
+    /** @const {!AbortSignal} */
+    this.signal_ = new AbortSignal();
+  }
+
+  /** */
+  abort() {
+    this.signal_.isAborted_ = true;
+  }
+
+  /**
+   * @return {!AbortSignal}
+   */
+  get signal() {
+    return this.signal_;
+  }
+}
+
+class AbortSignal {
+  /** */
+  constructor() {
+    /** @private {boolean} */
+    this.isAborted_ = false;
+  }
+
+  /**
+   * @return {boolean}
+   */
+  get aborted() {
+    return this.isAborted_;
+  }
+}
+
+/**
+ * @param {!Window} win
+ */
+export function install(win) {
+  if (win.AbortController) {
+    return;
+  }
+  Object.defineProperty(win, 'AbortController', {
+    configurable: true,
+    enumerable: false,
+    writable: true,
+    value: AbortController,
+  });
+  Object.defineProperty(win, 'AbortSignal', {
+    configurable: true,
+    enumerable: false,
+    writable: true,
+    value: AbortSignal,
+  });
+}

src/service/resource.js

@@ -17,9 +17,9 @@
 import {Deferred, tryResolve} from '../utils/promise';
 import {Layout} from '../layout';
 import {Services} from '../services';
+import {cancellation, isBlockedByConsent, reportError} from '../error';
 import {computedStyle, toggle} from '../style';
 import {dev, devAssert} from '../log';
-import {isBlockedByConsent, reportError} from '../error';
 import {
   layoutRectLtwh,
   layoutRectSizeEquals,
@@ -176,6 +176,13 @@ export class Resource {
     /** @private {number} */
     this.layoutCount_ = 0;
 
+    /**
+     * Used to signal that the current layoutCallback has been aborted by an
+     * unlayoutCallback.
+     * @private {?AbortController}
+     */
+    this.abortController_ = null;
+
     /** @private {*} */
     this.lastLayoutError_ = null;
 
@@ -932,30 +939,42 @@ export class Resource {
     dev().fine(TAG, 'start layout:', this.debugid, 'count:', this.layoutCount_);
     this.layoutCount_++;
     this.state_ = ResourceState.LAYOUT_SCHEDULED;
+    this.abortController_ = new AbortController();
+    const {signal} = this.abortController_;
 
     const promise = new Promise((resolve, reject) => {
       Services.vsyncFor(this.hostWin).mutate(() => {
         try {
-          resolve(this.element.layoutCallback());
+          resolve(this.element.layoutCallback(signal));
         } catch (e) {
           reject(e);
         }
       });
-    });
-
-    this.layoutPromise_ = promise.then(
-      () => this.layoutComplete_(true),
-      (reason) => this.layoutComplete_(false, reason)
+    }).then(
+      () => this.layoutComplete_(true, signal),
+      (reason) => this.layoutComplete_(false, signal, reason)
     );
-    return this.layoutPromise_;
+
+    return (this.layoutPromise_ = promise);
   }
 
   /**
    * @param {boolean} success
+   * @param {!AbortSignal} signal
    * @param {*=} opt_reason
    * @return {!Promise|undefined}
    */
-  layoutComplete_(success, opt_reason) {
+  layoutComplete_(success, signal, opt_reason) {
+    this.abortController_ = null;
+    if (signal.aborted) {
+      // 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');
+      err.associatedElement = this.element;
+      dev().expectedError(TAG, err);
+      throw cancellation();
+    }
     if (this.loadPromiseResolve_) {
       this.loadPromiseResolve_();
       this.loadPromiseResolve_ = null;
@@ -1030,10 +1049,15 @@ export class Resource {
   unlayout() {
     if (
       this.state_ == ResourceState.NOT_BUILT ||
-      this.state_ == ResourceState.NOT_LAID_OUT
+      this.state_ == ResourceState.NOT_LAID_OUT ||
+      this.state_ == ResourceState.READY_FOR_LAYOUT
     ) {
       return;
     }
+    if (this.abortController_) {
+      this.abortController_.abort();
+      this.abortController_ = null;
+    }
     this.setInViewport(false);
     if (this.element.unlayoutCallback()) {
       this.element.togglePlaceholder(true);