ServiceWorker request intercept plan 1: Create per-request URLRequestContext

Implement custom URLRequestJobFactory to intercept requests for ServiceWorker

This change adds new ServiceWorkerRequestContext class, which:
- inherits from URLRequestContext
- provides custom URLRequestJobFactory to handle requests for ServiceWorker
- is to be created per each request to provide pre-request customization

To ensure that ServiceWorkerRequestContext is valid during the lifetime
of URLRequest, it's attached to URLRequest via SupportsUserData API.

BUG=285976

[kinuko, 2014-02-24 13:16:29]

This is a new attempt to handle requests with ServiceWorker via
URLRequestContext::set_job_factory(factory). Can you take a look?

[michaeln, 2014-02-25 23:51:57]

I think the place to do this is probably somewhere in the neighborhood of
chrome/browser/profile/profile_io_data.cc.

Chrome defines a derivative of URLRequestContext (ChromeURLRequestContext). I
doubt if wrapping that context with this content class is workable. Chrome
probably expects to see its derivative when accessing request->context().

The profile startup/init code paths are crazily complicated. Probably we'll need
content lib api changes for the content lib to provide a set of 'intercept
handlers' to the embedder with the expectation that the embedder will configure
the URLRequestContext with those handlers. Currently the context lib passes up
other protocol handlers (thru this crazily complicated code) for things like
blob and filesystem urls. Looks like those get plugged in in profile_io_data.cc.

Seems like we will need to widen some interface to provide a list of 'intercept
handlers' too. I need to do some excavation to unravel the init sequence in more
detail.

[kinuko, 2014-02-26 02:49:16]

(This patch may not go anywhere, but continue discussion for exploration)

I think this could be workable, though I could be missing something.  (Just to
note, I also did look into profile_io_data.cc, ChromeURLRequstContext and some
of the startup stuff where the chrome's request context gets created in startup)

URLRequestContext is mostly a container of pointers, and SWRequestContext copies
all of them from chrome's one so that all acccessors returns the same objects
except for job_factory(). This step is basically same as how we set up
URLRequestContext variants for media/isolated context from main one, and that's
why I wanted to make this attempt. (And we never upcast request->context() to
ChromeURLRequestContext)

On 2014/02/25 23:51:57, michaeln wrote:
> Seems like we will need to widen some interface to provide a list of
'intercept
> handlers' too. I need to do some excavation to unravel the init sequence in
more
> detail.

I agree that we could make a small change in some interface so that we can
create our customized URLRequest context in a more well-mannered way, but I want
to make it without registering something in the startup/init code sequence,
which feels more intrusive.

[mmenke, 2014-02-26 19:08:56]

On 2014/02/26 02:49:16, kinuko wrote:
> (This patch may not go anywhere, but continue discussion for exploration)
> 
> I think this could be workable, though I could be missing something.  (Just to
> note, I also did look into profile_io_data.cc, ChromeURLRequstContext and some
> of the startup stuff where the chrome's request context gets created in
startup)
> 
> URLRequestContext is mostly a container of pointers, and SWRequestContext
copies
> all of them from chrome's one so that all acccessors returns the same objects
> except for job_factory(). This step is basically same as how we set up
> URLRequestContext variants for media/isolated context from main one, and
that's
> why I wanted to make this attempt. (And we never upcast request->context() to
> ChromeURLRequestContext)
> 
> On 2014/02/25 23:51:57, michaeln wrote:
> > Seems like we will need to widen some interface to provide a list of
> 'intercept
> > handlers' too. I need to do some excavation to unravel the init sequence in
> more
> > detail.
> 
> I agree that we could make a small change in some interface so that we can
> create our customized URLRequest context in a more well-mannered way, but I
want
> to make it without registering something in the startup/init code sequence,
> which feels more intrusive.

We actually want to be going in the direction of fewer, rather than more,
request contexts.  "Apps" have their own, and if we wanted to use service
workers with those, things just get ugly.

[michaeln, 2014-02-26 19:31:15]

I think i have a URLRequest feature request to help with this. Currently the
caller can set a factory for the context. In a large program like chrome, that
can be difficult to pull off. So here's the feature request.

Allow a factory to be set on the request object it self. If null or if the fact
method returns null, the net lib should fallthru to the factory set on the
context.

Some good things about that.
1) The scope can be highly constrained to an individual request. This directly
addresses one of the issues will brought up.
2) It's a lot easier to arrange for in a large program like chrome, we'd set
this in RDH::BeginRequest.

