network service: Plumbing to get URLLoaderFactory to renderer on nav commit

network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug. Sort of like https://codereview.chromium.org/2840343002/
but follows the improved plan.

BUG=715640, 712913
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2855763002
Cr-Commit-Position: refs/heads/master@{#468675}
Committed: https://chromium.googlesource.com/chromium/src/+/6b0131a01da5149cd7acb81973f881a0d85fbe59

[scottmg, 2017-05-01 23:40:03]

Description was changed from

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug.

BUG=715640
==========

to

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug.

BUG=715640
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[scottmg, 2017-05-02 00:02:24]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[scottmg, 2017-05-02 00:02:53]

Description was changed from

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug.

BUG=715640
==========

to

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug. Sort of like https://codereview.chromium.org/2840343002/
but follows the improved plan.

BUG=715640
==========

[scottmg, 2017-05-02 00:02:53]

scottmg@chromium.org changed reviewers:
+ kinuko@chromium.org

[commit-bot: I haz the power, 2017-05-02 00:03:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 01:04:14]

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

[commit-bot: I haz the power, 2017-05-02 01:04:15]

Dry run: This issue passed the CQ dry run.

[kinuko, 2017-05-02 03:37:50]

lgtm

(+cc yhirano@ as it's semi-related to per-frame url loader)

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

https://codereview.chromium.org/2855763002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:3025:
commit_data.url_loader_factory = mojo::MessagePipeHandle();
When network service is enabled we already have one (that might not really work
yet but anyways), is that right?

https://codereview.chromium.org/2855763002/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2855763002/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:320: // If there's a
URLLoaderFactory attached to the frame, use it.
Can you put a short todo comment like: "TODO(crbug.com/712913): make
URLLoaderFactory always a per-frame thing"

[scottmg, 2017-05-02 15:16:30]

Description was changed from

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug. Sort of like https://codereview.chromium.org/2840343002/
but follows the improved plan.

BUG=715640
==========

to

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug. Sort of like https://codereview.chromium.org/2840343002/
but follows the improved plan.

BUG=715640,712913
==========

[scottmg, 2017-05-02 15:17:42]

Description was changed from

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug. Sort of like https://codereview.chromium.org/2840343002/
but follows the improved plan.

BUG=715640,712913
==========

to

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug. Sort of like https://codereview.chromium.org/2840343002/
but follows the improved plan.

BUG=715640,712913
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[scottmg, 2017-05-02 15:18:38]

Description was changed from

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug. Sort of like https://codereview.chromium.org/2840343002/
but follows the improved plan.

BUG=715640,712913
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug. Sort of like https://codereview.chromium.org/2840343002/
but follows the improved plan.

BUG=715640,712913
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[scottmg, 2017-05-02 15:18:38]

scottmg@chromium.org changed reviewers:
+ wfh@chromium.org

[scottmg, 2017-05-02 15:19:22]

+wfh for content/common/frame_messages.h as SECURITY_OWNERS.

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

https://codereview.chromium.org/2855763002/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:3025:
commit_data.url_loader_factory = mojo::MessagePipeHandle();
On 2017/05/02 03:37:50, kinuko wrote:
> When network service is enabled we already have one (that might not really
work
> yet but anyways), is that right?

Right, when --enable-network-service is on, the renderer currently uses the
connection manager to connect directly to the network service. Once we figure
out where the other endpoint should live for SW, we can pass the url loader
through here, and then this per-frame one will override the global connection
straight to the network in the renderer.

https://codereview.chromium.org/2855763002/diff/1/content/renderer/renderer_b...
File content/renderer/renderer_blink_platform_impl.cc (right):

https://codereview.chromium.org/2855763002/diff/1/content/renderer/renderer_b...
content/renderer/renderer_blink_platform_impl.cc:320: // If there's a
URLLoaderFactory attached to the frame, use it.
On 2017/05/02 03:37:50, kinuko wrote:
> Can you put a short todo comment like: "TODO(crbug.com/712913): make
> URLLoaderFactory always a per-frame thing"

Done.

[scottmg, 2017-05-02 15:19:29]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-02 15:19:51]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Will Harris, 2017-05-02 15:55:51]

I can't reason what security implications there are for giving the renderer a
mojo::MessagePipeHandle here. What does this allow a renderer to do - where is
the code/endpoint that backs this MessagePipeHandle? where are the restrictions
on what it can do enforced?

[scottmg, 2017-05-02 16:14:36]

On 2017/05/02 15:55:51, Will Harris wrote:
> I can't reason what security implications there are for giving the renderer a
> mojo::MessagePipeHandle here. What does this allow a renderer to do - where is
> the code/endpoint that backs this MessagePipeHandle? where are the
restrictions
> on what it can do enforced?

