Pass origin information for remote frame creation.

Pass origin information for remote frame creation.

This is the first step towards making origin information available for RemoteFrames in Blink.  This CL ensures that we pass origin information whenever we create a RemoteFrame, which is currently three cases:
1. New view
2. As part of SwapOut
3. NewFrameProxy message

The origins are tracked on FrameTreeNodes in the browser process and updated when frame navigations commit (in NavigatorImpl::DidNavigate).

This CL doesn't yet address these issues, to be fixed in later CLs:
- situations that will require an explicit "origin update" message, including document.domain changes and certain frame navigations.
- iframe sandbox flags
- RenderFrameProxies aren't created in all cases (crbug.com/423587)
- for now, the origin on the browser side is a url::Origin which doesn't support calls like canAccess.  Eventually, we may need to use a richer origin class (like WebSecurityOrigin from Blink).

More information: https://docs.google.com/a/chromium.org/document/d/1Y0s76YK0ziiL8hddiFlNUyAF4hqRAGpZM8cfnnLsZPg/edit#heading=h.lrzgurbjttfm

The Blink-side of this CL is: https://codereview.chromium.org/520213002/

BUG=426512
Committed: https://crrev.com/bc7eafaba8bde63d70a270ee71a901545d9769f9
Cr-Commit-Position: refs/heads/master@{#307138}

[alexmos, 2014-11-12 00:05:45]

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

[alexmos, 2014-11-12 00:05:47]

Hey Charlie, could you please review this CL, which is the content side of my
first step for origin replication?  Nasko: I'm also including you since I know
you wanted to take a look as well.

I still want to figure out how to write more tests for this, and also which
class to use to store origins in the browser process (see comment below).  On
Nick's suggestion, I want to write tests that navigate to pages with cross-site
iframes and use ExecuteScriptAndExtractString to check for correct
location.ancestorOrigins, but I may need to do that in a separate CL, since
enabling location.ancestorOrigins to work for RemoteFrames needs both sides of
this CL to land first.

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_re...
content/common/frame_replication_state.h:20: url::Origin origin;
Is using url::Origin the right thing here?  Although it emphasizes that this is
an origin rather than any URL, it's also inflexible (can't extract parts of
origin, can't properly support unique origins which is why I have the flag for
that below, doesn't have canAccess which we will need in the future).  A GURL
could be slightly better, though it still won't handle unique origins. 
blink::WebSecurityOrigin may be even better, but I don't know if it's allowed to
use that here.  Thoughts?

https://codereview.chromium.org/692973005/diff/120001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/692973005/diff/120001/url/origin.cc#newcode17
url/origin.cc:17: // DCHECK_NE(origin[origin.size() - 1], '/');
I commented out this check since GURL::GetOrigin() always returns origins that
end with '/'.  I could just remove the trailing '/' instead of this, though it
feels the two classes should be  consistent for this.  This won't be necessary
if we switch FrameReplicationState to use GURL or another class to track origins
instead of url::Origin.

[dcheng, 2014-11-12 22:16:42]

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

[dcheng, 2014-11-12 22:16:44]

I didn't take a detailed look, but one high-level comment and one random comment
about casts.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:106:
url.SchemeIs(url::kAboutScheme) ||
Hm... this doesn't seem quite correct. about:blank inherits its parent's
security context, right?

https://codereview.chromium.org/692973005/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:294:
params.remote_frame_state = ((RenderFrameHostImpl*)GetMainFrame())->
Use static_cast here.

[alexmos, 2014-11-12 23:10:06]

On 2014/11/12 22:16:44, dcheng wrote:
> I didn't take a detailed look, but one high-level comment and one random
comment
> about casts.
> 
>
https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
> File content/browser/frame_host/frame_tree_node.cc (right):
> 
>
https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
> content/browser/frame_host/frame_tree_node.cc:106:
> url.SchemeIs(url::kAboutScheme) ||
> Hm... this doesn't seem quite correct. about:blank inherits its parent's
> security context, right?

Yes, you're right.  I forgot to change this; I think the right behavior is to
treat it as a unique origin if there's no parent to inherit from, and otherwise
use the parent's security context.  Rereading
blink::Document::initSecurityContext, it seems we should also inherit in the
case of an empty navigation url.

[Charlie Reis, 2014-11-13 18:00:59]

Nice!  I'm really happy that we'll end up with a general struct with data to
replicate across all RenderFrameProxies for a RenderFrame.

Lots of style nits (etc) below, but no worries there.

I noticed that we're not updating the replication state after the RemoteFrame is
constructed.  Wouldn't it need to change each time the frame navigates?  (e.g.,
A's RemoteFrame needs to know that the origin changed from B to C.)

This is slightly tricky to get right in the API-- many bits of
FrameReplicationState will probably need updating at the time of
DidCommitProvisionalLoad, but others may need to happen at other times (e.g.,
frame sizing state, focus, etc).  We'll want to think about whether the whole
struct gets passed each time, or if we'll a different way to broadcast custom
updates to each proxy.

Either way, we'll probably want a helper function in RenderFrameHostManager to
broadcast an IPC (or something) to all the RenderFrameProxies for the frame.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:203:
IN_PROC_BROWSER_TEST_F(FrameTreeBrowserTest, OriginSetOnNavigation) {
Perhaps we can also add a --site-per-process test on the renderer side (e.g.,
render_view_browsertest.cc), which could verify that the RenderFrameProxy ends
up with the right state?

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:206: GURL
main_url(test_server()->GetURL("files/site_per_process_main.html"));
nit: This isn't a site per process test, so it's a little unusual to use
site_per_process_main.html.  Would something like files/frame_tree/top.html work
here?

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:99: void
FrameTreeNode::SetOriginFromURL(const GURL& url) {
This is more SetReplicationState, right?

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:110: replication_state_.origin =
url::Origin();
Hmm, I'm wondering if there's a better place for this code to live, since it's a
little odd to have delicate origin logic in FrameTreeNode.  That can be part of
the discussion about which class to use for the origin.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:54: void SetOriginFromURL(const
GURL& url);
Perhaps this should be private and called from SetCurrentURL?

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:143: // rendering this frame
remotely.
are rendering this frame remotely -> have proxies for this frame.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:523: // OOPIF: Save the origin of
the new page here, so that it is available to
nit: Remove "OOPIF."  This looks like it happens in default mode as well, and we
may even find uses for it there (e.g., returning the proper origin in console
error messages rather than swappedout://).

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:524: // send as part of
FrameMsg_SwapOut, which will result in a new remote frame
Can we split this in two sentences, or rephrase it a bit?  This sentence is a
bit hard to follow.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:528:
render_frame_host->frame_tree_node()->SetOriginFromURL(params.url);
Would it be easier to just do this action inside the set_current_url call below?
 (Line 569.)

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

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:827: if
(IsRenderFrameLive())
Style: This needs braces if the body doesn't fit on one line.

(Note that the parameters don't have to each be on their own line for call
sites, but there's not a big difference between 2+1 vs each on their own line.)

https://codereview.chromium.org/692973005/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:293: if (!is_active_)
Style: Needs braces if the body doesn't fit on one line.

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_me...
content/common/frame_messages.h:389: content::FrameReplicationState)
nit: We put generally put names for the parameters in comments here.

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_re...
content/common/frame_replication_state.h:13: // This structure holds information
that needs to be replicated from a local
Let's use the content names for LocalFrame and RemoteFrame here: RenderFrame /
RenderFrameProxies.

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_re...
content/common/frame_replication_state.h:20: url::Origin origin;
On 2014/11/12 00:05:47, alexmos wrote:
> Is using url::Origin the right thing here?  Although it emphasizes that this
is
> an origin rather than any URL, it's also inflexible (can't extract parts of
> origin, can't properly support unique origins which is why I have the flag for
> that below, doesn't have canAccess which we will need in the future).  A GURL
> could be slightly better, though it still won't handle unique origins. 
> blink::WebSecurityOrigin may be even better, but I don't know if it's allowed
to
> use that here.  Thoughts?

Hmm,  I agree that url::Origin seems a bit limited for our needs, but we
generally can't use arbitrary Blink classes outside the renderer process.  I've
seen WebSecurityOrigin discussed in the past (e.g., on
https://codereview.chromium.org/452013002/), so I'm pretty sure we won't be able
to use that.

Let's discuss options a bit more in person.

https://codereview.chromium.org/692973005/diff/120001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/common/view_mes...
content/common/view_messages.h:467: // With OOPIF, if this view is swapped out,
its main frame is going to become
nit: With OOPIF -> In --site-per-process mode
(I'm still wondering if we can use this state for a swapped out RenderFrame in
normal mode, but we can consider that in a future CL.)

nit: view is |swapped_out|

nit: is going to become a render frame -> will become a RenderFrameProxy.

https://codereview.chromium.org/692973005/diff/120001/content/common/view_mes...
content/common/view_messages.h:470:
IPC_STRUCT_MEMBER(content::FrameReplicationState, remote_frame_state)
Let's move this just under |swapped_out|, since those two are related.

https://codereview.chromium.org/692973005/diff/120001/content/public/test/ren...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/public/test/ren...
content/public/test/render_view_test.cc:208: FrameReplicationState()); //
remote_frame_state
nit: The comment probably isn't necessary.  It's more useful when it's hard to
infer what the bool or int is for.

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:1198: // initialize remote frame with the
security origin that was passed to us
nit: Initialize the RemoteFrame with the replication state

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:1200: // TODO(alexmos): we cannot yet do
this for swapped-out main frames,
nit: We

This TODO is what Nasko is currently working on, right?

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_frame_proxy.cc:79: // initialize remote frame with the
security origin that was passed to us
nit: Capitalize
nit: RemoteFrame

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_frame_proxy.cc:81: web_frame->setReplicatedOrigin(
We should probably have a more general method for doing this.  Right now the
code is copied between here and SwapOut, and we'll want to add more thing to
replicated state eventually.

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_frame_proxy.h:81: const FrameReplicationState&);
Style: Needs a parameter name that matches the .cc file.

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_thread_impl.h:430: const FrameReplicationState&);
nit: Name parameter.

https://codereview.chromium.org/692973005/diff/120001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/692973005/diff/120001/url/origin.cc#newcode17
url/origin.cc:17: // DCHECK_NE(origin[origin.size() - 1], '/');
On 2014/11/12 00:05:47, alexmos wrote:
> I commented out this check since GURL::GetOrigin() always returns origins that
> end with '/'.  I could just remove the trailing '/' instead of this, though it
> feels the two classes should be  consistent for this.  This won't be necessary
> if we switch FrameReplicationState to use GURL or another class to track
origins
> instead of url::Origin.

Hmm, yes, let's continue the discussion about what class to use in
FrameReplicationState.

[alexmos, 2014-11-18 18:25:33]

Thanks for the comments, Charlie!  PTAL - I've uploaded PS8.  As we've
discussed, I've changed the origin to be sent from Blink with
DidCommitProvisionalLoad after it's already initialized, rather than extracted
from navigation URLs.  That takes care of tricky cases like inheriting parent
security contexts.  Also, added a couple of tests -- a render view test for
checking the renderer side of origin replication, and another test in
frame_tree_browsertests for inheriting origins for about:blank frames as well as
cross-site frame navigation.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:203:
IN_PROC_BROWSER_TEST_F(FrameTreeBrowserTest, OriginSetOnNavigation) {
On 2014/11/13 18:00:57, Charlie Reis wrote:
> Perhaps we can also add a --site-per-process test on the renderer side (e.g.,
> render_view_browsertest.cc), which could verify that the RenderFrameProxy ends
> up with the right state?

Done -- please see RenderViewImplTest.OriginReplicationForSwapOut.  I could also
add something similar for the other two remote frame creation paths
(ViewMsg_New, NewFrameProxy), if I can figure out how to get those to work as
render view tests.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:206: GURL
main_url(test_server()->GetURL("files/site_per_process_main.html"));
On 2014/11/13 18:00:57, Charlie Reis wrote:
> nit: This isn't a site per process test, so it's a little unusual to use
> site_per_process_main.html.  Would something like files/frame_tree/top.html
work
> here?

Done.  It doesn't matter to the test as long as there's at least one frame in
the file.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:106:
url.SchemeIs(url::kAboutScheme) ||
On 2014/11/12 22:16:43, dcheng wrote:
> Hm... this doesn't seem quite correct. about:blank inherits its parent's
> security context, right?

This and the other more complex cases are now fixed by letting Blink initialize
the origin and then passing it in serialized form in DidCommitProvisionalLoad. 
This function is no longer needed - yay.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:110: replication_state_.origin =
url::Origin();
On 2014/11/13 18:00:57, Charlie Reis wrote:
> Hmm, I'm wondering if there's a better place for this code to live, since it's
a
> little odd to have delicate origin logic in FrameTreeNode.  That can be part
of
> the discussion about which class to use for the origin.

See my answer for Daniel's question above.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:54: void SetOriginFromURL(const
GURL& url);
On 2014/11/13 18:00:57, Charlie Reis wrote:
> Perhaps this should be private and called from SetCurrentURL?

This function isn't necessary anymore with the new approach of passing a
serialized origin as part of DidCommitProvisionalLoad.  It's now replaced with
the simple setter set_current_origin().

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:143: // rendering this frame
remotely.
On 2014/11/13 18:00:57, Charlie Reis wrote:
> are rendering this frame remotely -> have proxies for this frame.

Done.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:523: // OOPIF: Save the origin of
the new page here, so that it is available to
On 2014/11/13 18:00:57, Charlie Reis wrote:
> nit: Remove "OOPIF."  This looks like it happens in default mode as well, and
we
> may even find uses for it there (e.g., returning the proper origin in console
> error messages rather than swappedout://).

Done.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:524: // send as part of
FrameMsg_SwapOut, which will result in a new remote frame
On 2014/11/13 18:00:57, Charlie Reis wrote:
> Can we split this in two sentences, or rephrase it a bit?  This sentence is a
> bit hard to follow.

Rephrased.  Please let me know if this makes more sense now.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:528:
render_frame_host->frame_tree_node()->SetOriginFromURL(params.url);
On 2014/11/13 18:00:57, Charlie Reis wrote:
> Would it be easier to just do this action inside the set_current_url call
below?
>  (Line 569.)

That would be too late because we need the origin on the next line inside
DidNavigateFrame (it calls CommitPending, which sends the SwapOut message, which
needs the origin).

There is actually another issue here that I wanted to discuss.  For main frames
only, DidNavigateFrame eventually calls CommitPending ->
RenderViewHostImpl::AttachToFrameTree -> FrameTree::ResetForMainFrameSwap ->
FrameTreeNode::ResetForNewProcess.  ResetForNewProcess is supposed to clear the
state of FrameTreeNodes, and I wanted to also clear the replicated_state_ there
to avoid stale origins.  However, doing that clears out the origin before
CommitPending gets a chance to grab it for the SwapOut message.  I'm not sure
how to best handle this; after talking to Nasko, some possibilities are:
- plumb navigation params (which contain the origin) through DidNavigateFrame
and CommitPending (unlikely, since CommitPending is also called from
RFHM::UpdateStateForNavigate where we don't have the params)
- save the origin in a temporary field (replication_state_for_swap_out_) on the
FTN and don't clear it in ResetForNewProcess.
- don't clear the origin at all in ResetForNewProcess (current approach; should
be currently harmless but doesn't feel ideal).
- CommitPending can save the FTN origin before calling AttachToFrameTree and
pass it along to SwapOutOldFrame.

Thoughts?

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

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:827: if
(IsRenderFrameLive())
On 2014/11/13 18:00:57, Charlie Reis wrote:
> Style: This needs braces if the body doesn't fit on one line.
> 
> (Note that the parameters don't have to each be on their own line for call
> sites, but there's not a big difference between 2+1 vs each on their own
line.)

Done.

https://codereview.chromium.org/692973005/diff/120001/content/browser/rendere...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:293: if (!is_active_)
On 2014/11/13 18:00:57, Charlie Reis wrote:
> Style: Needs braces if the body doesn't fit on one line.

Done.

https://codereview.chromium.org/692973005/diff/120001/content/browser/rendere...
content/browser/renderer_host/render_view_host_impl.cc:294:
params.remote_frame_state = ((RenderFrameHostImpl*)GetMainFrame())->
On 2014/11/12 22:16:43, dcheng wrote:
> Use static_cast here.

Done.

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_me...
content/common/frame_messages.h:389: content::FrameReplicationState)
On 2014/11/13 18:00:57, Charlie Reis wrote:
> nit: We put generally put names for the parameters in comments here.

Fixed here and in SwapOut below.

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_re...
content/common/frame_replication_state.h:13: // This structure holds information
that needs to be replicated from a local
On 2014/11/13 18:00:58, Charlie Reis wrote:
> Let's use the content names for LocalFrame and RemoteFrame here: RenderFrame /
> RenderFrameProxies.

Done.

https://codereview.chromium.org/692973005/diff/120001/content/common/frame_re...
content/common/frame_replication_state.h:20: url::Origin origin;
On 2014/11/13 18:00:58, Charlie Reis wrote:
> On 2014/11/12 00:05:47, alexmos wrote:
> > Is using url::Origin the right thing here?  Although it emphasizes that this
> is
> > an origin rather than any URL, it's also inflexible (can't extract parts of
> > origin, can't properly support unique origins which is why I have the flag
for
> > that below, doesn't have canAccess which we will need in the future).  A
GURL
> > could be slightly better, though it still won't handle unique origins. 
> > blink::WebSecurityOrigin may be even better, but I don't know if it's
allowed
> to
> > use that here.  Thoughts?
> 
> Hmm,  I agree that url::Origin seems a bit limited for our needs, but we
> generally can't use arbitrary Blink classes outside the renderer process. 
I've
> seen WebSecurityOrigin discussed in the past (e.g., on
> https://codereview.chromium.org/452013002/), so I'm pretty sure we won't be
able
> to use that.
> 
> Let's discuss options a bit more in person.

After our discussion, and reading the discussion on the CL for adding
url::Origin (https://codereview.chromium.org/170843007/), I'm keeping it as an
url::Origin for now.  Short-term, I think the browser process can just see this
as an opaque serialized origin, and let Blink handle serialization and
deserialization.  This works fine for unique origins as well (which use the
string "null" per RFC 6454), so we don't need the is_unique_origin flag.  We
will need to start beefing up url::Origin (or switch to a richer class) once the
browser process is ready to start interpreting/verifying this origin, which as I
understand is not very soon.  Still worth checking on plans for url::Origin with
tsepez@ and abarth@, but I don't think it's blocking this.

https://codereview.chromium.org/692973005/diff/120001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/common/view_mes...
content/common/view_messages.h:467: // With OOPIF, if this view is swapped out,
its main frame is going to become
On 2014/11/13 18:00:58, Charlie Reis wrote:
> nit: With OOPIF -> In --site-per-process mode
> (I'm still wondering if we can use this state for a swapped out RenderFrame in
> normal mode, but we can consider that in a future CL.)
> 
> nit: view is |swapped_out|
> 
> nit: is going to become a render frame -> will become a RenderFrameProxy.

Done.

https://codereview.chromium.org/692973005/diff/120001/content/common/view_mes...
content/common/view_messages.h:470:
IPC_STRUCT_MEMBER(content::FrameReplicationState, remote_frame_state)
On 2014/11/13 18:00:58, Charlie Reis wrote:
> Let's move this just under |swapped_out|, since those two are related.

Done.  I also moved it to be in the same order in parameter lists of all
affected functions.

https://codereview.chromium.org/692973005/diff/120001/content/public/test/ren...
File content/public/test/render_view_test.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/public/test/ren...
content/public/test/render_view_test.cc:208: FrameReplicationState()); //
remote_frame_state
On 2014/11/13 18:00:58, Charlie Reis wrote:
> nit: The comment probably isn't necessary.  It's more useful when it's hard to
> infer what the bool or int is for.

Done.

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:1198: // initialize remote frame with the
security origin that was passed to us
On 2014/11/13 18:00:58, Charlie Reis wrote:
> nit: Initialize the RemoteFrame with the replication state

Done.  Actually, rephrased a bit and used "WebRemoteFrame" given the new call.

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_frame_impl.cc:1200: // TODO(alexmos): we cannot yet do
this for swapped-out main frames,
On 2014/11/13 18:00:58, Charlie Reis wrote:
> nit: We

Done.

> 
> This TODO is what Nasko is currently working on, right?

Yes, exactly.

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_frame_proxy.cc:79: // initialize remote frame with the
security origin that was passed to us
On 2014/11/13 18:00:58, Charlie Reis wrote:
> nit: Capitalize
> nit: RemoteFrame

Done.

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_frame_proxy.cc:81: web_frame->setReplicatedOrigin(
On 2014/11/13 18:00:58, Charlie Reis wrote:
> We should probably have a more general method for doing this.  Right now the
> code is copied between here and SwapOut, and we'll want to add more thing to
> replicated state eventually.

Done.  Created a static method RenderFrameImpl::SetReplicateState to centralize
this logic.

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
File content/renderer/render_frame_proxy.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_frame_proxy.h:81: const FrameReplicationState&);
On 2014/11/13 18:00:58, Charlie Reis wrote:
> Style: Needs a parameter name that matches the .cc file.

Done.

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
File content/renderer/render_thread_impl.h (right):

https://codereview.chromium.org/692973005/diff/120001/content/renderer/render...
content/renderer/render_thread_impl.h:430: const FrameReplicationState&);
On 2014/11/13 18:00:58, Charlie Reis wrote:
> nit: Name parameter.

Done.

https://codereview.chromium.org/692973005/diff/120001/url/origin.cc
File url/origin.cc (right):

https://codereview.chromium.org/692973005/diff/120001/url/origin.cc#newcode17
url/origin.cc:17: // DCHECK_NE(origin[origin.size() - 1], '/');
On 2014/11/13 18:00:59, Charlie Reis wrote:
> On 2014/11/12 00:05:47, alexmos wrote:
> > I commented out this check since GURL::GetOrigin() always returns origins
that
> > end with '/'.  I could just remove the trailing '/' instead of this, though
it
> > feels the two classes should be  consistent for this.  This won't be
necessary
> > if we switch FrameReplicationState to use GURL or another class to track
> origins
> > instead of url::Origin.
> 
> Hmm, yes, let's continue the discussion about what class to use in
> FrameReplicationState.

Undone, this is no longer necessary given that Blink is now sending us the
serialized origin.  And actually, not having the '/' at the end was correct per
RFC 6454.

[alexmos, 2014-11-18 18:54:25]

Answering two more questions:

> I noticed that we're not updating the replication state after the RemoteFrame
is
> constructed.  Wouldn't it need to change each time the frame navigates? 
(e.g.,
> A's RemoteFrame needs to know that the origin changed from B to C.)

Yes.  As we discussed, my plan is to do those in a separate CL.

> This is slightly tricky to get right in the API-- many bits of
> FrameReplicationState will probably need updating at the time of
> DidCommitProvisionalLoad, but others may need to happen at other times (e.g.,
> frame sizing state, focus, etc).  We'll want to think about whether the whole
> struct gets passed each time, or if we'll a different way to broadcast custom
> updates to each proxy.
> 
> Either way, we'll probably want a helper function in RenderFrameHostManager to
> broadcast an IPC (or something) to all the RenderFrameProxies for the frame.
 
Agreed.  I guess this depends on how many different custom update messages we
will end up having.  Would it make sense then to (eventually?) have separate
structs for keeping the replication state on the FTN and for sending replication
state for RemoteFrame creation, etc.?  I don't think we need to pass the full
struct for simpler messages like origin updates; it seems those can just take an
origin parameter directly.

[ncarter (slow), 2014-11-18 20:59:00]

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

[ncarter (slow), 2014-11-18 20:59:02]

https://codereview.chromium.org/692973005/diff/140001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/692973005/diff/140001/content/common/frame_re...
content/common/frame_replication_state.h:6: #define
CONTENT_FRAME_REPLICATION_STATE_H_
This should be CONTENT_COMMON_FRAME_REPLICATION_STATE_H_

[Charlie Reis, 2014-11-19 00:46:18]

Great-- this looks almost ready to go.  A few minor things below.

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:203:
IN_PROC_BROWSER_TEST_F(FrameTreeBrowserTest, OriginSetOnNavigation) {
On 2014/11/18 18:25:31, alexmos wrote:
> On 2014/11/13 18:00:57, Charlie Reis wrote:
> > Perhaps we can also add a --site-per-process test on the renderer side
(e.g.,
> > render_view_browsertest.cc), which could verify that the RenderFrameProxy
ends
> > up with the right state?
> 
> Done -- please see RenderViewImplTest.OriginReplicationForSwapOut.  I could
also
> add something similar for the other two remote frame creation paths
> (ViewMsg_New, NewFrameProxy), if I can figure out how to get those to work as
> render view tests.

Thanks!

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/692973005/diff/120001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:528:
render_frame_host->frame_tree_node()->SetOriginFromURL(params.url);
On 2014/11/18 18:25:32, alexmos wrote:
> On 2014/11/13 18:00:57, Charlie Reis wrote:
> > Would it be easier to just do this action inside the set_current_url call
> below?
> >  (Line 569.)
> 
> That would be too late because we need the origin on the next line inside
> DidNavigateFrame (it calls CommitPending, which sends the SwapOut message,
which
> needs the origin).
> 
> There is actually another issue here that I wanted to discuss.  For main
frames
> only, DidNavigateFrame eventually calls CommitPending ->
> RenderViewHostImpl::AttachToFrameTree -> FrameTree::ResetForMainFrameSwap ->
> FrameTreeNode::ResetForNewProcess.  ResetForNewProcess is supposed to clear
the
> state of FrameTreeNodes, and I wanted to also clear the replicated_state_
there
> to avoid stale origins.  However, doing that clears out the origin before
> CommitPending gets a chance to grab it for the SwapOut message.  I'm not sure
> how to best handle this; after talking to Nasko, some possibilities are:
> - plumb navigation params (which contain the origin) through DidNavigateFrame
> and CommitPending (unlikely, since CommitPending is also called from
> RFHM::UpdateStateForNavigate where we don't have the params)
> - save the origin in a temporary field (replication_state_for_swap_out_) on
the
> FTN and don't clear it in ResetForNewProcess.
> - don't clear the origin at all in ResetForNewProcess (current approach;
should
> be currently harmless but doesn't feel ideal).
> - CommitPending can save the FTN origin before calling AttachToFrameTree and
> pass it along to SwapOutOldFrame.
> 
> Thoughts?

Good question.  As we chatted about in person, I think it makes sense to keep
the frame replication state after a commit, and even after a crash.

(In fact, most of ResetForNewProcess doesn't even make sense during
CommitPending.  The current URL doesn't need to be cleared because it's about to
be set to a new value.  We *definitely* shouldn't call
set_render_frame_created(false) during CommitPending, but luckily that's a no-op
on the main frame.  The only thing we need is to clear the children.  The other
things only make sense during RenderProcessGone.)

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

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:207: NavigateToURL(shell(),
main_url);
EXPECT_TRUE?  (Here and below.)

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:319:
host_resolver()->AddRule("*", "127.0.0.1");
nit: 2 space indent
(I think git cl format would probably catch that.)

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:322: NavigateToURL(shell(),
main_url);
EXPECT_TRUE?

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:341: // Load cross-site
page into the first frame
nit: End with period.

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:351: // the root's origin
shouldn't have changed
nit: Capitalize, end with period.

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

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:54: void SetOriginFromURL(const
GURL& url);
Don't forget to remove this.

https://codereview.chromium.org/692973005/diff/140001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/692973005/diff/140001/content/common/view_mes...
content/common/view_messages.h:447: // will become a RenderFrameProxy.  This
structure is used to replicate
nit: This structure -> |remote_frame_state|

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:562: void
RenderFrameImpl::SetReplicatedState(blink::WebRemoteFrame* frame,
On second thought, maybe this belongs as a non-static method on
RenderFrameProxy, since it operates on the proxy's WebRemoteFrame?  It's easy to
call that from both RenderFrameImpl::OnSwapOut and RenderViewImpl::Initialize,
since we have the proxy object in both places.

Sorry to make you move it.

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:564: CHECK(frame);
DCHECK is fine here.  It clarifies the intent, and we'll crash either way in
release builds.

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:3486: // Set the origin of the frame. 
This will be replicated to corresponding
nit: to the corresponding

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:3489: params.origin = url::Origin(
nit: 2 space indent

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:3490:
frame->document().securityOrigin().toString().utf8());
Just checking: is toString().utf8() the right way to convert a SecurityOrigin to
a url::Origin?  (I don't see an immediate issue with it.)

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_view_browsertest.cc:688:
frame()->GetWebFrame()->firstChild()->securityOrigin();
Great.  Thanks for adding this test.

[dcheng, 2014-11-19 00:50:11]

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_view_impl.cc:780: // In --site-per-process, initialize
the WebRemoteFrame with information
Any chance we can combine this if block with the above one?

[alexmos, 2014-11-19 02:49:27]

Please take a look at PS9.

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

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:207: NavigateToURL(shell(),
main_url);
On 2014/11/19 00:46:18, Charlie Reis wrote:
> EXPECT_TRUE?  (Here and below.)

Indeed :)  I was holding off on rebasing, which I need to be able to do that, so
will do a separate patch set for it (or I can do it along with other tests in a
separate CL).

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:319:
host_resolver()->AddRule("*", "127.0.0.1");
On 2014/11/19 00:46:18, Charlie Reis wrote:
> nit: 2 space indent
> (I think git cl format would probably catch that.)

Done.

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:322: NavigateToURL(shell(),
main_url);
On 2014/11/19 00:46:18, Charlie Reis wrote:
> EXPECT_TRUE?

See above.

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:341: // Load cross-site
page into the first frame
On 2014/11/19 00:46:18, Charlie Reis wrote:
> nit: End with period.

Done.

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:351: // the root's origin
shouldn't have changed
On 2014/11/19 00:46:18, Charlie Reis wrote:
> nit: Capitalize, end with period.

Done.

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

https://codereview.chromium.org/692973005/diff/140001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:54: void SetOriginFromURL(const
GURL& url);
On 2014/11/19 00:46:18, Charlie Reis wrote:
> Don't forget to remove this.

Done.

https://codereview.chromium.org/692973005/diff/140001/content/common/frame_re...
File content/common/frame_replication_state.h (right):

https://codereview.chromium.org/692973005/diff/140001/content/common/frame_re...
content/common/frame_replication_state.h:6: #define
CONTENT_FRAME_REPLICATION_STATE_H_
On 2014/11/18 20:59:02, ncarter wrote:
> This should be CONTENT_COMMON_FRAME_REPLICATION_STATE_H_

Done.

https://codereview.chromium.org/692973005/diff/140001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/692973005/diff/140001/content/common/view_mes...
content/common/view_messages.h:447: // will become a RenderFrameProxy.  This
structure is used to replicate
On 2014/11/19 00:46:18, Charlie Reis wrote:
> nit: This structure -> |remote_frame_state|

Done.

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:562: void
RenderFrameImpl::SetReplicatedState(blink::WebRemoteFrame* frame,
On 2014/11/19 00:46:18, Charlie Reis wrote:
> On second thought, maybe this belongs as a non-static method on
> RenderFrameProxy, since it operates on the proxy's WebRemoteFrame?  It's easy
to
> call that from both RenderFrameImpl::OnSwapOut and RenderViewImpl::Initialize,
> since we have the proxy object in both places.
> 
> Sorry to make you move it.

Done - indeed, that seems much better and the code is cleaner.

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:564: CHECK(frame);
On 2014/11/19 00:46:18, Charlie Reis wrote:
> DCHECK is fine here.  It clarifies the intent, and we'll crash either way in
> release builds.

Done.

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:3486: // Set the origin of the frame. 
This will be replicated to corresponding
On 2014/11/19 00:46:18, Charlie Reis wrote:
> nit: to the corresponding

Done.

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:3489: params.origin = url::Origin(
On 2014/11/19 00:46:18, Charlie Reis wrote:
> nit: 2 space indent

Done.

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:3490:
frame->document().securityOrigin().toString().utf8());
On 2014/11/19 00:46:18, Charlie Reis wrote:
> Just checking: is toString().utf8() the right way to convert a SecurityOrigin
to
> a url::Origin?  (I don't see an immediate issue with it.)

I based this off of how WebSerializedOrigin is initialized from a
SecurityOrigin, and how it exposes a url::Origin:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

I considered converting securityOrigin() to a WebSerializedOrigin first and
getting url::Origin off of that, but WebSerializedOrigin currently doesn't have
a constructor that takes in a WebSecurityOrigin outside of Blink, and also it
has no way to deserialize into a WebSecurityOrigin later (and adding a function
for the latter could lead to improper usage since we know deserialization
currently loses information for things like document.domain).  I also saw
origin.toString().utf8() getting used in a few other places to convert
WebSecurityOrigins to strings (e.g., RenderFrameImpl::didRunInsecureContent), so
went with this approach for now.

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_view_impl.cc:780: // In --site-per-process, initialize
the WebRemoteFrame with information
On 2014/11/19 00:50:11, dcheng wrote:
> Any chance we can combine this if block with the above one?

Done.

[Charlie Reis, 2014-11-19 19:31:47]

Great.  LGTM once you rebase and add the EXPECT_TRUEs in the test.

On 2014/11/18 18:54:25, alexmos wrote:
> > This is slightly tricky to get right in the API-- many bits of
> > FrameReplicationState will probably need updating at the time of
> > DidCommitProvisionalLoad, but others may need to happen at other times
(e.g.,
> > frame sizing state, focus, etc).  We'll want to think about whether the
whole
> > struct gets passed each time, or if we'll a different way to broadcast
custom
> > updates to each proxy.
> > 
> > Either way, we'll probably want a helper function in RenderFrameHostManager
to
> > broadcast an IPC (or something) to all the RenderFrameProxies for the frame.
>  
> Agreed.  I guess this depends on how many different custom update messages we
> will end up having.  Would it make sense then to (eventually?) have separate
> structs for keeping the replication state on the FTN and for sending
replication
> state for RemoteFrame creation, etc.?  I don't think we need to pass the full
> struct for simpler messages like origin updates; it seems those can just take
an
> origin parameter directly.

Perhaps, or maybe we can keep them all in the same struct and just send
piece-wise updates as needed.  We can cross that bridge when we come to it.

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/692973005/diff/140001/content/renderer/render...
content/renderer/render_frame_impl.cc:3490:
frame->document().securityOrigin().toString().utf8());
On 2014/11/19 02:49:27, alexmos wrote:
> On 2014/11/19 00:46:18, Charlie Reis wrote:
> > Just checking: is toString().utf8() the right way to convert a
SecurityOrigin
> to
> > a url::Origin?  (I don't see an immediate issue with it.)
> 
> I based this off of how WebSerializedOrigin is initialized from a
> SecurityOrigin, and how it exposes a url::Origin:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> I considered converting securityOrigin() to a WebSerializedOrigin first and
> getting url::Origin off of that, but WebSerializedOrigin currently doesn't
have
> a constructor that takes in a WebSecurityOrigin outside of Blink, and also it
> has no way to deserialize into a WebSecurityOrigin later (and adding a
function
> for the latter could lead to improper usage since we know deserialization
> currently loses information for things like document.domain).  I also saw
> origin.toString().utf8() getting used in a few other places to convert
> WebSecurityOrigins to strings (e.g., RenderFrameImpl::didRunInsecureContent),
so
> went with this approach for now.

Acknowledged.

https://codereview.chromium.org/692973005/diff/160001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/692973005/diff/160001/content/renderer/render...
content/renderer/render_frame_impl.cc:1206: if (is_site_per_process && proxy &&
!is_main_frame) {
nit: No braces needed anymore.

[alexmos, 2014-11-20 00:29:28]

Thanks for all the comments!

> Great.  LGTM once you rebase and add the EXPECT_TRUEs in the test.

Rebased in PS10 and added the EXPECT_TRUEs in PS11, which also fixes a couple of
nits and removes an #include that's no longer necessary.

https://codereview.chromium.org/692973005/diff/160001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/692973005/diff/160001/content/renderer/render...
content/renderer/render_frame_impl.cc:1206: if (is_site_per_process && proxy &&
!is_main_frame) {
On 2014/11/19 19:31:46, Charlie Reis wrote:
> nit: No braces needed anymore.

Done.

[Charlie Reis, 2014-11-20 17:20:28]

Still LGTM.  You'll need an IPC owner as well, like nasko@.

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
content/renderer/render_frame_proxy.cc:19: 
nit: No blank line here.

(Might as well remove the blank line in the .h file as well, since that's not
consistent with render_frame_impl or render_view_impl.)

[nasko, 2014-11-20 17:22:55]

Few more comments, mostly minor.

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:204:
host_resolver()->AddRule("*", "127.0.0.1");
Let's use the EmbeddedTestServer in all new tests. The tests in
site_per_process_browsertest.cc use it if you want to look up an example.

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:227:
EXPECT_TRUE(NavigateToURL(shell(), data_url));
Why not navigate another iframe on the page instead of the top-level? It will
give us a bit more variety, as lots of tests already navigate the top-level
frame.

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:319:
host_resolver()->AddRule("*", "127.0.0.1");
Same as the other tests, let's use EmbeddedTestServer.

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:338: GURL::Replacements
replace_host;
Ah, you also skip all that boilerplate code and just use GetURL(host, path) ;).

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:526: // origin because it creates a
RenderFrameProxy that needs this initialize
s/initialize/to initialize/

https://codereview.chromium.org/692973005/diff/200001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/692973005/diff/200001/content/common/view_mes...
content/common/view_messages.h:469:
IPC_STRUCT_MEMBER(content::FrameReplicationState, remote_frame_state)
We don't use the "remote" prefix in the chromium side. "replicated" is probably
a better prefix to use.

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.cc:1218: // and don't call swap() above.
Because swap() is what creates a RemoteFrame
nit: The comment is not quite correct, as the RemoteFrame is created as part of
the proxy creation. We just don't insert it in the Blink frame tree. So it
exists, but noone knows about it :).

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
content/renderer/render_view_browsertest.cc:551:
content::FrameReplicationState());
What would be the WebRemoteFrame origin if we pass a blank replication state?
The call to SetReplicationState is gated on valid proxy routing id, which is the
case here.
Shouldn't we verify the expected value once OnSwapOut returns?

[ncarter (slow), 2014-11-20 20:47:22]

nick@chromium.org changed reviewers:
- nick@chromium.org

[ncarter (slow), 2014-11-20 20:50:45]

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:204:
host_resolver()->AddRule("*", "127.0.0.1");
On 2014/11/20 17:22:54, nasko wrote:
> Let's use the EmbeddedTestServer in all new tests. The tests in
> site_per_process_browsertest.cc use it if you want to look up an example.

The three second version:
 - test_server() becomes embedded_test_server()
 - Start() becomes InitializeAndWaitUntilReady()
 - URLs that were "files/whatever" become "/whatever" (there's no /files
servelet -- the files are just reachable at URL paths relative to the test root
dir)

[alexmos, 2014-11-20 21:09:10]

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:204:
host_resolver()->AddRule("*", "127.0.0.1");
On 2014/11/20 17:22:54, nasko wrote:
> Let's use the EmbeddedTestServer in all new tests. The tests in
> site_per_process_browsertest.cc use it if you want to look up an example.

As we discussed in person, I can do that in a separate CL for all the tests in
this file.  That way we won't randomize this CL and can centralize the
EmbeddedTestServer initialization.

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:227:
EXPECT_TRUE(NavigateToURL(shell(), data_url));
On 2014/11/20 17:22:54, nasko wrote:
> Why not navigate another iframe on the page instead of the top-level? It will
> give us a bit more variety, as lots of tests already navigate the top-level
> frame.

Interesting -- doing that made the test crash when running without
--site-per-process, while passing just fine with --site-per-process.  So I think
your suspicion about NavigateFrameToURL misbehaving in regular mode is correct. 
So I'm leaving this test as is, and instead I've added the
navigate-frame-to-data-url test to the OriginSetOnCrossProcessNavigations below,
which only runs with --site-per-process.  For the record, the crash was this
CHECK(render_view_host):
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:319:
host_resolver()->AddRule("*", "127.0.0.1");
On 2014/11/20 17:22:54, nasko wrote:
> Same as the other tests, let's use EmbeddedTestServer.

See above -- will do the whole file in a separate CL.

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:338: GURL::Replacements
replace_host;
On 2014/11/20 17:22:54, nasko wrote:
> Ah, you also skip all that boilerplate code and just use GetURL(host, path)
;).

test_server() doesn't support this, right?  If so, how about I do that cleanup
in the CL above, as this file has a few more places using this?

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:526: // origin because it creates a
RenderFrameProxy that needs this initialize
On 2014/11/20 17:22:54, nasko wrote:
> s/initialize/to initialize/

Done.

https://codereview.chromium.org/692973005/diff/200001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/692973005/diff/200001/content/common/view_mes...
content/common/view_messages.h:469:
IPC_STRUCT_MEMBER(content::FrameReplicationState, remote_frame_state)
On 2014/11/20 17:22:54, nasko wrote:
> We don't use the "remote" prefix in the chromium side. "replicated" is
probably
> a better prefix to use.

Done.

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.cc:1218: // and don't call swap() above.
Because swap() is what creates a RemoteFrame
On 2014/11/20 17:22:54, nasko wrote:
> nit: The comment is not quite correct, as the RemoteFrame is created as part
of
> the proxy creation. We just don't insert it in the Blink frame tree. So it
> exists, but noone knows about it :).

I thought the proxy creation only creates the WebRemoteFrame, which stays
without a core RemoteFrame until we call initializeCoreFrame() in swap().  So,
for main frames the WebRemoteFrame does exist but has no RemoteFrame here, so I
can't set the origin on it (I did crash when trying to do this originally :) ).

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
File content/renderer/render_frame_proxy.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
content/renderer/render_frame_proxy.cc:19: 
On 2014/11/20 17:20:28, Charlie Reis wrote:
> nit: No blank line here.
> 
> (Might as well remove the blank line in the .h file as well, since that's not
> consistent with render_frame_impl or render_view_impl.)

Done.

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
content/renderer/render_view_browsertest.cc:551:
content::FrameReplicationState());
On 2014/11/20 17:22:54, nasko wrote:
> What would be the WebRemoteFrame origin if we pass a blank replication state?
> The call to SetReplicationState is gated on valid proxy routing id, which is
the
> case here.
> Shouldn't we verify the expected value once OnSwapOut returns?

It will be a unique origin.  Do you mean checking the WebRemoteFrame's origin
after the OnSwapOut call?  I think that won't work here, because this is
swapping out a main frame, and OnSwapOut only sets the origin for subframes
(until your work on swapping in all cases is complete) -- see also my other
comment for OnSwapOut.  But I think checking unique origin deserialization is
worth doing nonetheless, so I added that case to OriginReplicationForSwapOut --
please take a look.

[nasko, 2014-11-20 21:21:21]

LGTM with one nit.

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:204:
host_resolver()->AddRule("*", "127.0.0.1");
On 2014/11/20 21:09:10, alexmos wrote:
> On 2014/11/20 17:22:54, nasko wrote:
> > Let's use the EmbeddedTestServer in all new tests. The tests in
> > site_per_process_browsertest.cc use it if you want to look up an example.
> 
> As we discussed in person, I can do that in a separate CL for all the tests in
> this file.  That way we won't randomize this CL and can centralize the
> EmbeddedTestServer initialization.

Acknowledged.

https://codereview.chromium.org/692973005/diff/200001/content/browser/frame_h...
content/browser/frame_host/frame_tree_browsertest.cc:338: GURL::Replacements
replace_host;
On 2014/11/20 21:09:10, alexmos wrote:
> On 2014/11/20 17:22:54, nasko wrote:
> > Ah, you also skip all that boilerplate code and just use GetURL(host, path)
> ;).
> 
> test_server() doesn't support this, right?  If so, how about I do that cleanup
> in the CL above, as this file has a few more places using this?

Yes, the API is on the EmbeddedTestServer. This was a comment that you lose
complexity if you use it.

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/692973005/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.cc:1218: // and don't call swap() above.
Because swap() is what creates a RemoteFrame
On 2014/11/20 21:09:10, alexmos wrote:
> On 2014/11/20 17:22:54, nasko wrote:
> > nit: The comment is not quite correct, as the RemoteFrame is created as part
> of
> > the proxy creation. We just don't insert it in the Blink frame tree. So it
> > exists, but noone knows about it :).
> 
> I thought the proxy creation only creates the WebRemoteFrame, which stays
> without a core RemoteFrame until we call initializeCoreFrame() in swap().  So,
> for main frames the WebRemoteFrame does exist but has no RemoteFrame here, so
I
> can't set the origin on it (I did crash when trying to do this originally :)
).

Yes, you are correct. I was thinking of frames at the Web* layer, not at the
core layer.
Thanks!

https://codereview.chromium.org/692973005/diff/220001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/220001/content/renderer/render...
content/renderer/render_view_browsertest.cc:696: replication_state.origin =
url::Origin("null");
Let's use the default constructor, which is supposed to create an unique origin.
This helps us ensure that if we don't pass anything explicit, it will be unique
for replication.

[alexmos, 2014-11-20 21:55:14]

Thanks for reviewing!

https://codereview.chromium.org/692973005/diff/220001/content/renderer/render...
File content/renderer/render_view_browsertest.cc (right):

https://codereview.chromium.org/692973005/diff/220001/content/renderer/render...
content/renderer/render_view_browsertest.cc:696: replication_state.origin =
url::Origin("null");
On 2014/11/20 21:21:21, nasko wrote:
> Let's use the default constructor, which is supposed to create an unique
origin.
> This helps us ensure that if we don't pass anything explicit, it will be
unique
> for replication.

Done.

[alexmos, 2014-12-02 00:11:32]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2014-12-02 00:12:20]

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

[commit-bot: I haz the power, 2014-12-02 02:02:21]

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

[commit-bot: I haz the power, 2014-12-02 02:02:23]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[alexmos, 2014-12-05 23:48:15]

Charlie, could you please take a look at the tweak in PS16?  This hopefully gets
around the failures we saw for content:// URLs on Android.  Note that I had to
use the origin rather than params.url to do the IsStandard() check to not break
cases where the origin is inherited from the parent.

[Charlie Reis, 2014-12-06 00:01:21]

On 2014/12/05 23:48:15, alexmos wrote:
> Charlie, could you please take a look at the tweak in PS16?  This hopefully
gets
> around the failures we saw for content:// URLs on Android.  Note that I had to
> use the origin rather than params.url to do the IsStandard() check to not
break
> cases where the origin is inherited from the parent.

Yes, the diff LGTM, once I verified that the default constructor for Origin is
for a unique origin.

I wish we could include a test for this case, but I'm not sure if that would
need to be Android specific code.

[alexmos, 2014-12-06 00:32:33]

On 2014/12/06 00:01:21, Charlie Reis wrote:
> On 2014/12/05 23:48:15, alexmos wrote:
> > Charlie, could you please take a look at the tweak in PS16?  This hopefully
> gets
> > around the failures we saw for content:// URLs on Android.  Note that I had
to
> > use the origin rather than params.url to do the IsStandard() check to not
> break
> > cases where the origin is inherited from the parent.
> 
> Yes, the diff LGTM, once I verified that the default constructor for Origin is
> for a unique origin.
> 
> I wish we could include a test for this case, but I'm not sure if that would
> need to be Android specific code.

Thanks!  Yes, I'm not sure how to test this other than creating an
Android-specific test.  I can look into that; I was planning to see how
embedding works with content:// URLs anyway.

[alexmos, 2014-12-06 00:32:47]

The CQ bit was checked by alexmos@chromium.org

[commit-bot: I haz the power, 2014-12-06 00:33:26]

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

[commit-bot: I haz the power, 2014-12-06 01:38:18]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2014-12-06 01:39:05]

Patchset 16 (id:??) landed as
https://crrev.com/bc7eafaba8bde63d70a270ee71a901545d9769f9
Cr-Commit-Position: refs/heads/master@{#307138}