Fix crash when updating window.opener of inner WebContents.

It is possible for window objects in <webview> tags to have
window.opener relationships.  In particular, one <webview> can use
window.open('', 'foo') to look up another <webview> which has
window.name equal to 'foo', and update its window.opener to point to
itself.  This triggers an UpdateOpener IPC, which ensures that the new
opener chain has proper proxies so that 'foo' is able to reach any
frame on that chain (see RenderFrameHostManager::DidChangeOpener).

Unfortunately, this had a bug because the proxy creation also tried to
create proxies for the outer delegate (i.e., the chrome app)
SiteInstance, for all frames on the new opener chain, including
subframes.  This is wrong because we intentionally do not want to
expose a guest's subframes to the embedder -- see
RenderFrameHostManager::CreateProxiesForChildFrame(), which
intentionally skips the outer delegate's proxy.  In this case, this
later led to a crash in CreateRenderFrameProxy(), because the proxy we
tried to create for a subframe didn't have a RenderViewHost (which is
intentional -- we do not create a RVH for the outer delegate's
SiteInstance, just the proxy, and only for the guest's main frame).

This CL fixes this by ensuring that we skip the outer delegate
SiteInstance when creating proxies for subframes in
CreateProxiesForSiteInstance(), which is used when updating the opener
chain as well as more generically (e.g., when creating speculative
RenderFrameHosts).  This hopefully will better protect against this
type of bug in the future compared to a more targeted fix in
opener update logic (e.g., if/when we add OOPIF support in
GuestViews).

Change-Id: I6a2e452883faa140a86523b2afda372c62cbdbcb
Bug: 1013553
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1903245Reviewed-by: James MacLean <wjmaclean@chromium.org>
Commit-Queue: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713881}

chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc

@@ -1073,6 +1073,51 @@ IN_PROC_BROWSER_TEST_F(WebViewNewWindowInteractiveTest,
              NEEDS_TEST_SERVER);
 }
 
+// Ensure that when one <webview> makes a window.open() call that references
+// another <webview> by name, the opener is updated without a crash. Regression
+// test for https://crbug.com/1013553.
+IN_PROC_BROWSER_TEST_F(WebViewNewWindowInteractiveTest,
+                       NewWindow_UpdateOpener) {
+  TestHelper("testNewWindowAndUpdateOpener", "web_view/newwindow",
+             NEEDS_TEST_SERVER);
+
+  // The first <webview> tag in the test will run window.open(), which the
+  // embedder will translate into an injected second <webview> tag, after which
+  // test control will return here.  Wait until there are two guests; i.e.,
+  // until the second <webview>'s guest is also created.
+  GetGuestViewManager()->WaitForNumGuestsCreated(2);
+
+  std::vector<content::WebContents*> guest_contents_list;
+  GetGuestViewManager()->GetGuestWebContentsList(&guest_contents_list);
+  ASSERT_EQ(2u, guest_contents_list.size());
+  content::WebContents* guest1 = guest_contents_list[0];
+  content::WebContents* guest2 = guest_contents_list[1];
+  EXPECT_TRUE(content::WaitForLoadStop(guest1));
+  EXPECT_TRUE(content::WaitForLoadStop(guest2));
+  ASSERT_NE(guest1, guest2);
+
+  // Change first guest's window.name to "foo" and check that it does not
+  // have an opener to start with.
+  EXPECT_TRUE(content::ExecJs(guest1, "window.name = 'foo'"));
+  EXPECT_EQ("foo", content::EvalJs(guest1, "window.name"));
+  EXPECT_EQ(true, content::EvalJs(guest1, "window.opener == null"));
+
+  // Create a subframe in the second guest.  This is needed because the crash
+  // in crbug.com/1013553 only happened when trying to incorrectly create
+  // proxies for a subframe.
+  EXPECT_TRUE(content::ExecuteScript(
+      guest2, "document.body.appendChild(document.createElement('iframe'));"));
+
+  // Update the opener of |guest1| to point to |guest2|.  This triggers
+  // creation of proxies on the new opener chain, which should not crash.
+  EXPECT_TRUE(content::ExecuteScript(guest2, "window.open('', 'foo');"));
+
+  // Ensure both guests have the proper opener relationship set up.  Namely,
+  // each guest's opener should point to the other guest, creating a cycle.
+  EXPECT_EQ(true, content::EvalJs(guest1, "window.opener.opener === window"));
+  EXPECT_EQ(true, content::EvalJs(guest2, "window.opener.opener === window"));
+}
+
 // There is a problem of missing keyup events with the command key after
 // the NSEvent is sent to NSApplication in ui/base/test/ui_controls_mac.mm .
 // This test is disabled on only the Mac until the problem is resolved.

