Navigation transitions: Place transition page in same process as destination page.

Navigation transitions: Place transition page in same process as destination page.

Per creis' request, place the transition page in the same process as the destination page. This is done to avoid the overhead of potentially spawning an extra new renderer for the transition.

The idea behind this was to put the transition page into the same render process as the incoming page. Approach so far was to instantiate a new WebContents in the embedder via a new function createNativeWebContentsWithSharedSiteInstance(source_content_view_core), passing the SiteInstance of the incoming page. The transition ContentViewCore can then be navigated to a blank page and the serialized markup is added to the page. Since the transition and incoming pages share the same SiteInstance, they will be placed in the same process.


Design doc: https://docs.google.com/a/chromium.org/document/d/17jg1RRL3RI969cLwbKBIcoGDsPwqaEdBxafGNYGwiY4/edit# 
Implementation details: https://docs.google.com/a/chromium.org/document/d/1kREPtFJaeLoDKwrfmrYTD7DHCdxX1RzFBga2gNY8lyE/edit#heading=h.bng2kpmyvxq5
BUG=370696
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287192

[shatch, 2014-07-08 21:10:36]

Do you mind taking a look at this since you 2 reviewed the changes to CSRH in
https://codereview.chromium.org/297973002/.

[jam, 2014-07-14 23:14:21]

I looked through this and didn't see any issues popping up, although Nasko is
more familiar and either him or creis should review this.

Reading the description still leaves me with two questions, that you should at
least clarify in the cl description:
-why do we want to share the same process?
-why do we need a new scheme?

https://codereview.chromium.org/378743002/diff/140001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/378743002/diff/140001/content/browser/transit...
content/browser/transition_request_manager.h:27: // thread.
this comment would have to be updated per below

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

https://codereview.chromium.org/378743002/diff/140001/content/public/browser/...
content/public/browser/navigation_entry.h:224: virtual void
SetVirtualURLForTransition(const GURL& url) = 0;
these aren't used in chrome, so can you just put them on NavigationEntryImpl
(see http://www.chromium.org/developers/content-module/content-api)

[shatch, 2014-07-21 17:47:11]

Thanks for taking a look, I've updated the cl description as requested. Also
added creis to the review (he's been OOO since May, but he might be back
soon/now?).

The process sharing was something creis requested we add. Came out of a meeting
early on when we were discussing how to implement the whole transition feature.
IIRC (since it's been a while) the biggest concern was the overhead of
potentially spawning an extra renderer process during the navigation.

As for adding a new scheme. I don't know if we *need* a new scheme, but it
seemed like a clean way to add support for this.

https://codereview.chromium.org/378743002/diff/140001/content/browser/transit...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/378743002/diff/140001/content/browser/transit...
content/browser/transition_request_manager.h:27: // thread.
On 2014/07/14 23:14:21, jam wrote:
> this comment would have to be updated per below

Done.

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

https://codereview.chromium.org/378743002/diff/140001/content/public/browser/...
content/public/browser/navigation_entry.h:224: virtual void
SetVirtualURLForTransition(const GURL& url) = 0;
On 2014/07/14 23:14:21, jam wrote:
> these aren't used in chrome, so can you just put them on NavigationEntryImpl
> (see http://www.chromium.org/developers/content-module/content-api)

Done. Also did some renaming.

[Charlie Reis, 2014-07-22 21:14:17]

On 2014/07/21 17:47:11, shatch wrote:
> Thanks for taking a look, I've updated the cl description as requested. Also
> added creis to the review (he's been OOO since May, but he might be back
> soon/now?).
> 

I've just returned this week.  I'll try to take a look later today or tomorrow
morning, after refreshing myself on the context.

[Charlie Reis, 2014-07-23 17:32:48]

Thanks for looking to into the process sharing!  Your implementation doc was
especially helpful.  As we discussed, I think it would be nice for transition
pages to end up in the same process as the destination page, without affecting
our normal process model decision for the destination page.

I have some high level questions and concerns before getting into code review
comments, though, partly because I've been out on leave and missed the earlier
implementation stages.

1) What classes are you using to render the transition page?  Does it go into
its own WebContents, RenderViewHost, and/or RenderFrameHost?  And does this
happen after we've created the SiteInstance for the destination page?  Also, is
the transition page itself something like a data URL?

2) I'm concerned about using a URL as a lookup key for a process ID, because
it's not unique.  It would be easy to have two different tabs open to foo.com in
different processes (because they're in different BrowsingInstances), each of
which starts a transition navigation.  The two separate instances of the URL
would collide in the map.  (For more context, see
http://www.chromium.org/developers/design-documents/process-models.)

3) Adding new schemes tends to have risky security implications.  For example,
an attacker might try to navigate to a URL with the new transition scheme to get
it to load in an existing process, etc.  It would be best to avoid this if we
can, which we might be able to do when solving (2).

4) I think I read that this behavior was gated behind a flag, right?  It seems
like we should check for that flag when modifying things like
GetExistingProcessHost, since those changes can have security implications in
normal browsing.

Your answer to (1) might help me brainstorm some alternatives if we need them. 
For example, we might be able to put the transition page in the very same
SiteInstance as the destination page, without changing things like
GetExistingProcessHost.

[shatch, 2014-07-23 19:07:06]

On 2014/07/23 17:32:48, Charlie Reis wrote:
> Thanks for looking to into the process sharing!  Your implementation doc was
> especially helpful.  As we discussed, I think it would be nice for transition
> pages to end up in the same process as the destination page, without affecting
> our normal process model decision for the destination page.
> 
> I have some high level questions and concerns before getting into code review
> comments, though, partly because I've been out on leave and missed the earlier
> implementation stages.
> 

Welcome back :)

