Navigation transitions: Plumb data from the outgoing renderer to the incoming renderer

Navigation transitions: Plumb data from the outgoing renderer to the incoming renderer.

The outgoing renderer sends one or more sets of serialized markup of transition
 elements, the CSS selectors used to select each of these and its associated 
allowed destination origin, up to the TransitionRequestManager in Chrome. Once 
we have a navigation response we then pick up this data again (selecting one of 
these sets), send it up to the Android embedder, and from there it's used to 1) 
send a message down to the outgoing renderer to hide the transition elements 
we selected to use using the CSS selector, and 2) send the serialized markup 
down to the incoming renderer to inject into its document.

R=creis,jam,nasko
BUG=370696
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288228

[oystein (OOO til 10th of July), 2014-08-01 22:41:32]

Hi guys,

Last remaining part of Blink<->CrossSiteResourceHandler<->AndroidEmbedder
plumbing/infrastructure for this thing; PTAL :)

[nasko, 2014-08-04 13:36:48]

Few comments.

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
File content/browser/transition_request_manager.cc (right):

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:120: // FIXME(oysteine): Add CSP
check to validate the origin and the request_url.
Please add this check before committing the code.

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:132: 
nit: no need for empty line

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:134: DCHECK(transition_data);
What happens in release builds if transition_data is NULL?

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.h:28: struct TransitionLayerData {
Adding a comment that this structure is passed between threads is useful, as it
will help people avoid mistakes when adding code to it.

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.h:59: bool
HasPendingTransitionRequest(int renderer_id,
nit: why drop the "process" part of the variable name? I think it makes it more
clear that we are dealing with a process identifier. The caller passes in
"render_process_id", which I find to be a really good name.

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.h:68: int render_view_id,
Isn't this render_frame_id?

https://codereview.chromium.org/435833002/diff/40001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/435833002/diff/40001/content/common/frame_mes...
content/common/frame_messages.h:689: // Notifies the browser that the renderer
has a pending navigation transition.
nit: Add a comment saying that the strings are in UTF8 format.

https://codereview.chromium.org/435833002/diff/40001/content/common/frame_mes...
content/common/frame_messages.h:692: std::string  /* origin */,
Any reason this is not a GURL?

https://codereview.chromium.org/435833002/diff/40001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/435833002/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1252:
frame_->navigateToSandboxedMarkup(WebData(markup.data(), markup.length()));
Why not use c_str()? data() doesn't guarantee null-terminated string and WebData
expects that according to its signature.

[oystein (OOO til 10th of July), 2014-08-05 19:02:49]

Thanks nasko! New version up, with some questions below.

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
File content/browser/transition_request_manager.cc (right):

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:120: // FIXME(oysteine): Add CSP
check to validate the origin and the request_url.
On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> Please add this check before committing the code.

I would really prefer if that could be done in a follow-up CL, since
browser-side parsing is pretty sensitive and the check isn't necessary for
developer testing. Since this feature is behind a runtime-flag, can I just add a
CHECK() for that flag here, to ensure it's not invoked by accident?

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:132: 
On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> nit: no need for empty line

Done.

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:134: DCHECK(transition_data);
On 2014/08/04 13:36:47, nasko (in Munich) wrote:
> What happens in release builds if transition_data is NULL?

We'll crash; it should never be NULL. Should this be a CHECK() instead?

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.h:28: struct TransitionLayerData {
On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> Adding a comment that this structure is passed between threads is useful, as
it
> will help people avoid mistakes when adding code to it.

Done.

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.h:59: bool
HasPendingTransitionRequest(int renderer_id,
On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> nit: why drop the "process" part of the variable name? I think it makes it
more
> clear that we are dealing with a process identifier. The caller passes in
> "render_process_id", which I find to be a really good name.

Done; unintentional change actually due to some merges, thanks for pointing it
out.

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.h:68: int render_view_id,
On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> Isn't this render_frame_id?

Done; same as above.

https://codereview.chromium.org/435833002/diff/40001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/435833002/diff/40001/content/common/frame_mes...
content/common/frame_messages.h:689: // Notifies the browser that the renderer
has a pending navigation transition.
On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> nit: Add a comment saying that the strings are in UTF8 format.

Done.

https://codereview.chromium.org/435833002/diff/40001/content/common/frame_mes...
content/common/frame_messages.h:692: std::string  /* origin */,
On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> Any reason this is not a GURL?

The idea is that it can contain wildcards (*.google.com); can GURLS do that?

https://codereview.chromium.org/435833002/diff/40001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/435833002/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:1252:
frame_->navigateToSandboxedMarkup(WebData(markup.data(), markup.length()));
On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> Why not use c_str()? data() doesn't guarantee null-terminated string and
WebData
> expects that according to its signature.

Hm I don't see how WebData can expect that, when it also takes a size parameter?
Seems to be expecting just raw bytes (and the implementation just creates a
SharedBuffer with the same parameters).

[Charlie Reis, 2014-08-05 20:07:21]

A few nits below.

I don't suppose there's any way for the transition/destination process to verify
that the incoming markup is properly serialized and doesn't contain script code,
etc?  Seems like we have to trust the outgoing page's renderer to serialize it
correctly, isn't a great plan.

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.cc (right):

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.cc:120: // FIXME(oysteine): Add CSP
check to validate the origin and the request_url.
nit: We use TODO(username): in the Chrome repo.

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.h:80: TransitionRequestData();
nit: All of these indents are wrong.  Should be 2 spaces, not 4.

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.h:90: std::string origin;
Can we say something about these values?  I'm concerned from a security
perspective that one (possibly exploited) renderer process can pass CSS and
markup to another renderer process for a different site, maybe trying to take
over the new process.  I'm not sure whether there's much we can do here, but the
browser shouldn't trust these strings, etc.

https://codereview.chromium.org/435833002/diff/60001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/435833002/diff/60001/content/common/frame_mes...
content/common/frame_messages.h:385:
IPC_MESSAGE_ROUTED1(FrameMsg_SetupTransitionView,
These need comments like the other IPC messages.

[oystein (OOO til 10th of July), 2014-08-05 20:50:54]

Thanks!

On 2014/08/05 20:07:21, Charlie Reis wrote:
> I don't suppose there's any way for the transition/destination process to
verify
> that the incoming markup is properly serialized and doesn't contain script
code,
> etc?  Seems like we have to trust the outgoing page's renderer to serialize it
> correctly, isn't a great plan.

Not sure how much trust we could put in any kind of verification/validation
here. As it is, we load the markup into a fully sandboxed (meaning no script
execution) frame, and since it's always a freshly navigated renderer I hope that
means we don't *have* to trust the markup. Any other safeguards you can think
of, that we should add?

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.cc (right):

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.cc:120: // FIXME(oysteine): Add CSP
check to validate the origin and the request_url.
On 2014/08/05 20:07:20, Charlie Reis wrote:
> nit: We use TODO(username): in the Chrome repo.

Done.

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.h:80: TransitionRequestData();
On 2014/08/05 20:07:20, Charlie Reis wrote:
> nit: All of these indents are wrong.  Should be 2 spaces, not 4.

Done.

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.h:90: std::string origin;
On 2014/08/05 20:07:20, Charlie Reis wrote:
> Can we say something about these values?  I'm concerned from a security
> perspective that one (possibly exploited) renderer process can pass CSS and
> markup to another renderer process for a different site, maybe trying to take
> over the new process.  I'm not sure whether there's much we can do here, but
the
> browser shouldn't trust these strings, etc.

Added some comments. With the current implementation we only ever navigate to
the serialized markup in a new fully sandboxed renderer (instance, not process),
at least.

https://codereview.chromium.org/435833002/diff/60001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/435833002/diff/60001/content/common/frame_mes...
content/common/frame_messages.h:385:
IPC_MESSAGE_ROUTED1(FrameMsg_SetupTransitionView,
On 2014/08/05 20:07:21, Charlie Reis wrote:
> These need comments like the other IPC messages.

Done.

[Charlie Reis, 2014-08-05 20:58:14]

Thanks!  LGTM with nits, but let's wait for Nasko's approval as well.

> On 2014/08/05 20:07:21, Charlie Reis wrote:
> > I don't suppose there's any way for the transition/destination process to
> verify
> > that the incoming markup is properly serialized and doesn't contain script
> code,
> > etc?  Seems like we have to trust the outgoing page's renderer to serialize
it
> > correctly, isn't a great plan.
> 
> Not sure how much trust we could put in any kind of verification/validation
> here. As it is, we load the markup into a fully sandboxed (meaning no script
> execution) frame, and since it's always a freshly navigated renderer I hope
that
> means we don't *have* to trust the markup. Any other safeguards you can think
> of, that we should add?

Yes, that's the sort of thing I was hoping for.  Great.

https://codereview.chromium.org/435833002/diff/80001/content/browser/transiti...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/435833002/diff/80001/content/browser/transiti...
content/browser/transition_request_manager.h:90: // These strings could have
originated from a compromised renderer,
Need 2 space indent here as well.

https://codereview.chromium.org/435833002/diff/80001/content/browser/transiti...
content/browser/transition_request_manager.h:91: // and should not be trusted or
assumed safe.
I like your explanation in the review comment.  Let's add: "They are only used
within a sandboxed iframe with scripts disabled."

[nasko, 2014-08-06 10:58:49]

LGTM with a few nits/comments.

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
File content/browser/transition_request_manager.cc (right):

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:120: // FIXME(oysteine): Add CSP
check to validate the origin and the request_url.
On 2014/08/05 19:02:48, Oystein wrote:
> On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> > Please add this check before committing the code.
> 
> I would really prefer if that could be done in a follow-up CL, since
> browser-side parsing is pretty sensitive and the check isn't necessary for
> developer testing. Since this feature is behind a runtime-flag, can I just add
a
> CHECK() for that flag here, to ensure it's not invoked by accident?

Yes, adding a CHECK to ensure this code is not executed in regular builds will
be good.

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:134: DCHECK(transition_data);
On 2014/08/05 19:02:48, Oystein wrote:
> On 2014/08/04 13:36:47, nasko (in Munich) wrote:
> > What happens in release builds if transition_data is NULL?
> 
> We'll crash; it should never be NULL. Should this be a CHECK() instead?

It should be a CHECK, if and only if a compromised renderer cannot for the CHECK
to become true. This will allow a denial-of-service attack from compromised
renderers, which is something we shouldn't enable.

https://codereview.chromium.org/435833002/diff/40001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/435833002/diff/40001/content/common/frame_mes...
content/common/frame_messages.h:692: std::string  /* origin */,
On 2014/08/05 19:02:49, Oystein wrote:
> On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> > Any reason this is not a GURL?
> 
> The idea is that it can contain wildcards (*.google.com); can GURLS do that?

No, GURL doesn't support that. Then the name "origin" doesn't seem entirely
correct, as it is not an origin, but an origin/host pattern.

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.h:90: std::string origin;
On 2014/08/05 20:50:53, Oystein wrote:
> On 2014/08/05 20:07:20, Charlie Reis wrote:
> > Can we say something about these values?  I'm concerned from a security
> > perspective that one (possibly exploited) renderer process can pass CSS and
> > markup to another renderer process for a different site, maybe trying to
take
> > over the new process.  I'm not sure whether there's much we can do here, but
> the
> > browser shouldn't trust these strings, etc.
> 
> Added some comments. With the current implementation we only ever navigate to
> the serialized markup in a new fully sandboxed renderer (instance, not
process),
> at least.

I'm not sure I fully understand this comment. Are you saying that even when
origin A navigates to another page on origin A (so no process swap needed) with
transitions, it will still create a brand new RenderView/RenderFrame?

I think it doesn't make much difference where you are going to, whether it is a
new process or existing. The threat always comes from the originating page, so
as long as that is clear and taken into account, it is good.

https://codereview.chromium.org/435833002/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/435833002/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:385: // Requests a navigation to the supplied
markup, in a fully sandboxed frame
nit: let's be a bit more clear and use something like "in an iframe with sandbox
attributes". To me "sandboxed frame" means a frame object running in sandboxed
process, which is already the case.

[oystein (OOO til 10th of July), 2014-08-06 22:36:20]

Thanks nasko&creis! :)

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
File content/browser/transition_request_manager.cc (right):

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:120: // FIXME(oysteine): Add CSP
check to validate the origin and the request_url.
On 2014/08/06 10:58:48, nasko (in Munich) wrote:
> On 2014/08/05 19:02:48, Oystein wrote:
> > On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> > > Please add this check before committing the code.
> > 
> > I would really prefer if that could be done in a follow-up CL, since
> > browser-side parsing is pretty sensitive and the check isn't necessary for
> > developer testing. Since this feature is behind a runtime-flag, can I just
add
> a
> > CHECK() for that flag here, to ensure it's not invoked by accident?
> 
> Yes, adding a CHECK to ensure this code is not executed in regular builds will
> be good.

Done.

https://codereview.chromium.org/435833002/diff/40001/content/browser/transiti...
content/browser/transition_request_manager.cc:134: DCHECK(transition_data);
On 2014/08/06 10:58:48, nasko (in Munich) wrote:
> On 2014/08/05 19:02:48, Oystein wrote:
> > On 2014/08/04 13:36:47, nasko (in Munich) wrote:
> > > What happens in release builds if transition_data is NULL?
> > 
> > We'll crash; it should never be NULL. Should this be a CHECK() instead?
> 
> It should be a CHECK, if and only if a compromised renderer cannot for the
CHECK
> to become true. This will allow a denial-of-service attack from compromised
> renderers, which is something we shouldn't enable.

Done. Yeah if a renderer is compromised it could send empty strings, but this
struct itself would always be present still.

https://codereview.chromium.org/435833002/diff/40001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/435833002/diff/40001/content/common/frame_mes...
content/common/frame_messages.h:692: std::string  /* origin */,
On 2014/08/06 10:58:49, nasko (in Munich) wrote:
> On 2014/08/05 19:02:49, Oystein wrote:
> > On 2014/08/04 13:36:48, nasko (in Munich) wrote:
> > > Any reason this is not a GURL?
> > 
> > The idea is that it can contain wildcards (*.google.com); can GURLS do that?
> 
> No, GURL doesn't support that. Then the name "origin" doesn't seem entirely
> correct, as it is not an origin, but an origin/host pattern.

Yeah that makes sense. Renamed to allowed_destination_host_pattern.

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/435833002/diff/60001/content/browser/transiti...
content/browser/transition_request_manager.h:90: std::string origin;
On 2014/08/06 10:58:49, nasko (in Munich) wrote:
> On 2014/08/05 20:50:53, Oystein wrote:
> > On 2014/08/05 20:07:20, Charlie Reis wrote:
> > > Can we say something about these values?  I'm concerned from a security
> > > perspective that one (possibly exploited) renderer process can pass CSS
and
> > > markup to another renderer process for a different site, maybe trying to
> take
> > > over the new process.  I'm not sure whether there's much we can do here,
but
> > the
> > > browser shouldn't trust these strings, etc.
> > 
> > Added some comments. With the current implementation we only ever navigate
to
> > the serialized markup in a new fully sandboxed renderer (instance, not
> process),
> > at least.
> 
> I'm not sure I fully understand this comment. Are you saying that even when
> origin A navigates to another page on origin A (so no process swap needed)
with
> transitions, it will still create a brand new RenderView/RenderFrame?
> 
> I think it doesn't make much difference where you are going to, whether it is
a
> new process or existing. The threat always comes from the originating page, so
> as long as that is clear and taken into account, it is good.

The serialized markup is only ever deserialized in the transition RenderView
which the embedder has newly created, it doesn't touch the RenderView of the
page we're navigating from or to.

https://codereview.chromium.org/435833002/diff/80001/content/browser/transiti...
File content/browser/transition_request_manager.h (right):

https://codereview.chromium.org/435833002/diff/80001/content/browser/transiti...
content/browser/transition_request_manager.h:90: // These strings could have
originated from a compromised renderer,
On 2014/08/05 20:58:14, Charlie Reis wrote:
> Need 2 space indent here as well.

Done.

https://codereview.chromium.org/435833002/diff/80001/content/browser/transiti...
content/browser/transition_request_manager.h:91: // and should not be trusted or
assumed safe.
On 2014/08/05 20:58:14, Charlie Reis wrote:
> I like your explanation in the review comment.  Let's add: "They are only used
> within a sandboxed iframe with scripts disabled."

Done.

https://codereview.chromium.org/435833002/diff/80001/content/common/frame_mes...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/435833002/diff/80001/content/common/frame_mes...
content/common/frame_messages.h:385: // Requests a navigation to the supplied
markup, in a fully sandboxed frame
On 2014/08/06 10:58:49, nasko (in Munich) wrote:
> nit: let's be a bit more clear and use something like "in an iframe with
sandbox
> attributes". To me "sandboxed frame" means a frame object running in sandboxed
> process, which is already the case.

Done.

[oystein (OOO til 10th of July), 2014-08-07 00:40:20]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-08-07 00:42:38]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oysteine@chromium.org/435833002/180001

[oystein (OOO til 10th of July), 2014-08-07 01:20:01]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-08-07 01:21:52]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oysteine@chromium.org/435833002/200001

[commit-bot: I haz the power, 2014-08-07 06:19:05]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_dbg_triggered_tests on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tri...)

[commit-bot: I haz the power, 2014-08-07 07:30:38]

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

[commit-bot: I haz the power, 2014-08-07 07:30:39]

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

[oystein (OOO til 10th of July), 2014-08-07 21:45:43]

The CQ bit was checked by oysteine@chromium.org

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

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oysteine@chromium.org/435833002/210001

[commit-bot: I haz the power, 2014-08-08 00:58:37]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)
  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-08 06:35:30]

Change committed as 288228