amp-bind: Rate-limit history operations (#23938)

* First pass with promises.

* Use a queue with debounced flush instead.

* Minor clean up.

* Use enum instead of string for op.

* Oops.

* Immediately flush on push.

* A few fixes and add tests.

* Fix types, one more test.

* Minor simplification.

* Justin's comments.

* Avoid array deopt due to out-of-bounds access.

extensions/amp-bind/0.1/bind-impl.js

@@ -86,6 +86,23 @@ let BoundPropertyDef;
  */
 let BoundElementDef;
 
+/**
+ * @enum {number}
+ */
+const HistoryOp = {
+  REPLACE: 0,
+  PUSH: 1,
+};
+
+/**
+ * @typedef {{
+ *   op: !HistoryOp,
+ *   data: (!JsonObject|undefined),
+ *   onPop: (!Function|undefined)
+ * }}
+ */
+let HistoryTaskDef;
+
 /**
  * A map of tag names to arrays of attributes that do not have non-bind
  * counterparts. For instance, amp-carousel allows a `[slide]` attribute,
@@ -140,7 +157,7 @@ export class Bind {
      */
     this.actionSequenceIds_ = [];
 
-    /** @const @private {!Function} */
+    /** @const @private {function()} */
     this.eventuallyClearActionSequenceIds_ = debounce(
       this.win_,
       () => {
@@ -219,12 +236,25 @@ export class Bind {
     /** @const @private {!Deferred} */
     this.addMacrosDeferred_ = new Deferred();
 
-    /** @private {Promise} */
+    /** @private {?Promise} */
     this.setStatePromise_ = null;
 
     /** @private @const {!../../../src/utils/signals.Signals} */
     this.signals_ = new Signals();
 
+    /**
+     * A queue of history replace/push to adhere to browser rate-limits.
+     * @private @const {!Array<!HistoryTaskDef>}
+     */
+    this.historyQueue_ = [];
+
+    /** @private @const {!Function} */
+    this.debouncedFlushHistoryQueue_ = debounce(
+      this.win_,
+      () => this.flushHistoryQueue_(),
+      1000
+    );
+
     // Install debug tools.
     const g = self.AMP;
     g.printState = g.printState || this.debugPrintState_.bind(this);
@@ -346,12 +376,12 @@ export class Bind {
     dev().info(TAG, 'setState:', expression);
     this.setStatePromise_ = this.evaluateExpression_(expression, scope)
       .then(result => this.setState(result))
-      .then(() => this.getDataForHistory_())
-      .then(data => {
-        // Don't bother calling History.replace with empty data.
-        if (data) {
-          this.history_.replace(data);
-        }
+      .then(() => {
+        return this.getDataForHistory_().then(data => {
+          if (data) {
+            this.pushToHistoryQueue_({op: HistoryOp.REPLACE, data});
+          }
+        });
       });
     return this.setStatePromise_;
   }
@@ -378,32 +408,76 @@ export class Bind {
       });
 
       const onPop = () => this.setState(oldState);
-      return this.setState(result)
-        .then(() => this.getDataForHistory_())
-        .then(data => {
-          this.history_.push(onPop, data);
+      return this.setState(result).then(() => {
+        return this.getDataForHistory_().then(data => {
+          this.pushToHistoryQueue_({op: HistoryOp.PUSH, data, onPop});
         });
+      });
+    });
+  }
+
+  /**
+   * @param {!HistoryTaskDef} task
+   * @private
+   */
+  pushToHistoryQueue_(task) {
+    let enqueue = true;
+
+    // Collapse if consecutive "replace" operations.
+    if (task.op === HistoryOp.REPLACE && this.historyQueue_.length > 0) {
+      const lastTask = this.historyQueue_[this.historyQueue_.length - 1];
+      if (lastTask.op === HistoryOp.REPLACE) {
+        lastTask.data = task.data;
+        enqueue = false;
+      }
+    }
+
+    if (enqueue) {
+      this.historyQueue_.push(task);
+    }
+
+    if (task.op === HistoryOp.PUSH) {
+      // Immediately flush queue on "push" to avoid adding latency to
+      // the browser "back" functionality.
+      this.flushHistoryQueue_();
+    } else {
+      // Debounce flush queue on "replace" to rate limit History.replace.
+      this.debouncedFlushHistoryQueue_();
+    }
+  }
+
+  /**
+   * @private
+   */
+  flushHistoryQueue_() {
+    this.historyQueue_.forEach(({op, data, onPop}) => {
+      if (op === HistoryOp.REPLACE) {
+        this.history_.replace(data);
+      } else if (op === HistoryOp.PUSH) {
+        this.history_.push(onPop, data);
+      }
     });
+    this.historyQueue_.length = 0;
   }
 
   /**
    * Returns data that should be saved in browser history on AMP.setState() or
    * AMP.pushState(). This enables features like restoring browser tabs.
-   * @return {!Promise<?JsonObject>}
+   * @return {!Promise<(!JsonObject|undefined)>}
    */
   getDataForHistory_() {
+    // Copy so subsequent changes don't modify the stored object reference.
     const data = dict({
-      'data': dict({'amp-bind': this.state_}),
+      'data': dict({'amp-bind': this.copyJsonObject_(this.state_)}),
       'title': this.localWin_.document.title,
     });
     if (!this.viewer_.isEmbedded()) {
-      // CC doesn't recognize !JsonObject as a subtype of (JsonObject|null).
-      return /** @type {!Promise<?JsonObject>} */ (Promise.resolve(data));
+      return Promise.resolve(data);
     }
     // Only pass state for history updates to trusted viewers, since they
     // may contain user data e.g. form input.
     return this.viewer_.isTrustedViewer().then(trusted => {
-      return trusted ? data : null;
+      return trusted ? data : undefined;
     });
   }
 

extensions/amp-bind/0.1/test/integration/test-bind-impl.js

@@ -828,8 +828,8 @@ describe
 
         describe('history', () => {
           beforeEach(() => {
-            sandbox.spy(history, 'replace');
-            sandbox.spy(history, 'push');
+            sandbox.stub(history, 'replace');
+            sandbox.stub(history, 'push');
             sandbox.stub(viewer, 'isEmbedded').returns(true);
           });
 
@@ -842,6 +842,9 @@ describe
                 {one: 1}
               );
               return promise.then(() => {
+                // History.replace has a 1s debounce.
+                clock.tick(1000);
+
                 // Shouldn't call replace() with null `data`.
                 expect(history.replace).to.not.be.called;
               });
@@ -855,7 +858,10 @@ describe
               return promise.then(() => {
                 expect(history.push).to.be.called;
                 // `data` param should be null on untrusted viewers.
-                expect(history.push).to.be.calledWith(sinon.match.func, null);
+                expect(history.push).to.be.calledWith(
+                  sinon.match.func,
+                  undefined
+                );
               });
             });
           });
@@ -875,6 +881,11 @@ describe
                 {one: 1}
               );
               return promise.then(() => {
+                expect(history.replace).to.not.be.called;
+
+                // History.replace has a 1s debounce.
+                clock.tick(1000);
+
                 expect(history.replace).calledOnce;
                 // `data` param should exist on trusted viewers.
                 expect(history.replace).calledWith({
@@ -898,6 +909,39 @@ describe
                 });
               });
             });
+
+            it('should collapse consecutive replaces', async () => {
+              await bind.setStateWithExpression('{foo: 1}', {});
+              await bind.setStateWithExpression('{foo: 2}', {});
+
+              expect(history.replace).to.not.be.called;
+
+              clock.tick(1000);
+
+              expect(history.replace).calledOnce;
+              expect(history.replace).calledWith({
+                data: {'amp-bind': {foo: 2}},
+                title: '',
+              });
+            });
+
+            it('should flush history immediately after push', async () => {
+              await bind.setStateWithExpression('{foo: 1}', {});
+
+              expect(history.replace).to.not.be.called;
+              expect(history.push).to.not.be.called;
+
+              await bind.pushStateWithExpression('{bar: 2}', {});
+
+              expect(history.replace).calledWith({
+                data: {'amp-bind': {foo: 1}},
+                title: '',
+              });
+              expect(history.push).calledWith(sinon.match.func, {
+                data: {'amp-bind': {foo: 1, bar: 2}},
+                title: '',
+              });
+            });
           });
         });