> 1) What classes are you using to render the transition page?  Does it go into
> its own WebContents, RenderViewHost, and/or RenderFrameHost?  And does this
> happen after we've created the SiteInstance for the destination page?  Also,
is
> the transition page itself something like a data URL?
> 

Right now the transition page gets its own ContentViewCore. I believe we've
created the SiteInstance for the destination page at this point. We create the
transition page's ContentViewCore when the TransitionPageHelper receives
didStartProvisionalLoad, although we don't actually navigate it until we reach
CrossSiteResourceHandler::OnResponseStarted.

The transition page itself is just a blank page that we load the serialized
markup into. The transition elements from the outgoing page are serialized and
the markup is sent to the TransitionPageHelper, which 

https://codereview.chromium.org/316053007/ loads the serialized markup.


> 2) I'm concerned about using a URL as a lookup key for a process ID, because
> it's not unique.  It would be easy to have two different tabs open to
http://foo.com in
> different processes (because they're in different BrowsingInstances), each of
> which starts a transition navigation.  The two separate instances of the URL
> would collide in the map.  (For more context, see
> http://www.chromium.org/developers/design-documents/process-models.)
> 

We don't actually use the URL as the lookup key. A new unique "transition url"
is generated per navigation transition, and passed down to the
TransitionPageHelper in the embedder. That unique url is passed back when we
call loadUrl as the virtual_url_for_transition, and the RFHM checks for that in
GetSiteInstanceForEntry.

Hopefully that means, in the situation you described, the 2 separate tabs
wouldn't collide in the map.


> 3) Adding new schemes tends to have risky security implications.  For example,
> an attacker might try to navigate to a URL with the new transition scheme to
get
> it to load in an existing process, etc.  It would be best to avoid this if we
> can, which we might be able to do when solving (2).
> 

That makes sense. Is it still as risky with the explanation to (2) above? I
guess that, during a transition, you could try to navigate directly to a
transition url? ie. transition://unique_url if you knew it or guessed it.


> 4) I think I read that this behavior was gated behind a flag, right?  It seems
> like we should check for that flag when modifying things like
> GetExistingProcessHost, since those changes can have security implications in
> normal browsing.
> 

Yep we gate it behind the experimental features flag in blink. I can add a check
for that.


> Your answer to (1) might help me brainstorm some alternatives if we need them.

> For example, we might be able to put the transition page in the very same
> SiteInstance as the destination page, without changing things like
> GetExistingProcessHost.

[Charlie Reis, 2014-07-23 22:02:28]

On 2014/07/23 19:07:06, shatch wrote:
> On 2014/07/23 17:32:48, Charlie Reis wrote:
> > Thanks for looking to into the process sharing!  Your implementation doc was
> > especially helpful.  As we discussed, I think it would be nice for
transition
> > pages to end up in the same process as the destination page, without
affecting
> > our normal process model decision for the destination page.
> > 
> > I have some high level questions and concerns before getting into code
review
> > comments, though, partly because I've been out on leave and missed the
earlier
> > implementation stages.
> > 
> 
> Welcome back :)
> 
> > 1) What classes are you using to render the transition page?  Does it go
into
> > its own WebContents, RenderViewHost, and/or RenderFrameHost?  And does this
> > happen after we've created the SiteInstance for the destination page?  Also,
> is
> > the transition page itself something like a data URL?
> > 
> 
> Right now the transition page gets its own ContentViewCore.

Oh.  Isn't that an Android class?  Is this feature Android-only?

> I believe we've
> created the SiteInstance for the destination page at this point.

Is it possible to just use that SiteInstance for the new ContentViewCore?  It
shouldn't ever be a problem to load a blank page into the destination
SiteInstance, and that would save us from having to do any transition URLs /
maps / changes to GetExistingProcessHost.  We would automatically be in the same
process by being in the same SiteInstance.

