Keep web popups opened from extensions in same BrowsingInstance.

content/browser/frame_host/render_frame_host_manager.cc

Index: content/browser/frame_host/render_frame_host_manager.cc
diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc
index 46ddfa26acfde24fcf4f5a1b1d834452f8fe95b2..4434891bd182714958647ba018360df5dfca5e1f 100644
--- a/content/browser/frame_host/render_frame_host_manager.cc
+++ b/content/browser/frame_host/render_frame_host_manager.cc
@@ -1091,11 +1091,26 @@ bool RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
   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.
@@ -1114,7 +1129,7 @@ bool RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation(
     }
   } 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;
     }
@@ -1199,6 +1214,10 @@ RenderFrameHostManager::GetSiteInstanceForNavigation(
   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;
 }