Extensions: Only load incognito-enabled extensions in an incognito renderer.

extensions/browser/renderer_startup_helper.cc

Index: extensions/browser/renderer_startup_helper.cc
diff --git a/extensions/browser/renderer_startup_helper.cc b/extensions/browser/renderer_startup_helper.cc
index 8d0a9b31e3910bc02f66c9b246d4a0dc14cc9693..86c4ae1d88193ab59890b523809889d4cbdf9436 100644
--- a/extensions/browser/renderer_startup_helper.cc
+++ b/extensions/browser/renderer_startup_helper.cc
@@ -7,11 +7,13 @@
 #include "base/stl_util.h"
 #include "base/values.h"
 #include "components/keyed_service/content/browser_context_dependency_manager.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/render_process_host.h"
 #include "extensions/browser/extension_function_dispatcher.h"
 #include "extensions/browser/extension_registry.h"
+#include "extensions/browser/extension_util.h"
 #include "extensions/browser/extensions_browser_client.h"
 #include "extensions/browser/guest_view/web_view/web_view_guest.h"
 #include "extensions/common/extension_messages.h"
@@ -25,6 +27,28 @@ using content::BrowserContext;
 
 namespace extensions {
 
+namespace {
+
+// Returns whether the |extension| should be loaded in the given
+// renderer |process|.
+bool IsExtensionVisibleToProcess(const Extension& extension,
+                                 content::RenderProcessHost* process) {
+  // Renderers don't need to know about themes.
+  if (extension.is_theme())
+    return false;
+
+  // Only extensions enabled in incognito mode should be loaded in an incognito
+  // renderer. However extensions which can't be loaded in the incognito mode
+  // (e.g. platform apps) should also be loaded in an incognito renderer to
+  // ensure connections from incognito tabs to such extensions work fine.

[Devlin, 2017/03/23 22:08:37]

nitty nit: omit ' fine' -> 'ensure connections from incognito tabs to such
extensions work.'

[karandeepb, 2017/04/04 03:44:15]

Done.

+  content::BrowserContext* browser_context = process->GetBrowserContext();
+  return !browser_context->IsOffTheRecord() ||
+         !util::CanBeIncognitoEnabled(&extension) ||
+         util::IsIncognitoEnabled(extension.id(), browser_context);
+}
+
+}  // namespace
+
 RendererStartupHelper::RendererStartupHelper(BrowserContext* browser_context)
     : browser_context_(browser_context) {
   DCHECK(browser_context);
@@ -98,17 +122,17 @@ void RendererStartupHelper::InitializeProcess(
   const ExtensionSet& extensions =
       ExtensionRegistry::Get(browser_context_)->enabled_extensions();
   for (const auto& ext : extensions) {
-    // Renderers don't need to know about themes.
-    if (!ext->is_theme()) {
-      // TODO(kalman): Only include tab specific permissions for extension
-      // processes, no other process needs it, so it's mildly wasteful.
-      // I am not sure this is possible to know this here, at such a low
-      // level of the stack. Perhaps site isolation can help.
-      bool include_tab_permissions = true;
-      loaded_extensions_params.push_back(
-          ExtensionMsg_Loaded_Params(ext.get(), include_tab_permissions));
-      loaded_extensions->insert(ext->id());
-    }
+    if (!IsExtensionVisibleToProcess(*ext, process))

[Devlin, 2017/03/23 22:08:37]

nitty nit: We actually only need the browser context here, so we can cache it
before iterating through the loop and just pass it directly, saving the
(admittedly very cheap) repeated call to RenderProcessHost::GetBrowserContext().

(Note that you'll still want to use process->GetBrowserContext() when you cache
it, rather than passing in browser_context_, since this handles both on- and
off-the-record versions.)

[karandeepb, 2017/04/04 03:44:15]

Done.

+      continue;
+
+    // TODO(kalman): Only include tab specific permissions for extension
+    // processes, no other process needs it, so it's mildly wasteful.
+    // I am not sure this is possible to know this here, at such a low
+    // level of the stack. Perhaps site isolation can help.
+    bool include_tab_permissions = true;
+    loaded_extensions_params.push_back(
+        ExtensionMsg_Loaded_Params(ext.get(), include_tab_permissions));
+    loaded_extensions->insert(ext->id());
   }
 
   // Activate pending extensions.
@@ -138,10 +162,7 @@ void RendererStartupHelper::UntrackProcess(
 void RendererStartupHelper::ActivateExtensionInProcess(
     const Extension& extension,
     content::RenderProcessHost* process) {
-  // Renderers don't need to know about themes. We also don't normally
-  // "activate" themes, but this could happen if someone tries to open a tab
-  // to the e.g. theme's manifest.
-  if (extension.is_theme())
+  if (!IsExtensionVisibleToProcess(extension, process))
     return;
 
   const auto& process_extensions_pair = loaded_extensions_.find(process);
@@ -159,10 +180,6 @@ void RendererStartupHelper::ActivateExtensionInProcess(
 }
 
 void RendererStartupHelper::OnExtensionLoaded(const Extension& extension) {
-  // Renderers don't need to know about themes.
-  if (extension.is_theme())
-    return;

[Devlin, 2017/03/23 22:08:37]

I wonder if it's worth keeping this short-circuit, since the creation of
ExtensionMsg_Loaded_Params is non-trivial, and we know that no process ever
needs to know about themes.

[karandeepb, 2017/04/04 03:44:15]

Centralizing the logic in IsExtensionVisibleToProcess seemed better to me.
Though don't mind changing it either.

[Devlin, 2017/04/04 15:15:05]

Given it's somewhat performance-sensitive code, let's keep it with a comment
like:
// IsExtensionVisibleToProcess() would filter out themes, but we choose
// to early-out for performance reasons.

-
   // We don't need to include tab permisisons here, since the extension
   // was just loaded.
   // Uninitialized renderers will be informed of the extension load during the
@@ -177,6 +194,9 @@ void RendererStartupHelper::OnExtensionLoaded(const Extension& extension) {
       continue;
 
     content::RenderProcessHost* process = process_extensions_pair.first;
+    if (!IsExtensionVisibleToProcess(extension, process))
+      continue;
+
     process->Send(new ExtensionMsg_Loaded(params));
     process_extensions_pair.second.insert(extension.id());
   }