Replicate a new navigation's state after destroying old frame's subtree.

Previously, when a frame A navigates to B, we set and replicated the
origin and a few other things (such as insecure request policy or CSP
headers) prior to calling DidNavigateFrame().  This turned out to be
problematic because DidNavigateFrame() destroys the A frame's subtree
(as part of CommitPending() -> ResetForNewProcess()), which might
trigger unload handlers in the old frame's subframes.  Those unload
handlers would incorrectly see the new frame's origin (B) in the old
frame's proxy.  To fix this, this CL moves DidNavigateFrame() to
be done prior to the replication of origin and other properties.

Bug: 825283
Change-Id: Iff86ac8cad17cfef5349d9bbbc41d8dadd681bb1
Reviewed-on: https://chromium-review.googlesource.com/984729Reviewed-by: Charlie Reis <creis@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546956}

content/browser/frame_host/navigator_impl.cc

@@ -474,16 +474,19 @@ void NavigatorImpl::DidNavigate(
     }
   }
 
-  // Save the origin of the new page.  Do this before calling
-  // DidNavigateFrame(), because the origin needs to be included in the SwapOut
-  // message, which is sent inside DidNavigateFrame().  SwapOut needs the
-  // origin because it creates a RenderFrameProxy that needs this to initialize
-  // its security context. This origin will also be sent to RenderFrameProxies
-  // created via mojom::Renderer::CreateView and
-  // mojom::Renderer::CreateFrameProxy.
+  // DidNavigateFrame() must be called before replicating the new origin and
+  // other properties to proxies.  This is because it destroys the subframes of
+  // the frame we're navigating from, which might trigger those subframes to
+  // run unload handlers.  Those unload handlers should still see the old
+  // frame's origin.  See https://crbug.com/825283.
+  frame_tree_node->render_manager()->DidNavigateFrame(
+      render_frame_host, params.gesture == NavigationGestureUser);
+
+  // Save the new page's origin and other properties, and replicate them to
+  // proxies, including the proxy created in DidNavigateFrame() to replace the
+  // old frame in cross-process navigation cases.
   frame_tree_node->SetCurrentOrigin(
       params.origin, params.has_potentially_trustworthy_unique_origin);
-
   frame_tree_node->SetInsecureRequestPolicy(params.insecure_request_policy);
   frame_tree_node->SetInsecureNavigationsSet(params.insecure_navigations_set);
 
@@ -494,9 +497,6 @@ void NavigatorImpl::DidNavigate(
     frame_tree_node->ResetForNavigation();
   }
 
-  frame_tree_node->render_manager()->DidNavigateFrame(
-      render_frame_host, params.gesture == NavigationGestureUser);
-
   // Update the site of the SiteInstance if it doesn't have one yet, unless
   // assigning a site is not necessary for this URL or the commit was for an
   // error page.  In that case, the SiteInstance can still be considered unused

content/browser/site_per_process_browsertest.cc

@@ -11073,4 +11073,53 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, VisibilityFrameDepthTest) {
   EXPECT_EQ(0u, popup_process->GetFrameDepthForTesting());
 }
 
+// Ensure that after a main frame with an OOPIF is navigated cross-site, the
+// unload handler in the OOPIF sees correct main frame origin, namely the old
+// and not the new origin.  See https://crbug.com/825283.
+IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
+                       ParentOriginDoesNotChangeInUnloadHandler) {
+  GURL main_url(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(b)"));
+  EXPECT_TRUE(NavigateToURL(shell(), main_url));
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+
+  // Open a popup on b.com.  The b.com subframe on the main frame will use this
+  // in its unload handler.
+  GURL b_url(embedded_test_server()->GetURL("b.com", "/title1.html"));
+  EXPECT_TRUE(OpenPopup(shell()->web_contents(), b_url, "popup"));
+
+  // Add an unload handler to b.com subframe, which will look up the top
+  // frame's origin and send it via domAutomationController.  Unfortunately,
+  // the subframe's browser-side state will have been torn down when it runs
+  // the unload handler, so to ensure that the message can be received, send it
+  // through the popup.
+  EXPECT_TRUE(
+      ExecuteScript(root->child_at(0),
+                    "window.onunload = function(e) {"
+                    "  window.open('','popup').domAutomationController.send("
+                    "      'top-origin ' + location.ancestorOrigins[0]);"
+                    "};"));
+
+  // Navigate the main frame to c.com and wait for the message from the
+  // subframe's unload handler.
+  GURL c_url(embedded_test_server()->GetURL("c.com", "/title1.html"));
+  DOMMessageQueue msg_queue;
+  EXPECT_TRUE(NavigateToURL(shell(), c_url));
+  std::string message, top_origin;
+  while (msg_queue.WaitForMessage(&message)) {
+    base::TrimString(message, "\"", &message);
+    auto message_parts = base::SplitString(message, " ", base::TRIM_WHITESPACE,
+                                           base::SPLIT_WANT_NONEMPTY);
+    if (message_parts[0] == "top-origin") {
+      top_origin = message_parts[1];
+      break;
+    }
+  }
+
+  // The top frame's origin should be a.com, not c.com.
+  EXPECT_EQ(top_origin + "/", main_url.GetOrigin().spec());
+}
+
 }  // namespace content