Extension-created windows should share the creator's BrowingInstance.

Extension-created windows should share the creator's BrowingInstance.

Without plumbing through the source SiteInstance, the created window
always ends up cross process and unscriptable by the background page
that created it. This means a background page that opens an extension
resource won't be able to script it, even though they are in related
browsing contexts and have the same origin. Note that navigations to
non-extension origins will still trigger a process swap so we don't
lose extension isolation.

This also fixes CreateTargetContents to use the source_site_instance
parameter if specified; otherwise, the newly created contents will
never be scriptable by same-site pages.

Also see r365431 which fixed the same problem for panels.

BUG=597750
Committed: https://crrev.com/7a9e836958ce3a7a2ddf40a4c8548678ffcbefc8
Cr-Commit-Position: refs/heads/master@{#384655}

[dcheng, 2016-03-31 21:19:39]

dcheng@chromium.org changed reviewers:
+ msw@chromium.org, rdevlin.cronin@chromium.org

[dcheng, 2016-03-31 21:20:47]

Description was changed from

==========
Extension-created windows should share the creator's BrowingInstance.

Without plumbing through the source SiteInstance, the created window
ends up cross process and unscriptable by the background page that
created it. See r365431 which fixes the same problem for panels.

BUG=597750
==========

to

==========
Extension-created windows should share the creator's BrowingInstance.

Without plumbing through the source SiteInstance, the created window
ends up cross process and unscriptable by the background page that
created it. See r365431 which fixes the same problem for panels.

This also fixes CreateTargetContents to use the source_site_instance
parameter if specified; otherwise, the newly created contents will
never be scriptable by same-site pages.

BUG=597750
==========

[msw, 2016-04-01 16:52:41]

c/b/ui rubber stamp lgtm

[Devlin, 2016-04-01 17:20:57]

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (left):

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:645: WebContents* tab =
chrome::AddSelectedTabWithURL(
Do we still need browser_tabstrip.h?

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:649:
navigate_params.source_site_instance =
"Without plumbing through the source SiteInstance, the created window
ends up cross process and unscriptable by the background page that
created it."

I'm not quite sure I follow - don't we sometimes (often) want that?  If an
extension creates a window that has urls to google.com, facebook.com, and
yourbank.com, why would we want that in the same process and scriptable?

https://codereview.chromium.org/1843943003/diff/80001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/window_open/panel_browsing_instance/background.js
(right):

https://codereview.chromium.org/1843943003/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/window_open/panel_browsing_instance/background.js:67:
case 'panel':
indentation

[dcheng, 2016-04-01 17:42:00]

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (left):

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:645: WebContents* tab =
chrome::AddSelectedTabWithURL(
On 2016/04/01 at 17:20:57, Devlin wrote:
> Do we still need browser_tabstrip.h?

Done.

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:649:
navigate_params.source_site_instance =
On 2016/04/01 at 17:20:57, Devlin wrote:
> "Without plumbing through the source SiteInstance, the created window
> ends up cross process and unscriptable by the background page that
> created it."
> 
> I'm not quite sure I follow - don't we sometimes (often) want that?  If an
extension creates a window that has urls to google.com, facebook.com, and
yourbank.com, why would we want that in the same process and scriptable?

It'd cause a navigation from an extension origin to a web origin, which should
process swap. What's broken is if an extension navigates to its own resource, it
can't script itself even though it should be in a 'related browsing context'.

https://codereview.chromium.org/1843943003/diff/80001/chrome/test/data/extens...
File
chrome/test/data/extensions/api_test/window_open/panel_browsing_instance/background.js
(right):

https://codereview.chromium.org/1843943003/diff/80001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/window_open/panel_browsing_instance/background.js:67:
case 'panel':
On 2016/04/01 at 17:20:57, Devlin wrote:
> indentation

Done.

[Devlin, 2016-04-01 18:56:34]

lgtm

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:649:
navigate_params.source_site_instance =
On 2016/04/01 17:42:00, dcheng wrote:
> On 2016/04/01 at 17:20:57, Devlin wrote:
> > "Without plumbing through the source SiteInstance, the created window
> > ends up cross process and unscriptable by the background page that
> > created it."
> > 
> > I'm not quite sure I follow - don't we sometimes (often) want that?  If an
> extension creates a window that has urls to http://google.com,
http://facebook.com, and
> http://yourbank.com, why would we want that in the same process and
scriptable?
> 
> It'd cause a navigation from an extension origin to a web origin, which should
> process swap. What's broken is if an extension navigates to its own resource,
it
> can't script itself even though it should be in a 'related browsing context'.

Ah, got it - can you add that to the CL description?

[dcheng, 2016-04-01 19:01:04]

Description was changed from

==========
Extension-created windows should share the creator's BrowingInstance.

Without plumbing through the source SiteInstance, the created window
ends up cross process and unscriptable by the background page that
created it. See r365431 which fixes the same problem for panels.

This also fixes CreateTargetContents to use the source_site_instance
parameter if specified; otherwise, the newly created contents will
never be scriptable by same-site pages.

BUG=597750
==========

to

==========
Extension-created windows should share the creator's BrowingInstance.

Without plumbing through the source SiteInstance, the created window
always ends up cross process and unscriptable by the background page
that created it. This means a background page that opens an extension
resource won't be able to script it, even though they are in related
browsing contexts and have the same origin. Note that navigations to
non-extension origins will still trigger a process swap so we don't
lose extension isolation.

This also fixes CreateTargetContents to use the source_site_instance
parameter if specified; otherwise, the newly created contents will
never be scriptable by same-site pages.

Also see r365431 which fixed the same problem for panels.

BUG=597750
==========

[dcheng, 2016-04-01 19:01:15]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2016-04-01 19:01:15]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1843943003/#ps100001
(title: "Review comments")

[commit-bot: I haz the power, 2016-04-01 19:02:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1843943003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1843943003/100001

[commit-bot: I haz the power, 2016-04-01 19:09:40]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-01 19:11:51]

Description was changed from

==========
Extension-created windows should share the creator's BrowingInstance.

Without plumbing through the source SiteInstance, the created window
always ends up cross process and unscriptable by the background page
that created it. This means a background page that opens an extension
resource won't be able to script it, even though they are in related
browsing contexts and have the same origin. Note that navigations to
non-extension origins will still trigger a process swap so we don't
lose extension isolation.

This also fixes CreateTargetContents to use the source_site_instance
parameter if specified; otherwise, the newly created contents will
never be scriptable by same-site pages.

Also see r365431 which fixed the same problem for panels.

BUG=597750
==========

to

==========
Extension-created windows should share the creator's BrowingInstance.

Without plumbing through the source SiteInstance, the created window
always ends up cross process and unscriptable by the background page
that created it. This means a background page that opens an extension
resource won't be able to script it, even though they are in related
browsing contexts and have the same origin. Note that navigations to
non-extension origins will still trigger a process swap so we don't
lose extension isolation.

This also fixes CreateTargetContents to use the source_site_instance
parameter if specified; otherwise, the newly created contents will
never be scriptable by same-site pages.

Also see r365431 which fixed the same problem for panels.

BUG=597750

Committed: https://crrev.com/7a9e836958ce3a7a2ddf40a4c8548678ffcbefc8
Cr-Commit-Position: refs/heads/master@{#384655}
==========

[commit-bot: I haz the power, 2016-04-01 19:11:52]

Patchset 6 (id:??) landed as
https://crrev.com/7a9e836958ce3a7a2ddf40a4c8548678ffcbefc8
Cr-Commit-Position: refs/heads/master@{#384655}

[dcheng, 2016-04-02 00:54:25]

Oops, forgot to send draft comments.

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/tabs/tabs_api.cc (right):

https://codereview.chromium.org/1843943003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/tabs/tabs_api.cc:649:
navigate_params.source_site_instance =
On 2016/04/01 at 18:56:34, Devlin wrote:
> On 2016/04/01 17:42:00, dcheng wrote:
> > On 2016/04/01 at 17:20:57, Devlin wrote:
> > > "Without plumbing through the source SiteInstance, the created window
> > > ends up cross process and unscriptable by the background page that
> > > created it."
> > > 
> > > I'm not quite sure I follow - don't we sometimes (often) want that?  If an
> > extension creates a window that has urls to http://google.com,
http://facebook.com, and
> > http://yourbank.com, why would we want that in the same process and
scriptable?
> > 
> > It'd cause a navigation from an extension origin to a web origin, which
should
> > process swap. What's broken is if an extension navigates to its own
resource, it
> > can't script itself even though it should be in a 'related browsing
context'.
> 
> Ah, got it - can you add that to the CL description?

Done.