Detached subframes with unload handler should not be visible to others.

When a frame is unloading in one process, it must not be visible from the other
processes at some point. Previously, it was done when the frame completed the
unload, now it will be done when it starts unloading.

To achieve that, the RenderFrameProxyHost are removed when the frame starts
unloading. Moreover, no new RenderFrameProxyHosts are created in a frame with an
unloading current document.

+ 2 tests added, originally made by lukasza@chromium.org (Thanks!)

Bug: 960006, 950625
Change-Id: I3f6fc405219a08d9d61f5c0ed8772601f9dc8835
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1599182
Commit-Queue: Arthur Sonzogni <arthursonzogni@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659004}

content/browser/frame_host/render_frame_host_impl.cc

@@ -2080,6 +2080,11 @@ void RenderFrameHostImpl::OnDetach() {
   // descendant frames to execute unload handlers. Start executing those
   // handlers now.
   StartPendingDeletionOnSubtree();
+
+  // Ensure that deleted subframes are not visible from the others processes
+  // anymore.
+  frame_tree_node_->render_manager()->ResetProxyHosts();
+
   // Some children with no unload handler may be eligible for immediate
   // deletion. Cut the dead branches now. This is a performance optimization.
   PendingDeletionCheckCompletedOnSubtree();  // Can delete |this|.
@@ -2372,9 +2377,14 @@ void RenderFrameHostImpl::DetachFromProxy() {
   // Start pending deletion on this frame and its children.
   DeleteRenderFrame();
   StartPendingDeletionOnSubtree();
+
+  // Ensure that deleted subframes are not visible from the others processes
+  // anymore.
+  frame_tree_node()->render_manager()->ResetProxyHosts();
+
   // Some children with no unload handler may be eligible for immediate
   // deletion. Cut the dead branches now. This is a performance optimization.
-  PendingDeletionCheckCompletedOnSubtree();
+  PendingDeletionCheckCompletedOnSubtree();  // May delete |this|.
 }
 
 void RenderFrameHostImpl::OnBeforeUnloadACK(
@@ -4514,6 +4524,10 @@ void RenderFrameHostImpl::StartPendingDeletionOnSubtree() {
                 ? UnloadState::InProgress
                 : UnloadState::Completed;
       }
+
+      // Ensure that deleted subframes are not visible from the others processes
+      // anymore.
+      node->render_manager()->ResetProxyHosts();
     }
   }
 }

content/browser/frame_host/render_frame_host_impl.h

@@ -1055,6 +1055,12 @@ class CONTENT_EXPORT RenderFrameHostImpl
                            NavigationCommitInIframePendingDeletionABC);
   FRIEND_TEST_ALL_PREFIXES(SitePerProcessBrowserTest,
                            CommittedOriginIncompatibleWithOriginLock);
+  FRIEND_TEST_ALL_PREFIXES(
+      SitePerProcessBrowserTest,
+      IsDetachedSubframeObservableDuringUnloadHandlerSameProcess);
+  FRIEND_TEST_ALL_PREFIXES(
+      SitePerProcessBrowserTest,
+      IsDetachedSubframeObservableDuringUnloadHandlerCrossProcess);
 
   class DroppedInterfaceRequestLogger;
 

content/browser/frame_host/render_frame_proxy_host.cc

@@ -168,6 +168,13 @@ bool RenderFrameProxyHost::OnMessageReceived(const IPC::Message& msg) {
 bool RenderFrameProxyHost::InitRenderFrameProxy() {
   DCHECK(!render_frame_proxy_created_);
 
+  // If the current RenderFrameHost is pending deletion, no new proxies should
+  // be created for it, since this frame should no longer be visible from other
+  // processes. We can get here with postMessage while trying to recreate
+  // proxies for the sender.
+  if (!frame_tree_node_->current_frame_host()->is_active())
+    return false;
+
   // It is possible to reach this when the process is dead (in particular, when
   // creating proxies from CreateProxiesForChildFrame).  In that case, don't
   // create the proxy.  The process shouldn't be resurrected just to create

content/browser/site_per_process_unload_browsertest.cc

@@ -1016,4 +1016,187 @@ IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest,
   }
 }
 
