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

chrome/browser/extensions/api/messaging/message_service.cc

Index: chrome/browser/extensions/api/messaging/message_service.cc
diff --git a/chrome/browser/extensions/api/messaging/message_service.cc b/chrome/browser/extensions/api/messaging/message_service.cc
index 44ce1f03e74791fd9be07d4d5f58d3ff494b9395..a10c3079672b754d92ffe42f02ddae3f630365e8 100644
--- a/chrome/browser/extensions/api/messaging/message_service.cc
+++ b/chrome/browser/extensions/api/messaging/message_service.cc
@@ -327,16 +327,18 @@ void MessageService::OpenChannelToExtension(
       PendingMessagesQueue();
   if (context->IsOffTheRecord() &&
       !util::IsIncognitoEnabled(target_extension_id, context)) {
+    // The extension is not enabled in incognito mode. However it is loaded in
+    // the associated incognito renderer process. This can only happen when the
+    // extension can't be enabled in incognito mode. This can include platform
+    // apps, component extensions and extensions which are not allowed in the
+    // incognito mode.
+    DCHECK(!util::CanBeIncognitoEnabled(target_extension));

[karandeepb, 2017/03/23 02:54:03]

At least in this case, CanBeIncognitoEnabled seems like a misnomer. Wonder if we
should also introduce a separate SupportsIncognitoToggle or maybe we can
directly use checks to find whether it is a platform app or component extension.

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

Is this check correct?  What about the case of a website trying to connect with
an extension that can be incognito enabled, but isn't?  (Given this passed on
the bots, does that indicate we need another test?)

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

I'm not sure I love SupportsIncognitoToggle, since it's pretty UI-dependent,
and, as mentioned in the bug, might lead to confusion if policy settings get
involved.  I think what we're trying to convey here is that the extension can't
ever "run" in an incognito context (and that assumption is valid here - we only
pass messages back and forth).  But CanExtensionBeEnabledToRunInIncognito is too
long. ;)  If you have other suggestions, I'm happy to hear them, but I don't
think the name is dramatically incorrect.

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

It isn't. I had misunderstood what was happening. So as I now understand,
bindings for the chrome.runtime API will only be generated into a
(non-extension) renderer, if at least one of the extensions loaded into it is
externally_connectible. Modified a test so that this DCHECK was triggered.

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

The current naming SGTM then.

+
     // Give the user a chance to accept an incognito connection from the web if
-    // they haven't already, with the conditions:
-    // - Only for spanning-mode incognito. We don't want the complication of
-    //   spinning up an additional process here which might need to do some
-    //   setup that we're not expecting.
-    // - Only for extensions that can't normally be enabled in incognito, since
-    //   that surface (e.g. chrome://extensions) should be the only one for
-    //   enabling in incognito. In practice this means platform apps only.
-    if (!is_web_connection || IncognitoInfo::IsSplitMode(target_extension) ||
-        util::CanBeIncognitoEnabled(target_extension)) {
+    // they haven't already. But don't do this for split mode incognito. We
+    // don't want the complication of spinning up an additional process here
+    // which might need to do some setup that we're not expecting.
+    if (!is_web_connection || IncognitoInfo::IsSplitMode(target_extension)) {
       OnOpenChannelAllowed(std::move(params), false);
       return;
     }