It would probably make sense to have a getter and setter so consumers could more
easily chain them if that's needed.

wdyt?

[kinuko, 2014-02-28 04:06:07]

(Per offline discussion I assume it's worth pursuing this direction more)

On 2014/02/26 19:31:15, michaeln wrote:
> Allow a factory to be set on the request object it self. If null or if the
fact
> method returns null, the net lib should fallthru to the factory set on the
> context.

That'd be neat if net folks are happy with the change, or I think we could also
work on the current approach too (after all URLRequestContext is where we give
context to the URLRequest)

The gist of this patch is basically:

custom_request_context = new URLRequestContext();
custom_request_context->CopyFrom(global_request_context);
custom_request_context_->set_job_factory(custom_job_factory);
// (and attach the context to the request)

And this actually achieves what we want to do, i.e. per-request job
customization (I verified replacing job factory in this way works well with all
existing tests that could be tested on trybots).

[mmenke, 2014-02-28 20:09:16]

On 2014/02/28 04:06:07, kinuko wrote:
> (Per offline discussion I assume it's worth pursuing this direction more)
> 
> On 2014/02/26 19:31:15, michaeln wrote:
> > Allow a factory to be set on the request object it self. If null or if the
> fact
> > method returns null, the net lib should fallthru to the factory set on the
> > context.
> 
> That'd be neat if net folks are happy with the change, or I think we could
also
> work on the current approach too (after all URLRequestContext is where we give
> context to the URLRequest)
> 
> The gist of this patch is basically:
> 
> custom_request_context = new URLRequestContext();
> custom_request_context->CopyFrom(global_request_context);
> custom_request_context_->set_job_factory(custom_job_factory);
> // (and attach the context to the request)
> 
> And this actually achieves what we want to do, i.e. per-request job
> customization (I verified replacing job factory in this way works well with
all
> existing tests that could be tested on trybots).

I'm assuming you're thinking of doing this on a per-request basis, as opposed to
having a global ServiceWorkerRequestContext?

So we have a couple ways to do this...  None of which I like all that much:

1)  Create a RequestContext for each request from a frame that has a
ServiceWorker.
  - Upside:  Requires no net changes.
  - Upside:  Only touches requests where it may be necessary, we have RDH
information when we create it.
  - Downside:  Creates more request contexts, which seems hairy and can be
difficult to debug.
  - Downside:  Hung requests won't appear in net-internals.
  - Downside:  Who owns the context?

2)  Explicitly set a factory for certain URLRequests.
  - Upside:  Only touches requests where it may be necessary, we have RDH
information when we create it.
  - Upside:  Hung requests will appear in net-internals.
  - Downside:  Requires adding yet another way to create URLRequestJobs.  So now
we'd have...5?  That seems rather excessive.

3)  Use ProtocolInterceptJobFactory.
  - Upside:  No net changes, and this method is already in use on Android
WebView (And for debugging devtools, but the latter requires an extra
compile-time define).  So it is used, but not often.
  - Downside:  Affects all requests, have to figure out which ones it needs to
muck with.

4)  Use the deprecated interceptor API.
  - Update:  Already in use.
  - Downside:  Affects all requests, have to figure out which ones it needs to
muck with.
  - Downside:  Deprecated.

[michaeln, 2014-02-28 21:27:00]

> 1)  Create a RequestContext for each request from a frame

This feels wrong and awkward to me (tail wagging dog). Also i suspect
something(s) will break somewhere with this (I think there is logic in /chrome
to figure out the profile/iodata given a request context).

> 2)  Explicitly set a factory for certain URLRequests.

This seems ideal to me. Scoped and simple to setup. The best answer imho. It
directly addresses the downsides of 3.

> 3)  Use ProtocolInterceptJobFactory.
>   - Downside:  Affects all requests, have to figure out which ones

That's not really much of a downside. The larger downside from my perspective is
that it can be hairy to arrange and maintain in a program as large as chrome.
Code in /chrome owns the context, code buried down in /content needs to add
handler. It's doable but not straight forward.

> 4)  Use the deprecated interceptor API.

