Fix LazyLoad crash when multiple navigations are queued

Putting `loading=lazy` on an out-of-view iframe creates an internal LazyLoadFrameObserver owned by the HTMLFrameOwnerElement associated with the iframe element. Before this CL, `src` mutations reset the HTMLFrameOwnerElement's LazyLoadFrameObserver member, however old observers may hang around until they are garbage collected. Until this happens, they are technically active observers and still reference the HTMLFrameOwnerElement. This means if you queue multiple lazy loaded navigations via `src` attribute mutations, there may be multiple LazyLoadFrameObservers that are "active" and referencing the element. When the iframe is scrolled into view, the old still-alive LazyLoadFrameObserver's LoadImmediately() method may is called before the observer is GC'd. In that case, we start to navigate the iframe. This calls Frame::Navigate, which is expected to call LazyLoadFrameObserver::CancelPendingLazyLoad(), which is ensured by this CHECK [1]. However, the "old" LazyLoadFrameObserver::LoadImmediately() method invokes Frame::Navigate(), which invokes the "new" LazyLoadFrameObserver::CancelPendingLazyLoad() method. Therefore the CHECK mentioned above actually fails, because the "old" LazyLoadFrameObserver was not cancelled correctly during the navigation flow. After this CL, upon creating a new LazyLoadFrameObserver for the HTMLFrameOwnerElement we reset old LazyLoadFrameObservers by calling CancelPendingLazyLoad() on them. Then they are effectively disabled until they are GC'd. This CL also includes a test which crashes Chrome's renderer before the fix. [1]: https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/html/lazy_load_frame_observer.cc;l=197;drc=6e8b6ca4b8f8f7ff0b41ba613edd47453bb3aafe R=dcheng@chromium.org, sclittle@chromium.org Bug: 1137708, 1126127 Change-Id: I0a7eef854bed3e192a5ef68b9cec18c95353298a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2497424 Commit-Queue: Dominic Farolino <dom@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#821313}

Fix LazyLoad crash when multiple navigations are queued
Putting `loading=lazy` on an out-of-view iframe creates an internal LazyLoadFrameObserver owned by the HTMLFrameOwnerElement associated with the iframe element. Before this CL, `src` mutations reset the HTMLFrameOwnerElement's LazyLoadFrameObserver member, however old observers may hang around until they are garbage collected. Until this happens, they are technically active observers and still reference the HTMLFrameOwnerElement. This means if you queue multiple lazy loaded navigations via `src` attribute mutations, there may be multiple LazyLoadFrameObservers that are "active" and referencing the element. When the iframe is scrolled into view, the old still-alive LazyLoadFrameObserver's LoadImmediately() method may is called before the observer is GC'd. In that case, we start to navigate the iframe. This calls Frame::Navigate, which is expected to call LazyLoadFrameObserver::CancelPendingLazyLoad(), which is ensured by this CHECK [1]. However, the "old" LazyLoadFrameObserver::LoadImmediately() method invokes Frame::Navigate(), which invokes the "new" LazyLoadFrameObserver::CancelPendingLazyLoad() method. Therefore the CHECK mentioned above actually fails, because the "old" LazyLoadFrameObserver was not cancelled correctly during the navigation flow. After this CL, upon creating a new LazyLoadFrameObserver for the HTMLFrameOwnerElement we reset old LazyLoadFrameObservers by calling CancelPendingLazyLoad() on them. Then they are effectively disabled until they are GC'd. This CL also includes a test which crashes Chrome's renderer before the fix. [1]: https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/html/lazy_load_frame_observer.cc;l=197;drc=6e8b6ca4b8f8f7ff0b41ba613edd47453bb3aafe R=dcheng@chromium.org, sclittle@chromium.org Bug: 1137708, 1126127 Change-Id: I0a7eef854bed3e192a5ef68b9cec18c95353298a Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2497424 Commit-Queue: Dominic Farolino <dom@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Cr-Commit-Position: refs/heads/master@{#821313}
00bf927c · Dominic Farolino · Commit Bot · a77f0c0f · 00bf927c · 00bf927c
Commit 00bf927c authored Oct 27, 2020 by Dominic Farolino Committed by Commit Bot Oct 27, 2020
2 changed files
--- a/third_party/blink/renderer/core/html/html_frame_owner_element.cc
+++ b/third_party/blink/renderer/core/html/html_frame_owner_element.cc
@@ -546,6 +546,9 @@ bool HTMLFrameOwnerElement::LoadOrRedirectSubframe(
      // once they're near the viewport or visible.
      should_lazy_load_children_ = false;

+      if (lazy_load_frame_observer_)
+        lazy_load_frame_observer_->CancelPendingLazyLoad();
+
      lazy_load_frame_observer_ = MakeGarbageCollected<LazyLoadFrameObserver>(
          *this, LazyLoadFrameObserver::LoadType::kSubsequent);


--- a/third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-iframe-element/iframe-loading-lazy-multiple-queued-navigations.html
+++ b/third_party/blink/web_tests/external/wpt/html/semantics/embedded-content/the-iframe-element/iframe-loading-lazy-multiple-queued-navigations.html
+<!DOCTYPE html>
+<head>
+  <title>Multiple queued lazy load navigations do not crash the page</title>
+  <link rel="author" title="Dom Farolino" href="mailto:dom@chromium.org">
+  <link rel="help" href="https://html.spec.whatwg.org/multipage/urls-and-fetching.html#lazy-loading-attributes">
+  <script src="/resources/testharness.js"></script>
+  <script src="/resources/testharnessreport.js"></script>
+</head>
+
+<script>
+  const t = async_test('Multiple queued lazy load navigations do not crash ' +
+                       'the page');
+
+  let has_below_viewport_loaded = false;
+
+  window.addEventListener("load", t.step_func(() => {
+    assert_false(has_below_viewport_loaded,
+                "The below_viewport element should not have loaded before " +
+                "window.load().");
+
+    // Queue another lazy load navigation on the iframe. This should not result
+    // in multiple internal intersection observers being created for the iframe
+    // element, but instead should result in only one intersection observer
+    // associated with the iframe element, and the resulting navigation should
+    // be for the latest `src` attribute mutation.
+    const target = document.querySelector('#below_viewport');
+    target.src = 'resources/subframe.html?new-src';
+    target.scrollIntoView();
+  }));
+
+  const below_viewport_iframe_onload = t.step_func_done(() => {
+    const target = document.querySelector('#below_viewport');
+    // We check both of these to ensure that the `src` attribute and actual
+    // navigated resource do not get out-of-sync when navigating to lazy loaded
+    // resources.
+    assert_true(
+      target.src.includes('new-src'),
+      "The iframe's src should be updated to reflect the latest `src` " +
+      "mutation");
+    assert_true(
+      target.contentDocument.location.href.includes('new-src'),
+      'The iframe should be navigated to the resource provided by the latest ' +
+      '`src` mutation');
+  });
+</script>
+
+<body>
+  <div style="height:3000vh;"></div>
+  <iframe id="below_viewport" src="resources/subframe.html?old-src"
+          loading="lazy" width="200px" height="100px"
+          onload="below_viewport_iframe_onload();"></iframe>
+</body>