Keep web popups opened from extensions in same BrowsingInstance.

chrome/browser/ui/browser_navigator.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/ui/browser_navigator.h"
 
 #include <algorithm>
 
 #include "base/command_line.h"
 #include "base/macros.h"
@@ skipping 230 matching lines @@
       params->tabstrip_add_types |= TabStripModel::ADD_ACTIVE;
       break;
 
     default:
       break;
   }
 }
 
 // Obtain the profile used by the code that originated the Navigate() request.
 Profile* GetSourceProfile(chrome::NavigateParams* params) {
+  if (params->source_site_instance) {

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

Turned out the source_site_instance check needed to precede the source_contents
check.  This mattered in the case where chrome.windows.create was used with
multiple URLs to be opened (i.e. chrome.windows.create({url: ['http://foo.com',
'http://bar.com'], incognito: true}); see ExtensionApiTest.TabQuery [1]).  In
that case, a navigation to the first URL in the list could've modified
source_contents in |params| (to the first URL's WebContents, I think) which
would've been used for subsequent URLs.  See the loop in [2] and how it reuses
the params.  I think in that case, each navigation's source profile should be
considered to be the source site instance's profile.

[1]
https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/tab...

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

[Lei Zhang, 2016/10/06 00:36:47]

You may want to write down some of your notes here as an actual comment in the
code. It's not obvious that the order matters.

[alexmos, 2016/10/06 00:42:00]

Thanks!  I'll add the comment in a followup so I can land this sooner and
hopefully make tomorrow's canary.

+    return Profile::FromBrowserContext(
+        params->source_site_instance->GetBrowserContext());
+  }
+
   if (params->source_contents) {
     return Profile::FromBrowserContext(
         params->source_contents->GetBrowserContext());
   }
 
   return params->initiating_profile;
 }
 
 void LoadURLInContents(WebContents* target_contents,
                        const GURL& url,
@@ skipping 197 matching lines @@
       }
     }
   }
 #endif
 
   // Navigate() must not return early after this point.
 
   if (GetSourceProfile(params) != params->browser->profile()) {
     // A tab is being opened from a link from a different profile, we must reset
     // source information that may cause state to be shared.
-    params->source_contents = NULL;
+    params->source_contents = nullptr;
+    params->source_site_instance = nullptr;
     params->referrer = content::Referrer();
   }
 
   // Make sure the Browser is shown if params call for it.
   ScopedBrowserShower shower(params);
 
   // Makes sure any WebContents created by this function is destroyed if
   // not properly added to a tab strip.
   ScopedTargetContentsOwner target_contents_owner(params);
 
@@ skipping 188 matching lines @@
   bool reverse_on_redirect = false;
   content::BrowserURLHandler::GetInstance()->RewriteURLIfNecessary(
       &rewritten_url, browser_context, &reverse_on_redirect);
 
   // Some URLs are mapped to uber subpages. Do not allow them in incognito.
   return !(rewritten_url.scheme() == content::kChromeUIScheme &&
            rewritten_url.host() == chrome::kChromeUIUberHost);
 }
 
 }  // namespace chrome