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

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

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/extensions/api/messaging/message_service.h"
 
 #include <stdint.h>
 #include <limits>
 #include <utility>
 
@@ skipping 309 matching lines @@
   std::unique_ptr<OpenChannelParams> params(new OpenChannelParams(
       source_process_id, source_routing_id, std::move(source_tab),
       source_frame_id, nullptr, receiver_port_id, source_extension_id,
       target_extension_id, source_url, channel_name, include_tls_channel_id,
       include_guest_process_info));
 
   pending_incognito_channels_[params->receiver_port_id.GetChannelId()] =
       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;
     }
 
     // If the target extension isn't even listening for connect/message events,
     // there is no need to go any further and the connection should be
     // rejected without showing a prompt. See http://crbug.com/442497
     EventRouter* event_router = EventRouter::Get(context);
     const char* const events[] = {"runtime.onConnectExternal",
                                   "runtime.onMessageExternal",
@@ skipping 637 matching lines @@
   MessageChannelMap::iterator channel_iter = channels_.find(channel_id);
   if (channel_iter != channels_.end()) {
     for (const PendingMessage& message : queue) {
       DispatchMessage(message.first, channel_iter->second.get(),
                       message.second);
     }
   }
 }
 
 }  // namespace extensions