> We create the
> transition page's ContentViewCore when the TransitionPageHelper receives
> didStartProvisionalLoad, although we don't actually navigate it until we reach
> CrossSiteResourceHandler::OnResponseStarted.
> 
> The transition page itself is just a blank page that we load the serialized
> markup into. The transition elements from the outgoing page are serialized and
> the markup is sent to the TransitionPageHelper, which 
> 
> https://codereview.chromium.org/316053007/ loads the serialized markup.
> 
> 
> > 2) I'm concerned about using a URL as a lookup key for a process ID, because
> > it's not unique.  It would be easy to have two different tabs open to
> http://foo.com in
> > different processes (because they're in different BrowsingInstances), each
of
> > which starts a transition navigation.  The two separate instances of the URL
> > would collide in the map.  (For more context, see
> > http://www.chromium.org/developers/design-documents/process-models.)
> > 
> 
> We don't actually use the URL as the lookup key. A new unique "transition url"
> is generated per navigation transition, and passed down to the
> TransitionPageHelper in the embedder. That unique url is passed back when we
> call loadUrl as the virtual_url_for_transition, and the RFHM checks for that
in
> GetSiteInstanceForEntry.
> 
> Hopefully that means, in the situation you described, the 2 separate tabs
> wouldn't collide in the map.
> 
> 
> > 3) Adding new schemes tends to have risky security implications.  For
example,
> > an attacker might try to navigate to a URL with the new transition scheme to
> get
> > it to load in an existing process, etc.  It would be best to avoid this if
we
> > can, which we might be able to do when solving (2).
> > 
> 
> That makes sense. Is it still as risky with the explanation to (2) above? I
> guess that, during a transition, you could try to navigate directly to a
> transition url? ie. transition://unique_url if you knew it or guessed it.
> 
> 
> > 4) I think I read that this behavior was gated behind a flag, right?  It
seems
> > like we should check for that flag when modifying things like
> > GetExistingProcessHost, since those changes can have security implications
in
> > normal browsing.
> > 
> 
> Yep we gate it behind the experimental features flag in blink. I can add a
check
> for that.
> 
> 
> > Your answer to (1) might help me brainstorm some alternatives if we need
them.
> 
> > For example, we might be able to put the transition page in the very same
> > SiteInstance as the destination page, without changing things like
> > GetExistingProcessHost.

[shatch, 2014-07-23 22:12:14]

On 2014/07/23 22:02:28, Charlie Reis wrote:
> On 2014/07/23 19:07:06, shatch wrote:
> > On 2014/07/23 17:32:48, Charlie Reis wrote:
> > > Thanks for looking to into the process sharing!  Your implementation doc
was
> > > especially helpful.  As we discussed, I think it would be nice for
> transition
> > > pages to end up in the same process as the destination page, without
> affecting
> > > our normal process model decision for the destination page.
> > > 
> > > I have some high level questions and concerns before getting into code
> review
> > > comments, though, partly because I've been out on leave and missed the
> earlier
> > > implementation stages.
> > > 
> > 
> > Welcome back :)
> > 
> > > 1) What classes are you using to render the transition page?  Does it go
> into
> > > its own WebContents, RenderViewHost, and/or RenderFrameHost?  And does
this
> > > happen after we've created the SiteInstance for the destination page? 
Also,
> > is
> > > the transition page itself something like a data URL?
> > > 
> > 
> > Right now the transition page gets its own ContentViewCore.
> 
> Oh.  Isn't that an Android class?  Is this feature Android-only?
> 

Yeah, at the moment it's android only.


> > I believe we've
> > created the SiteInstance for the destination page at this point.
> 
> Is it possible to just use that SiteInstance for the new ContentViewCore?  It
> shouldn't ever be a problem to load a blank page into the destination
> SiteInstance, and that would save us from having to do any transition URLs /
> maps / changes to GetExistingProcessHost.  We would automatically be in the
same
> process by being in the same SiteInstance.
> 

Ah I didn't know that, I'll give that a try and get back to you.