+// Regression test for https://crbug.com/960006.
+//
+// 1. Navigate to a1(a2(b3),c4),
+// 2. b3 has a slow unload handler.
+// 3. a2 navigates same process.
+// 4. When the new document is loaded, a message is sent to c4 to check it
+//    cannot see b3 anymore, even if b3 is still unloading.
+IN_PROC_BROWSER_TEST_F(
+    SitePerProcessBrowserTest,
+    IsDetachedSubframeObservableDuringUnloadHandlerSameProcess) {
+  GURL page_url(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(a(b),c)"));
+  EXPECT_TRUE(NavigateToURL(shell(), page_url));
+  RenderFrameHostImpl* node1 =
+      static_cast<WebContentsImpl*>(shell()->web_contents())
+          ->GetFrameTree()
+          ->root()
+          ->current_frame_host();
+  RenderFrameHostImpl* node2 = node1->child_at(0)->current_frame_host();
+  RenderFrameHostImpl* node3 = node2->child_at(0)->current_frame_host();
+  RenderFrameHostImpl* node4 = node1->child_at(1)->current_frame_host();
+  ASSERT_TRUE(ExecJs(node1, "window.name = 'node1'"));
+  ASSERT_TRUE(ExecJs(node2, "window.name = 'node2'"));
+  ASSERT_TRUE(ExecJs(node3, "window.name = 'node3'"));
+  ASSERT_TRUE(ExecJs(node4, "window.name = 'node4'"));
+
+  // Test sanity check.
+  EXPECT_EQ(true, EvalJs(node1, "!!top.node2.node3"));
+  EXPECT_EQ(true, EvalJs(node2, "!!top.node2.node3"));
+  EXPECT_EQ(true, EvalJs(node3, "!!top.node2.node3"));
+  EXPECT_EQ(true, EvalJs(node4, "!!top.node2.node3"));
+
+  // Simulate a long-running unload handler in |node3|.
+  auto detach_filter = base::MakeRefCounted<DropMessageFilter>(
+      FrameMsgStart, FrameHostMsg_Detach::ID);
+  node3->GetProcess()->AddFilter(detach_filter.get());
+  node2->DisableSwapOutTimerForTesting();
+  ASSERT_TRUE(ExecJs(node3, "window.onunload = ()=>{}"));
+
+  // Prepare |node4| to respond to postMessage with a report of whether it can
+  // still find |node3|.
+  const char* kPostMessageHandlerScript = R"(
+      window.postMessageGotData == false;
+      window.postMessageCallback = function() {};
+      function receiveMessage(event) {
+          console.log('node4 - receiveMessage...');
+
+          var can_node3_be_found = false;
+          try {
+            can_node3_be_found = !!top.node2.node3;
+          } catch(e) {
+            can_node3_be_found = false;
+          }
+
+          window.postMessageGotData = true;
+          window.postMessageData = can_node3_be_found;
+          window.postMessageCallback(window.postMessageData);
+      }
+      window.addEventListener("message", receiveMessage, false);
+  )";
+  ASSERT_TRUE(ExecJs(node4, kPostMessageHandlerScript));
+
+  // Make |node1| navigate |node2| same process and after the navigation
+  // succeeds, send a post message to |node4|. We expect that the effects of the
+  // commit should be visible to |node4| by the time it receives the posted
+  // message.
+  const char* kNavigationScript = R"(
+      var node2_frame = document.getElementsByTagName('iframe')[0];
+      node2_frame.onload = function() {
+          console.log('node2_frame.onload ...');
+          node4.postMessage('try to find node3', '*');
+      };
+      node2_frame.src = $1;
+  )";
+  GURL url = embedded_test_server()->GetURL("a.com", "/title1.html");
+  ASSERT_TRUE(ExecJs(node1, JsReplace(kNavigationScript, url)));
+
+  // Check if |node4| has seen |node3| even after |node2| navigation finished
+  // (no other frame should see |node3| after the navigation of its parent).
+  const char* kPostMessageResultsScript = R"(
+      new Promise(function (resolve, reject) {
+          if (window.postMessageGotData)
+            resolve(window.postMessageData);
+          else
+            window.postMessageCallback = resolve;
+      });
+  )";
+  EXPECT_EQ(false, EvalJs(node4, kPostMessageResultsScript));
+}
+
+// Regression test for https://crbug.com/960006.
+//
+// 1. Navigate to a1(a2(b3),c4),
+// 2. b3 has a slow unload handler.
+// 3. a2 navigates cross process.
+// 4. When the new document is loaded, a message is sent to c4 to check it
+//    cannot see b3 anymore, even if b3 is still unloading.
+//
+// Note: This test is the same as the above, except it uses a cross-process
+// navigation at step 3.
+IN_PROC_BROWSER_TEST_F(
+    SitePerProcessBrowserTest,
+    IsDetachedSubframeObservableDuringUnloadHandlerCrossProcess) {
+  GURL page_url(embedded_test_server()->GetURL(
+      "a.com", "/cross_site_iframe_factory.html?a(a(b),c)"));
+  EXPECT_TRUE(NavigateToURL(shell(), page_url));
+  RenderFrameHostImpl* node1 =
+      static_cast<WebContentsImpl*>(shell()->web_contents())
+          ->GetFrameTree()
+          ->root()
+          ->current_frame_host();
+  RenderFrameHostImpl* node2 = node1->child_at(0)->current_frame_host();
+  RenderFrameHostImpl* node3 = node2->child_at(0)->current_frame_host();
+  RenderFrameHostImpl* node4 = node1->child_at(1)->current_frame_host();
+  ASSERT_TRUE(ExecJs(node1, "window.name = 'node1'"));
+  ASSERT_TRUE(ExecJs(node2, "window.name = 'node2'"));
+  ASSERT_TRUE(ExecJs(node3, "window.name = 'node3'"));
+  ASSERT_TRUE(ExecJs(node4, "window.name = 'node4'"));
+
+  // Test sanity check.
+  EXPECT_EQ(true, EvalJs(node1, "!!top.node2.node3"));
+  EXPECT_EQ(true, EvalJs(node2, "!!top.node2.node3"));
+  EXPECT_EQ(true, EvalJs(node3, "!!top.node2.node3"));
+  EXPECT_EQ(true, EvalJs(node4, "!!top.node2.node3"));
+
+  // Add a long-running unload handler to |node3|.
+  auto detach_filter = base::MakeRefCounted<DropMessageFilter>(
+      FrameMsgStart, FrameHostMsg_Detach::ID);
+  node3->GetProcess()->AddFilter(detach_filter.get());
+  node2->DisableSwapOutTimerForTesting();
+  ASSERT_TRUE(ExecJs(node3, "window.onunload = ()=>{}"));
+
+  // Prepare |node4| to respond to postMessage with a report of whether it can
+  // still find |node3|.
+  const char* kPostMessageHandlerScript = R"(
+      window.postMessageGotData == false;
+      window.postMessageCallback = function() {};
+      function receiveMessage(event) {
+          console.log('node4 - receiveMessage...');
+
+          var can_node3_be_found = false;
+          try {
+            can_node3_be_found = !!top.node2.node3;
+          } catch(e) {
+            can_node3_be_found = false;
+          }
+
+          window.postMessageGotData = true;
+          window.postMessageData = can_node3_be_found;
+          window.postMessageCallback(window.postMessageData);
+      }
+      window.addEventListener("message", receiveMessage, false);
+  )";
+  ASSERT_TRUE(ExecJs(node4, kPostMessageHandlerScript));
+
+  // Make |node1| navigate |node2| cross process and after the navigation
+  // succeeds, send a post message to |node4|. We expect that the effects of the
+  // commit should be visible to |node4| by the time it receives the posted
+  // message.
+  const char* kNavigationScript = R"(
+      var node2_frame = document.getElementsByTagName('iframe')[0];
+      node2_frame.onload = function() {
+          console.log('node2_frame.onload ...');
+          node4.postMessage('try to find node3', '*');
+      };
+      node2_frame.src = $1;
+  )";
+  GURL url = embedded_test_server()->GetURL("d.com", "/title1.html");
+  ASSERT_TRUE(ExecJs(node1, JsReplace(kNavigationScript, url)));
+
+  // Check if |node4| has seen |node3| even after |node2| navigation finished
+  // (no other frame should see |node3| after the navigation of its parent).
+  const char* kPostMessageResultsScript = R"(
+      new Promise(function (resolve, reject) {
+          if (window.postMessageGotData)
+            resolve(window.postMessageData);
+          else
+            window.postMessageCallback = resolve;
+      });
+  )";
+  EXPECT_EQ(false, EvalJs(node4, kPostMessageResultsScript));
+}
+
 }  // namespace content