New issue
Advanced search Search tips

Issue 613244 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 583586



Sign in to add a comment

OOPIF: Navigating in the unload handler causes the browser and renderer to go out-of-sync

Project Member Reported by lfg@chromium.org, May 19 2016

Issue description

During a cross-process navigation, if the renderer navigates in an unload handler, we leak the process and leave the renderer/browser process out-of-sync. This happens because the browser has already added the RFH to the pending delete list, and created a new process/RFH for the next navigation.

Only in-page navigations are allowed in unload handlers, here's a test that reproduces the problem.

IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, NavigateInUnloadHandler) {
  GURL main_url(embedded_test_server()->GetURL(
    "a.com", "/cross_site_iframe_factory.html?a(b(b))"));
  NavigateToURL(shell(), main_url);

  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
    ->GetFrameTree()
    ->root();

  EXPECT_EQ(
    " Site A ------------ proxies for B\n"
    "   +--Site B ------- proxies for A\n"
    "        +--Site B -- proxies for A\n"
    "Where A = http://a.com/\n"
    "      B = http://b.com/",
    DepictFrameTree(root));

  int child_count = 0;
  EXPECT_TRUE(ExecuteScriptAndExtractInt(
    root->child_at(0)->current_frame_host(),
    "window.domAutomationController.send(frames.length);", &child_count));
  EXPECT_EQ(1, child_count);

  EXPECT_TRUE(ExecuteScript(
    root->child_at(0)->child_at(0)->current_frame_host(),
    "window.onunload=function(e){\n"
    "    window.location = '#navigate';\n"
    "};\n"));
  std::string script =
    std::string("window.document.getElementById('child-0').src = \"") +
    embedded_test_server()
    ->GetURL("c.com", "/cross_site_iframe_factory.html?c")
    .spec() +
    "\"";
  EXPECT_TRUE(
    ExecuteScript(root->child_at(0)->current_frame_host(), script.c_str()));

  RenderFrameDeletedObserver deleted_observer(
    root->child_at(0)->child_at(0)->current_frame_host());
  deleted_observer.WaitUntilDeleted();

  EXPECT_TRUE(ExecuteScriptAndExtractInt(
    root->child_at(0)->current_frame_host(),
    "window.domAutomationController.send(frames.length);", &child_count));
  EXPECT_EQ(0, child_count);

  EXPECT_EQ(
    " Site A ------------ proxies for B\n"
    "   +--Site B ------- proxies for A\n"
    "Where A = http://a.com/\n"
    "      B = http://b.com/",
    DepictFrameTree(root));
}

 

Comment 1 by creis@chromium.org, May 19 2016

Components: UI>Browser>Navigation
For reference, here's the Intent on blink-dev for changing this behavior:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/VfItzNe3WO0

The current plan is to add a use counter and deprecation message to see how many sites would be affected by blocking this.
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18ed527d63e722878207ae59b2f5744fca338152

commit 18ed527d63e722878207ae59b2f5744fca338152
Author: lfg <lfg@chromium.org>
Date: Mon Sep 12 20:47:08 2016

Adds a test that verifies that navigating in the unload handler while
performing a process transfer for a subframe doesn't crash the browser.

BUG= 613244 

Review-Url: https://codereview.chromium.org/2332073002
Cr-Commit-Position: refs/heads/master@{#418034}

[modify] https://crrev.com/18ed527d63e722878207ae59b2f5744fca338152/content/browser/site_per_process_browsertest.cc
[modify] https://crrev.com/18ed527d63e722878207ae59b2f5744fca338152/content/renderer/render_frame_impl.cc

Comment 6 by lfg@chromium.org, Sep 12 2016

Status: Fixed (was: Assigned)

Comment 7 by rbyers@chromium.org, Apr 15 2017

Blocking: 583586

Sign in to add a comment