Keep web popups opened from extensions in same BrowsingInstance.

Keep web popups opened from extensions in same BrowsingInstance.

Previously, popups for web URLs opened from extensions ended up in a
separate BrowsingInstance, which prevented the popup from
communicating to its opener via window.opener.  This is a legitimate
use case for something like OAuth, so this CL relaxes that restriction
and allows them to coexist in the same BrowsingInstance. (The popup
will still be put into a separate SiteInstance and have process
isolation thanks to --isolate-extensions.)  The same is done for one
extension opening a popup to another extension.

Two checks had to be changed to avoid the BrowsingInstance swap.  One
is ChromeContentBrowserClientExtensionsPart::
ShouldSwapBrowsingInstancesForNavigation, which now only forces a
BrowsingInstance swap when navigating to/from CWS.  The other deals
with WebUI.  Since all packaged extensions are considered to need
WebUI, whether or not they actually need WebUI bindings (see
ExtensionWebUI and UseWebUIForURL), the WebUI checks in
RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation were
relaxed to only require a BrowsingInstance swap when there's a change
in WebUI bindings.

BUG=590068
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/1f48ef6ac2070ccd99c1c54729602e249c4369d6
Cr-Commit-Position: refs/heads/master@{#423361}

[alexmos, 2016-09-27 21:52:59]

Description was changed from

==========
Keep web popups opened from extensions in same BrowsingInstance.

BUG=
==========

to

==========
Keep web popups opened from extensions in same BrowsingInstance.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-09-27 21:53:02]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 21:53:17]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 22:02:51]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-27 22:02:52]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[alexmos, 2016-09-28 21:56:53]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 21:57:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2016-09-28 22:26:27]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 22:26:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 23:06:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-28 23:06:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alexmos, 2016-09-29 00:15:10]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-29 00:15:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-29 01:11:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-29 01:11:51]

Dry run: This issue passed the CQ dry run.

[alexmos, 2016-09-29 16:39:45]

Description was changed from

==========
Keep web popups opened from extensions in same BrowsingInstance.

BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Keep web popups opened from extensions in same BrowsingInstance.

Previously, popups for web URLs opened from extensions ended up in a
separate BrowsingInstance, which prevented the popup from
communicating to its opener via window.opener.  This is a legitimate
use case for something like OAuth, so this CL relaxes that restriction
and allows them to coexist in the same BrowsingInstance. (The popup
will still be put into a separate SiteInstance and have process
isolation thanks to --isolate-extensions.)  The same is done for one
extension opening a popup to another extension.

Two checks had to be changed to avoid the BrowsingInstance swap.  One
is ChromeContentBrowserClientExtensionsPart::
ShouldSwapBrowsingInstancesForNavigation, which now only forces a
BrowsingInstance swap when navigating to/from CWS.  The other deals
with WebUI.  Since all packaged extensions are considered to need
WebUI, whether or not they actually need WebUI bindings (see
ExtensionWebUI and UseWebUIForURL), the WebUI checks in
RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation were
relaxed to only require a BrowsingInstance swap when there's a change
in WebUI bindings.

BUG=590068
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[alexmos, 2016-09-29 16:59:17]

alexmos@chromium.org changed reviewers:
+ creis@chromium.org

[alexmos, 2016-09-29 16:59:19]

Charlie, can you please take a look?

https://codereview.chromium.org/2372323003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2372323003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1086: 
Oops - will remove this blank line in next PS.

