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..69369e008f82e2f08934a21da7eb9efcfa9072e9 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,120 @@ IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest, SetWebUIProperty) {
   terminated.Wait();
 }
 
+// RenderViewHostFactory object which tracks creations of RenderViewHost objects
+// to ensure there are no duplicates. It is used by the following
+// AttemptDuplicateRVH test case.
+class TrackingRenderViewHostFactory : public RenderViewHostFactory {
+ public:
+  explicit TrackingRenderViewHostFactory() : has_duplicates_(false) {

[Charlie Reis, 2013/12/02 19:22:50]

nit: explicit is only needed for one-arg constructors.

[nasko, 2013/12/02 20:19:13]

Done.

+    RegisterFactory(this);
+  }
+
+  virtual ~TrackingRenderViewHostFactory() {
+    UnregisterFactory();
+  }
+
+  // RenderViewHostFactory implementation.
+  virtual RenderViewHost* CreateRenderViewHost(
+      SiteInstance* instance,
+      RenderViewHostDelegate* delegate,
+      RenderWidgetHostDelegate* widget_delegate,
+      int routing_id,
+      int main_frame_routing_id,
+      bool swapped_out) OVERRIDE {
+    std::pair<int, int> id = std::make_pair(
+        instance->GetProcess()->GetID(), routing_id);
+
+    DuplicatesMap::iterator it = rvh_map.find(id);
+    if (it != rvh_map.end())
+      has_duplicates_ = true;
+
+    RenderViewHostImpl* rvh = new RenderViewHostImpl(
+        instance, delegate, widget_delegate, routing_id, main_frame_routing_id,
+        swapped_out, false);
+    rvh_map.insert(std::make_pair(id, rvh));
+
+    return rvh;
+  }
+
+  bool has_duplicates() {
+    return has_duplicates_;
+  }
+
+ private:
+  typedef base::hash_map<std::pair<int,int>, RenderViewHostImpl*> DuplicatesMap;

[Charlie Reis, 2013/12/02 19:22:50]

Is this really a map of duplicates?  Looks like we might be replacing the
previous RVH if we see a duplicate, since it's a hash_map.  If we don't need to
retrieve the values at all, maybe it should just be a hash_set of ID pairs,
called something like IDMap?

[nasko, 2013/12/02 20:19:13]

Yes, using a hash_set definitely fits here. Fixed.

+  DuplicatesMap rvh_map;

[Charlie Reis, 2013/12/02 19:22:50]

style nit: trailing underscore

[nasko, 2013/12/02 20:19:13]

Done.

+
+  bool has_duplicates_;
+
+  DISALLOW_COPY_AND_ASSIGN(TrackingRenderViewHostFactory);
+};
+
+// This is a test for crbug.com/312016. It tries to create two RenderViewHosts
+// 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

[Charlie Reis, 2013/12/02 19:22:50]

nit: No parens around 1 or 2.  That makes it feel like a bulleted list to me.

[nasko, 2013/12/02 20:19:13]

Done.

+// 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.
+// Before the commit of the new pending RenderViewHost, it creates a new window
+// through process (2). The original bug caused the new window RenderViewHost
+// to be created in process (1) with a routing id which has already been
+// allocated.
+IN_PROC_BROWSER_TEST_F(SecurityExploitBrowserTest, AttemptDuplicateRVH) {
+  TrackingRenderViewHostFactory rvh_factory;
+  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());
+  int window2_routing_id =
+      shell2->web_contents()->GetRenderViewHost()->GetRoutingID();

[Charlie Reis, 2013/12/02 19:22:50]

Should we have a EXPECT_NE for the routing IDs, since it's mentioned in the
comment?

[nasko, 2013/12/02 20:19:13]

Done.

+
+  // Now, simulate a click to chrome-extension:// URL coming from the renderer.

[Charlie Reis, 2013/12/02 19:22:50]

chrome-extension:// shouldn't do anything special in content_browsertests, since
there's no ChromeContentBrowserClient.  What's the reason it's needed?

[nasko, 2013/12/02 20:19:13]

No real reason other than the PoC used it and it stuck. I've changed it to a
different cross-site URL.

+  GURL extension_url("chrome-extension://foo/");
+  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());
+
+  // 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(
+              wc->GetBrowserContext(),
+              pending_rvh->GetSiteInstance())->GetDOMStorageContext());
+  SessionStorageNamespaceImpl* session_storage =
+      new SessionStorageNamespaceImpl(dom_storage_context);

[Charlie Reis, 2013/12/02 19:22:50]

Huh, unfortunate that we have to add this SessionStorageNamespace creation here,
since it could be fragile if the original code changes.  If it's copied from
somewhere, perhaps we could make it a private helper method exposed to the test
instead?

[nasko, 2013/12/02 20:19:13]

This pattern is used very rarely. I'd rather not complicate this CL with
extracting out a pattern into a helper method.

+  // Cause a deliberate collision in routing ids.
+  int routing_id = window2_routing_id;
+  int main_frame_routing_id = routing_id + 1;
+
+  pending_rvh->CreateNewWindow(
+      routing_id, main_frame_routing_id, params, session_storage);
+
+  EXPECT_FALSE(rvh_factory.has_duplicates());

[Charlie Reis, 2013/12/02 19:22:50]

Do we expect the process to be killed?

[nasko, 2013/12/02 20:19:13]

In the lifetime of the test, the process doesn't have a chance to be created.
This is the main reason I'm relying on the RVH factory than an actual process
kill.

 }
+
+}  // namespace content