Skip to content

GitLab

T
tangled
Commit 4c7cbb60 authored by Arthur Sonzogni's avatar Arthur Sonzogni Committed by Commit Bot
Browse files
Options

Fix frame being detached while replacing DocumentLoader. 

When the FrameLoader start a new load. It replaces the current
provisional DocumentLoader by a new one. Removing the first one can
leave the frame without any loading DocumentLoader and triggers the
'load' event, which can detach the frame.

The bug was caused by:
https://chromium-review.googlesource.com/c/chromium/src/+/1107808
where the line checking whether the frame was detached or not was
removed.

It is now clear why this line was useful, so this CL adds it back.

Bug: 856396
Change-Id: I1bdeb47e546dbb1805659bc986d590e9900c51d1
Reviewed-on: https://chromium-review.googlesource.com/1114975Reviewed-by: default avatarDmitry Gozman <dgozman@chromium.org>
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#570754}
parent fa541a8e
Hide whitespace changes
Inline Side-by-side
Showing with 61 additions and 0 deletions
content/browser/browser_side_navigation_browsertest.cc
View file @ 4c7cbb60
...@@ -802,4 +802,59 @@ IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBaseBrowserTest, ...@@ -802,4 +802,59 @@ IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBaseBrowserTest,
EXPECT_FALSE(dom_message_queue.PopMessage(&message)); EXPECT_FALSE(dom_message_queue.PopMessage(&message));
} }
// Regression test for https://crbug.com/856396.
IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBaseBrowserTest,
ReplacingDocumentLoaderFiresLoadEvent) {
net::test_server::ControllableHttpResponse main_document_response(
embedded_test_server(), "/main_document");
net::test_server::ControllableHttpResponse iframe_response(
embedded_test_server(), "/iframe");
ASSERT_TRUE(embedded_test_server()->Start());
// 1) Load the main document.
shell()->LoadURL(embedded_test_server()->GetURL("/main_document"));
main_document_response.WaitForRequest();
main_document_response.Send(
"HTTP/1.1 200 OK\r\n"
"Content-Type: text/html; charset=utf-8\r\n"
"\r\n"
"<script>"
" var detach_iframe = function() {"
" var iframe = document.querySelector('iframe');"
" iframe.parentNode.removeChild(iframe);"
" }"
"</script>"
"<body onload='detach_iframe()'>"
" <iframe src='/iframe'></iframe>"
"</body>");
main_document_response.Done();
// 2) The iframe starts to load, but the server only have time to send the
// response's headers, not the response's body. A provisional DocumentLoader
// will be created in the renderer process, but it will never commit.
iframe_response.WaitForRequest();
iframe_response.Send(
"HTTP/1.1 200 OK\r\n"
"Content-Type: text/html; charset=utf-8\r\n"
"\r\n");
// 3) In the meantime the iframe navigates elsewhere. It causes the previous
// provisional DocumentLoader to be replaced by the new one. Removing it may
// trigger the 'load' event and delete the iframe.
EXPECT_TRUE(ExecuteScript(
shell(), "document.querySelector('iframe').src = '/title1.html'"));
// Wait for the iframe to be deleted and check the renderer process is still
// alive.
int iframe_count = 1;
while (iframe_count != 0) {
ASSERT_TRUE(ExecuteScriptAndExtractInt(
shell(),
"var iframe_count = document.getElementsByTagName('iframe').length;"
"window.domAutomationController.send(iframe_count);",
&iframe_count));
}
}
} // namespace content } // namespace content
third_party/blink/renderer/core/loader/frame_loader.cc
View file @ 4c7cbb60
...@@ -1456,6 +1456,12 @@ void FrameLoader::StartLoad(FrameLoadRequest& frame_load_request, ...@@ -1456,6 +1456,12 @@ void FrameLoader::StartLoad(FrameLoadRequest& frame_load_request,
provisional_document_loader_->SetSentDidFinishLoad(); provisional_document_loader_->SetSentDidFinishLoad();
DetachDocumentLoader(provisional_document_loader_); DetachDocumentLoader(provisional_document_loader_);
// Detaching the provisional DocumentLoader above may leave the frame without
// any loading DocumentLoader. It can causes the 'load' event to fire, which
// can be used to detach this frame.
if (!frame_->GetPage())
return;
progress_tracker_->ProgressStarted(); progress_tracker_->ProgressStarted();
// TODO(japhet): This case wants to flag the frame as loading and do nothing // TODO(japhet): This case wants to flag the frame as loading and do nothing
// else. It'd be nice if it could go through the placeholder DocumentLoader // else. It'd be nice if it could go through the placeholder DocumentLoader
......
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