Browser Plugin: Guests should be in their own BrowsingInstance/SiteInstances, Sandbox needs to be relaxed on Windows

Due to the complexity of managing graphics contexts across channels, and the need for relaxing sandboxing on Windows to allow renderer-to-renderer communication, guests should have their own BrowsingInstance/SiteInstance for the time being.

This restriction needs to go away in the long term.


BUG=none
TEST=content_unittests --gtest_filter="RenderViewHostManager*", and content_unittests --gtest_filter="SiteInstance*"

Review URL: https://chromiumcodereview.appspot.com/10500011

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@143544 0039d316-1c4b-4281-b951-d872f2087c98

15 files changed, 130 insertions, 9 deletions

diff --git a/content/browser/browser_plugin/old/browser_plugin_host.cc b/content/browser/browser_plugin/old/browser_plugin_host.cc
index 9997f09..8df87c4 100644
--- a/content/browser/browser_plugin/old/browser_plugin_host.cc
+++ b/content/browser/browser_plugin/old/browser_plugin_host.cc
@@ -8,6 +8,7 @@
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/common/browser_plugin_messages.h"
+#include "content/public/common/url_constants.h"
 #include "content/public/browser/notification_details.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/notification_types.h"
@@ -81,12 +82,22 @@ void BrowserPluginHost::NavigateGuestFromEmbedder(
   WebContentsImpl* guest_web_contents =
       guest_observer ?
           static_cast<WebContentsImpl*>(guest_observer->web_contents()): NULL;
+  GURL url(src);
   if (!guest_observer) {
+    std::string host = render_view_host->GetSiteInstance()->GetSite().host();
+    GURL guest_url(
+        base::StringPrintf("%s://%s", chrome::kGuestScheme, host.c_str()));
+    // The SiteInstance of a given guest is based on the fact that it's a guest
+    // in addition to which platform application the guest belongs to, rather
+    // than the URL that the guest is being navigated to.
+    SiteInstance* guest_site_instance =
+        SiteInstance::CreateForURL(web_contents()->GetBrowserContext(),
+        guest_url);
     guest_web_contents =
         static_cast<WebContentsImpl*>(
             WebContents::Create(
                 web_contents()->GetBrowserContext(),
-                render_view_host->GetSiteInstance(),
+                guest_site_instance,
                 MSG_ROUTING_NONE,
                 NULL, // base WebContents
                 NULL  // session storage namespace
@@ -99,7 +110,6 @@ void BrowserPluginHost::NavigateGuestFromEmbedder(
     RegisterContainerInstance(container_instance_id, guest_observer);
     AddGuest(guest_web_contents, frame_id);
   }
-  GURL url(src);
   guest_observer->web_contents()->SetDelegate(guest_observer);
   guest_observer->web_contents()->GetController().LoadURL(
       url,
diff --git a/content/browser/renderer_host/render_process_host_impl.cc b/content/browser/renderer_host/render_process_host_impl.cc
index 127029c..8e75933 100644
--- a/content/browser/renderer_host/render_process_host_impl.cc
+++ b/content/browser/renderer_host/render_process_host_impl.cc
@@ -98,6 +98,7 @@
 #include "content/public/common/content_switches.h"
 #include "content/public/common/process_type.h"
 #include "content/public/common/result_codes.h"
+#include "content/public/common/url_constants.h"
 #include "content/renderer/render_process_impl.h"
 #include "content/renderer/render_thread_impl.h"
 #include "ipc/ipc_channel.h"
@@ -261,7 +262,7 @@ void RenderProcessHost::SetMaxRendererProcessCount(size_t count) {
 }
 
 RenderProcessHostImpl::RenderProcessHostImpl(
-    BrowserContext* browser_context)
+    BrowserContext* browser_context, bool is_guest)
         : fast_shutdown_started_(false),
           deleting_soon_(false),
           pending_views_(0),
@@ -274,7 +275,8 @@ RenderProcessHostImpl::RenderProcessHostImpl(
           id_(ChildProcessHostImpl::GenerateChildProcessUniqueId()),
           browser_context_(browser_context),
           sudden_termination_allowed_(true),
-          ignore_input_events_(false) {
+          ignore_input_events_(false),
+          is_guest_(is_guest) {
   widget_helper_ = new RenderWidgetHelper();
 
   ChildProcessSecurityPolicyImpl::GetInstance()->Add(GetID());
@@ -599,11 +601,17 @@ int RenderProcessHostImpl::VisibleWidgetCount() const {
   return visible_widgets_;
 }
 
+bool RenderProcessHostImpl::IsGuest() const {
+  return is_guest_;
+}
+
 void RenderProcessHostImpl::AppendRendererCommandLine(
     CommandLine* command_line) const {
   // Pass the process type first, so it shows first in process listings.
   command_line->AppendSwitchASCII(switches::kProcessType,
                                   switches::kRendererProcess);
+  if (is_guest_)
+    command_line->AppendSwitch(switches::kGuestRenderer);
 
   // Now send any options from our own command line we want to propagate.
   const CommandLine& browser_command_line = *CommandLine::ForCurrentProcess();
@@ -1138,6 +1146,16 @@ bool RenderProcessHostImpl::IsSuitableHost(
   if (host->GetBrowserContext() != browser_context)
     return false;
 
+  // All URLs are suitable if this is associated with a guest renderer process.
+  // TODO(fsamuel, creis): Further validation is needed to ensure that only
+  // normal web URLs are permitted in guest processes. We need to investigate
+  // where this validation should happen.
+  if (host->IsGuest())
+    return true;
+
+  if (!host->IsGuest() && site_url.SchemeIs(chrome::kGuestScheme))
+    return false;
+
   WebUIControllerFactory* factory =
       GetContentClient()->browser()->GetWebUIControllerFactory();
   if (factory &&
diff --git a/content/browser/renderer_host/render_process_host_impl.h b/content/browser/renderer_host/render_process_host_impl.h
index 5d1ce28..d16181f 100644
--- a/content/browser/renderer_host/render_process_host_impl.h
+++ b/content/browser/renderer_host/render_process_host_impl.h
@@ -52,7 +52,7 @@ class CONTENT_EXPORT RenderProcessHostImpl
       public ChildProcessLauncher::Client,
       public base::WaitableEventWatcher::Delegate {
  public:
-  explicit RenderProcessHostImpl(BrowserContext* browser_context);
+  RenderProcessHostImpl(BrowserContext* browser_context, bool is_guest);
   virtual ~RenderProcessHostImpl();
 
   // RenderProcessHost implementation (public portion).
@@ -69,6 +69,7 @@ class CONTENT_EXPORT RenderProcessHostImpl
   virtual void WidgetRestored() OVERRIDE;
   virtual void WidgetHidden() OVERRIDE;
   virtual int VisibleWidgetCount() const OVERRIDE;
+  virtual bool IsGuest() const OVERRIDE;
   virtual bool FastShutdownIfPossible() OVERRIDE;
   virtual void DumpHandles() OVERRIDE;
   virtual base::ProcessHandle GetHandle() OVERRIDE;
@@ -266,6 +267,10 @@ class CONTENT_EXPORT RenderProcessHostImpl
   // Records the last time we regarded the child process active.
   base::TimeTicks child_process_activity_time_;
 
+  // Indicates whether this is a RenderProcessHost of a Browser Plugin guest
+  // renderer.
+  bool is_guest_;
+
   DISALLOW_COPY_AND_ASSIGN(RenderProcessHostImpl);
 };
 
diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc
index c906136..8cfd20e 100644
--- a/content/browser/site_instance_impl.cc
+++ b/content/browser/site_instance_impl.cc
@@ -90,7 +90,8 @@ content::RenderProcessHost* SiteInstanceImpl::GetProcess() {
             browsing_instance_->browser_context());
       } else {
         process_ =
-            new RenderProcessHostImpl(browsing_instance_->browser_context());
+            new RenderProcessHostImpl(browsing_instance_->browser_context(),
+                                      site_.SchemeIs(chrome::kGuestScheme));
       }
     }
 
@@ -187,6 +188,10 @@ SiteInstance* SiteInstance::CreateForURL(
 /*static*/
 GURL SiteInstanceImpl::GetSiteForURL(content::BrowserContext* browser_context,
                                      const GURL& real_url) {
+  // TODO(fsamuel, creis): For some reason appID is not recognized as a host.
+  if (real_url.SchemeIs(chrome::kGuestScheme))
+    return real_url;
+
   GURL url = SiteInstanceImpl::GetEffectiveURL(browser_context, real_url);
 
   // URLs with no host should have an empty site.
diff --git a/content/browser/site_instance_impl_unittest.cc b/content/browser/site_instance_impl_unittest.cc
index 37ab73a..fa4ea57 100644
--- a/content/browser/site_instance_impl_unittest.cc
+++ b/content/browser/site_instance_impl_unittest.cc
@@ -371,6 +371,9 @@ TEST_F(SiteInstanceTest, GetSiteForURL) {
   test_url = GURL("file:///C:/Downloads/");
   EXPECT_EQ(GURL(), SiteInstanceImpl::GetSiteForURL(NULL, test_url));
 
+  test_url = GURL("guest://abc123");
+  EXPECT_EQ(GURL("guest://abc123"), SiteInstanceImpl::GetSiteForURL(
+      NULL, test_url));
   // TODO(creis): Do we want to special case file URLs to ensure they have
   // either no site or a special "file://" site?  We currently return
   // "file://home/" as the site, which seems broken.
diff --git a/content/browser/web_contents/render_view_host_manager.cc b/content/browser/web_contents/render_view_host_manager.cc
index cb7ead5..a440682 100644
--- a/content/browser/web_contents/render_view_host_manager.cc
+++ b/content/browser/web_contents/render_view_host_manager.cc
@@ -725,11 +725,13 @@ RenderViewHostImpl* RenderViewHostManager::UpdateRendererStateForNavigate(
   SiteInstance* new_instance = curr_instance;
   const content::NavigationEntry* curr_entry =
       delegate_->GetLastCommittedNavigationEntryForRenderManager();
+  bool is_guest_scheme = curr_instance->GetSite().SchemeIs(
+      chrome::kGuestScheme);
   bool force_swap = ShouldSwapProcessesForNavigation(curr_entry, &entry);
-  if (ShouldTransitionCrossSite() || force_swap)
+  if (!is_guest_scheme && (ShouldTransitionCrossSite() || force_swap))
     new_instance = GetSiteInstanceForEntry(entry, curr_instance);
 
-  if (new_instance != curr_instance || force_swap) {
+  if (!is_guest_scheme && (new_instance != curr_instance || force_swap)) {
     // New SiteInstance.
     DCHECK(!cross_navigation_pending_);
 
diff --git a/content/browser/web_contents/render_view_host_manager_unittest.cc b/content/browser/web_contents/render_view_host_manager_unittest.cc
index d438235..2d50c08 100644
--- a/content/browser/web_contents/render_view_host_manager_unittest.cc
+++ b/content/browser/web_contents/render_view_host_manager_unittest.cc
@@ -874,3 +874,63 @@ TEST_F(RenderViewHostManagerTest, EnableWebUIWithSwappedOutOpener) {
   // Ensure the new RVH has WebUI bindings.
   EXPECT_TRUE(rvh2->GetEnabledBindings() & content::BINDINGS_POLICY_WEB_UI);
 }
+
+// Test that we reuse the same guest SiteInstance if we navigate across sites.
+TEST_F(RenderViewHostManagerTest, NoSwapOnGuestNavigations) {
+  content::TestNotificationTracker notifications;
+
+  GURL guest_url("guest://abc123");
+  SiteInstance* instance =
+      SiteInstance::CreateForURL(browser_context(), guest_url);
+  TestWebContents web_contents(browser_context(), instance);
+
+  // Create.
+  RenderViewHostManager manager(&web_contents, &web_contents, &web_contents);
+
+  manager.Init(browser_context(), instance, MSG_ROUTING_NONE);
+
+  RenderViewHost* host;
+
+  // 1) The first navigation. --------------------------
+  const GURL kUrl1("http://www.google.com/");
+  NavigationEntryImpl entry1(
+      NULL /* instance */, -1 /* page_id */, kUrl1, content::Referrer(),
+      string16() /* title */, content::PAGE_TRANSITION_TYPED,
+      false /* is_renderer_init */);
+  host = manager.Navigate(entry1);
+
+  // The RenderViewHost created in Init will be reused.
+  EXPECT_TRUE(host == manager.current_host());
+  EXPECT_FALSE(manager.pending_render_view_host());
+  EXPECT_EQ(manager.current_host()->GetSiteInstance(), instance);
+
+  // Commit.
+  manager.DidNavigateMainFrame(host);
+  // Commit to SiteInstance should be delayed until RenderView commit.
+  EXPECT_EQ(host, manager.current_host());
+  ASSERT_TRUE(host);
+  EXPECT_TRUE(static_cast<SiteInstanceImpl*>(host->GetSiteInstance())->
+      HasSite());
+
+  // 2) Navigate to a different domain. -------------------------
+  // Guests stay in the same process on navigation.
+  const GURL kUrl2("http://www.chromium.org");
+  NavigationEntryImpl entry2(
+      NULL /* instance */, -1 /* page_id */, kUrl2,
+      content::Referrer(kUrl1, WebKit::WebReferrerPolicyDefault),
+      string16() /* title */, content::PAGE_TRANSITION_LINK,
+      true /* is_renderer_init */);
+  host = manager.Navigate(entry2);
+
+  // The RenderViewHost created in Init will be reused.
+  EXPECT_EQ(host, manager.current_host());
+  EXPECT_FALSE(manager.pending_render_view_host());
+
+  // Commit.
+  manager.DidNavigateMainFrame(host);
+  EXPECT_EQ(host, manager.current_host());
+  ASSERT_TRUE(host);
+  EXPECT_EQ(static_cast<SiteInstanceImpl*>(host->GetSiteInstance()),
+      instance);
+
+}
diff --git a/content/common/sandbox_policy.cc b/content/common/sandbox_policy.cc
index 6626763..9c4efa1 100644
--- a/content/common/sandbox_policy.cc
+++ b/content/common/sandbox_policy.cc
@@ -724,9 +724,11 @@ base::ProcessHandle StartProcessWithAccess(CommandLine* cmd_line,
     if (type == content::PROCESS_TYPE_RENDERER ||
         type == content::PROCESS_TYPE_WORKER) {
       AddBaseHandleClosePolicy(policy);
+    }
 
     // Pepper uses the renderer's policy, whith some tweaks.
-    } else if (type == content::PROCESS_TYPE_PPAPI_PLUGIN) {
+    if (cmd_line->HasSwitch(switches::kGuestRenderer) ||
+        type == content::PROCESS_TYPE_PPAPI_PLUGIN) {
       if (!AddPolicyForPepperPlugin(policy))
         return 0;
     }
diff --git a/content/public/browser/render_process_host.h b/content/public/browser/render_process_host.h
index 6b128dc..7f5e3b7 100644
--- a/content/public/browser/render_process_host.h
+++ b/content/public/browser/render_process_host.h
@@ -93,6 +93,10 @@ class CONTENT_EXPORT RenderProcessHost : public IPC::Sender,
   virtual void WidgetHidden() = 0;
   virtual int VisibleWidgetCount() const = 0;
 
+  // Indicates whether the current RenderProcessHost associated with a guest
+  // renderer process.
+  virtual bool IsGuest() const = 0;
+
   // Try to shutdown the associated renderer process as fast as possible.
   // If this renderer has any RenderViews with unload handlers, then this
   // function does nothing.  The current implementation uses TerminateProcess.
diff --git a/content/public/common/content_switches.cc b/content/public/common/content_switches.cc
index 0cc3e59..177813b 100644
--- a/content/public/common/content_switches.cc
+++ b/content/public/common/content_switches.cc
@@ -418,6 +418,10 @@ const char kGpuStartupDialog[]              = "gpu-startup-dialog";
 // Passes gpu vendor_id from browser process to GPU process.
 const char kGpuVendorID[]                   = "gpu-vendor-id";
 
+// Used in conjunction with kRendererProcess. This causes the process
+// to run as a guest renderer instead of a regular renderer.
+const char kGuestRenderer[]                 = "guest-renderer";
+
 // Run the GPU process as a thread in the browser process.
 const char kInProcessGPU[]                  = "in-process-gpu";
 
diff --git a/content/public/common/content_switches.h b/content/public/common/content_switches.h
index 52ae02c..4b25727 100644
--- a/content/public/common/content_switches.h
+++ b/content/public/common/content_switches.h
@@ -130,6 +130,7 @@ extern const char kGpuLauncher[];
 CONTENT_EXPORT extern const char kGpuProcess[];
 extern const char kGpuStartupDialog[];
 extern const char kGpuVendorID[];
+CONTENT_EXPORT extern const char kGuestRenderer[];
 extern const char kInProcessGPU[];
 extern const char kInProcessPlugins[];
 CONTENT_EXPORT extern const char kInProcessWebGL[];
diff --git a/content/public/common/url_constants.cc b/content/public/common/url_constants.cc
index 95e1224..85c3ead 100644
--- a/content/public/common/url_constants.cc
+++ b/content/public/common/url_constants.cc
@@ -20,6 +20,7 @@ const char kDataScheme[] = "data";
 const char kFileScheme[] = "file";
 const char kFileSystemScheme[] = "filesystem";
 const char kFtpScheme[] = "ftp";
+const char kGuestScheme[] = "guest";
 const char kHttpScheme[] = "http";
 const char kHttpsScheme[] = "https";
 const char kJavaScriptScheme[] = "javascript";
diff --git a/content/public/common/url_constants.h b/content/public/common/url_constants.h
index bf290fb..55185a4 100644
--- a/content/public/common/url_constants.h
+++ b/content/public/common/url_constants.h
@@ -26,6 +26,7 @@ CONTENT_EXPORT extern const char kDataScheme[];
 CONTENT_EXPORT extern const char kFileScheme[];
 CONTENT_EXPORT extern const char kFileSystemScheme[];
 CONTENT_EXPORT extern const char kFtpScheme[];
+CONTENT_EXPORT extern const char kGuestScheme[];
 CONTENT_EXPORT extern const char kHttpScheme[];
 CONTENT_EXPORT extern const char kHttpsScheme[];
 CONTENT_EXPORT extern const char kJavaScriptScheme[];
diff --git a/content/public/test/mock_render_process_host.h b/content/public/test/mock_render_process_host.h
index 648f697..5465336 100644
--- a/content/public/test/mock_render_process_host.h
+++ b/content/public/test/mock_render_process_host.h
@@ -46,6 +46,7 @@ class MockRenderProcessHost : public RenderProcessHost {
   virtual void WidgetRestored() OVERRIDE;
   virtual void WidgetHidden() OVERRIDE;
   virtual int VisibleWidgetCount() const OVERRIDE;
+  virtual bool IsGuest() const OVERRIDE;
   virtual void AddWord(const string16& word);
   virtual bool FastShutdownIfPossible() OVERRIDE;
   virtual bool FastShutdownStarted() const OVERRIDE;
diff --git a/content/test/mock_render_process_host.cc b/content/test/mock_render_process_host.cc
index 6f56b4b..31d94db 100644
--- a/content/test/mock_render_process_host.cc
+++ b/content/test/mock_render_process_host.cc
@@ -79,6 +79,10 @@ int MockRenderProcessHost::VisibleWidgetCount() const {
   return 1;
 }
 
+bool MockRenderProcessHost::IsGuest() const {
+  return false;
+}
+
 void MockRenderProcessHost::AddWord(const string16& word) {
 }