Merge pull request #561 from PolymerElements/559-kschaaf-unbounded-items

[3.x] Fix issue where _increasePoolIfNeeded could loop indefinitely. Fixes #559

iron-list.js

@@ -406,7 +406,7 @@ Polymer({
   _scrollerPaddingTop: 0,
 
   /**
-   * This value is the same as `scrollTop`.
+   * This value is a cached value of `scrollTop` from the last `scroll` event.
    */
   _scrollPosition: 0,
 
@@ -803,9 +803,16 @@ Polymer({
           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);
@@ -841,7 +848,8 @@ Polymer({
     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) {
@@ -1362,7 +1370,8 @@ Polymer({
     // 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);

test/basic.html

@@ -32,10 +32,16 @@
     </template>
   </test-fixture>
 
+  <test-fixture id="oddSizedList">
+    <template>
+      <odd-sized-list></odd-sized-list>
+    </template>
+  </test-fixture>
 
 <script type="module">
   import './fixtures/helpers.js';
   import './fixtures/x-list.js';
+  import './fixtures/odd-sized-list.js';
   import {dom} from '@polymer/polymer/lib/legacy/polymer.dom.js';
   suite('basic features', function() {
     var list, container;
@@ -300,6 +306,37 @@
           });
         });
   });
+
+  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>

test/fixtures/odd-sized-list.js

@@ -0,0 +1,58 @@
+/**
+@license
+Copyright (c) 3015 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
+*/
+import '@polymer/polymer/polymer-legacy.js';
+
+import '../../iron-list.js';
+import {Polymer} from '@polymer/polymer/lib/legacy/polymer-fn.js';
+import {html} from '@polymer/polymer/lib/utils/html-tag.js';
+Polymer({
+  is: 'odd-sized-list',
+  _template: html`
+    <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>`,
+  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;
+      }
+    }
+  }
+});