Allow extension process reuse in --site-per-process;

make SiteIsolationPolicy public.

The problem was that --site-per-process disabled extension
process sharing, but the site-per-process base::Feature (which
we've been field trialing) did not. This was due to the
extensions code checking only for the flag, and not considering
the field trial state as well.

components/printing actually got the logic right, but only by
reproducing a lot of business logic. Thus, it seems
appropriate to move SiteIsolationPolicy to content/public,
so that we can centralize the "what kind of oopifs are there"
logic. For printing, this change adds a new getter function
specific to oopif compositor, since that's basically a
derived policy of the process model.

For extensions, we've decided to disable LockToOrigin in
--site-per-process (rather than to enable it in the feature),
since origin-locking extensions doesn't help with the spectre
threat, and --site-per-process is about spectre these days.
[Charlie suggests we develop some kind of "extension isolation v2"
proposal, maybe reviving the --isolate-extension flag for that
purpose!]

Bug: 824966, 766267

Change-Id: Ibf7592c9d522fd0c99057358bcc34b5881780db8
Reviewed-on: https://chromium-review.googlesource.com/949966
Commit-Queue: Nick Carter <nick@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Wei Li <weili@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548645}

chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc

@@ -383,12 +383,14 @@ bool ChromeContentBrowserClientExtensionsPart::ShouldLockToOrigin(
     if (extension && extension->is_hosted_app())
       return false;
 
-    // http://crbug.com/600441 workaround: Extension process reuse, implemented
-    // in ShouldTryToUseExistingProcessHost(), means that extension processes
-    // aren't always actually dedicated to a single origin.
-    // TODO(nick): Fix this.
-    if (!base::CommandLine::ForCurrentProcess()->HasSwitch(
-            ::switches::kSitePerProcess))
+    // Extensions are allowed to share processes, even in --site-per-process
+    // currently. See https://crbug.com/600441#c1 for some background on the
+    // intersection of extension process reuse and site isolation.
+    //
+    // TODO(nick): Fix this, possibly by revamping the extensions process model
+    // so that sharing is determined by privilege level, as described in
+    // https://crbug.com/766267
+    if (extension)
       return false;
   }
   return true;

chrome/browser/extensions/process_management_browsertest.cc