chrome/test/data/extensions/platform_apps/web_view/newwindow/embedder.js

@@ -570,6 +570,33 @@ function testNewWindowWebRequestRemoveElement() {
   embedder.setUpNewWindowRequest_(webview, 'guest.html', '', testName);
 }
 
+function testNewWindowAndUpdateOpener() {
+  var testName = 'testNewWindowAndUpdateOpener';
+  var webview = embedder.setUpGuest_('foobar');
+
+  // Convert window.open requests into new <webview> tags.
+  var onNewWindow = function(e) {
+    chrome.test.log('Embedder notified on newwindow');
+    embedder.assertCorrectEvent_(e, '');
+
+    var newwebview = document.createElement('webview');
+    document.querySelector('#webview-tag-container').appendChild(newwebview);
+    try {
+      e.window.attach(newwebview);
+    } catch (e) {
+      embedder.test.fail();
+    }
+
+    // Exit after the first opened window is attached.  The rest of the test is
+    // implemented on the C++ side.
+    embedder.test.succeed();
+  };
+  webview.addEventListener('newwindow', onNewWindow);
+
+  // Load a new window with the given name.
+  embedder.setUpNewWindowRequest_(webview, 'guest.html', '', testName);
+}
+
 embedder.test.testList = {
   'testNewWindowAttachAfterOpenerDestroyed':
       testNewWindowAttachAfterOpenerDestroyed,
@@ -589,7 +616,8 @@ embedder.test.testList = {
   'testNewWindowWebRequestCloseWindow': testNewWindowWebRequestCloseWindow,
   'testNewWindowWebRequestRemoveElement': testNewWindowWebRequestRemoveElement,
   'testNewWindowWebViewNameTakesPrecedence':
-      testNewWindowWebViewNameTakesPrecedence
+      testNewWindowWebViewNameTakesPrecedence,
+  'testNewWindowAndUpdateOpener': testNewWindowAndUpdateOpener
 };
 
 onload = function() {

content/browser/frame_host/frame_tree.cc

@@ -272,6 +272,17 @@ void FrameTree::CreateProxiesForSiteInstance(FrameTreeNode* source,
     }
   }
 
+  // Check whether we're in an inner delegate and |site_instance| corresponds
+  // to the outer delegate.  Subframe proxies aren't needed if this is the
+  // case.
+  bool is_site_instance_for_outer_delegate = false;
+  RenderFrameProxyHost* outer_delegate_proxy =
+      root()->render_manager()->GetProxyToOuterDelegate();
+  if (outer_delegate_proxy) {
+    is_site_instance_for_outer_delegate =
+        (site_instance == outer_delegate_proxy->GetSiteInstance());
+  }
+
   // Proxies are created in the FrameTree in response to a node navigating to a
   // new SiteInstance. Since |source|'s navigation will replace the currently
   // loaded document, the entire subtree under |source| will be removed, and
@@ -299,6 +310,14 @@ void FrameTree::CreateProxiesForSiteInstance(FrameTreeNode* source,
         // race described above not possible.
         continue;
       }
+
+      // Do not create proxies for subframes in the outer delegate's
+      // SiteInstance, since there is no need to expose these subframes to the
+      // outer delegate.  See also comments in CreateProxiesForChildFrame() and
+      // https://crbug.com/1013553.
+      if (!node->IsMainFrame() && is_site_instance_for_outer_delegate)
+        continue;
+
       node->render_manager()->CreateRenderFrameProxy(site_instance);
     }
   }