OOPIF: Data URLs are now rendered in the renderer that initiated the navigation.

OOPIF: Data URLs are now rendered in the renderer that initiated the navigation.

BUG=433014
Committed: https://crrev.com/9ef7d2d3f6122d74e3bbba893dc14ca83cffaa78
Cr-Commit-Position: refs/heads/master@{#308445}

[lfg, 2014-11-19 23:23:49]

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

[lfg, 2014-11-19 23:23:49]

Hey Charlie, PTAL.

I went with the swap policy decision happening on the browser process, it keeps
things simpler and concentrates the swap decision in the browser process (at the
cost of an extra round of IPC calls).

[Charlie Reis, 2014-11-20 19:58:42]

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

[Charlie Reis, 2014-11-20 19:58:43]

[+nasko,site-isolation-reviews@chromium.org]

Thanks for looking at this, and finding a way to get it to work!

I'm not thrilled about passing the SiteInstance all the way through the OpenURL
plumbing, since that can be easily misinterpreted and potentially lead to
security bugs.  (For example, loading an unprivileged page into a privileged
WebUI SiteInstance.)

That said, I'm not seeing a better alternative at the moment, since the OpenURL
plumbing wasn't designed for this kind of thing.  Maybe we can pursue this
approach with care, unless Nasko sees an alternative.  I do feel like this will
probably be better than short-cutting in the renderer, which would require some
careful coordination of creating the new LocalFrame/RenderFrameImpl.

Can you also check whether this bug affects about: URLs as well?  about:blank
typically commits synchronously, so I'm not sure what to expect.  Same for
setting srcdoc on the iframe, which is similar to a data URL.

Please also add a test.  site_per_process_browsertest.cc is probably a good
place, which will let you check which SIteInstance/process the new URL ends up
in.  (It'd be nice to test this from a sibling frame and not just a parent,
which should be possible today if they're all in different processes.)

https://codereview.chromium.org/743773003/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/743773003/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_navigator.cc:260: content::SiteInstance*
site_instance,
Isn't this already in |params|?

https://codereview.chromium.org/743773003/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_navigator.h (right):

https://codereview.chromium.org/743773003/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_navigator.h:69: // Optional site instance that
initiated the navigation.
This should be more precise.  When should callers set this?  The name should
probably more carefully indicate what it's for as well.

I'm concerned that a bug in setting this incorrectly will lead to a security bug
(putting the wrong page into a SiteInstance), which means we may want to take a
different approach.

https://codereview.chromium.org/743773003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/743773003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:724: if
(params.url.scheme() == url::kDataScheme && params.site_instance.get()) {
Hmm, this doesn't feel right to me.  Process model decisions should generally be
made in RenderFrameHostManager, not NavigationController.  This feels like a
corner case that is better handled there (e.g., something like
GetSiteInstanceForNavigation), if we can.

That said, I don't yet see a clean way to do that in the current API.

https://codereview.chromium.org/743773003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/743773003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:98: SiteInstance* site_instance,
It's not clear from the API what this is for.  It doesn't match
|render_frame_host|'s SiteInstance, and it's not always used.  A better name and
comment would help unless we can come up with a simpler approach.

https://codereview.chromium.org/743773003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/743773003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:631: // TODO(creis): Move this to
RenderFrameProxyHost::OpenURL.
Side note: There's a lot of cleanup needed here, which I'll look into. 
RequestOpenURL should probably take in a FrameTreeNode rather than a
RenderFrameHostImpl, and this check needs to be moved to RenderFrameProxyHost.

I mention because it's relevant to what information we have on hand for fixing
this bug.  It seems like we may want some notion of which RFH/RFPH initiated the
navigation in a few places (e.g., when deciding which SiteInstance to use for
data URLs, what to pass to WebContentsObserver::DidOpenRequestedURL).  Maybe a
source_site_instance is useful after all.

https://codereview.chromium.org/743773003/diff/20001/content/public/browser/n...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/743773003/diff/20001/content/public/browser/n...
content/public/browser/navigation_controller.h:109: scoped_refptr<SiteInstance>
site_instance;
Same issues as mentioned before.

[lfg, 2014-12-02 01:03:15]

Hey Charlie, I've implemented the policy on the renderer side.

I had to rebase against a patch that changed the way pending RFH are created, so
I hacked around that a bit to get it uploaded today so we can discuss it in the
meeting tomorrow.

Just want to get some quick feedback, to decide on which approach to continue on
(renderer or browser initiated transition to a local frame).

[Charlie Reis, 2014-12-02 22:47:42]

On 2014/12/02 01:03:15, lfg wrote:
> Hey Charlie, I've implemented the policy on the renderer side.
> 
> I had to rebase against a patch that changed the way pending RFH are created,
so
> I hacked around that a bit to get it uploaded today so we can discuss it in
the
> meeting tomorrow.
> 
> Just want to get some quick feedback, to decide on which approach to continue
on
> (renderer or browser initiated transition to a local frame).

Thanks for trying out that approach.  As we discussed at the meeting, this is
probably a bit too complex and leads to tricky corner cases in the pending RFH
code.

Let's try to pursue the browser-side approach, since it looks like we don't need
to make about:blank URLs commit synchronously (thanks to the NavigationScheduler
behavior).  Clearly labeling the source_site_instance might be a good start.

[lfg, 2014-12-05 21:55:42]

PTAL.

There's one thing I'm not sure is if we need to do something about the
source_site_instance in the entry (i.e. does it need to be serialized in the
session?).

Second issue is that I have the tests disabled now :(. The renderer crashes at
the end of he test because of crbug.com/433012, and since I had to rely on the
renderer to set the title after navigation (because loadstop doesn't work
properly), and because NAV_ENTRY_COMMITED doesn't get triggered because of
crbug.com/236848 . Note that the renderer also crashes at the end of the
NavigateRemoteFrame test, however we don't need the renderer anymore by the time
it crashes, so the test passes.

This also fixes about:blank, we should discuss that offline.

https://codereview.chromium.org/743773003/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_navigator.cc (right):

https://codereview.chromium.org/743773003/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_navigator.cc:260: content::SiteInstance*
site_instance,
On 2014/11/20 19:58:43, Charlie Reis wrote:
> Isn't this already in |params|?

Yep. Removed.

https://codereview.chromium.org/743773003/diff/20001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_navigator.h (right):

https://codereview.chromium.org/743773003/diff/20001/chrome/browser/ui/browse...
chrome/browser/ui/browser_navigator.h:69: // Optional site instance that
initiated the navigation.
On 2014/11/20 19:58:43, Charlie Reis wrote:
> This should be more precise.  When should callers set this?  The name should
> probably more carefully indicate what it's for as well.
> 
> I'm concerned that a bug in setting this incorrectly will lead to a security
bug
> (putting the wrong page into a SiteInstance), which means we may want to take
a
> different approach.

Improved the commend and renamed it to source_site_instance.

https://codereview.chromium.org/743773003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/743773003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_controller_impl.cc:724: if
(params.url.scheme() == url::kDataScheme && params.site_instance.get()) {
On 2014/11/20 19:58:43, Charlie Reis wrote:
> Hmm, this doesn't feel right to me.  Process model decisions should generally
be
> made in RenderFrameHostManager, not NavigationController.  This feels like a
> corner case that is better handled there (e.g., something like
> GetSiteInstanceForNavigation), if we can.
> 
> That said, I don't yet see a clean way to do that in the current API.

Done that, though there's a scary comment about the serialized session, would
the source site instance need to be serialized in the session?

https://codereview.chromium.org/743773003/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/743773003/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator.h:98: SiteInstance* site_instance,
On 2014/11/20 19:58:43, Charlie Reis wrote:
> It's not clear from the API what this is for.  It doesn't match
> |render_frame_host|'s SiteInstance, and it's not always used.  A better name
and
> comment would help unless we can come up with a simpler approach.

Done.

https://codereview.chromium.org/743773003/diff/20001/content/public/browser/n...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/743773003/diff/20001/content/public/browser/n...
content/public/browser/navigation_controller.h:109: scoped_refptr<SiteInstance>
site_instance;
On 2014/11/20 19:58:43, Charlie Reis wrote:
> Same issues as mentioned before.

Done.

[Charlie Reis, 2014-12-06 00:18:51]

Thanks!  I think this general approach will probably work.  Another round of
comments below.

https://codereview.chromium.org/743773003/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_navigator.h (right):

https://codereview.chromium.org/743773003/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser_navigator.h:70: // cross-site navigation is initiated
by a remote frame.
Why?  It may only matter in that case for now, but it would be more consistent
to set it in all cases.

I'd suggest simplifying the comment to "SiteInstance of the frame that initiated
the navigation" and setting it in the LocalFrame case as well.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_entry_impl.h:110: // The
source_site_instance is used to identify the SiteInstance that
nit: the SiteInstance of the frame that initiated the navigation.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_entry_impl.h:237:
scoped_refptr<SiteInstanceImpl> source_site_instance_;
This is solely used as input for determining site_instance_ before the entry
commits, so we will not persist it.  Please move it further down near the fields
that get cleared, and give it a comment saying that (plus that we'll clear it in
|ResetForCommit|).

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:585: OpenURL(params,
nullptr);
Let's pass GetSiteInstance().

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:277: // initiated by a
remote frame.
Let's call it source_site_instance and update the comment as in the other cases.

Please also add:
TODO(creis): Remove this method and have RenderFrameProxyHost call
RequestOpenURL with its FrameTreeNode.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:379:
pending_render_frame_host, transfer_url, nullptr, rest_of_chain, referrer,
Darn.  We don't have the source SiteInstance at this point in time (after server
redirects).  It might still be on the pending entry but that's not guaranteed.

The good news is, that won't break our current use case, since a server redirect
can't go to about:blank, at least in a way that allows you to script the
resulting page.

I suppose nullptr is ok in this case, requiring us to document that it may be
null when we don't know it.  I still think it's worth setting it in the common
case.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:971: return
source_instance ? source_instance : current_instance;
Ooh, this is wrong, and if we don't have any tests that would catch it, we
should add one.

Suppose a window on site A is telling another window to go from B1 to B2.  This
logic would load B2 in A's SiteInstance.

We only want to use source_site_instance if the dest_url is about:blank or a
data URL, right?  Oh, and I guess we should test iframe srcdoc as well.  I don't
know that feature as well, but I think it behaves similarly to data URLs.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:440: SiteInstance*
current_instance,
Please put current_instance between source_instance and dest_instance, and
explain the difference in the comment.  (Source == the frame that initiated the
navigation, current == the frame being navigated, dest == a possibly
predetermined result for the frame being navigated.)

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:521: ui::PageTransition
dest_transition,
Actually, let's remove dest_ from transition in the cases above rather than add
it here.  Transition is a property of the navigation, not the source or the
destination.

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_ins...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_ins...
content/browser/site_instance_impl.cc:276: if (dest_url.scheme() ==
url::kDataScheme)
I'd like to avoid this if we can.  Generally, typing a data URL into the omnibox
should cause a process swap.  In fact, I'd almost like the reverse (always
return false if the dest is a data URL), since two data URLs are effectively
different sites.  That should be part of a separate CL, though.

It would be better to treat data URLs and about URLs as a special case in RFHM
before getting to IsSameWebSite.

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:169: void
SitePerProcessBrowserTest::NavigateIframeToURL(Shell* window,
Please don't change this.  It's more useful to have the EXPECT_TRUE line in the
test than in the helper function, because the top line number will be the line
of the test that's to blame.

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:729: // Disabled because of
http://crbug.com/433012 .
If we can avoid using script to check the title below, will this pass (similarly
to NavigateRemoteFrame)?

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:758: "iframe.onload = function()
{ document.title = 'LOADED'; };"
Can we avoid using the title?  I don't understand your comment about
NAV_ENTRY_COMMITTED not working due to crbug.com/236848.  It should work for
anything past the first navigation in the frame, and we're using it in other
tests in this class.

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:776:
DISABLED_NavigateRemoteBlankURL) {
nit: NavigateRemoteToBlankURL

https://codereview.chromium.org/743773003/diff/80001/content/public/browser/n...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/743773003/diff/80001/content/public/browser/n...
content/public/browser/navigation_controller.h:109: // cross-site navigation is
initiated by a remote frame.
Same comments as elsewhere.

[lfg, 2014-12-08 20:45:33]

I've made most of the changes suggested.

The EXPECT_TRUE in the browsertest was broken the way it was, so it would be
better to find another way to better report that. Also, look at the comment
about the NAV_ENTRY_COMMITED, that may be another bug that we need to look at.

https://codereview.chromium.org/743773003/diff/80001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_navigator.h (right):

https://codereview.chromium.org/743773003/diff/80001/chrome/browser/ui/browse...
chrome/browser/ui/browser_navigator.h:70: // cross-site navigation is initiated
by a remote frame.
On 2014/12/06 00:18:50, Charlie Reis wrote:
> Why?  It may only matter in that case for now, but it would be more consistent
> to set it in all cases.
> 
> I'd suggest simplifying the comment to "SiteInstance of the frame that
initiated
> the navigation" and setting it in the LocalFrame case as well.

Done.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_entry_impl.h:110: // The
source_site_instance is used to identify the SiteInstance that
On 2014/12/06 00:18:50, Charlie Reis wrote:
> nit: the SiteInstance of the frame that initiated the navigation.

Done.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/navigation_entry_impl.h:237:
scoped_refptr<SiteInstanceImpl> source_site_instance_;
On 2014/12/06 00:18:50, Charlie Reis wrote:
> This is solely used as input for determining site_instance_ before the entry
> commits, so we will not persist it.  Please move it further down near the
fields
> that get cleared, and give it a comment saying that (plus that we'll clear it
in
> |ResetForCommit|).

Done.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:585: OpenURL(params,
nullptr);
On 2014/12/06 00:18:50, Charlie Reis wrote:
> Let's pass GetSiteInstance().

Done.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:277: // initiated by a
remote frame.
On 2014/12/06 00:18:50, Charlie Reis wrote:
> Let's call it source_site_instance and update the comment as in the other
cases.
> 
> Please also add:
> TODO(creis): Remove this method and have RenderFrameProxyHost call
> RequestOpenURL with its FrameTreeNode.

Done.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:379:
pending_render_frame_host, transfer_url, nullptr, rest_of_chain, referrer,
On 2014/12/06 00:18:50, Charlie Reis wrote:
> Darn.  We don't have the source SiteInstance at this point in time (after
server
> redirects).  It might still be on the pending entry but that's not guaranteed.
> 
> The good news is, that won't break our current use case, since a server
redirect
> can't go to about:blank, at least in a way that allows you to script the
> resulting page.
> 
> I suppose nullptr is ok in this case, requiring us to document that it may be
> null when we don't know it.  I still think it's worth setting it in the common
> case.

Done.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:971: return
source_instance ? source_instance : current_instance;
On 2014/12/06 00:18:50, Charlie Reis wrote:
> Ooh, this is wrong, and if we don't have any tests that would catch it, we
> should add one.
> 
> Suppose a window on site A is telling another window to go from B1 to B2. 
This
> logic would load B2 in A's SiteInstance.
> 
> We only want to use source_site_instance if the dest_url is about:blank or a
> data URL, right?  Oh, and I guess we should test iframe srcdoc as well.  I
don't
> know that feature as well, but I think it behaves similarly to data URLs.

I switched to a specific case instead of using IsSameWebSite(). I looked iframe
srcdoc, and it seems to navigate to about:blank first, which causes the correct
process to be selected, however, after navigating to about:blank it doesn't
render the correct html. We should create a new bug to fix that.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:440: SiteInstance*
current_instance,
On 2014/12/06 00:18:50, Charlie Reis wrote:
> Please put current_instance between source_instance and dest_instance, and
> explain the difference in the comment.  (Source == the frame that initiated
the
> navigation, current == the frame being navigated, dest == a possibly
> predetermined result for the frame being navigated.)

Done.

https://codereview.chromium.org/743773003/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.h:521: ui::PageTransition
dest_transition,
On 2014/12/06 00:18:50, Charlie Reis wrote:
> Actually, let's remove dest_ from transition in the cases above rather than
add
> it here.  Transition is a property of the navigation, not the source or the
> destination.

Done.

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_ins...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_ins...
content/browser/site_instance_impl.cc:276: if (dest_url.scheme() ==
url::kDataScheme)
On 2014/12/06 00:18:50, Charlie Reis wrote:
> I'd like to avoid this if we can.  Generally, typing a data URL into the
omnibox
> should cause a process swap.  In fact, I'd almost like the reverse (always
> return false if the dest is a data URL), since two data URLs are effectively
> different sites.  That should be part of a separate CL, though.
> 
> It would be better to treat data URLs and about URLs as a special case in RFHM
> before getting to IsSameWebSite.

Should we also remove about:blank urls from IsSameSiteInstance() ?

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:169: void
SitePerProcessBrowserTest::NavigateIframeToURL(Shell* window,
On 2014/12/06 00:18:51, Charlie Reis wrote:
> Please don't change this.  It's more useful to have the EXPECT_TRUE line in
the
> test than in the helper function, because the top line number will be the line
> of the test that's to blame.

I agree it's more useful to see the test immediately, however the way it was
currently implemented is broken, because if ExecuteScript() fails, then the
load_observer will never return from the message loop and the test will time
out.

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:729: // Disabled because of
http://crbug.com/433012 .
On 2014/12/06 00:18:51, Charlie Reis wrote:
> If we can avoid using script to check the title below, will this pass
(similarly
> to NavigateRemoteFrame)?

Yes, it would. If we don't depend on the renderer after we send the load
request, it'll pass.

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:758: "iframe.onload = function()
{ document.title = 'LOADED'; };"
On 2014/12/06 00:18:51, Charlie Reis wrote:
> Can we avoid using the title?  I don't understand your comment about
> NAV_ENTRY_COMMITTED not working due to crbug.com/236848.  It should work for
> anything past the first navigation in the frame, and we're using it in other
> tests in this class.

RenderFrameHostImpl::IsCrossProcessSubframe() returns false, since we are
navigating to the same site instance as the parent frame. That causes
NavigationControllerImpl::ClassifyNavigation() to classify the navigation as
NAVIGATION_TYPE_AUTO_SUBFRAME, which causes
NavigationControllerImpl::RendererDidNavigate() to early return and not send the
notification.

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:776:
DISABLED_NavigateRemoteBlankURL) {
On 2014/12/06 00:18:51, Charlie Reis wrote:
> nit: NavigateRemoteToBlankURL

Done.

[Charlie Reis, 2014-12-09 19:14:38]

Great.  A few nits and two thoughts on the test code.

Also, it occurred to me that this is changing default Chrome's behavior as well,
in a good way.  We're currently broken in the following case:
1) Window 1 opens window 2.
2) Window 2 navigates cross-process.
3) Window 1 tells window 2 to go to about:blank and then tries to script it.