> > We create the
> > transition page's ContentViewCore when the TransitionPageHelper receives
> > didStartProvisionalLoad, although we don't actually navigate it until we
reach
> > CrossSiteResourceHandler::OnResponseStarted.
> > 
> > The transition page itself is just a blank page that we load the serialized
> > markup into. The transition elements from the outgoing page are serialized
and
> > the markup is sent to the TransitionPageHelper, which 
> > 
> > https://codereview.chromium.org/316053007/ loads the serialized markup.
> > 
> > 
> > > 2) I'm concerned about using a URL as a lookup key for a process ID,
because
> > > it's not unique.  It would be easy to have two different tabs open to
> > http://foo.com in
> > > different processes (because they're in different BrowsingInstances), each
> of
> > > which starts a transition navigation.  The two separate instances of the
URL
> > > would collide in the map.  (For more context, see
> > > http://www.chromium.org/developers/design-documents/process-models.)
> > > 
> > 
> > We don't actually use the URL as the lookup key. A new unique "transition
url"
> > is generated per navigation transition, and passed down to the
> > TransitionPageHelper in the embedder. That unique url is passed back when we
> > call loadUrl as the virtual_url_for_transition, and the RFHM checks for that
> in
> > GetSiteInstanceForEntry.
> > 
> > Hopefully that means, in the situation you described, the 2 separate tabs
> > wouldn't collide in the map.
> > 
> > 
> > > 3) Adding new schemes tends to have risky security implications.  For
> example,
> > > an attacker might try to navigate to a URL with the new transition scheme
to
> > get
> > > it to load in an existing process, etc.  It would be best to avoid this if
> we
> > > can, which we might be able to do when solving (2).
> > > 
> > 
> > That makes sense. Is it still as risky with the explanation to (2) above? I
> > guess that, during a transition, you could try to navigate directly to a
> > transition url? ie. transition://unique_url if you knew it or guessed it.
> > 
> > 
> > > 4) I think I read that this behavior was gated behind a flag, right?  It
> seems
> > > like we should check for that flag when modifying things like
> > > GetExistingProcessHost, since those changes can have security implications
> in
> > > normal browsing.
> > > 
> > 
> > Yep we gate it behind the experimental features flag in blink. I can add a
> check
> > for that.
> > 
> > 
> > > Your answer to (1) might help me brainstorm some alternatives if we need
> them.
> > 
> > > For example, we might be able to put the transition page in the very same
> > > SiteInstance as the destination page, without changing things like
> > > GetExistingProcessHost.

[shatch, 2014-07-24 18:39:11]

On 2014/07/23 22:12:14, shatch wrote:
> On 2014/07/23 22:02:28, Charlie Reis wrote:
> > On 2014/07/23 19:07:06, shatch wrote:
> > > On 2014/07/23 17:32:48, Charlie Reis wrote:
> > > > Thanks for looking to into the process sharing!  Your implementation doc
> was
> > > > especially helpful.  As we discussed, I think it would be nice for
> > transition
> > > > pages to end up in the same process as the destination page, without
> > affecting
> > > > our normal process model decision for the destination page.
> > > > 
> > > > I have some high level questions and concerns before getting into code
> > review
> > > > comments, though, partly because I've been out on leave and missed the
> > earlier
> > > > implementation stages.
> > > > 
> > > 
> > > Welcome back :)
> > > 
> > > > 1) What classes are you using to render the transition page?  Does it go
> > into
> > > > its own WebContents, RenderViewHost, and/or RenderFrameHost?  And does
> this
> > > > happen after we've created the SiteInstance for the destination page? 
> Also,
> > > is
> > > > the transition page itself something like a data URL?
> > > > 
> > > 
> > > Right now the transition page gets its own ContentViewCore.
> > 
> > Oh.  Isn't that an Android class?  Is this feature Android-only?
> > 
> 
> Yeah, at the moment it's android only.
> 
> 
> > > I believe we've
> > > created the SiteInstance for the destination page at this point.
> > 
> > Is it possible to just use that SiteInstance for the new ContentViewCore? 
It
> > shouldn't ever be a problem to load a blank page into the destination
> > SiteInstance, and that would save us from having to do any transition URLs /
> > maps / changes to GetExistingProcessHost.  We would automatically be in the
> same
> > process by being in the same SiteInstance.
> > 
> 
> Ah I didn't know that, I'll give that a try and get back to you.
> 

So I gave sharing the SiteInstance between the transition and incoming pages.
What I do is instantiate a new WebContents in the embedder with
createNativeWebContentsWithSharedSiteInstance(source_content_view_core), then
navigate that ContentViewCore to about:blank before adding the serialized
markup.

Once I navigate the new ContentViewCore, it passes through
RenderFrameHostManager::GetSiteInstanceForEntry, which generates a new
SiteInstance and then a new RenderProcessHostImpl via
SiteInstanceImpl::GetProcess(). Based on your comment above, I'm assuming I
should be able to reuse the SiteInstance if I'm just navigating to a blank page?
So I added a check after the SiteInstanceImpl::IsSameSite() check in
RFHM::GetSiteInstanceForEntry() to see if the destination url was just
about:blank, and if so, use the current SiteInstance. Not sure if that's legit
or not, so I've uploaded it for you to take look. If this all seems ok, I'll go
ahead and add tests, revise the CL description, and the implementation docs.


