Fix issue where _increasePoolIfNeeded could loop indefinitely. Fixes #…

…559 Ensure the _physicalTop is never recalculated to be below the current scroll position, and make sure cached scroll positions are used in methods which may be called outside of a scroll event handler for internal consistency.
PolymerElements · Oct 11, 2019 · 5b5c78b · 5b5c78b
1 parent 5fb80b1
commit 5b5c78b
Show file tree

Hide file tree

Showing 4 changed files with 114 additions and 5 deletions.
diff --git a/.gitignore b/.gitignore
@@ -1,2 +1,4 @@
 bower_components*
 bower-*.json
+node_modules
+*.d.ts
diff --git a/iron-list.html b/iron-list.html
@@ -436,7 +436,7 @@
     _scrollerPaddingTop: 0,
 
     /**
-     * This value is the same as `scrollTop`.
+     * This value is a cached value of `scrollTop` from the last `scroll` event.
      */
     _scrollPosition: 0,
 
@@ -824,8 +824,16 @@
         var idxAdjustment = Math.round(delta / this._physicalAverage) * this._itemsPerRow;
         this._virtualStart = this._virtualStart + idxAdjustment;
         this._physicalStart = this._physicalStart + idxAdjustment;
-        // Estimate new physical offset.
-        this._physicalTop = Math.floor(this._virtualStart / this._itemsPerRow) * this._physicalAverage;
+        // Estimate new physical offset based on the virtual start index.
+        // adjusts the physical start position to stay in sync with the clamped
+        // virtual start index. It's critical not to let this value be
+        // more than the scroll position however, since that would result in
+        // the physical items not covering the viewport, and leading to
+        // _increasePoolIfNeeded to run away creating items to try to fill it.
+        this._physicalTop = Math.min(
+            Math.floor(this._virtualStart / this._itemsPerRow) *
+                this._physicalAverage,
+            this._scrollPosition);
         this._update();
       } else if (this._physicalCount > 0) {
         var reusables = this._getReusables(isScrollingDown);
@@ -857,7 +865,8 @@
       var physicalCount = this._physicalCount;
       var top = this._physicalTop + this._scrollOffset;
       var bottom = this._physicalBottom + this._scrollOffset;
-      var scrollTop = this._scrollTop;
+      // This may be called outside of a scrollHandler, so use last cached position
+      var scrollTop = this._scrollPosition;
       var scrollBottom = this._scrollBottom;
 
       if (fromTop) {
@@ -1371,7 +1380,8 @@
       // Note: the delta can be positive or negative.
       if (deltaHeight !== 0) {
         this._physicalTop = this._physicalTop - deltaHeight;
-        var scrollTop = this._scrollTop;
+        // This may be called outside of a scrollHandler, so use last cached position
+        var scrollTop = this._scrollPosition;
         // juking scroll position during interial scrolling on iOS is no bueno
         if (!IOS_TOUCH_SCROLLING && scrollTop > 0) {
           this._resetScrollPosition(scrollTop - deltaHeight);

diff --git a/test/basic.html b/test/basic.html
@@ -18,6 +18,7 @@
   <script src="../../web-component-tester/browser.js"></script>
   <link rel="import" href="fixtures/helpers.html">
   <link rel="import" href="fixtures/x-list.html">
+  <link rel="import" href="fixtures/odd-sized-list.html">
 </head>
 <body>
 
@@ -35,6 +36,11 @@
     </template>
   </test-fixture>
 
+  <test-fixture id="oddSizedList">
+    <template>
+      <odd-sized-list></odd-sized-list>
+    </template>
+  </test-fixture>
 
 <script>
   'use strict';
@@ -297,6 +303,35 @@
     });
 
   });
+
+  suite('scrolling through heterogeneously-sized list', function() {
+    var list;
+    setup(function() {
+      list = fixture('oddSizedList').$.list;
+      PolymerFlush();
+    });
+    // This test catches a runaway condition where _increasePoolIfNeeded
+    // could recurse indefinitely due to the physical average becoming
+    // large enough (due to heterogeneously-sized items) to position the
+    // physical top lower than the scroll position, causing _isClientFull
+    // to return true; this
+    test('Issue #559: scroll heterogeneously sized list', function(done) {
+      list.scrollTop = 2300;
+      requestAnimationFrame(function() {
+        list.scrollTop = 0;
+        // Trigger the list to call _increasePoolIfNeeded (resize,
+        // non-random-access scrolling, itemsChanged, etc. would)
+        list.fire('iron-resize');
+        requestAnimationFrame(function() {
+          // In the current implementation, there should be ~29, but
+          // being generous here to allow for changes in the algorithm;
+          // idea being to catch a runaway situation
+          assert.isAtMost(list.children.length, 50);
+          done();
+        });
+      });
+    });
+  });
 </script>
 
 </body>

diff --git a/test/fixtures/odd-sized-list.html b/test/fixtures/odd-sized-list.html
@@ -0,0 +1,62 @@
+<!--
+@license
+Copyright (c) 2015 The Polymer Project Authors. All rights reserved.
+This code may only be used under the BSD style license found at http://polymer.github.io/LICENSE
+The complete set of authors may be found at http://polymer.github.io/AUTHORS
+The complete set of contributors may be found at http://polymer.github.io/CONTRIBUTORS
+Code distributed by Google as part of the polymer project is also
+subject to an additional IP rights grant found at http://polymer.github.io/PATENTS
+-->
+
+<link rel="import" href="../../polymer/polymer.html">
+<link rel="import" href="../iron-list.html">
+
+<dom-module id="odd-sized-list">
+  <template>
+    <style>
+      :host {
+        display: block;
+        width: 300px;
+        height: 300px;
+        border: 1px solid black;
+      }
+      iron-list {
+        height: 100%;
+      }
+      .item {
+        overflow: hidden;
+        border-bottom: 1px solid gray;
+      }
+    </style>
+    <iron-list id="list" items="{{items}}">
+      <template>
+        <div class="item" style$="height: [[item]]px;">Item [[index]] ([[item]]px)</div>
+      </template>
+    </iron-list>
+  </template>
+</dom-module>
+
+<script>
+Polymer({
+  is: 'odd-sized-list',
+  properties: {
+    items: {
+      value: function() {
+        // Some hand-tuned sizes to ensure the physical average
+        // after scrolling back up is such to trigger the bug
+        const items = [];
+        while (items.length < 70) {
+          items.push(30);
+        }
+        while (items.length < 90) {
+          items.push(90);
+        }
+        while (items.length < 5000) {
+          items.push(30);
+        }
+        return items;
+      }
+    }
+  }
+});
+</script>