True. :( It will be the consumer endpoint of a URLLoaderFactory
https://cs.chromium.org/chromium/src/content/common/url_loader_factory.mojom?...
and implemented by a service worker (i.e. another renderer). I don't know that
we can express that constraint here in the type information though.

Would you be any happier if it was clearly typed as a URLLoaderFactory?

(I don't think it's currently possible to pass a URLLoaderFactoryPtrInfo through
IPC because it's move-only but maybe there's some way to do that...)

[commit-bot: I haz the power, 2017-05-02 16:16:59]

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

[commit-bot: I haz the power, 2017-05-02 16:17:00]

Dry run: This issue passed the CQ dry run.

[Will Harris, 2017-05-02 16:21:14]

On 2017/05/02 16:14:36, scottmg wrote:
> On 2017/05/02 15:55:51, Will Harris wrote:
> > I can't reason what security implications there are for giving the renderer
a
> > mojo::MessagePipeHandle here. What does this allow a renderer to do - where
is
> > the code/endpoint that backs this MessagePipeHandle? where are the
> restrictions
> > on what it can do enforced?
> 
> True. :( It will be the consumer endpoint of a URLLoaderFactory
>
https://cs.chromium.org/chromium/src/content/common/url_loader_factory.mojom?...
> and implemented by a service worker (i.e. another renderer). I don't know that
> we can express that constraint here in the type information though.
> 
> Would you be any happier if it was clearly typed as a URLLoaderFactory?
> 
> (I don't think it's currently possible to pass a URLLoaderFactoryPtrInfo
through
> IPC because it's move-only but maybe there's some way to do that...)

hmm I suppose I'm happy that the handle is a URLLoaderFactory because I see that
in the code and hopefully that won't change without another security review, but
I'm more asking about the consequences of exposing CreateLoaderAndStart and
SyncLoad to a renderer - can you confirm that these APIs are just equivalent to
existing APIs we already expose to renderers, or do I have to review all this
new code that's being exposed?

[scottmg, 2017-05-02 16:24:47]

On 2017/05/02 16:21:14, Will Harris wrote:
> On 2017/05/02 16:14:36, scottmg wrote:
> > On 2017/05/02 15:55:51, Will Harris wrote:
> > > I can't reason what security implications there are for giving the
renderer
> a
> > > mojo::MessagePipeHandle here. What does this allow a renderer to do -
where
> is
> > > the code/endpoint that backs this MessagePipeHandle? where are the
> > restrictions
> > > on what it can do enforced?
> > 
> > True. :( It will be the consumer endpoint of a URLLoaderFactory
> >
>
https://cs.chromium.org/chromium/src/content/common/url_loader_factory.mojom?...
> > and implemented by a service worker (i.e. another renderer). I don't know
that
> > we can express that constraint here in the type information though.
> > 
> > Would you be any happier if it was clearly typed as a URLLoaderFactory?
> > 
> > (I don't think it's currently possible to pass a URLLoaderFactoryPtrInfo
> through
> > IPC because it's move-only but maybe there's some way to do that...)
> 
> hmm I suppose I'm happy that the handle is a URLLoaderFactory because I see
that
> in the code and hopefully that won't change without another security review,
but

Yes, that's cruddy, I agree.

> I'm more asking about the consequences of exposing CreateLoaderAndStart and
> SyncLoad to a renderer - can you confirm that these APIs are just equivalent
to
> existing APIs we already expose to renderers, or do I have to review all this
> new code that's being exposed?

Ah, I see. Yes, we're already giving the renderer an attachment to a (currently
unsandboxed) version of this interface implemented in the network service
process. You can see that connection in the code that's changing in this CL in
content/renderer/renderer_blink_platform_impl.cc using
connector_->BindInterface(), which now becomes the fallback case.

[Will Harris, 2017-05-02 16:29:12]

okay thanks for the clarification, lgtm

[scottmg, 2017-05-02 16:29:46]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2017-05-02 16:29:47]

The patchset sent to the CQ was uploaded after l-g-t-m from kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/2855763002/#ps20001
(title: "fixes")

[commit-bot: I haz the power, 2017-05-02 16:30:12]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-02 16:34:36]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1493742586931990,
"parent_rev": "4af2f52e3010ba02c05e7a8c3c435f66425804c8", "commit_rev":
"6b0131a01da5149cd7acb81973f881a0d85fbe59"}

[commit-bot: I haz the power, 2017-05-02 16:34:56]

Description was changed from

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug. Sort of like https://codereview.chromium.org/2840343002/
but follows the improved plan.

BUG=715640,712913
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
network service: Plumbing to get URLLoaderFactory to renderer on nav commit

See #c11 in linked bug. Sort of like https://codereview.chromium.org/2840343002/
but follows the improved plan.

BUG=715640,712913
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2855763002
Cr-Commit-Position: refs/heads/master@{#468675}
Committed:
https://chromium.googlesource.com/chromium/src/+/6b0131a01da5149cd7acb81973f8...
==========

[commit-bot: I haz the power, 2017-05-02 16:34:57]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/6b0131a01da5149cd7acb81973f8...