> 
> > > We create the
> > > transition page's ContentViewCore when the TransitionPageHelper receives
> > > didStartProvisionalLoad, although we don't actually navigate it until we
> reach
> > > CrossSiteResourceHandler::OnResponseStarted.
> > > 
> > > The transition page itself is just a blank page that we load the
serialized
> > > markup into. The transition elements from the outgoing page are serialized
> and
> > > the markup is sent to the TransitionPageHelper, which 
> > > 
> > > https://codereview.chromium.org/316053007/ loads the serialized markup.
> > > 
> > > 
> > > > 2) I'm concerned about using a URL as a lookup key for a process ID,
> because
> > > > it's not unique.  It would be easy to have two different tabs open to
> > > http://foo.com in
> > > > different processes (because they're in different BrowsingInstances),
each
> > of
> > > > which starts a transition navigation.  The two separate instances of the
> URL
> > > > would collide in the map.  (For more context, see
> > > > http://www.chromium.org/developers/design-documents/process-models.)
> > > > 
> > > 
> > > We don't actually use the URL as the lookup key. A new unique "transition
> url"
> > > is generated per navigation transition, and passed down to the
> > > TransitionPageHelper in the embedder. That unique url is passed back when
we
> > > call loadUrl as the virtual_url_for_transition, and the RFHM checks for
that
> > in
> > > GetSiteInstanceForEntry.
> > > 
> > > Hopefully that means, in the situation you described, the 2 separate tabs
> > > wouldn't collide in the map.
> > > 
> > > 
> > > > 3) Adding new schemes tends to have risky security implications.  For
> > example,
> > > > an attacker might try to navigate to a URL with the new transition
scheme
> to
> > > get
> > > > it to load in an existing process, etc.  It would be best to avoid this
if
> > we
> > > > can, which we might be able to do when solving (2).
> > > > 
> > > 
> > > That makes sense. Is it still as risky with the explanation to (2) above?
I
> > > guess that, during a transition, you could try to navigate directly to a
> > > transition url? ie. transition://unique_url if you knew it or guessed it.
> > > 
> > > 
> > > > 4) I think I read that this behavior was gated behind a flag, right?  It
> > seems
> > > > like we should check for that flag when modifying things like
> > > > GetExistingProcessHost, since those changes can have security
implications
> > in
> > > > normal browsing.
> > > > 
> > > 
> > > Yep we gate it behind the experimental features flag in blink. I can add a
> > check
> > > for that.
> > > 
> > > 
> > > > Your answer to (1) might help me brainstorm some alternatives if we need
> > them.
> > > 
> > > > For example, we might be able to put the transition page in the very
same
> > > > SiteInstance as the destination page, without changing things like
> > > > GetExistingProcessHost.

[Charlie Reis, 2014-07-24 19:59:25]

On 2014/07/24 18:39:11, shatch wrote:
> On 2014/07/23 22:12:14, shatch wrote:
> > On 2014/07/23 22:02:28, Charlie Reis wrote:
> > > On 2014/07/23 19:07:06, shatch wrote:
> > > > On 2014/07/23 17:32:48, Charlie Reis wrote:
> > > > > Thanks for looking to into the process sharing!  Your implementation
doc
> > was
> > > > > especially helpful.  As we discussed, I think it would be nice for
> > > transition
> > > > > pages to end up in the same process as the destination page, without
> > > affecting
> > > > > our normal process model decision for the destination page.
> > > > > 
> > > > > I have some high level questions and concerns before getting into code
> > > review
> > > > > comments, though, partly because I've been out on leave and missed the
> > > earlier
> > > > > implementation stages.
> > > > > 
> > > > 
> > > > Welcome back :)
> > > > 
> > > > > 1) What classes are you using to render the transition page?  Does it
go
> > > into
> > > > > its own WebContents, RenderViewHost, and/or RenderFrameHost?  And does
> > this
> > > > > happen after we've created the SiteInstance for the destination page? 
> > Also,
> > > > is
> > > > > the transition page itself something like a data URL?
> > > > > 
> > > > 
> > > > Right now the transition page gets its own ContentViewCore.
> > > 
> > > Oh.  Isn't that an Android class?  Is this feature Android-only?
> > > 
> > 
> > Yeah, at the moment it's android only.
> > 
> > 
> > > > I believe we've
> > > > created the SiteInstance for the destination page at this point.
> > > 
> > > Is it possible to just use that SiteInstance for the new ContentViewCore? 
> It
> > > shouldn't ever be a problem to load a blank page into the destination
> > > SiteInstance, and that would save us from having to do any transition URLs
/
> > > maps / changes to GetExistingProcessHost.  We would automatically be in
the
> > same
> > > process by being in the same SiteInstance.
> > > 
> > 
> > Ah I didn't know that, I'll give that a try and get back to you.
> > 
> 
> So I gave sharing the SiteInstance between the transition and incoming pages.
> What I do is instantiate a new WebContents in the embedder with
> createNativeWebContentsWithSharedSiteInstance(source_content_view_core), then
> navigate that ContentViewCore to about:blank before adding the serialized
> markup.
> 
> Once I navigate the new ContentViewCore, it passes through
> RenderFrameHostManager::GetSiteInstanceForEntry, which generates a new
> SiteInstance and then a new RenderProcessHostImpl via
> SiteInstanceImpl::GetProcess(). Based on your comment above, I'm assuming I
> should be able to reuse the SiteInstance if I'm just navigating to a blank
page?
> So I added a check after the SiteInstanceImpl::IsSameSite() check in
> RFHM::GetSiteInstanceForEntry() to see if the destination url was just
> about:blank, and if so, use the current SiteInstance. Not sure if that's legit
> or not, so I've uploaded it for you to take look. If this all seems ok, I'll
go
> ahead and add tests, revise the CL description, and the implementation docs.

