Keep web popups opened from extensions in same BrowsingInstance.

content/browser/frame_host/render_frame_host_manager.cc

 // Copyright 2013 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 "content/browser/frame_host/render_frame_host_manager.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <utility>
@@ skipping 1073 matching lines @@
   // doesn't usually swap (e.g., process-per-tab).  Any time we return true,
   // the new_entry will be rendered in a new SiteInstance AND BrowsingInstance.
   BrowserContext* browser_context =
       delegate_->GetControllerForRenderManager().GetBrowserContext();
 
   // Don't force a new BrowsingInstance for debug URLs that are handled in the
   // renderer process, like javascript: or chrome://crash.
   if (IsRendererDebugURL(new_effective_url))
     return false;
 
+  // Transitions across BrowserContexts should always require a
+  // BrowsingInstance swap. For example, this can happen if an extension in a
+  // normal profile opens an incognito window with a web URL using
+  // chrome.windows.create().
+  //
+  // TODO(alexmos): This check should've been enforced earlier in the
+  // navigation, in chrome::Navigate().  Verify this, and then convert this to
+  // a CHECK and remove the fallback.
+  DCHECK_EQ(browser_context,
+            render_frame_host_->GetSiteInstance()->GetBrowserContext());
+  if (browser_context !=
+      render_frame_host_->GetSiteInstance()->GetBrowserContext()) {

[alexmos, 2016/10/04 21:24:44]

I was thinking of keeping this for now as a defense-in-depth check, especially
for the merge, and then seeing if we can convert this to a CHECK later on.  Let
me know if you think we still need this at all.

+    return true;
+  }
+
   // For security, we should transition between processes when one is a Web UI
   // page and one isn't, or if the WebUI types differ.
   if (ChildProcessSecurityPolicyImpl::GetInstance()->HasWebUIBindings(
           render_frame_host_->GetProcess()->GetID()) ||
-      WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
+      WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL(
           browser_context, current_effective_url)) {
     // If so, force a swap if destination is not an acceptable URL for Web UI.
     // Here, data URLs are never allowed.
     if (!WebUIControllerFactoryRegistry::GetInstance()->IsURLAcceptableForWebUI(
             browser_context, new_effective_url)) {
       return true;
     }
 
     // Force swap if the current WebUI type differs from the one for the
     // destination.
     if (WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
             browser_context, current_effective_url) !=
         WebUIControllerFactoryRegistry::GetInstance()->GetWebUIType(
             browser_context, new_effective_url)) {
       return true;
     }
   } else {
     // Force a swap if it's a Web UI URL.
-    if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
+    if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL(
             browser_context, new_effective_url)) {
       return true;
     }
   }
 
   // Check with the content client as well.  Important to pass
   // current_effective_url here, which uses the SiteInstance's site if there is
   // no current_entry.
   if (GetContentClient()->browser()->ShouldSwapBrowsingInstancesForNavigation(
           render_frame_host_->GetSiteInstance(),
@@ skipping 64 matching lines @@
 
   scoped_refptr<SiteInstance> new_instance =
       ConvertToSiteInstance(new_instance_descriptor, candidate_instance);
   // If |force_swap| is true, we must use a different SiteInstance than the
   // current one. If we didn't, we would have two RenderFrameHosts in the same
   // SiteInstance and the same frame, resulting in page_id conflicts for their
   // NavigationEntries.
   if (force_swap)
     CHECK_NE(new_instance, current_instance);
 
+  // Double-check that the new SiteInstance is associated with the right
+  // BrowserContext.
+  DCHECK_EQ(new_instance->GetBrowserContext(), browser_context);

[alexmos, 2016/10/04 21:24:44]

This was getting hit in ExtensionTabsTest.DefaultToIncognitoWhenItIsForced from
browser_tests in PlzNavigate mode, which recently became part of the CQ.  That
test involved chrome.windows.create, and with PlzNav, my BrowsingInstance
profile check wasn't getting hit (and force_swap ended up as false), but we
still ended up with a different BrowserContext because
DetermineSiteInstanceForURL made the new SiteInstance out of the
source_site_instance, because the dest_url was about:blank [1].  (I didn't check
why it was fine without PlzNav -- it might be related to how
source_site_instance is set slightly differently.)

In any case, you were right that it's best to force this at a higher level: the
check I added to detect the source profile based on source site instance in
chrome::Navigate, and to clear source site instance in case of profile mismatch,
takes care of both this failure, and the previous one in TriggerEvent.   
chrome.windows.create always sets the source_site_instance [2], so I think it
makes sense to base the source profile off of that.

[1]
https://cs.chromium.org/chromium/src/content/browser/frame_host/render_frame_...

[2]
https://cs.chromium.org/chromium/src/chrome/browser/extensions/api/tabs/tabs_...

+
   return new_instance;
 }
 
 RenderFrameHostManager::SiteInstanceDescriptor
 RenderFrameHostManager::DetermineSiteInstanceForURL(
     const GURL& dest_url,
     SiteInstance* source_instance,
     SiteInstance* current_instance,
     SiteInstance* dest_instance,
     ui::PageTransition transition,
@@ skipping 1456 matching lines @@
                                              resolved_url)) {
     DCHECK(!dest_instance ||
            dest_instance == render_frame_host_->GetSiteInstance());
     return false;
   }
 
   return true;
 }
 
 }  // namespace content