Service CreateNewWindow on the UI thread with a new mojo interface

content/browser/security_exploit_browsertest.cc

Index: content/browser/security_exploit_browsertest.cc
diff --git a/content/browser/security_exploit_browsertest.cc b/content/browser/security_exploit_browsertest.cc
index 6ae8ed6c56688d63329b8042bb3881588f37ee26..722a83997cba2ab8d308349c8b6df0c55c0a680e 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -265,28 +265,13 @@ IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
       PrepareToDuplicateHosts(shell(), &duplicate_routing_id);
   EXPECT_NE(MSG_ROUTING_NONE, duplicate_routing_id);
 
-  // Since this test executes on the UI thread and hopping threads might cause
-  // different timing in the test, let's simulate a CreateNewWindow call coming
-  // from the IO thread.
-  DOMStorageContextWrapper* dom_storage_context =
-      static_cast<DOMStorageContextWrapper*>(
-          BrowserContext::GetStoragePartition(
-              shell()->web_contents()->GetBrowserContext(),
-              pending_rvh->GetSiteInstance())->GetDOMStorageContext());
-  scoped_refptr<SessionStorageNamespaceImpl> session_storage(
-      new SessionStorageNamespaceImpl(dom_storage_context));
-  // Cause a deliberate collision in routing ids.
-  int32_t main_frame_routing_id = duplicate_routing_id + 1;
-  // TODO(avi): This should be made unique from the view routing ID once
-  // RenderViewHostImpl has-a RenderWidgetHostImpl. https://crbug.com/545684
-  int32_t main_frame_widget_routing_id = duplicate_routing_id;
-  static_cast<RenderFrameHostImpl*>(pending_rvh->GetMainFrame())
-      ->OnCreateNewWindow(duplicate_routing_id, main_frame_routing_id,
-                          main_frame_widget_routing_id,
-                          mojom::CreateNewWindowParams(),
-                          session_storage.get());
-
-  // If the above operation doesn't cause a crash, the test has succeeded!
+  RenderFrameHostImpl* opener =
+      static_cast<RenderFrameHostImpl*>(pending_rvh->GetMainFrame());
+  // TODO(nick,csharrison): This test doesn't test much, because new windows
+  // cannot be opened from RFH that are not current. It would be nice to
+  // refactor this test to actually call into the delegate to open a new window
+  // if possible.
+  EXPECT_NE(opener->frame_tree_node()->current_frame_host(), opener);

[ncarter (slow), 2017/04/20 21:30:36]

I remember spending some time thinking about this test with the previous
refactor. I agree it's basically lame now -- the original bug had to do with the
fact that we were mixing a routing id from the pending RVH, with the process id
of the current RVH (I think via WebContents::GetRenderProcessHost() or
something).

So, lately, this test's purpose was to make sure that we rejected the
OnCreateNewWindow from a non-current host. I think it can still basically do
that, if we add a FRIEND_TEST to be able to call the mojo fn directly?

I'm also okay deleting the test. I'm pretty sure we don't have the underlying
bug anymore.

[Charlie Harrison, 2017/04/21 15:30:07]

FRIENDED and tested the mojo method directly.

 }
 
 // This is a test for crbug.com/312016. It tries to create two RenderWidgetHosts