I'm thinking about it, and I think that might be fine.  Kind of the same
reasoning as reusing an about:blank SiteInstance for a real site when a blank
page is the only thing that has been in there, and it's consistent with a page
using window.open("about:blank") and having script access to it.

It might make sense to make that change in SiteInstanceImpl::IsSameWebSite (near
the IsRendererDebugURL checks), but that's a smaller issue for the review. 
Unless Nasko can think of issues I'm missing, I'd suggest proceeding with that
route.

[shatch, 2014-07-24 22:34:39]

On 2014/07/24 19:59:25, Charlie Reis wrote:
> On 2014/07/24 18:39:11, shatch wrote:
> > On 2014/07/23 22:12:14, shatch wrote:
> > > On 2014/07/23 22:02:28, Charlie Reis wrote:
> > > > On 2014/07/23 19:07:06, shatch wrote:
> > > > > On 2014/07/23 17:32:48, Charlie Reis wrote:
> > > > > > Thanks for looking to into the process sharing!  Your implementation
> doc
> > > was
> > > > > > especially helpful.  As we discussed, I think it would be nice for
> > > > transition
> > > > > > pages to end up in the same process as the destination page, without
> > > > affecting
> > > > > > our normal process model decision for the destination page.
> > > > > > 
> > > > > > I have some high level questions and concerns before getting into
code
> > > > review
> > > > > > comments, though, partly because I've been out on leave and missed
the
> > > > earlier
> > > > > > implementation stages.
> > > > > > 
> > > > > 
> > > > > Welcome back :)
> > > > > 
> > > > > > 1) What classes are you using to render the transition page?  Does
it
> go
> > > > into
> > > > > > its own WebContents, RenderViewHost, and/or RenderFrameHost?  And
does
> > > this
> > > > > > happen after we've created the SiteInstance for the destination
page? 
> > > Also,
> > > > > is
> > > > > > the transition page itself something like a data URL?
> > > > > > 
> > > > > 
> > > > > Right now the transition page gets its own ContentViewCore.
> > > > 
> > > > Oh.  Isn't that an Android class?  Is this feature Android-only?
> > > > 
> > > 
> > > Yeah, at the moment it's android only.
> > > 
> > > 
> > > > > I believe we've
> > > > > created the SiteInstance for the destination page at this point.
> > > > 
> > > > Is it possible to just use that SiteInstance for the new
ContentViewCore? 
> > It
> > > > shouldn't ever be a problem to load a blank page into the destination
> > > > SiteInstance, and that would save us from having to do any transition
URLs
> /
> > > > maps / changes to GetExistingProcessHost.  We would automatically be in
> the
> > > same
> > > > process by being in the same SiteInstance.
> > > > 
> > > 
> > > Ah I didn't know that, I'll give that a try and get back to you.
> > > 
> > 
> > So I gave sharing the SiteInstance between the transition and incoming
pages.
> > What I do is instantiate a new WebContents in the embedder with
> > createNativeWebContentsWithSharedSiteInstance(source_content_view_core),
then
> > navigate that ContentViewCore to about:blank before adding the serialized
> > markup.
> > 
> > Once I navigate the new ContentViewCore, it passes through
> > RenderFrameHostManager::GetSiteInstanceForEntry, which generates a new
> > SiteInstance and then a new RenderProcessHostImpl via
> > SiteInstanceImpl::GetProcess(). Based on your comment above, I'm assuming I
> > should be able to reuse the SiteInstance if I'm just navigating to a blank
> page?
> > So I added a check after the SiteInstanceImpl::IsSameSite() check in
> > RFHM::GetSiteInstanceForEntry() to see if the destination url was just
> > about:blank, and if so, use the current SiteInstance. Not sure if that's
legit
> > or not, so I've uploaded it for you to take look. If this all seems ok, I'll
> go
> > ahead and add tests, revise the CL description, and the implementation docs.
> 
> I'm thinking about it, and I think that might be fine.  Kind of the same
> reasoning as reusing an about:blank SiteInstance for a real site when a blank
> page is the only thing that has been in there, and it's consistent with a page
> using window.open("about:blank") and having script access to it.
> 
> It might make sense to make that change in SiteInstanceImpl::IsSameWebSite
(near
> the IsRendererDebugURL checks), but that's a smaller issue for the review. 
> Unless Nasko can think of issues I'm missing, I'd suggest proceeding with that
> route.

Awesome, went ahead and moved the check inside SiteInstance::IsSameWebSite,
added some tests, and revised the CL description. ptal

[Charlie Reis, 2014-07-25 17:31:22]

Cool.  One issue to update below.

One question before you make that change, though-- do we actually need to
explicitly navigate the new ContentViewCore to about:blank?  It does start with
a blank page, which you might be able to add your serialized markup to.  If so,
maybe it's possible to skip the change to IsSameWebSite entirely.