Let's take that option off the table, unless you want to un-deprecate that api.

[mmenke, 2014-02-28 21:36:50]

On 2014/02/28 21:27:00, michaeln wrote:
> > 1)  Create a RequestContext for each request from a frame
> 
> This feels wrong and awkward to me (tail wagging dog). Also i suspect
> something(s) will break somewhere with this (I think there is logic in /chrome
> to figure out the profile/iodata given a request context).
> 
> > 2)  Explicitly set a factory for certain URLRequests.
> 
> This seems ideal to me. Scoped and simple to setup. The best answer imho. It
> directly addresses the downsides of 3.
> 
> > 3)  Use ProtocolInterceptJobFactory.
> >   - Downside:  Affects all requests, have to figure out which ones
> 
> That's not really much of a downside. The larger downside from my perspective
is
> that it can be hairy to arrange and maintain in a program as large as chrome.
> Code in /chrome owns the context, code buried down in /content needs to add
> handler. It's doable but not straight forward.
> 
> > 4)  Use the deprecated interceptor API.
> 
> Let's take that option off the table, unless you want to un-deprecate that
api.

I rather agree with you in eliminating 1 and 4 (I just listed 4 because I wasn't
too happy with the others, but I also don't like 4).

I think adding a per-request factory is actually more complicated, in that it
adds more control flow paths, and another place job factories are hooked in.  As
ugly as ProfileIOData is, at least we create all the factories there in one
localized location, so one can see how the onion of network stack-related
objects are created, which I find quite helpful.

[michaeln, 2014-02-28 22:18:28]

> I rather agree with you in eliminating 1 and 4 (I just listed 4 because I
wasn't
> too happy with the others, but I also don't like 4).

ok, 2 or 3, pick your poison

> I think adding a per-request factory is actually more complicated, in that it
> adds more control flow paths, and another place job factories are hooked in. 
As
> ugly as ProfileIOData is, at least we create all the factories there in one
> localized location, so one can see how the onion of network stack-related
> objects are created, which I find quite helpful.

More complicated in some ways, less in others. The act of adding a new one is
easier but its more hidden from view. I think either of these can work. How
about we give untangling/excavating ProfileIOData setup a shot and see what it
looks like. If too hairy, we revisit, otherwise we go with it.

wdyt?

[kinuko, 2014-03-03 04:12:23]

I do like to eliminate 4 too, while I don't see why 1 is particularly worse
than 2 or 3, it's also scoped, simple to setup and utilizing what each
class's supposed to do imho (request context gives a context to requests).

But I can live with either of 1-3, or can create patches for each of them
if it helps us decide.
(I have a feeling that we can switch one to another among these 1-3
relatively easily, and I'm eager to pick one of them)


On Sat, Mar 1, 2014 at 5:09 AM, <mmenke@chromium.org> wrote:

> 1)  Create a RequestContext for each request from a frame that has a
>  ServiceWorker.
>   - Upside:  Requires no net changes.
>   - Upside:  Only touches requests where it may be necessary, we have RDH
> information when we create it.
>   - Downside:  Creates more request contexts, which seems hairy and can be
> difficult to debug.

  - Downside:  Hung requests won't appear in net-internals.
>

I think this is wrong? New context can just inherit all env including
net_log from the global request context.

  - Downside:  Who owns the context?
>

Is this a downside?  URLRequest can, and 2) seems to have the same issue
(i.e. who owns the per-request job_factory).

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[kinuko, 2014-03-03 12:45:14]

I took rough attempts to compare three options:

Plan 1: this CL (https://codereview.chromium.org/177733003/)
Plan 2: https://codereview.chromium.org/185403004/
Plan 3: https://codereview.chromium.org/185593004/

None of them are complete, but has minimal code to show how intercept works in
each case. In my rough local testing (with actual wiring part) any of them seem
to work for our use case without visible breakages.

Overall I prefer 2 -> 1 -> 3 (best to worst), though I'm biased by the amount of
code I needed to add in my attempts. My plumbing for plan 3 could be worse than
optimal but in general it'd need a lot plumbing around Profile/ProfileIOData.

I hope we can decide one of them in a day or two.

[mmenke, 2014-03-03 15:57:03]

About approach 1:  The URLRequest can't own the context.  The context has to be
destroyed after the URLRequest, since the request references Context objects. 
Contexts have a CHECK to ensure this, since this is a long standing source of
bugs.

Thanks for the code for the three approaches!

Your code for approach 1 doesn't go into the full details of actually plugging
it in, but I think 2 is better, so let's stick with 2 or 3.

Want to spend some time digging through them, but I certainly don't want to
block you on this, and agree that we should resolve this quickly.

On 2014/03/03 12:45:14, kinuko wrote:
> I took rough attempts to compare three options:
> 
> Plan 1: this CL (https://codereview.chromium.org/177733003/)
> Plan 2: https://codereview.chromium.org/185403004/
> Plan 3: https://codereview.chromium.org/185593004/
> 
> None of them are complete, but has minimal code to show how intercept works in
> each case. In my rough local testing (with actual wiring part) any of them
seem
> to work for our use case without visible breakages.
> 
> Overall I prefer 2 -> 1 -> 3 (best to worst), though I'm biased by the amount
of
> code I needed to add in my attempts. My plumbing for plan 3 could be worse
than
> optimal but in general it'd need a lot plumbing around Profile/ProfileIOData.
> 
> I hope we can decide one of them in a day or two.

[kinuko, 2014-03-04 04:52:38]

On 2014/03/03 15:57:03, mmenke wrote:
> About approach 1:  The URLRequest can't own the context.  The context has to
be
> destroyed after the URLRequest, since the request references Context objects. 
> Contexts have a CHECK to ensure this, since this is a long standing source of
> bugs.

Yup, and attaching the context to URLRequest's base class (i.e.
SupportsUserData) did a trick for me to make it work-- but I wouldn't stick to
the plan 1 any more (if we keep option 2).

> Thanks for the code for the three approaches!
> 
> Your code for approach 1 doesn't go into the full details of actually plugging
> it in, but I think 2 is better, so let's stick with 2 or 3.
> 
> Want to spend some time digging through them, but I certainly don't want to
> block you on this, and agree that we should resolve this quickly.

Thanks, waiting for your pick :) (Meanwhile I'll start implementing
ServiceWorkerURLRequestJob etc)

[mmenke, 2014-03-04 17:51:34]

On 2014/03/04 04:52:38, kinuko wrote:
> On 2014/03/03 15:57:03, mmenke wrote:
> > About approach 1:  The URLRequest can't own the context.  The context has to
> be
> > destroyed after the URLRequest, since the request references Context
objects. 
> > Contexts have a CHECK to ensure this, since this is a long standing source
of
> > bugs.
> 
> Yup, and attaching the context to URLRequest's base class (i.e.
> SupportsUserData) did a trick for me to make it work-- but I wouldn't stick to
> the plan 1 any more (if we keep option 2).
> 
> > Thanks for the code for the three approaches!
> > 
> > Your code for approach 1 doesn't go into the full details of actually
plugging
> > it in, but I think 2 is better, so let's stick with 2 or 3.
> > 
> > Want to spend some time digging through them, but I certainly don't want to
> > block you on this, and agree that we should resolve this quickly.
> 
> Thanks, waiting for your pick :) (Meanwhile I'll start implementing
> ServiceWorkerURLRequestJob etc)

As completely hideous as all the boilerplate is, I still think we should go with
3.  I don't like adding yet another way of doing things for 2, and I think we'd
also want another class to do it right (I think we'd want another
interceptor-type class, rather than a class that wraps either its own or a new
URLRequestJobFactory).

I suggest landing all the boilerplate separate from the
service_worker_request_interceptor class.  Since it goes through a bunch of
classes, can send that out quickly, which should hopefully minimize the pain.

[kinuko, 2014-03-05 04:38:29]

> As completely hideous as all the boilerplate is, I still think we should go
with
> 3.  I don't like adding yet another way of doing things for 2, and I think
we'd
> also want another class to do it right (I think we'd want another
> interceptor-type class, rather than a class that wraps either its own or a new
> URLRequestJobFactory).

O...key, sure we can go with plan 3, and hopefully other content consumers can
reuse the same pattern when necessayr.

> I suggest landing all the boilerplate separate from the
> service_worker_request_interceptor class.  Since it goes through a bunch of
> classes, can send that out quickly, which should hopefully minimize the pain.

Makes sense, will split the patch and re-upload for reviews.