Prevent the browser process from creating duplicate RenderViewHosts.

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 2eb7b51885b48168df9a248d249eee2bc44195dd..ae35fddc588c4827089019540e6d439665efcd17 100644
--- a/content/browser/security_exploit_browsertest.cc
+++ b/content/browser/security_exploit_browsertest.cc
@@ -3,11 +3,19 @@
 // found in the LICENSE file.
 
 #include "base/command_line.h"
+#include "base/containers/hash_tables.h"
+#include "content/browser/dom_storage/dom_storage_context_wrapper.h"
+#include "content/browser/dom_storage/session_storage_namespace_impl.h"
+#include "content/browser/renderer_host/render_view_host_factory.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
+#include "content/common/view_messages.h"
+#include "content/public/browser/browser_context.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
+#include "content/public/browser/storage_partition.h"
 #include "content/public/common/content_switches.h"
+#include "content/public/test/browser_test_utils.h"
 #include "content/public/test/test_utils.h"
 #include "content/shell/browser/shell.h"
 #include "content/test/content_browser_test.h"
@@ -52,4 +60,86 @@ IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest, SetWebUIProperty) {
   terminated.Wait();
 }
 
+namespace {
+
+// This is a helper function for the tests which attempt to create a

[Charlie Reis, 2013/12/03 22:15:49]

I think helper functions for tests usually go at the top of the file, above the
class definition (e.g., browser_browsertest.cc).

[nasko, 2013/12/03 22:25:38]

Done.

+// duplicate RenderViewHost or RenderWidgetHost. It tries to create two objects
+// with the same process and routing ids, which causes a collision.
+// It creates a couple of windows in process 1, which causes a few routing ids
+// to be allocated. Then a cross-process navigation is initiated, which causes a
+// new process 2 to be created and have a pending RenderViewHost for it.

[Charlie Reis, 2013/12/03 22:15:49]

Please mention something about how the pending RenderViewHost is returned and
what the out parameter is.

Actually, I think the names here are a bit confusing, since this helper function
doesn't appear to create any duplicates.  Maybe PrepareToDuplicateHosts(shell,
target_routing_id) would be clearer?

[nasko, 2013/12/03 22:25:38]

Done.

+RenderViewHostImpl* SetupForDuplicateHosts(Shell* shell,
+                                           int* duplicate_routing_id) {
+  GURL foo("http://foo.com/files/simple_page.html");
+
+  // Start off with initial navigation, so we get the first process allocated.
+  NavigateToURL(shell, foo);
+
+  // Open another window, so we generate some more routing ids.
+  ShellAddedObserver shell2_observer;
+  EXPECT_TRUE(ExecuteScript(
+      shell->web_contents(), "window.open(document.URL + '#2');"));
+  Shell* shell2 = shell2_observer.GetShell();
+
+  // The new window must be in the same process, but have a new routing id.
+  EXPECT_EQ(shell->web_contents()->GetRenderViewHost()->GetProcess()->GetID(),
+            shell2->web_contents()->GetRenderViewHost()->GetProcess()->GetID());
+  *duplicate_routing_id =
+      shell2->web_contents()->GetRenderViewHost()->GetRoutingID();
+  EXPECT_NE(*duplicate_routing_id,
+            shell->web_contents()->GetRenderViewHost()->GetRoutingID());
+
+  // Now, simulate a link click coming from the renderer.
+  GURL extension_url("https://bar.com/files/simple_page.html");
+  WebContentsImpl* wc = static_cast<WebContentsImpl*>(shell->web_contents());
+  wc->RequestOpenURL(
+      shell->web_contents()->GetRenderViewHost(), extension_url,
+      Referrer(), CURRENT_TAB, wc->GetFrameTree()->root()->frame_id(),
+      false, true);
+
+  // Since the navigation above requires a cross-process swap, there will be a
+  // pending RenderViewHost. Ensure it exists and is in a different process
+  // than the initial page.
+  RenderViewHostImpl* pending_rvh =
+      wc->GetRenderManagerForTesting()->pending_render_view_host();
+  EXPECT_TRUE(pending_rvh != NULL);
+  EXPECT_NE(shell->web_contents()->GetRenderViewHost()->GetProcess()->GetID(),
+            pending_rvh->GetProcess()->GetID());
+
+  return pending_rvh;
 }
+
+}  // namespace
+
+// This is a test for crbug.com/312016 attempting to create duplicate
+// RenderViewHosts. SetupForDuplicateHosts sets up this test case and leaves
+// it in a state with pending RenderViewHost. Before the commit of the new
+// pending RenderViewHost, this test case creates a new window through the new
+// process.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest,
+                       AttemptDuplicateRenderViewHost) {
+  int duplicate_routing_id = MSG_ROUTING_NONE;
+  RenderViewHostImpl* pending_rvh =
+      SetupForDuplicateHosts(shell(), &duplicate_routing_id);

[Charlie Reis, 2013/12/03 22:15:49]

Let's check that this is no longer MSG_ROUTING_NONE.

[nasko, 2013/12/03 22:25:38]

Done.

+
+  // 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.
+  ViewHostMsg_CreateWindow_Params params;
+  DOMStorageContextWrapper* dom_storage_context =
+      static_cast<DOMStorageContextWrapper*>(
+          BrowserContext::GetStoragePartition(
+              shell()->web_contents()->GetBrowserContext(),
+              pending_rvh->GetSiteInstance())->GetDOMStorageContext());
+  SessionStorageNamespaceImpl* session_storage =
+      new SessionStorageNamespaceImpl(dom_storage_context);
+  // Cause a deliberate collision in routing ids.
+  int main_frame_routing_id = duplicate_routing_id + 1;
+

[Charlie Reis, 2013/12/03 22:15:49]

nit: No blank line needed.

[nasko, 2013/12/03 22:25:38]

Done.

+  pending_rvh->CreateNewWindow(
+      duplicate_routing_id, main_frame_routing_id, params, session_storage);
+
+  // If the above operation doesn't cause a crash, the test has succeeded!
+}
+
+}  // namespace content