@@ -42,6 +42,12 @@ namespace extensions {
 
 namespace {
 
+bool IsExtensionProcessSharingAllowed() {
+  // TODO(nick): Currently, process sharing is allowed even in
+  // --site-per-process. Lock this down.  https://crbug.com/766267
+  return true;
+}
+
 class ProcessManagementTest : public ExtensionBrowserTest {
  private:
   // This is needed for testing isolated apps, which are still experimental.
@@ -268,12 +274,11 @@ IN_PROC_BROWSER_TEST_F(ProcessManagementTest, MAYBE_ProcessOverflow) {
   EXPECT_EQ(web1_host, web2_host);
   EXPECT_NE(web1_host, extension1_host);
 
-  if (!content::AreAllSitesIsolatedForTesting()) {
+  if (IsExtensionProcessSharingAllowed()) {
     // Extensions only share with each other ...
     EXPECT_EQ(extension1_host, extension2_host);
   } else {
-    // ... unless site-per-process is enabled - in this case no sharing
-    // is possible.
+    // Unless extensions are not allowed to share, even with each other.
     EXPECT_NE(extension1_host, extension2_host);
   }
 }
@@ -333,13 +338,13 @@ IN_PROC_BROWSER_TEST_F(ProcessManagementTest, MAYBE_ExtensionProcessBalancing) {
 
   // We've loaded 5 extensions with background pages
   // (api_test/browser_action/*), 1 extension without background page
-  // (api_test/management), and one isolated app. We expect only 2 unique
-  // processes hosting the background pages/scripts of these extensions without
-  // site-per-process (which extension gets assigned to which process is
-  // randomized).  With site-per-process (which is not subject to the 1/3rd of
-  // process limit cap) each of the 5 background pages/scripts will be hosted in
-  // a separate process.
-  if (!content::AreAllSitesIsolatedForTesting())
+  // (api_test/management), and one isolated app. With extension process
+  // sharing, we expect only 2 unique processes hosting the background
+  // pages/scripts of these extensions (which extension gets assigned to which
+  // process is randomized).  If extension process sharing is disabled, there is
+  // no process limit, and each of the 5 background pages/scripts will be hosted
+  // in a separate process.
+  if (IsExtensionProcessSharingAllowed())
     EXPECT_EQ(2u, process_ids.size());
   else
     EXPECT_EQ(5u, process_ids.size());

chrome/browser/extensions/process_manager_browsertest.cc

@@ -59,6 +59,12 @@ namespace extensions {
 
 namespace {
 
+bool IsExtensionProcessSharingAllowed() {
+  // TODO(nick): Currently, process sharing is allowed even in
+  // --site-per-process. Lock this down.  https://crbug.com/766267
+  return true;
+}
+
 void AddFrameToSet(std::set<content::RenderFrameHost*>* frames,
                    content::RenderFrameHost* rfh) {
   if (rfh->IsRenderFrameLive())
@@ -719,10 +725,9 @@ IN_PROC_BROWSER_TEST_F(ProcessManagerBrowserTest, ExtensionProcessReuse) {
 
   EXPECT_EQ(kNumExtensions, installed_extensions.size());
 
-  if (content::AreAllSitesIsolatedForTesting()) {
+  if (!IsExtensionProcessSharingAllowed()) {
     EXPECT_EQ(kNumExtensions, processes.size()) << "Extension process reuse is "
-                                                   "expected to be disabled in "
-                                                   "--site-per-process.";
+                                                   "expected to be disabled.";
   } else {
     EXPECT_LT(processes.size(), kNumExtensions)
         << "Expected extension process reuse, but none happened.";

components/printing/browser/print_manager_utils.cc

@@ -8,7 +8,7 @@
 #include "components/printing/browser/features.h"
 #include "components/printing/browser/print_composite_client.h"
 #include "components/printing/common/print_messages.h"
-#include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/site_isolation_policy.h"
 #include "content/public/common/content_features.h"
 #include "content/public/common/content_switches.h"
 #include "printing/print_settings.h"
@@ -34,25 +34,7 @@ void CreateCompositeClientIfNeeded(content::WebContents* web_contents) {
   // where OOPIF is used such as isolate-extensions, but should be good for
   // feature testing purpose. Eventually, we will remove this check and use pdf
   // compositor service by default for printing.
-
-  bool is_auxiliary_site_isolation_enabled;
-  if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-          switches::kIsolateOrigins)) {
-    is_auxiliary_site_isolation_enabled = true;
-  } else if (base::CommandLine::ForCurrentProcess()->HasSwitch(
-                 switches::kDisableSiteIsolationTrials)) {
-    is_auxiliary_site_isolation_enabled = false;
-  } else {
-    // The features need to be checked last, because checking a feature
-    // activates the field trial and assigns the client either to a control or
-    // an experiment group.
-    is_auxiliary_site_isolation_enabled =
-        base::FeatureList::IsEnabled(::features::kIsolateOrigins) ||
-        base::FeatureList::IsEnabled(::features::kTopDocumentIsolation);
-  }
-
-  if (is_auxiliary_site_isolation_enabled ||
-      content::ContentBrowserClient::IsStrictSiteIsolationEnabled() ||
+  if (content::SiteIsolationPolicy::ShouldPdfCompositorBeEnabledForOopifs() ||
       base::FeatureList::IsEnabled(
           printing::features::kUsePdfCompositorServiceForPrint)) {
     PrintCompositeClient::CreateForWebContents(web_contents);

content/browser/BUILD.gn

@@ -1559,8 +1559,6 @@ jumbo_source_set("browser") {
     "shared_worker/shared_worker_service_impl.h",
     "site_instance_impl.cc",
     "site_instance_impl.h",
-    "site_isolation_policy.cc",
-    "site_isolation_policy.h",
     "speech/speech_recognition_dispatcher_host.cc",
     "speech/speech_recognition_dispatcher_host.h",
     "speech/speech_recognition_manager_impl.cc",

content/browser/browser_main_loop.cc

@@ -83,7 +83,6 @@
 #include "content/browser/renderer_host/media/media_stream_manager.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
 #include "content/browser/service_manager/service_manager_context.h"
-#include "content/browser/site_isolation_policy.h"
 #include "content/browser/speech/speech_recognition_manager_impl.h"
 #include "content/browser/startup_task_runner.h"
 #include "content/browser/tracing/background_tracing_manager_impl.h"
@@ -99,6 +98,7 @@
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/gpu_data_manager_observer.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/site_isolation_policy.h"
 #include "content/public/browser/swap_metrics_driver.h"
 #include "content/public/common/content_client.h"
 #include "content/public/common/content_features.h"

content/browser/browsing_instance.cc

@@ -7,9 +7,9 @@
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "content/browser/site_instance_impl.h"
-#include "content/browser/site_isolation_policy.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/site_isolation_policy.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.h"
 

content/browser/child_process_security_policy_impl.cc

@@ -21,12 +21,12 @@
 #include "content/browser/bad_message.h"
 #include "content/browser/isolated_origin_util.h"
 #include "content/browser/site_instance_impl.h"
-#include "content/browser/site_isolation_policy.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/child_process_data.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/site_isolation_policy.h"
 #include "content/public/browser/storage_partition.h"
 #include "content/public/common/bindings_policy.h"
 #include "content/public/common/url_constants.h"

content/browser/frame_host/render_frame_host_manager.cc

@@ -35,7 +35,6 @@
 #include "content/browser/renderer_host/render_view_host_factory.h"
 #include "content/browser/renderer_host/render_view_host_impl.h"
 #include "content/browser/site_instance_impl.h"
-#include "content/browser/site_isolation_policy.h"
 #include "content/browser/webui/web_ui_controller_factory_registry.h"
 #include "content/common/frame_messages.h"
 #include "content/common/frame_owner_properties.h"
@@ -46,6 +45,7 @@
 #include "content/public/browser/render_process_host_observer.h"
 #include "content/public/browser/render_widget_host_iterator.h"
 #include "content/public/browser/render_widget_host_view.h"
+#include "content/public/browser/site_isolation_policy.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/referrer.h"
 #include "content/public/common/url_constants.h"

content/browser/loader/cross_site_document_resource_handler.cc

@@ -22,10 +22,10 @@
 #include "content/browser/loader/detachable_resource_handler.h"
 #include "content/browser/loader/resource_request_info_impl.h"
 #include "content/browser/site_instance_impl.h"
-#include "content/browser/site_isolation_policy.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/resource_context.h"
+#include "content/public/browser/site_isolation_policy.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_client.h"
 #include "net/base/io_buffer.h"

content/browser/loader/resource_dispatcher_host_impl.cc

@@ -64,7 +64,6 @@
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_navigation_handle_core.h"
 #include "content/browser/service_worker/service_worker_request_handler.h"
-#include "content/browser/site_isolation_policy.h"
 #include "content/browser/streams/stream.h"
 #include "content/browser/streams/stream_context.h"
 #include "content/browser/streams/stream_registry.h"
@@ -81,6 +80,7 @@
 #include "content/public/browser/plugin_service.h"
 #include "content/public/browser/resource_dispatcher_host_delegate.h"
 #include "content/public/browser/resource_throttle.h"
+#include "content/public/browser/site_isolation_policy.h"
 #include "content/public/browser/stream_handle.h"
 #include "content/public/browser/stream_info.h"
 #include "content/public/common/browser_side_navigation_policy.h"

content/browser/renderer_host/render_process_host_impl.cc

@@ -131,7 +131,6 @@
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_dispatcher_host.h"
 #include "content/browser/site_instance_impl.h"
-#include "content/browser/site_isolation_policy.h"
 #include "content/browser/speech/speech_recognition_dispatcher_host.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/browser/streams/stream_context.h"
@@ -157,6 +156,7 @@
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/render_widget_host_iterator.h"
 #include "content/public/browser/resource_context.h"
+#include "content/public/browser/site_isolation_policy.h"
 #include "content/public/common/bind_interface_helpers.h"
 #include "content/public/common/child_process_host.h"
 #include "content/public/common/connection_filter.h"

content/browser/site_instance_impl.cc

@@ -15,10 +15,10 @@
 #include "content/browser/frame_host/debug_urls.h"
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/renderer_host/render_process_host_impl.h"
-#include "content/browser/site_isolation_policy.h"
 #include "content/browser/storage_partition_impl.h"
 #include "content/public/browser/content_browser_client.h"
 #include "content/public/browser/render_process_host_factory.h"
+#include "content/public/browser/site_isolation_policy.h"
 #include "content/public/browser/web_ui_controller_factory.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/url_constants.h"

content/public/browser/BUILD.gn

@@ -241,6 +241,8 @@ jumbo_source_set("browser_sources") {
     "session_storage_usage_info.h",
     "shared_worker_service.h",
     "site_instance.h",
+    "site_isolation_policy.cc",
+    "site_isolation_policy.h",
     "speech_recognition_event_listener.h",
     "speech_recognition_manager.h",
     "speech_recognition_manager_delegate.h",

content/public/browser/content_browser_client.cc

@@ -11,7 +11,6 @@
 #include "base/guid.h"
 #include "base/logging.h"
 #include "build/build_config.h"
-#include "content/browser/site_isolation_policy.h"
 #include "content/public/browser/client_certificate_delegate.h"
 #include "content/public/browser/login_delegate.h"
 #include "content/public/browser/memory_coordinator_delegate.h"
@@ -186,11 +185,6 @@ bool ContentBrowserClient::ShouldEnableStrictSiteIsolation() {
   return false;
 }
 
-// static
-bool ContentBrowserClient::IsStrictSiteIsolationEnabled() {
-  return SiteIsolationPolicy::UseDedicatedProcessesForAllSites();
-}
-
 bool ContentBrowserClient::IsFileAccessAllowed(
     const base::FilePath& path,
     const base::FilePath& absolute_path,

content/public/browser/content_browser_client.h

@@ -367,14 +367,6 @@ class CONTENT_EXPORT ContentBrowserClient {
   // See also https://crbug.com/825369
   virtual bool ShouldEnableStrictSiteIsolation();
 
-  // Allows //content embedders to check if --site-per-process has been enabled
-  // (via cmdline flag or via a field trial).
-  //
-  // TODO(lukasza, weili): https://crbug.com/824867: Remove this method after
-  // shipping OOPIF printing (the only caller is in
-  // components/printing/browser/print_manager_utils.cc).
-  static bool IsStrictSiteIsolationEnabled();
-
   // Indicates whether a file path should be accessible via file URL given a
   // request from a browser context which lives within |profile_path|.
   virtual bool IsFileAccessAllowed(const base::FilePath& path,

content/browser/site_isolation_policy.cc → content/public/browser/site_isolation_policy.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "content/browser/site_isolation_policy.h"
+#include "content/public/browser/site_isolation_policy.h"
 
 #include <algorithm>
 #include <iterator>
@@ -40,7 +40,8 @@ bool SiteIsolationPolicy::UseDedicatedProcessesForAllSites() {
   // ContentBrowserClient consults a base::Feature, then it will activate the
   // field trial and assigns the client either to a control or an experiment
   // group - such assignment should be final.
-  return GetContentClient()->browser()->ShouldEnableStrictSiteIsolation();
+  return GetContentClient() &&
+         GetContentClient()->browser()->ShouldEnableStrictSiteIsolation();
 }
 
 // static
@@ -78,6 +79,9 @@ bool SiteIsolationPolicy::IsTopDocumentIsolationEnabled() {
 
 // static
 bool SiteIsolationPolicy::AreIsolatedOriginsEnabled() {
+  // NOTE: Because it is possible for --isolate-origins to be isolating origins
+  // at a finer-than-site granularity, we do not suppress --isolate-origins when
+  // --site-per-process is also enabled.
   if (base::CommandLine::ForCurrentProcess()->HasSwitch(
           switches::kIsolateOrigins)) {
     return true;
@@ -94,6 +98,19 @@ bool SiteIsolationPolicy::AreIsolatedOriginsEnabled() {
   return base::FeatureList::IsEnabled(features::kIsolateOrigins);
 }
 
+// static
+
+bool SiteIsolationPolicy::ShouldPdfCompositorBeEnabledForOopifs() {
+  // TODO(weili): We only create pdf compositor client and use pdf compositor
+  // service when site-per-process or isolate-origins flag/feature is enabled,
+  // or top-document-isolation feature is enabled. This may not cover all cases
+  // where OOPIF is used such as isolate-extensions, but should be good for
+  // feature testing purpose. Eventually, we will remove this check and use pdf
+  // compositor service by default for printing.
+  return AreIsolatedOriginsEnabled() || IsTopDocumentIsolationEnabled() ||
+         UseDedicatedProcessesForAllSites();
+}
+
 // static
 std::vector<url::Origin>
 SiteIsolationPolicy::GetIsolatedOriginsFromEnvironment() {

content/browser/site_isolation_policy.h → content/public/browser/site_isolation_policy.h

@@ -2,8 +2,8 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef CONTENT_BROWSER_SITE_ISOLATION_POLICY_H_
-#define CONTENT_BROWSER_SITE_ISOLATION_POLICY_H_
+#ifndef CONTENT_PUBLIC_BROWSER_SITE_ISOLATION_POLICY_H_
+#define CONTENT_PUBLIC_BROWSER_SITE_ISOLATION_POLICY_H_
 
 #include <vector>
 
@@ -43,6 +43,10 @@ class CONTENT_EXPORT SiteIsolationPolicy {
   // Returns true if isolated origins feature is enabled.
   static bool AreIsolatedOriginsEnabled();
 
+  // Returns true if the PDF compositor should be enabled to allow out-of-
+  // process iframes (OOPIF's) to print properly.
+  static bool ShouldPdfCompositorBeEnabledForOopifs();
+
   // Returns the origins to isolate.  See also AreIsolatedOriginsEnabled.
   // This list applies globally to the whole browser in all profiles.
   static std::vector<url::Origin> GetIsolatedOrigins();
@@ -70,4 +74,4 @@ class CONTENT_EXPORT SiteIsolationPolicy {
 
 }  // namespace content
 
-#endif  // CONTENT_BROWSER_SITE_ISOLATION_POLICY_H_
+#endif  // CONTENT_PUBLIC_BROWSER_SITE_ISOLATION_POLICY_H_

content/browser/site_isolation_policy_unittest.cc → content/public/browser/site_isolation_policy_unittest.cc

@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "content/browser/site_isolation_policy.h"
+#include "content/public/browser/site_isolation_policy.h"
 
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"

content/public/test/test_utils.cc

@@ -28,6 +28,7 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/site_isolation_policy.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/process_type.h"
@@ -194,8 +195,7 @@ std::unique_ptr<base::Value> ExecuteScriptAndGetValue(
 }
 
 bool AreAllSitesIsolatedForTesting() {
-  return base::CommandLine::ForCurrentProcess()->HasSwitch(
-      switches::kSitePerProcess);
+  return SiteIsolationPolicy::UseDedicatedProcessesForAllSites();
 }
 
 void IsolateAllSitesForTesting(base::CommandLine* command_line) {

content/test/BUILD.gn

@@ -1480,7 +1480,6 @@ test("content_unittests") {
     "../browser/shared_worker/shared_worker_instance_unittest.cc",
     "../browser/shared_worker/shared_worker_service_impl_unittest.cc",
     "../browser/site_instance_impl_unittest.cc",
-    "../browser/site_isolation_policy_unittest.cc",
     "../browser/startup_task_runner_unittest.cc",
     "../browser/storage_partition_impl_map_unittest.cc",
     "../browser/storage_partition_impl_unittest.cc",
@@ -1551,6 +1550,7 @@ test("content_unittests") {
     "../common/throttling_url_loader_unittest.cc",
     "../common/unique_name_helper_unittest.cc",
     "../common/webplugininfo_unittest.cc",
+    "../public/browser/site_isolation_policy_unittest.cc",
 
     # TODO(jam): move these network/ tests to services/network.
     "../public/common/drop_data_unittest.cc",