https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
content/browser/site_instance_impl.cc:264: // If either url is just a blank
page, we treat them as part of the same site.
Ah, we don't actually want this for navigating from about:blank to a normal URL,
since it's possible for one website to inject content into an about:blank popup
(via JavaScript) and then navigate it elsewhere.  It's only ok when going from a
normal URL to about:blank.

I still think this change makes sense for src -> dest, so perhaps we should
change the names of url1 and url2 to src_url and dest_url (here and in the .h
file), and add a mention of this behavior in the declaration comment in
site_instance.h.

https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
File content/browser/site_instance_impl_unittest.cc (right):

https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
content/browser/site_instance_impl_unittest.cc:392: // Blank pages should be
considered same site for anything.
Great.  Let's update this to show that it's for blank destination URLs only,
though.

https://codereview.chromium.org/378743002/diff/200001/content/browser/transit...
File content/browser/transition_browsertest.cc (right):

https://codereview.chromium.org/378743002/diff/200001/content/browser/transit...
content/browser/transition_browsertest.cc:145: ASSERT_EQ(outgoing_process_id,
transition_process_id);
nit: Use EXPECT_* for things we're testing (so that the test continues to run
and might print multiple EXPECT failures) and save ASSERT_* for test
preconditions (e.g., the embedded_test_server init above).

[shatch, 2014-07-25 18:42:01]

On 2014/07/25 17:31:22, Charlie Reis wrote:
> Cool.  One issue to update below.
> 
> One question before you make that change, though-- do we actually need to
> explicitly navigate the new ContentViewCore to about:blank?  It does start
with
> a blank page, which you might be able to add your serialized markup to.  If
so,
> maybe it's possible to skip the change to IsSameWebSite entirely.
> 

Holding off the rest of changes for now.

It's my understanding that the navigation is what creates the RenderFrameImpl?
Poking around a bit, it looks like that's created when we hit
RenderFrameHostManager::Navigate(), specifically the calls to
WebContentsImpl::CreateRenderViewForRenderManager() from
RenderFrameHostManager::Navigate() and RenderFrameHostManager::InitRenderView().
Tried with and without the initial navigation to about:blank, and without the
navigation, I couldn't see the RenderFrameImpl being created.

>
https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
> File content/browser/site_instance_impl.cc (right):
> 
>
https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
> content/browser/site_instance_impl.cc:264: // If either url is just a blank
> page, we treat them as part of the same site.
> Ah, we don't actually want this for navigating from about:blank to a normal
URL,
> since it's possible for one website to inject content into an about:blank
popup
> (via JavaScript) and then navigate it elsewhere.  It's only ok when going from
a
> normal URL to about:blank.
> 
> I still think this change makes sense for src -> dest, so perhaps we should
> change the names of url1 and url2 to src_url and dest_url (here and in the .h
> file), and add a mention of this behavior in the declaration comment in
> site_instance.h.
> 
>
https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
> File content/browser/site_instance_impl_unittest.cc (right):
> 
>
https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
> content/browser/site_instance_impl_unittest.cc:392: // Blank pages should be
> considered same site for anything.
> Great.  Let's update this to show that it's for blank destination URLs only,
> though.
> 
>
https://codereview.chromium.org/378743002/diff/200001/content/browser/transit...
> File content/browser/transition_browsertest.cc (right):
> 
>
https://codereview.chromium.org/378743002/diff/200001/content/browser/transit...
> content/browser/transition_browsertest.cc:145: ASSERT_EQ(outgoing_process_id,
> transition_process_id);
> nit: Use EXPECT_* for things we're testing (so that the test continues to run
> and might print multiple EXPECT failures) and save ASSERT_* for test
> preconditions (e.g., the embedded_test_server init above).

[Charlie Reis, 2014-07-25 18:45:13]

On 2014/07/25 18:42:01, shatch wrote:
> On 2014/07/25 17:31:22, Charlie Reis wrote:
> > Cool.  One issue to update below.
> > 
> > One question before you make that change, though-- do we actually need to
> > explicitly navigate the new ContentViewCore to about:blank?  It does start
> with
> > a blank page, which you might be able to add your serialized markup to.  If
> so,
> > maybe it's possible to skip the change to IsSameWebSite entirely.
> > 
> 
> Holding off the rest of changes for now.
> 
> It's my understanding that the navigation is what creates the RenderFrameImpl?
> Poking around a bit, it looks like that's created when we hit
> RenderFrameHostManager::Navigate(), specifically the calls to
> WebContentsImpl::CreateRenderViewForRenderManager() from
> RenderFrameHostManager::Navigate() and
RenderFrameHostManager::InitRenderView().
> Tried with and without the initial navigation to about:blank, and without the
> navigation, I couldn't see the RenderFrameImpl being created.
> 