https://codereview.chromium.org/2372323003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1100:
render_frame_host_->GetSiteInstance()->GetBrowserContext()) {
The test that caught this was ActivityLogApiTest.TriggerEvent.  (It's a huge
test, but this occurred in the webrequest_incognito part of it, when exercising
windows.create.  In that case, render_frame_host_->GetSiteInstance()
corresponded to the opener extension in a normal profile, while delegate_
pointed to the WebContents already opened in incognito profile.)

https://codereview.chromium.org/2372323003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1108:
WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL(
I think converting these to UseWebUIBindingsForURL gives us what we want. 
Looking at its implementation in
ChromeWebUIControllerFactory::UseWebUIBindingsForURL, it returns exactly the
subset of UseWebUIForURL that actually requires bindings.

I ran a separate experiment for completely disabling NeedsExtensionWebUI (i.e.,
making it so that extensions that don't need WebUI bindings don't return true
for UseWebForURL and don't use any WebUI type):
https://codereview.chromium.org/2371273002/. Surprisingly, nothing broke.  So
there might be more cleanup we can do around the whole ExtensionWebUI thing,
which I don't really understand.  But that can probably happen in a separate CL
after we chat to Devlin about it.

https://codereview.chromium.org/2372323003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1214:
DCHECK_EQ(new_instance->GetBrowserContext(), browser_context);
I was thinking whether this should be a CHECK, but it seemed a bit risky given
that we probably want to merge this to M54.  We could run a separate experiment
to convert this to a CHECK on canary in a separate CL.

[Charlie Reis, 2016-09-29 21:16:10]

Nice, LGTM!

In case we want to merge this to M54, can you check that the first test passes
when --isolate-extensions is disabled?  I'm hoping we don't break anything for
the users in the control group.

https://codereview.chromium.org/2372323003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2372323003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:429:
// RenderFrameHostManager.  See https://crbug.com/590068.
Good explanation.  Might also be worth comparing this to extension OOPIFs, which
exist in the same BrowsingInstance anyway (e.g., page A embeds iframes to
extensions E1 and E2, all of which are in the same BrowsingInstance).  We're
doing the same for top-level navigations here.

https://codereview.chromium.org/2372323003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:433:
current_extension && current_extension->id() == kWebStoreAppId;
Sure, the hosted app check is probably redundant.

https://codereview.chromium.org/2372323003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2372323003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1100:
render_frame_host_->GetSiteInstance()->GetBrowserContext()) {
On 2016/09/29 16:59:18, alexmos wrote:
> The test that caught this was ActivityLogApiTest.TriggerEvent.  (It's a huge
> test, but this occurred in the webrequest_incognito part of it, when
exercising
> windows.create.  In that case, render_frame_host_->GetSiteInstance()
> corresponded to the opener extension in a normal profile, while delegate_
> pointed to the WebContents already opened in incognito profile.)

Nice!

The check is interesting to me-- we were actually creating a WebContents whose
NavigationController is in one BrowserContext (presumably incognito) and the
current (as yet unused) RFH is in a different BrowserContext?  That sounds
pretty broken!

This check seems worthwhile, but I wonder if we should fix it earlier than here
(so that this can be more of a CHECK and not a necessary conditional).  Seems
like we shouldn't be passing a SiteInstance across BrowserContexts when creating
a WebContents.

Do you think that's worth doing here?  Maybe in a followup CL?

https://codereview.chromium.org/2372323003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1108:
WebUIControllerFactoryRegistry::GetInstance()->UseWebUIBindingsForURL(
On 2016/09/29 16:59:18, alexmos wrote:
> I think converting these to UseWebUIBindingsForURL gives us what we want. 
> Looking at its implementation in
> ChromeWebUIControllerFactory::UseWebUIBindingsForURL, it returns exactly the
> subset of UseWebUIForURL that actually requires bindings.

Yes, seems reasonable.

> 
> I ran a separate experiment for completely disabling NeedsExtensionWebUI
(i.e.,
> making it so that extensions that don't need WebUI bindings don't return true
> for UseWebForURL and don't use any WebUI type):
> https://codereview.chromium.org/2371273002/. Surprisingly, nothing broke.  So
> there might be more cleanup we can do around the whole ExtensionWebUI thing,
> which I don't really understand.  But that can probably happen in a separate
CL
> after we chat to Devlin about it.

Agreed-- that has a much higher risk of regression.  :)  That said, I'm excited
if we can remove WebUI for most extensions!  If so, we should probably put this
back to UseWebUIForURL afterward just to be conservative.

https://codereview.chromium.org/2372323003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1214:
DCHECK_EQ(new_instance->GetBrowserContext(), browser_context);
On 2016/09/29 16:59:18, alexmos wrote:
> I was thinking whether this should be a CHECK, but it seemed a bit risky given
> that we probably want to merge this to M54.  We could run a separate
experiment
> to convert this to a CHECK on canary in a separate CL.

Agreed.

[alexmos, 2016-09-30 00:08:11]

Thanks!

> In case we want to merge this to M54, can you check that the first test passes
> when --isolate-extensions is disabled?  I'm hoping we don't break anything for
> the users in the control group.

Yes, I checked and the main frame test still works with --isolate-extensions
disabled.  The web popup still ends up in a separate process from the extension
opener.  (Need to verify what forces this to happen exactly.)

https://codereview.chromium.org/2372323003/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc
(right):

https://codereview.chromium.org/2372323003/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_content_browser_client_extensions_part.cc:429:
// RenderFrameHostManager.  See https://crbug.com/590068.
On 2016/09/29 21:16:10, Charlie Reis wrote:
> Good explanation.  Might also be worth comparing this to extension OOPIFs,
which
> exist in the same BrowsingInstance anyway (e.g., page A embeds iframes to
> extensions E1 and E2, all of which are in the same BrowsingInstance).  We're
> doing the same for top-level navigations here.

Done - added a sentence about this to the comment.

https://codereview.chromium.org/2372323003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2372323003/diff/60001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:1100:
render_frame_host_->GetSiteInstance()->GetBrowserContext()) {
On 2016/09/29 21:16:10, Charlie Reis wrote:
> On 2016/09/29 16:59:18, alexmos wrote:
> > The test that caught this was ActivityLogApiTest.TriggerEvent.  (It's a huge
> > test, but this occurred in the webrequest_incognito part of it, when
> exercising
> > windows.create.  In that case, render_frame_host_->GetSiteInstance()
> > corresponded to the opener extension in a normal profile, while delegate_
> > pointed to the WebContents already opened in incognito profile.)
> 
> Nice!
> 
> The check is interesting to me-- we were actually creating a WebContents whose
> NavigationController is in one BrowserContext (presumably incognito) and the
> current (as yet unused) RFH is in a different BrowserContext?  That sounds
> pretty broken!

Correct.  This never manifested as we always forced a BrowsingInstance swap in
that test case.  But perhaps we might have reached the broken state if an
extension used chrome.windows.create with a URL in its own origin and a flipped
the incognito bit.

> 
> This check seems worthwhile, but I wonder if we should fix it earlier than
here
> (so that this can be more of a CHECK and not a necessary conditional).  Seems
> like we shouldn't be passing a SiteInstance across BrowserContexts when
creating
> a WebContents.
> 
> Do you think that's worth doing here?  Maybe in a followup CL?

Yes, I think that's worth doing.  For this specific case, the path we took to
create the WebContents was:

#1 content::WebContentsImpl::WebContentsImpl()
#2 content::WebContentsImpl::CreateWithOpener()
#3 content::WebContents::Create()
#4 (anonymous namespace)::CreateTargetContents()
#5 chrome::Navigate()
#6 extensions::WindowsCreateFunction::RunSync()

Looking through chrome::Navigate, there's actually already a place to check
whether the profiles match, and clear some state if they don't: 
https://cs.chromium.org/chromium/src/chrome/browser/ui/browser_navigator.cc?l...

But I tried clearing source_site_instance there, and that didn't work
(apparently, because both profiles already show the incognito one).  I can
probably find how to fix that logic, but I don't know if there are any other
WebContents creation paths that might lead to this problem.  So overall, I think
this is better to explore in a follow CL (which we also won't need to merge).  I
can then combine this with the other DCHECK I added and convert to a CHECK.

[alexmos, 2016-09-30 00:09:28]

alexmos@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[alexmos, 2016-09-30 00:09:28]

Devlin, can you please review chrome/browser/extensions for OWNERS?

[Devlin, 2016-10-01 00:54:58]

lgtm

https://codereview.chromium.org/2372323003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2372323003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:833:
NavigateToURL(extension->url().Resolve("empty.html"));
nit: extension->GetResourceURL()

https://codereview.chromium.org/2372323003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:889: const GURL
extension_url(extension->url().Resolve("empty.html"));
ditto

[alexmos, 2016-10-03 20:13:08]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-10-03 20:13:08]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org,
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2372323003/#ps100001
(title: "Devlin's comments")

[alexmos, 2016-10-03 20:13:17]

Thanks!

https://codereview.chromium.org/2372323003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/process_manager_browsertest.cc (right):

https://codereview.chromium.org/2372323003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:833:
NavigateToURL(extension->url().Resolve("empty.html"));
On 2016/10/01 00:54:58, Devlin (catching up) wrote:
> nit: extension->GetResourceURL()

Done.

https://codereview.chromium.org/2372323003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/process_manager_browsertest.cc:889: const GURL
extension_url(extension->url().Resolve("empty.html"));
On 2016/10/01 00:54:58, Devlin (catching up) wrote:
> ditto

Done.

[commit-bot: I haz the power, 2016-10-03 20:13:38]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-03 22:26:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-03 22:26:57]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[alexmos, 2016-10-04 04:13:08]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 04:13:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 04:54:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 04:54:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[alexmos, 2016-10-04 08:51:29]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-04 08:51:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 09:32:15]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 09:32:17]

Dry run: This issue passed the CQ dry run.

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

Charlie, can you please take another look at the PS6-PS8 diff?  There was
another test failure with PlzNavigate, and I ended up following your advice and
moving the profile mismatch detection into chrome::Navigate, which already had
it in place and just needed a bit of extra work to make it work with
chrome.windows.create.  A few notes below.

https://codereview.chromium.org/2372323003/diff/140001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/2372323003/diff/140001/chrome/browser/ui/brow...
chrome/browser/ui/browser_navigator.cc:251: if (params->source_site_instance) {
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_...

https://codereview.chromium.org/2372323003/diff/140001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/2372323003/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1105:
render_frame_host_->GetSiteInstance()->GetBrowserContext()) {
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.

https://codereview.chromium.org/2372323003/diff/140001/content/browser/frame_...
content/browser/frame_host/render_frame_host_manager.cc:1219:
DCHECK_EQ(new_instance->GetBrowserContext(), browser_context);
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_...

[Charlie Reis, 2016-10-05 22:51:18]

Thanks for the explanations!  (I think I follow most of it.)  :)

Updated patch LGTM.

[alexmos, 2016-10-05 22:56:41]

The CQ bit was checked by alexmos@chromium.org

[alexmos, 2016-10-05 22:56:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2372323003/#ps140001
(title: "Fix another test (ExtensionApiTest.TabQuery)")

[alexmos, 2016-10-05 22:57:00]

The CQ bit was unchecked by alexmos@chromium.org

[alexmos, 2016-10-05 22:57:22]

The CQ bit was checked by alexmos@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-05 22:57:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[alexmos, 2016-10-05 22:59:12]

alexmos@chromium.org changed reviewers:
+ sky@chromium.org

[alexmos, 2016-10-05 22:59:13]

sky@: can you please review browser_navigator.cc for owners?  Ideally this would
land today in time for tomorrow's canary.

[commit-bot: I haz the power, 2016-10-06 00:03:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-06 00:03:17]

Dry run: This issue passed the CQ dry run.

[Lei Zhang, 2016-10-06 00:36:46]

thestig@chromium.org changed reviewers:
+ thestig@chromium.org

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

lgtm

https://codereview.chromium.org/2372323003/diff/140001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/2372323003/diff/140001/chrome/browser/ui/brow...
chrome/browser/ui/browser_navigator.cc:251: if (params->source_site_instance) {
On 2016/10/04 21:24:44, alexmos wrote:
> 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_...

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:01]

Thanks Lei!  (sky@: no need for review anymore, as Lei took care of it, though
you're still welcome to take a look if you have any concerns.)

https://codereview.chromium.org/2372323003/diff/140001/chrome/browser/ui/brow...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/2372323003/diff/140001/chrome/browser/ui/brow...
chrome/browser/ui/browser_navigator.cc:251: if (params->source_site_instance) {
On 2016/10/06 00:36:47, Lei Zhang wrote:
> On 2016/10/04 21:24:44, alexmos wrote:
> > 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_...
> 
> 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.

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

[alexmos, 2016-10-06 00:42:08]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2016-10-06 00:43:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Lei Zhang, 2016-10-06 00:43:40]

On 2016/10/06 00:42:01, alexmos wrote:
> Thanks!  I'll add the comment in a followup so I can land this sooner and
> hopefully make tomorrow's canary.

Sure, no problem.

[commit-bot: I haz the power, 2016-10-06 00:52:11]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-06 00:53:36]

Description was changed from

==========
Keep web popups opened from extensions in same BrowsingInstance.

Previously, popups for web URLs opened from extensions ended up in a
separate BrowsingInstance, which prevented the popup from
communicating to its opener via window.opener.  This is a legitimate
use case for something like OAuth, so this CL relaxes that restriction
and allows them to coexist in the same BrowsingInstance. (The popup
will still be put into a separate SiteInstance and have process
isolation thanks to --isolate-extensions.)  The same is done for one
extension opening a popup to another extension.

Two checks had to be changed to avoid the BrowsingInstance swap.  One
is ChromeContentBrowserClientExtensionsPart::
ShouldSwapBrowsingInstancesForNavigation, which now only forces a
BrowsingInstance swap when navigating to/from CWS.  The other deals
with WebUI.  Since all packaged extensions are considered to need
WebUI, whether or not they actually need WebUI bindings (see
ExtensionWebUI and UseWebUIForURL), the WebUI checks in
RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation were
relaxed to only require a BrowsingInstance swap when there's a change
in WebUI bindings.

BUG=590068
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Keep web popups opened from extensions in same BrowsingInstance.

Previously, popups for web URLs opened from extensions ended up in a
separate BrowsingInstance, which prevented the popup from
communicating to its opener via window.opener.  This is a legitimate
use case for something like OAuth, so this CL relaxes that restriction
and allows them to coexist in the same BrowsingInstance. (The popup
will still be put into a separate SiteInstance and have process
isolation thanks to --isolate-extensions.)  The same is done for one
extension opening a popup to another extension.

Two checks had to be changed to avoid the BrowsingInstance swap.  One
is ChromeContentBrowserClientExtensionsPart::
ShouldSwapBrowsingInstancesForNavigation, which now only forces a
BrowsingInstance swap when navigating to/from CWS.  The other deals
with WebUI.  Since all packaged extensions are considered to need
WebUI, whether or not they actually need WebUI bindings (see
ExtensionWebUI and UseWebUIForURL), the WebUI checks in
RenderFrameHostManager::ShouldSwapBrowsingInstancesForNavigation were
relaxed to only require a BrowsingInstance swap when there's a change
in WebUI bindings.

BUG=590068
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/1f48ef6ac2070ccd99c1c54729602e249c4369d6
Cr-Commit-Position: refs/heads/master@{#423361}
==========

[commit-bot: I haz the power, 2016-10-06 00:53:38]

Patchset 8 (id:??) landed as
https://crrev.com/1f48ef6ac2070ccd99c1c54729602e249c4369d6
Cr-Commit-Position: refs/heads/master@{#423361}