Take session history SiteInstance into account for unique origin navigations.

Take session history SiteInstance into account for unique origin navigations.

This CL fixes an issue with navigating to data: URLs in --site-per-process
mode, which caused the data: URL to be loaded in incorrect process.

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

Committed: https://crrev.com/58b07f5147cb47dc85302bd3a112efb627c972ff
Cr-Commit-Position: refs/heads/master@{#392450}

[nasko, 2016-05-07 00:08:17]

Description was changed from

==========
Take session history SiteInstance into account for unique origin navigations.

This CL fixes an issue with navigating to data: URLs in --site-per-process
mode, which caused the data: URL to be loaded in incorrect process.

BUG=606996
==========

to

==========
Take session history SiteInstance into account for unique origin navigations.

This CL fixes an issue with navigating to data: URLs in --site-per-process
mode, which caused the data: URL to be loaded in incorrect process.

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

[nasko, 2016-05-07 00:09:31]

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

[nasko, 2016-05-07 00:09:32]

Hey Charlie,
Can you review this CL for me? It fixes the bug where we put the data: URLs in
the wrong process when doing session history navigations.
Thanks in advance!
Nasko

[Charlie Reis, 2016-05-09 17:05:10]

Great!

This looks like it improves the about:blank case as well, at least in terms of
matching the spec (i.e., putting about:blank in the SiteInstance that navigated
it).  Is that right?  If so, please add a similar test for about:blank.

Otherwise looks good with nits.

https://codereview.chromium.org/1960703003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1960703003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1114: // and the frame
being navigated differ, |souce_instance| is set to the
nit: source_instance

https://codereview.chromium.org/1960703003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2530: SiteInstance*
dest_instance) {
nit: Let's repeat that DCHECK(!source_instance || !dest_instance) here.  I know
that both the normal and PlzNavigate places that call here also call
GetSiteInstanceForNavigation, but the normal path (UpdateStateForNavigate) calls
this one first.  Might as well catch it early if it ever goes wrong.

https://codereview.chromium.org/1960703003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2547: return false;
This is a multi-line body with the comments here, so we should put braces on the
whole thing.

https://codereview.chromium.org/1960703003/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1960703003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:6290: GURL
about_blank_url("about:blank");
Looks unused.

https://codereview.chromium.org/1960703003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:6311: SiteInstanceImpl*
orig_site_instance =
scoped_refptr

https://codereview.chromium.org/1960703003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:6313:
EXPECT_NE(orig_site_instance, root->current_frame_host()->GetSiteInstance());
nit: Reverse order.  (orig_site_instance is the actual value in this case, being
tested against the expected value of the root SiteInstance.)

The other cases below look correct.

[nasko, 2016-05-09 18:33:15]

Test case added in a separate patchset.

https://codereview.chromium.org/1960703003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/1960703003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:1114: // and the frame
being navigated differ, |souce_instance| is set to the
On 2016/05/09 17:05:10, Charlie Reis wrote:
> nit: source_instance

Done.

https://codereview.chromium.org/1960703003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2530: SiteInstance*
dest_instance) {
On 2016/05/09 17:05:10, Charlie Reis wrote:
> nit: Let's repeat that DCHECK(!source_instance || !dest_instance) here.  I
know
> that both the normal and PlzNavigate places that call here also call
> GetSiteInstanceForNavigation, but the normal path (UpdateStateForNavigate)
calls
> this one first.  Might as well catch it early if it ever goes wrong.

Done.

https://codereview.chromium.org/1960703003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_manager.cc:2547: return false;
On 2016/05/09 17:05:10, Charlie Reis wrote:
> This is a multi-line body with the comments here, so we should put braces on
the
> whole thing.

Done.

https://codereview.chromium.org/1960703003/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1960703003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:6290: GURL
about_blank_url("about:blank");
On 2016/05/09 17:05:10, Charlie Reis wrote:
> Looks unused.

I wonder why clang didn't complain.

https://codereview.chromium.org/1960703003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:6311: SiteInstanceImpl*
orig_site_instance =
On 2016/05/09 17:05:10, Charlie Reis wrote:
> scoped_refptr

Done.

https://codereview.chromium.org/1960703003/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:6313:
EXPECT_NE(orig_site_instance, root->current_frame_host()->GetSiteInstance());
On 2016/05/09 17:05:10, Charlie Reis wrote:
> nit: Reverse order.  (orig_site_instance is the actual value in this case,
being
> tested against the expected value of the root SiteInstance.)
> 
> The other cases below look correct.

Done.

[Charlie Reis, 2016-05-09 20:21:52]

Heh, one nit multiplied into 4 places, but otherwise LGTM!

https://codereview.chromium.org/1960703003/diff/20001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1960703003/diff/20001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6320:
EXPECT_NE(child->current_frame_host()->GetSiteInstance(), orig_site_instance);
Oops, this is the wrong one to change.  In this case, orig_site_instance is the
expected value to compare against, and should go first.  I meant the earlier one
where we're comparing orig_site_instance against root's SiteInstance.

https://codereview.chromium.org/1960703003/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1960703003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6312:
EXPECT_NE(orig_site_instance, root->current_frame_host()->GetSiteInstance());
nit: Swap order.

https://codereview.chromium.org/1960703003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6320:
EXPECT_NE(child->current_frame_host()->GetSiteInstance(), orig_site_instance);
nit: Swap order.

https://codereview.chromium.org/1960703003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6360:
EXPECT_NE(orig_site_instance, root->current_frame_host()->GetSiteInstance());
nit: Swap order.

https://codereview.chromium.org/1960703003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6368:
EXPECT_NE(child->current_frame_host()->GetSiteInstance(), orig_site_instance);
nit: Swap order.

[nasko, 2016-05-09 20:39:21]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-05-09 20:39:21]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1960703003/#ps60001
(title: "Fixed ordering.")