Ok.  That may be a new thing as of RenderFrames, since I think it was possible
to create a RenderView without a navigation in the past.  In that case, let's
continue with this CL as we have been.  Thanks for checking.

[shatch, 2014-07-25 20:57:06]

New snapshot uploaded, ptal

https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
content/browser/site_instance_impl.cc:264: // If either url is just a blank
page, we treat them as part of the same site.
On 2014/07/25 17:31:22, Charlie Reis wrote:
> Ah, we don't actually want this for navigating from about:blank to a normal
URL,
> since it's possible for one website to inject content into an about:blank
popup
> (via JavaScript) and then navigate it elsewhere.  It's only ok when going from
a
> normal URL to about:blank.
> 
> I still think this change makes sense for src -> dest, so perhaps we should
> change the names of url1 and url2 to src_url and dest_url (here and in the .h
> file), and add a mention of this behavior in the declaration comment in
> site_instance.h.

Done.

https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
File content/browser/site_instance_impl_unittest.cc (right):

https://codereview.chromium.org/378743002/diff/200001/content/browser/site_in...
content/browser/site_instance_impl_unittest.cc:392: // Blank pages should be
considered same site for anything.
On 2014/07/25 17:31:22, Charlie Reis wrote:
> Great.  Let's update this to show that it's for blank destination URLs only,
> though.

Done.

https://codereview.chromium.org/378743002/diff/200001/content/browser/transit...
File content/browser/transition_browsertest.cc (right):

https://codereview.chromium.org/378743002/diff/200001/content/browser/transit...
content/browser/transition_browsertest.cc:145: ASSERT_EQ(outgoing_process_id,
transition_process_id);
On 2014/07/25 17:31:22, Charlie Reis wrote:
> nit: Use EXPECT_* for things we're testing (so that the test continues to run
> and might print multiple EXPECT failures) and save ASSERT_* for test
> preconditions (e.g., the embedded_test_server init above).

Done.

[Charlie Reis, 2014-07-25 21:03:37]

Thanks, LGTM

[shatch, 2014-07-25 21:09:05]

Hi David, looking for an owner for:

chrome/android/
chrome/browser/android/

ptal

[shatch, 2014-07-30 14:25:34]

On 2014/07/25 21:09:05, shatch wrote:
> Hi David, looking for an owner for:
> 
> chrome/android/
> chrome/browser/android/
> 
> ptal

ping dtrainor

[David Trainor- moved to gerrit, 2014-07-31 00:48:48]

nits but lgtm.

https://codereview.chromium.org/378743002/diff/240001/chrome/browser/android/...
File chrome/browser/android/content_view_util.cc (right):

https://codereview.chromium.org/378743002/diff/240001/chrome/browser/android/...
chrome/browser/android/content_view_util.cc:30: Profile* profile =
g_browser_process->profile_manager()->GetLastUsedProfile();
Not sure, but should we be grabbing the profile for the web contents we pass in?

https://codereview.chromium.org/378743002/diff/240001/content/browser/site_in...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/378743002/diff/240001/content/browser/site_in...
content/browser/site_instance_impl.cc:268: GURL blank_page =
GURL(url::kAboutBlankURL);
GURL blank_page(url::kAboutBlankURL); ?

[shatch, 2014-07-31 18:41:34]

https://codereview.chromium.org/378743002/diff/240001/chrome/browser/android/...
File chrome/browser/android/content_view_util.cc (right):

https://codereview.chromium.org/378743002/diff/240001/chrome/browser/android/...
chrome/browser/android/content_view_util.cc:30: Profile* profile =
g_browser_process->profile_manager()->GetLastUsedProfile();
On 2014/07/31 00:48:47, David Trainor wrote:
> Not sure, but should we be grabbing the profile for the web contents we pass
in?

Not sure either, but that sounds reasonable. Made the changes.

https://codereview.chromium.org/378743002/diff/240001/content/browser/site_in...
File content/browser/site_instance_impl.cc (right):

https://codereview.chromium.org/378743002/diff/240001/content/browser/site_in...
content/browser/site_instance_impl.cc:268: GURL blank_page =
GURL(url::kAboutBlankURL);
On 2014/07/31 00:48:47, David Trainor wrote:
> GURL blank_page(url::kAboutBlankURL); ?

Done.

[shatch, 2014-07-31 22:34:07]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-07-31 22:37:08]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/simonhatch@chromium.org/378743002/260001

[commit-bot: I haz the power, 2014-08-01 03:12:27]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/36231)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/41265)
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_compile_dbg on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-08-01 03:20:15]

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

[commit-bot: I haz the power, 2014-08-01 03:20:16]

Try jobs failed on following builders:
  android_clang_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[shatch, 2014-08-02 02:18:10]

The CQ bit was checked by simonhatch@chromium.org

[commit-bot: I haz the power, 2014-08-02 02:20:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/simonhatch@chromium.org/378743002/280001

[commit-bot: I haz the power, 2014-08-02 11:18:24]

Change committed as 287192