Can you also add a browsertest for that to
render_frame_host_manager_browsertest.cc?  It should just work with your change,
and it covers the change to default Chrome (as opposed to --site-per-process).

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:169: void
SitePerProcessBrowserTest::NavigateIframeToURL(Shell* window,
On 2014/12/08 20:45:33, lfg wrote:
> On 2014/12/06 00:18:51, Charlie Reis wrote:
> > Please don't change this.  It's more useful to have the EXPECT_TRUE line in
> the
> > test than in the helper function, because the top line number will be the
line
> > of the test that's to blame.
> 
> I agree it's more useful to see the test immediately, however the way it was
> currently implemented is broken, because if ExecuteScript() fails, then the
> load_observer will never return from the message loop and the test will time
> out.

Ah, good point.  Instead, let's return false before calling load_observer.Wait()
if ExecuteScript returns false.  That would let this still return a bool and
have the EXPECT_TRUE in the test itself.

https://codereview.chromium.org/743773003/diff/80001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:758: "iframe.onload = function()
{ document.title = 'LOADED'; };"
On 2014/12/08 20:45:33, lfg wrote:
> On 2014/12/06 00:18:51, Charlie Reis wrote:
> > Can we avoid using the title?  I don't understand your comment about
> > NAV_ENTRY_COMMITTED not working due to crbug.com/236848.  It should work for
> > anything past the first navigation in the frame, and we're using it in other
> > tests in this class.
> 
> RenderFrameHostImpl::IsCrossProcessSubframe() returns false, since we are
> navigating to the same site instance as the parent frame. That causes
> NavigationControllerImpl::ClassifyNavigation() to classify the navigation as
> NAVIGATION_TYPE_AUTO_SUBFRAME, which causes
> NavigationControllerImpl::RendererDidNavigate() to early return and not send
the
> notification.

Ah!  Good find.

We could try to revise that check in ClassifyNavigation, since it's just a
temporary hack.  That will let these tests listen for commit and pass.

And actually, I'm curious about why we get page ID -1 in that case.  When it
comes back into the parent process, it should get the page ID from the existing
RenderView (shared with the main frame's RenderFrame).  I wonder if we're
incorrectly constructing a new RenderView?

Fixing that might be better than changing ClassifyNavigation.

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:273: // TODO(creis): Remove
this method and have RenderFrameProxyHost call
nit: Move TODO to the end of the comment (since the rest will stick around when
we remove it).

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:275: // Treat this
prospective navigation as thought it originated from the frame.
nit: thought -> though

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:276: // Used, e.g., for a
navigation request that originated from a a RemoteFrame.
nit: fix "a a"

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

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:963: // Use the source
site instance in case of data URLs or about:blank pages.
nit: source SiteInstance
nit: Add ", because the content is then controlled and/or scriptable by the
source SiteInstance."

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:438: // used for the
resulting frame.
nit: "resulting frame" seems to imply it could end up in a different
FrameTreeNode.  Let's just say "is a predetermined SiteInstance that will be
used if not null."

[lfg, 2014-12-10 21:53:00]

Instead of adding a new test, I modified NoScriptAccessAfterSwapOut to navigate
to a blank frame at the end and test scripting again. I thought it made more
sense than adding a new test.

Now for the ClassifyNavigation problem, my understanding of it was incorrect.
The page_id is indeed != -1, we never get into that code path. However, the
problem still exists, as the return of NAVIGATION_TYPE_AUTO_SUBFRAME still
happens, and that cause it to not dispatch the notification. How do you think we
should solve this? Even if it's a hack in similar ways to what we currently
have, I couldn't think about how to do that, since both frames are in the same
SiteInstance, so I'm not sure how to hack it without modifying the current
behavior.

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:273: // TODO(creis): Remove
this method and have RenderFrameProxyHost call
On 2014/12/09 19:14:37, Charlie Reis wrote:
> nit: Move TODO to the end of the comment (since the rest will stick around
when
> we remove it).

Done.

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:275: // Treat this
prospective navigation as thought it originated from the frame.
On 2014/12/09 19:14:37, Charlie Reis wrote:
> nit: thought -> though

Done.

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:276: // Used, e.g., for a
navigation request that originated from a a RemoteFrame.
On 2014/12/09 19:14:37, Charlie Reis wrote:
> nit: fix "a a"

Done.

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

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:963: // Use the source
site instance in case of data URLs or about:blank pages.
On 2014/12/09 19:14:37, Charlie Reis wrote:
> nit: source SiteInstance
> nit: Add ", because the content is then controlled and/or scriptable by the
> source SiteInstance."

Done.

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/743773003/diff/100001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:438: // used for the
resulting frame.
On 2014/12/09 19:14:37, Charlie Reis wrote:
> nit: "resulting frame" seems to imply it could end up in a different
> FrameTreeNode.  Let's just say "is a predetermined SiteInstance that will be
> used if not null."

Done.

[Charlie Reis, 2014-12-11 00:43:39]

On 2014/12/10 21:53:00, lfg wrote:
> Instead of adding a new test, I modified NoScriptAccessAfterSwapOut to
navigate
> to a blank frame at the end and test scripting again. I thought it made more
> sense than adding a new test.

Yep, that sounds good.

> Now for the ClassifyNavigation problem, my understanding of it was incorrect.
> The page_id is indeed != -1, we never get into that code path. However, the
> problem still exists, as the return of NAVIGATION_TYPE_AUTO_SUBFRAME still
> happens, and that cause it to not dispatch the notification. How do you think
we
> should solve this? Even if it's a hack in similar ways to what we currently
> have, I couldn't think about how to do that, since both frames are in the same
> SiteInstance, so I'm not sure how to hack it without modifying the current
> behavior.

I patched in the CL to try to understand where we're going wrong. 
Unfortunately, I don't see an great way forward:
1) NavigateFrameToURL doesn't work because it doesn't pass a source
SiteInstance.  It crashes, but even with my suggested fix to RFHM.cc we'll end
up in the wrong process.
2) NavigateIframeToURL doesn't work because the renderer process is not
recognizing it as a new navigation.  This appears to stem from Blink's
HistoryCommitType, and may not be trivial to fix just yet.
3) The title watcher triggers 433012.

