amp-list: Fix Bind.rescan vs. diffing race condition (#32650)

* amp-list: Fix rescan vs. diffing race condition.

* Missing second param.

* Fix tests.

* Add unit tests.

extensions/amp-list/0.1/amp-list.js

@@ -1070,21 +1070,19 @@ export class AmpList extends AMP.BaseElement {
 
     // binding=refresh: Only do render-blocking update after initial render.
     if (binding && binding.startsWith('refresh')) {
-      // Bind service must be available after first mutation, so don't
-      // wait on the async service getter.
-      if (this.bind_ && this.bind_.signals().get('FIRST_MUTATE')) {
+      // Don't bother using bindForDocOrNull() since the Bind service must be available after first mutate.
+      const afterFirstMutate =
+        this.bind_ && this.bind_.signals().get('FIRST_MUTATE');
+      if (afterFirstMutate) {
         return updateWith(this.bind_);
       } else {
-        // On initial render, do a non-blocking scan and don't update.
-        Services.bindForDocOrNull(this.element).then((bind) => {
-          if (bind) {
-            const evaluate = binding == 'refresh-evaluate';
-            bind.rescan(elements, [], {
-              'fast': true,
-              'update': evaluate ? 'evaluate' : false,
-            });
-          }
-        });
+        // This must be initial render, so do a non-blocking scan for bindings only.
+        // [diffable] is a special case that is handled later in render_(), see comment there.
+        if (!this.element.hasAttribute('diffable')) {
+          this.scanForBindings_(elements, []);
+        }
+
+        // Don't block render and return synchronously.
         return Promise.resolve(elements);
       }
     }
@@ -1099,6 +1097,32 @@ export class AmpList extends AMP.BaseElement {
     });
   }
 
+  /**
+   * Scans for bindings in `addedElements` and removes bindings in `removedElements`.
+   * Unlike updateBindings(), does NOT apply bindings or update DOM.
+   * Should only be used for binding="refresh" or binding="refresh-evaluate".
+   * @param {!Array<!Element>} addedElements
+   * @param {!Array<!Element>} removedElements
+   * @private
+   */
+  scanForBindings_(addedElements, removedElements) {
+    const binding = this.element.getAttribute('binding');
+    if (!binding || !binding.startsWith('refresh')) {
+      return;
+    }
+    Services.bindForDocOrNull(this.element).then((bind) => {
+      if (bind) {
+        // For binding="refresh-evaluate", we scan for bindings, evaluate+cache expressions, but skip DOM update.
+        // For binding="refresh", we only scan for bindings.
+        const update = binding == 'refresh-evaluate' ? 'evaluate' : false;
+        bind.rescan(addedElements, removedElements, {
+          'fast': true,
+          'update': update,
+        });
+      }
+    });
+  }
+
   /**
    * @param {!Array<!Element>} elements
    * @param {boolean=} opt_append
@@ -1115,6 +1139,14 @@ export class AmpList extends AMP.BaseElement {
         // TODO:(wg-performance)(#28781) Ensure owners_.scheduleUnlayout() is
         // called for diff elements that are removed
         this.diff_(container, elements);
+
+        // For diffable amp-list, we have to wait until DOM diffing is done here to scan for new bindings
+        // (vs. asynchronously in updateBindings()), and scan the entire container (vs. just `elements`).
+        //
+        // This is because instead of replacing the entire DOM subtree, the diffing process removes
+        // select children from `elements` and inserts them into the container. This results in a race
+        // between diff_() and Bind.rescan(), which we avoid by delaying the latter until now.
+        this.scanForBindings_([container], [container]);
       } else {
         if (!opt_append) {
           this.owners_./*OK*/ scheduleUnlayout(this.element, container);

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

@@ -1403,6 +1403,50 @@ describes.repeated(
                 }
               );
             });
+
+            describe('rescan vs. diff race', () => {
+              async function rescanVsDiffTest() {
+                env.sandbox.spy(list, 'diff_');
+                env.sandbox.spy(list, 'render_');
+
+                // Diffing is skipped if there's no existing children to diff against.
+                const oldChild = doc.createElement('p');
+                oldChild.textContent = 'foo';
+                list.container_.appendChild(oldChild);
+
+                const newChild = doc.createElement('p');
+                newChild.textContent = 'bar';
+                // New children must have at least one binding to trigger rescan.
+                newChild.setAttribute('i-amphtml-binding', '');
+                const rendered = expectFetchAndRender(DEFAULT_FETCHED_DATA, [
+                  newChild,
+                ]);
+                await list.layoutCallback().then(() => rendered);
+              }
+
+              it('without diffing, should rescan _before_ render', async () => {
+                await rescanVsDiffTest();
+
+                expect(list.diff_).to.not.have.been.called;
+                expect(bind.rescan).to.have.been.calledOnce;
+
+                // Without diffable, rescan should happen before rendering the new children.
+                expect(bind.rescan).calledBefore(list.render_);
+              });
+
+              it('with diffing, should rescan _after_ render/diff', async () => {
+                element.setAttribute('diffable', '');
+
+                await rescanVsDiffTest();
+
+                expect(list.diff_).to.have.been.calledOnce;
+                expect(bind.rescan).to.have.been.calledOnce;
+
+                // With diffable, rescanning must happen after rendering (diffing) the new children.
+                expect(bind.rescan).calledAfter(list.render_);
+                expect(bind.rescan).calledAfter(list.diff_);
+              });
+            });
           });
 
           describe('binding="no"', () => {