[commit-bot: I haz the power, 2016-05-09 20:39:58]

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

[nasko, 2016-05-09 22:36:21]

https://codereview.chromium.org/1960703003/diff/40001/content/browser/site_pe...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1960703003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6312:
EXPECT_NE(orig_site_instance, root->current_frame_host()->GetSiteInstance());
On 2016/05/09 20:21:52, Charlie Reis wrote:
> nit: Swap order.

Done.

https://codereview.chromium.org/1960703003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6320:
EXPECT_NE(child->current_frame_host()->GetSiteInstance(), orig_site_instance);
On 2016/05/09 20:21:52, Charlie Reis wrote:
> nit: Swap order.

Done.

https://codereview.chromium.org/1960703003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6360:
EXPECT_NE(orig_site_instance, root->current_frame_host()->GetSiteInstance());
On 2016/05/09 20:21:52, Charlie Reis wrote:
> nit: Swap order.

Done.

https://codereview.chromium.org/1960703003/diff/40001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:6368:
EXPECT_NE(child->current_frame_host()->GetSiteInstance(), orig_site_instance);
On 2016/05/09 20:21:52, Charlie Reis wrote:
> nit: Swap order.

Done.

[commit-bot: I haz the power, 2016-05-09 22:38:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-05-09 22:39:42]

Description was changed from

==========
Take session history SiteInstance into account for unique origin navigations.

This CL fixes an issue with navigating to data: URLs in --site-per-process
mode, which caused the data: URL to be loaded in incorrect process.

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

to

==========
Take session history SiteInstance into account for unique origin navigations.

This CL fixes an issue with navigating to data: URLs in --site-per-process
mode, which caused the data: URL to be loaded in incorrect process.

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

Committed: https://crrev.com/58b07f5147cb47dc85302bd3a112efb627c972ff
Cr-Commit-Position: refs/heads/master@{#392450}
==========

[commit-bot: I haz the power, 2016-05-09 22:39:43]

Patchset 4 (id:??) landed as
https://crrev.com/58b07f5147cb47dc85302bd3a112efb627c972ff
Cr-Commit-Position: refs/heads/master@{#392450}