Sigh.  I suppose we can leave the tests disabled for now, and find ways to
re-enable them.  Fixing the Blink issue and using NavigateIframeToURL would be
my vote, since that's broken in general.

(Alternatively, Nasko, what would you think about adding a version of
NavigateFrameToURL that takes in a source SiteInstance?  That might let us
enable the test.)

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

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:972: if (dest_url ==
blank_page || dest_url.scheme() == url::kDataScheme)
We should also test whether source_instance is null here, right?  We wouldn't
want to return null.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:161: // We
now navigate to a blank window.
nit: We now navigate the window to an about:blank page.

("Blank" is ambiguous because the other tests are referring to target=_blank,
not about:blank.)

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:171:
EXPECT_EQ("blank",
Let's just compare GetLastCommittedURL() to GURL(kAboutBlankURL), rather than
comparing paths.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:173:
scoped_refptr<SiteInstance> blank_site_instance2(
This can then be blank_site_instance.

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

https://codereview.chromium.org/743773003/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:874: // Disabled because of
http://crbug.com/433012 .
nit: No space before period.

Let's also rephrase, since there's a few ways this could be fixed:
Disabled, because the script call for the title watcher triggers
http://crbug.com/433012.  Alternatively, this test could use NavigateIframeToURL
if we classified the navigation as MANUAL_SUBFRAME (and thus generated a commit
notification) or if it waited for DidStopLoading (currently broken).

https://codereview.chromium.org/743773003/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:919: // Disabled because of
http://crbug.com/433012 .
Same as above.

https://codereview.chromium.org/743773003/diff/140001/content/test/data/click...
File content/test/data/click-noreferrer-links.html (right):

https://codereview.chromium.org/743773003/diff/140001/content/test/data/click...
content/test/data/click-noreferrer-links.html:27: function
clickBlankTargetedLink() {
nit: Let's move this after clickNoRefLink(), since all the other functions here
are related to a different type of test.

https://codereview.chromium.org/743773003/diff/140001/content/test/data/click...
content/test/data/click-noreferrer-links.html:98:
blank_targeted_link=foo</a><br>
Same here (move after noref_link).

[nasko, 2014-12-11 01:14:44]

> (Alternatively, Nasko, what would you think about adding a version of
> NavigateFrameToURL that takes in a source SiteInstance?  That might let us
> enable the test.)

I don't see any problem adding a variation of NavigateFrameToURL. I'd rather do
that than keep tests disabled. We can always remove it later if when we fix
enough code to make testing possible without it.

https://codereview.chromium.org/743773003/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator.h (right):

https://codereview.chromium.org/743773003/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator.h:69: // Site instance of the frame that
initiated the navigation or null if we
nit: s/Site instance/SiteInstance/

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:110: // The
source_site_instance is used to identify the SiteInstance of the frame
nit: |source_site_instance|

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1281:
TRACE_EVENT1("navigation", "RenderFrameHostImpl::OnOpenURL", "url",
nit: The method is no longer called OnOpenURL, so the string must be updated.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:275: //
|source_site_instance| is the site instance of the frame that initiated the
nit: s/site instance/SiteInstance/

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

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:971: GURL
blank_page(url::kAboutBlankURL);
nit: s/blank_page/about_blank/. Any page with valid URL (not about:blank) can
also be blank. Let's be a bit more specific.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:434: // |source_instance|
is the SiteInstance of the frame that initiated the
nit: I'd avoid putting each param description on a new line, as it leaves lots
of whitespace that isn't used.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:436: //
|current_instance| is the SiteInstance of the frame that is currently
An example explaining the difference between the current and source will be
helpful. I'm not quite sure I get it just be reading the comment.

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

https://codereview.chromium.org/743773003/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:898: // parent renderer, so the
data URL should be rendered in the parent renderer.
I'd rephrase it a bit to be more specific.

The navigation is initiated from a script in the parent frame, so the data URL
should be committed in the same SiteInstance as the parent frame.

https://codereview.chromium.org/743773003/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:922: GURL
main_url(embedded_test_server()->GetURL("/site_per_process_main.html"));
Both this method and the above differ only in the URL that is being loaded. That
is too much code duplication. Why not pull all the code into a common method and
call it with the respective URL as input?

https://codereview.chromium.org/743773003/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:944: GURL
data_url("about:blank");
nit: this is no longer data_url

https://codereview.chromium.org/743773003/diff/140001/content/public/browser/...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/743773003/diff/140001/content/public/browser/...
content/public/browser/navigation_controller.h:108: // Site instance of the
frame that initiated the navigation or null if we
nit: s/Site instance/SiteInstance/

[lfg, 2014-12-11 23:55:25]

So, I tried two different approaches to try to fix the tests, and both failed.

First, I tried adding a NavigateFrameToURL with a source site instance option.
While it can navigate the correct frame, the renderer still sees it as an
AUTO_SUBFRAME navigation, which causes the issues classifying the navigation in
the browser.

Then I thought maybe if I forced mouse events into the renderer I could make the
renderer return a MANUAL_SUBFRAME, but that also doesn't work, because the
renderer always sees the navigation as being AUTO. That happens because it sees
as a browser initiated navigation, without user interaction. We'll need to send
more information in the IPCs that navigate the subframe to fix it. Also, there's
a comment in RenderFrameImpl::SendDidCommitProvisionalLoad that says we should
call WillNavigateToURL, but I couldn't find that function.

I think a way to hack the tests into working would be to just run all the
pending messages in the queue, without waiting for any specific notification,
though it may lead to timing issues in the tests.

https://codereview.chromium.org/743773003/diff/140001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator.h (right):

https://codereview.chromium.org/743773003/diff/140001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator.h:69: // Site instance of the frame that
initiated the navigation or null if we
On 2014/12/11 01:14:43, nasko wrote:
> nit: s/Site instance/SiteInstance/

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/navigation_entry_impl.h:110: // The
source_site_instance is used to identify the SiteInstance of the frame
On 2014/12/11 01:14:44, nasko wrote:
> nit: |source_site_instance|

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1281:
TRACE_EVENT1("navigation", "RenderFrameHostImpl::OnOpenURL", "url",
On 2014/12/11 01:14:44, nasko wrote:
> nit: The method is no longer called OnOpenURL, so the string must be updated.

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.h:275: //
|source_site_instance| is the site instance of the frame that initiated the
On 2014/12/11 01:14:44, nasko wrote:
> nit: s/site instance/SiteInstance/

Done.

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

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:971: GURL
blank_page(url::kAboutBlankURL);
On 2014/12/11 01:14:44, nasko wrote:
> nit: s/blank_page/about_blank/. Any page with valid URL (not about:blank) can
> also be blank. Let's be a bit more specific.

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.cc:972: if (dest_url ==
blank_page || dest_url.scheme() == url::kDataScheme)
On 2014/12/11 00:43:39, Charlie Reis wrote:
> We should also test whether source_instance is null here, right?  We wouldn't
> want to return null.

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager.h (right):

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:434: // |source_instance|
is the SiteInstance of the frame that initiated the
On 2014/12/11 01:14:44, nasko wrote:
> nit: I'd avoid putting each param description on a new line, as it leaves lots
> of whitespace that isn't used.

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager.h:436: //
|current_instance| is the SiteInstance of the frame that is currently
On 2014/12/11 01:14:44, nasko wrote:
> An example explaining the difference between the current and source will be
> helpful. I'm not quite sure I get it just be reading the comment.

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_manager_browsertest.cc
(right):

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:161: // We
now navigate to a blank window.
On 2014/12/11 00:43:39, Charlie Reis wrote:
> nit: We now navigate the window to an about:blank page.
> 
> ("Blank" is ambiguous because the other tests are referring to target=_blank,
> not about:blank.)

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:171:
EXPECT_EQ("blank",
On 2014/12/11 00:43:39, Charlie Reis wrote:
> Let's just compare GetLastCommittedURL() to GURL(kAboutBlankURL), rather than
> comparing paths.

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_manager_browsertest.cc:173:
scoped_refptr<SiteInstance> blank_site_instance2(
On 2014/12/11 00:43:39, Charlie Reis wrote:
> This can then be blank_site_instance.

Removed, as we don't need to save the reference in this case.

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

https://codereview.chromium.org/743773003/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:874: // Disabled because of
http://crbug.com/433012 .
On 2014/12/11 00:43:39, Charlie Reis wrote:
> nit: No space before period.
> 
> Let's also rephrase, since there's a few ways this could be fixed:
> Disabled, because the script call for the title watcher triggers
> http://crbug.com/433012.  Alternatively, this test could use
NavigateIframeToURL
> if we classified the navigation as MANUAL_SUBFRAME (and thus generated a
commit
> notification) or if it waited for DidStopLoading (currently broken).

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:898: // parent renderer, so the
data URL should be rendered in the parent renderer.
On 2014/12/11 01:14:44, nasko wrote:
> I'd rephrase it a bit to be more specific.
> 
> The navigation is initiated from a script in the parent frame, so the data URL
> should be committed in the same SiteInstance as the parent frame.

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:919: // Disabled because of
http://crbug.com/433012 .
On 2014/12/11 00:43:39, Charlie Reis wrote:
> Same as above.

Done.

https://codereview.chromium.org/743773003/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:922: GURL
main_url(embedded_test_server()->GetURL("/site_per_process_main.html"));
On 2014/12/11 01:14:44, nasko wrote:
> Both this method and the above differ only in the URL that is being loaded.
That
> is too much code duplication. Why not pull all the code into a common method
and
> call it with the respective URL as input?

I think those two should be in the same test. The only reason I tried to split
them is to try to get around the renderer crash (which I couldn't find any way).

Since it's also very similar to NavigateRemoteFrame test, I put a TODO to merge
them all together when these issues are fixed.

https://codereview.chromium.org/743773003/diff/140001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:944: GURL
data_url("about:blank");
On 2014/12/11 01:14:44, nasko wrote:
> nit: this is no longer data_url

Done.

https://codereview.chromium.org/743773003/diff/140001/content/public/browser/...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/743773003/diff/140001/content/public/browser/...
content/public/browser/navigation_controller.h:108: // Site instance of the
frame that initiated the navigation or null if we
On 2014/12/11 01:14:44, nasko wrote:
> nit: s/Site instance/SiteInstance/

Done.

https://codereview.chromium.org/743773003/diff/140001/content/test/data/click...
File content/test/data/click-noreferrer-links.html (right):

https://codereview.chromium.org/743773003/diff/140001/content/test/data/click...
content/test/data/click-noreferrer-links.html:27: function
clickBlankTargetedLink() {
On 2014/12/11 00:43:39, Charlie Reis wrote:
> nit: Let's move this after clickNoRefLink(), since all the other functions
here
> are related to a different type of test.

Done.

https://codereview.chromium.org/743773003/diff/140001/content/test/data/click...
content/test/data/click-noreferrer-links.html:98:
blank_targeted_link=foo</a><br>
On 2014/12/11 00:43:39, Charlie Reis wrote:
> Same here (move after noref_link).

Done.

[Charlie Reis, 2014-12-12 20:19:22]

Ok, after more digging I think we'll probably want to fix
http://crbug.com/441863 to make the subframe tests work.

I think this is ok to land in the meantime, because it already improves behavior
in default Chrome and we have a working test for that case.

LGTM!

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

https://codereview.chromium.org/743773003/diff/170001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:878: // and classified the
navigation as MANUAL_SUBFRAME or if we waited for
Let's reference http://crbug.com/441863 for the classification bug.

[lfg, 2014-12-12 21:54:08]

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

https://codereview.chromium.org/743773003/diff/170001/content/browser/site_pe...
content/browser/site_per_process_browsertest.cc:878: // and classified the
navigation as MANUAL_SUBFRAME or if we waited for
On 2014/12/12 20:19:22, Charlie Reis wrote:
> Let's reference http://crbug.com/441863 for the classification bug.

Done.

[lfg, 2014-12-12 21:54:52]

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

[lfg, 2014-12-12 21:54:53]

sky@chromium.org: Please review changes in 

chrome/browser/ui/browser_navigator.{h,cc}

[sky, 2014-12-13 00:08:57]

https://codereview.chromium.org/743773003/diff/190001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator.h (right):

https://codereview.chromium.org/743773003/diff/190001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator.h:71: scoped_refptr<content::SiteInstance>
source_site_instance;
You need to improve this description. Is this an in param? Out param? Who sets
it? What is it used for? Do I need to set it? Also, since it's not that common
to use this how about moving it to the end?

[lfg, 2014-12-15 20:24:34]

https://codereview.chromium.org/743773003/diff/190001/chrome/browser/ui/brows...
File chrome/browser/ui/browser_navigator.h (right):

https://codereview.chromium.org/743773003/diff/190001/chrome/browser/ui/brows...
chrome/browser/ui/browser_navigator.h:71: scoped_refptr<content::SiteInstance>
source_site_instance;
On 2014/12/13 00:08:57, sky wrote:
> You need to improve this description. Is this an in param? Out param? Who sets
> it? What is it used for? Do I need to set it? Also, since it's not that common
> to use this how about moving it to the end?

Moved it to the end and tried to improve the comment.

[sky, 2014-12-15 20:52:03]

LGTM

[lfg, 2014-12-15 22:26:42]

The CQ bit was checked by lfg@chromium.org

[commit-bot: I haz the power, 2014-12-15 22:27:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/743773003/230001

[commit-bot: I haz the power, 2014-12-15 22:32:48]

Committed patchset #13 (id:230001)

[commit-bot: I haz the power, 2014-12-15 22:34:05]

Patchset 13 (id:??) landed as
https://crrev.com/9ef7d2d3f6122d74e3bbba893dc14ca83cffaa78
Cr-Commit-Position: refs/heads/master@{#308445}