Skip to content

GitLab

T
tangled
Commit 88a32f24 authored by Dominic Farolino's avatar Dominic Farolino Committed by Commit Bot
Browse files
Options

Fix iframe lazy loading subsequent navigation crash 

This CL fixes a renderer crash caused by blink::RemoteFrame::Navigate
not calling LazyLoadFrameObserver::CancelPendingLazyLoad(), thus
disconnecting the underlying intersection observer.

The problem was that when the intersection observer does not get
disconnected and detached from the iframe element, then
LazyLoadFrameObserver::LoadImmediately gets called N times for a given
element, however after the first time it gets called, its
|lazy_load_request_info_| gets moved, and therefore is in an invalid
state, so future invocations of LoadImmediately try and reuse this
invalid state.

The problem could have been caught if the DCHECKs in this method were
CHECKs, so this CL turns them into CHECKs until we confirm that there
are no more issues. If there are, the CHECKs will make them much easier
to track down.

R=kinuko@chromium.org, sclittle@chromium.org

Bug: 1104664
Change-Id: I0adc8e26375406f0016031dd2d2d89ad043dd6f1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2298331Reviewed-by: default avatarKinuko Yasuda <kinuko@chromium.org>
Reviewed-by: default avatarScott Little <sclittle@chromium.org>
Commit-Queue: Dominic Farolino <dom@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788508}
parent dd519d3b
Hide whitespace changes
Inline Side-by-side
Showing with 68 additions and 6 deletions
third_party/blink/renderer/core/frame/remote_frame.cc
View file @ 88a32f24
...@@ -139,6 +139,9 @@ void RemoteFrame::Navigate(FrameLoadRequest& frame_request, ...@@ -139,6 +139,9 @@ void RemoteFrame::Navigate(FrameLoadRequest& frame_request,
if (!navigation_rate_limiter().CanProceed()) if (!navigation_rate_limiter().CanProceed())
return; return;
if (HTMLFrameOwnerElement* element = DeprecatedLocalOwner())
element->CancelPendingLazyLoad();
frame_request.SetFrameType(IsMainFrame() frame_request.SetFrameType(IsMainFrame()
? mojom::RequestContextFrameType::kTopLevel ? mojom::RequestContextFrameType::kTopLevel
: mojom::RequestContextFrameType::kNested); : mojom::RequestContextFrameType::kNested);
......
third_party/blink/renderer/core/html/lazy_load_frame_observer.cc
View file @ 88a32f24
...@@ -157,8 +157,10 @@ void LazyLoadFrameObserver::LoadIfHiddenOrNearViewport( ...@@ -157,8 +157,10 @@ void LazyLoadFrameObserver::LoadIfHiddenOrNearViewport(
} }
void LazyLoadFrameObserver::LoadImmediately() { void LazyLoadFrameObserver::LoadImmediately() {
DCHECK(IsLazyLoadPending()); // TODO(domfarolino): Change the CHECKs to DCHECKs once we confirm there are
DCHECK(lazy_load_request_info_); // no crashes.
CHECK(IsLazyLoadPending());
CHECK(lazy_load_request_info_);
if (was_recorded_as_deferred_) { if (was_recorded_as_deferred_) {
DCHECK(element_->GetDocument().GetFrame()); DCHECK(element_->GetDocument().GetFrame());
...@@ -177,10 +179,10 @@ void LazyLoadFrameObserver::LoadImmediately() { ...@@ -177,10 +179,10 @@ void LazyLoadFrameObserver::LoadImmediately() {
// The content frame of the element should not have changed, since any // The content frame of the element should not have changed, since any
// pending lazy load should have been already been cancelled in // pending lazy load should have been already been cancelled in
// DisconnectContentFrame() if the content frame changes. // DisconnectContentFrame() if the content frame changes.
DCHECK(element_->ContentFrame()); CHECK(element_->ContentFrame());
// Note that calling FrameLoader::StartNavigation() causes the // Note that calling both FrameLoader::StartNavigation() and Frame::Navigate()
// |lazy_load_intersection_observer_| to be disconnected. // causes the |lazy_load_intersection_observer_| to be disconnected.
FrameLoadRequest request(element_->GetDocument().domWindow(), FrameLoadRequest request(element_->GetDocument().domWindow(),
scoped_request_info->resource_request); scoped_request_info->resource_request);
...@@ -193,7 +195,7 @@ void LazyLoadFrameObserver::LoadImmediately() { ...@@ -193,7 +195,7 @@ void LazyLoadFrameObserver::LoadImmediately() {
scoped_request_info->frame_load_type); scoped_request_info->frame_load_type);
} }
DCHECK(!IsLazyLoadPending()); CHECK(!IsLazyLoadPending());
} }
void LazyLoadFrameObserver::StartTrackingVisibilityMetrics() { void LazyLoadFrameObserver::StartTrackingVisibilityMetrics() {
......
third_party/blink/web_tests/wpt_internal/html/semantics/embedded-content/the-iframe-element/iframe-subsequent-navigations-crash.sub.html 0 → 100644
View file @ 88a32f24
<!DOCTYPE html>
<head>
<title>Subsequent navigations to lazily loaded iframes do not crash the renderer</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>
<body>
<!-- This is used to represent the top of the viewport, so we can scroll the
below-viewport iframe out-of-view later in the test -->
<div id="top_div"></div>
<div style="height:1000vh;"></div>
<iframe id="below_viewport" loading="lazy"
src="http://{{domains[www]}}:{{ports[https][0]}}/html/semantics/embedded-content/the-iframe-element/resources/subframe.html">
</iframe>
<script>
const t = async_test();
const iframe = document.querySelector('#below_viewport');
const top_div = document.querySelector('#top_div');
let has_window_load_fired = false;
// This should be triggered first.
window.addEventListener('load', t.step_func(() => {
has_window_load_fired = true;
// Scroll the loading=lazy below-viewport iframe into view, so that it loads.
iframe.scrollIntoView();
}));
iframe.onload = t.step_func_done(async () => {
assert_true(has_window_load_fired,
"The loading=lazy below-viewport iframe should not block the " +
"window load event");
await changeIframeSourceAndScrollToTop();
});
async function changeIframeSourceAndScrollToTop() {
// Lazily load a different iframe.
iframe.src =
'http://{{domains[www2]}}:{{ports[https][0]}}/html/semantics/embedded-content/the-iframe-element/resources/subframe.html'
await new Promise(resolve => {
iframe.onload = resolve;
});
top_div.scrollIntoView();
await new Promise(resolve => {
setTimeout(resolve, 500);
});
iframe.scrollIntoView();
}
</script>
</body>
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment