[Proof-of-concept] PlzNavigate and Service Worker

[Proof-of-concept] PlzNavigate and Service Worker

BUG=440463

[horo, 2015-07-24 11:35:39]

Patchset #5 (id:80001) has been deleted

[horo, 2015-07-24 11:35:49]

Patchset #1 (id:1) has been deleted

[horo, 2015-07-24 11:35:54]

Patchset #1 (id:20001) has been deleted

[horo, 2015-07-24 11:35:59]

Patchset #1 (id:40001) has been deleted

[michaeln, 2015-07-24 22:22:27]

https://codereview.chromium.org/1257553002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/1257553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_context_core.h:167: const GURL&
response_stream_url,
This tripped me up a little, at first glance i thought "there's no way the url
is a good identifier, what about concurrent navs to the same url". Only after
looking more closely at the usage did it make sense.

At this level, the basis of the 'key' used to identify these providerhost is
unimportant, maybe consider making it a std::string type and renaming the param
to 'key'.

https://codereview.chromium.org/1257553002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/1257553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_request_handler.cc:69: if
(request->url().SchemeIsBlob()) {
Is there a more direct way of identifying plznavigate requests? Using the
request url is a little overloaded.

(aside: Wow, so as far as the loader logic in the renderer knows, its loading a
blob?)

[horo, 2015-07-27 08:59:54]

Patchset #2 (id:100001) has been deleted

[horo, 2015-07-27 09:11:17]

I stopped using the stream URL to identifying the plznavigate requests.
I added navigation_provider_id to FrameMsg_CommitNavigation and
ServiceWorkerHostMsg_ProviderCreated message.
And when navigation_provider_id is set,
ServiceWorkerDispatcherHost::OnProviderCreated will transfer the navigation
ProviderHost to the normal ProviderHost.

What do you think about this plan?

https://codereview.chromium.org/1257553002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.h (right):

https://codereview.chromium.org/1257553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_context_core.h:167: const GURL&
response_stream_url,
On 2015/07/24 22:22:26, michaeln wrote:
> This tripped me up a little, at first glance i thought "there's no way the url
> is a good identifier, what about concurrent navs to the same url". Only after
> looking more closely at the usage did it make sense.
> 
> At this level, the basis of the 'key' used to identify these providerhost is
> unimportant, maybe consider making it a std::string type and renaming the
param
> to 'key'.

Removed this argument.

https://codereview.chromium.org/1257553002/diff/60001/content/browser/service...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/1257553002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_request_handler.cc:69: if
(request->url().SchemeIsBlob()) {
On 2015/07/24 22:22:27, michaeln wrote:
> Is there a more direct way of identifying plznavigate requests? Using the
> request url is a little overloaded.
> 
> (aside: Wow, so as far as the loader logic in the renderer knows, its loading
a
> blob?)

Removed these code from ServiceWorkerRequestHandler::InitializeHandler().

[horo, 2015-07-29 02:02:48]

horo@chromium.org changed reviewers:
+ michaeln@chromium.org

[horo, 2015-07-29 02:02:49]

+Reviewers: michaeln@

Ah, the email for the comment might not be sent.

[michaeln, 2015-07-30 01:35:08]

I think it be good if clamy or somebody from the core plznav team could take a
look too.

https://codereview.chromium.org/1257553002/diff/120001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1257553002/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2010:
ServiceWorkerRequestHandler::InitializeNavigationHandler(
Would it be help to identify the request with the existing request_id_ instead
of making up a new id type.

  // Request ID for browser initiated requests. request_ids generated by
  // child processes are counted up from 0, while browser created requests
  // start at -2 and go down from there. (We need to start at -2 because -1 is
  // used as a special value all over the resource_dispatcher_host for
  // uninitialized variables.) This way, we no longer have the unlikely (but
  // observed in the real world!) event where we have two requests with the same
  // request_id_.
  int request_id_;

I'm wondering using that value to eliminate/reduce the need for plumbing a new
'id' around? When the target renderer finally gets around the Committing the nav
request, does it have access to this existing request_id_ value already w/o any
new plumbing being needed?

Is this request_id_ value amoungst the many params in the various
NavigationParams structures, and if not can it be?

https://codereview.chromium.org/1257553002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/1257553002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:701: int
navigation_provider_id) {
i don't like long names but 'nav_provider_id' is very generic and i can imagine
that random sw engineer will have a hard time figuring out what the int value
refers too when seeing it here and there (and there and there). i wish
'plznavigate' were actually used in symbol names because that term is very
informative, i see its used in comments everywhere, if it were used for
class/variable naming lots of the comments wouldn't be needed.

anyway, long winded way of saying... consider renaming this param to something
more informative like 'plznavigate_service_worker_provider_id' through out

alternatively, see other comments about using 'request_id_' as the basis of
identifying plaz navigate reques, and adopting the swproviderhost at InitHandler
time.

https://codereview.chromium.org/1257553002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:721:
DCHECK(navigation_provider_host);
we can't trust these values are legit, !nav_provider_host here probably warrants
a bad_message::ReceivedBadMessage call

https://codereview.chromium.org/1257553002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/1257553002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler.cc:56: void
ServiceWorkerRequestHandler::InitializeHandler(
InitializeHandler could be an alternative place to adopt the
ServiceWorkerProviderHosts created in InitializeNavigationHandler. Provided
would could identify the original nav_request_id at this time, and lookup the
plznav_provider_host given that id.

I'm wondering if we can reduce the id value plumbing?

https://codereview.chromium.org/1257553002/diff/120001/content/child/resource...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/1257553002/diff/120001/content/child/resource...
content/child/resource_dispatcher.cc:768: if ((request_info.referrer.policy ==
blink::WebReferrerPolicyDefault ||
Would it make sense to explicitly use a different more lax referrer policy for
for plznavigate requests?

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

https://codereview.chromium.org/1257553002/diff/120001/content/common/frame_m...
content/common/frame_messages.h:638: int /* navigation_provider_id */)
There are many existing params values related to navigations, i'm not so
familiar with the varuios NavigationParams structs (CommonNavParams,
StartNavigationParams, or RequestNavigationParams). Would it make sense to
incorporate this new value into one of those existing param structures instead
of tacking on the additional parameter here and elsewhere?

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

https://codereview.chromium.org/1257553002/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4465: pending_navigation_params_.reset(
According to the comments about this data member, seems like this struct might
be a better place to store the new magic number needed inside of
didCreateDataSource().

  // Temporarily holds state pertaining to a navigation that has been initiated
  // until the NavigationState corresponding to the new navigation is created in
  // didCreateDataSource().
  scoped_ptr<NavigationParams> pending_navigation_params_;

So the new id value is received by this class in OnCommitNavigation, stored by
this class in pending_navigation_params_, and then finally used in
didCreateDataSource. I think that could reduce the scope of 'things' that need
to be aware of the new value, wdyt?

[horo, 2015-07-30 11:05:41]

+Reviewers: clamy@

clamy@
Do you have any idea about what is the best way to identify the
ServiceWorkerProviderHost when the message (ServiceWorkerHostMsg_ProviderCreated
or ResourceHostMsg_RequestResource) come from the renderer?

We have to create a ServiceWorkerProviderHost in the browser process before
starting the browser-initiatide fetch request.
This ServiceWorkerProviderHost will handle the request and decide whether to
pass the request to ServiceWorker.
And ServiceWorkerHostMsg_ProviderCreated is sent when
ServiceWorkerNetworkProvider is created in the renderer process.

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

https://codereview.chromium.org/1257553002/diff/120001/content/common/frame_m...
content/common/frame_messages.h:638: int /* navigation_provider_id */)
On 2015/07/30 01:35:08, michaeln wrote:
> There are many existing params values related to navigations, i'm not so
> familiar with the varuios NavigationParams structs (CommonNavParams,
> StartNavigationParams, or RequestNavigationParams). Would it make sense to
> incorporate this new value into one of those existing param structures instead
> of tacking on the additional parameter here and elsewhere?

Humm.. I am also not familiar with the NavigationParams structs.

[horo, 2015-07-30 11:05:48]

horo@chromium.org changed reviewers:
+ clamy@chromium.org

[horo, 2015-07-30 11:05:49]

+Reviewers: clamy@

clamy@
Do you have any idea about what is the best way to identify the
ServiceWorkerProviderHost when the message (ServiceWorkerHostMsg_ProviderCreated
or ResourceHostMsg_RequestResource) come from the renderer?

We have to create a ServiceWorkerProviderHost in the browser process before
starting the browser-initiatide fetch request.
This ServiceWorkerProviderHost will handle the request and decide whether to
pass the request to ServiceWorker.
And ServiceWorkerHostMsg_ProviderCreated is sent when
ServiceWorkerNetworkProvider is created in the renderer process.

[Fabrice (no longer in Chrome), 2015-07-30 16:47:37]

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

[Fabrice (no longer in Chrome), 2015-08-06 15:39:15]

Sorry for the late answer, clamy is on vacation until the end of the month and I
was sheriffing. I put some answers inline. I am not clear on what
navigation_provider_id refers to and there might be better ways around it but I
would like to make sure what it is used for first.

https://codereview.chromium.org/1257553002/diff/120001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1257553002/diff/120001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:2012: false,  //
TODO(horo): Add skip_service_worker to NavigationRequestInfo.
I think this should be added to BeginNavigationParams and initialized to false
for browser-initiated navigations.

https://codereview.chromium.org/1257553002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/1257553002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host.cc:701: int
navigation_provider_id) {
On 2015/07/30 01:35:08, michaeln wrote:
> i don't like long names but 'nav_provider_id' is very generic and i can
imagine
> that random sw engineer will have a hard time figuring out what the int value
> refers too when seeing it here and there (and there and there). i wish
> 'plznavigate' were actually used in symbol names because that term is very
> informative, i see its used in comments everywhere, if it were used for
> class/variable naming lots of the comments wouldn't be needed.
> 
> anyway, long winded way of saying... consider renaming this param to something
> more informative like 'plznavigate_service_worker_provider_id' through out
> 
> alternatively, see other comments about using 'request_id_' as the basis of
> identifying plaz navigate reques, and adopting the swproviderhost at
InitHandler
> time.

Not using PlzNavigate prefix in variable/class names is a design choice as
PlzNavigate will entirely replace current navigations so please do not use that.
I agree this can be confusing in the meantime while we are transitioning and
have to support both code paths.
I'm not clear on what navigation_

https://codereview.chromium.org/1257553002/diff/120001/content/browser/servic...
File content/browser/service_worker/service_worker_request_handler.cc (right):

https://codereview.chromium.org/1257553002/diff/120001/content/browser/servic...
content/browser/service_worker/service_worker_request_handler.cc:56: void
ServiceWorkerRequestHandler::InitializeHandler(
On 2015/07/30 01:35:08, michaeln wrote:
> InitializeHandler could be an alternative place to adopt the
> ServiceWorkerProviderHosts created in InitializeNavigationHandler. Provided
> would could identify the original nav_request_id at this time, and lookup the
> plznav_provider_host given that id.
> 
> I'm wondering if we can reduce the id value plumbing?

We should avoid duplicating code. I think the proper way of handling this would
be to add if blocks with different code paths for PlzNavigate and current
navigation.

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

https://codereview.chromium.org/1257553002/diff/120001/content/common/frame_m...
content/common/frame_messages.h:638: int /* navigation_provider_id */)
On 2015/07/30 11:05:41, horo wrote:
> On 2015/07/30 01:35:08, michaeln wrote:
> > There are many existing params values related to navigations, i'm not so
> > familiar with the varuios NavigationParams structs (CommonNavParams,
> > StartNavigationParams, or RequestNavigationParams). Would it make sense to
> > incorporate this new value into one of those existing param structures
instead
> > of tacking on the additional parameter here and elsewhere?
> 
> Humm.. I am also not familiar with the NavigationParams structs.

CommonNavigationParams are for parameters needed by all navigations.
BeginNavigationParams are for extra parameters needed by renderer-inititated
navigations.

In our case, I think the argument should be in CommonNavigationParams and the
parameters passed around in this CL should be refactored as such.

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

https://codereview.chromium.org/1257553002/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4465: pending_navigation_params_.reset(
On 2015/07/30 01:35:08, michaeln wrote:
> According to the comments about this data member, seems like this struct might
> be a better place to store the new magic number needed inside of
> didCreateDataSource().
> 
>   // Temporarily holds state pertaining to a navigation that has been
initiated
>   // until the NavigationState corresponding to the new navigation is created
in
>   // didCreateDataSource().
>   scoped_ptr<NavigationParams> pending_navigation_params_;
> 
> So the new id value is received by this class in OnCommitNavigation, stored by
> this class in pending_navigation_params_, and then finally used in
> didCreateDataSource. I think that could reduce the scope of 'things' that need
> to be aware of the new value, wdyt?

Yes, CommonNavigationParams seems like a better use for this. It is a member of
NavigationParams.

[carlosk, 2015-08-06 16:52:51]

carlosk@chromium.org changed reviewers:
+ carlosk@chromium.org, nasko@chromium.org

[carlosk, 2015-08-06 16:52:52]

As Camille is out I am adding nasko@ as a reviewer of this change as he is the
main reviewer of all PlzNavigate code, especially in content. I think he'll be
able to help with most of the answers you are looking for.

[nasko, 2015-08-07 17:51:12]

Overall, this CL needs a high level description of what is being changed and
how. I don't quite understand what the new navigation_provider_id is and why it
is introduced.

Also, adding davidben@ as a reviewer will be very good thing to do, once the CL
gets closer to committable code.

https://codereview.chromium.org/1257553002/diff/120001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1257553002/diff/120001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:434: int
navigation_provider_id);
Why does CommitNavigation need to know that the response came from SW?

https://codereview.chromium.org/1257553002/diff/120001/content/child/resource...
File content/child/resource_dispatcher.cc (right):

https://codereview.chromium.org/1257553002/diff/120001/content/child/resource...
content/child/resource_dispatcher.cc:768: if ((request_info.referrer.policy ==
blink::WebReferrerPolicyDefault ||
On 2015/07/30 01:35:08, michaeln wrote:
> Would it make sense to explicitly use a different more lax referrer policy for
> for plznavigate requests?

What is the rationale for that? There should be nothing different whether the
request is made by existing codepaths or by PlzNavigate.

https://codereview.chromium.org/1257553002/diff/120001/content/child/resource...
content/child/resource_dispatcher.cc:799: // PlzNavigate: Hack to avoid the
referrer check failure in browser process.
What is this failure?

https://codereview.chromium.org/1257553002/diff/120001/content/child/service_...
File content/child/service_worker/service_worker_network_provider.h (right):

https://codereview.chromium.org/1257553002/diff/120001/content/child/service_...
content/child/service_worker/service_worker_network_provider.h:44: int
navigation_provider_id);
navigation_provider_id needs a fat comment. As it is, it tells me very little,
if anything :(.

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

https://codereview.chromium.org/1257553002/diff/120001/content/common/frame_m...
content/common/frame_messages.h:638: int /* navigation_provider_id */)
On 2015/08/06 15:39:15, Fabrice wrote:
> On 2015/07/30 11:05:41, horo wrote:
> > On 2015/07/30 01:35:08, michaeln wrote:
> > > There are many existing params values related to navigations, i'm not so
> > > familiar with the varuios NavigationParams structs (CommonNavParams,
> > > StartNavigationParams, or RequestNavigationParams). Would it make sense to
> > > incorporate this new value into one of those existing param structures
> instead
> > > of tacking on the additional parameter here and elsewhere?
> > 
> > Humm.. I am also not familiar with the NavigationParams structs.
> 
> CommonNavigationParams are for parameters needed by all navigations.
> BeginNavigationParams are for extra parameters needed by renderer-inititated
> navigations.
> 
> In our case, I think the argument should be in CommonNavigationParams and the
> parameters passed around in this CL should be refactored as such.

Do we need to carry the provider ID from the browser to the renderer? If not,
then it probably doesn't belong in Common*, but rather in
RequestNavigationParams.

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

https://codereview.chromium.org/1257553002/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4473: common_params, stream_params.Pass(),
navigation_provider_id,
Why do we need to pass this? The stream is the result of what ServiceWorker
returned to us (if it kicked in), so adding it again will likely not do
anything. It will be even worse if SW is engaged on requesting the response from
a SW.

[kinuko, 2015-08-11 14:43:04]

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

[kinuko, 2015-08-11 14:43:05]

Hi, 

Looks like most are on vacation now :)  Anyways since horo's bit busy for other
high-priority tasks let me respond to some of the questions instead.  (By the
way this patch's intended to be a 'proof-of-concept' as its title implies,
there're several hacks exist where we just wanted to let tests pass or where we
aren’t sure what to do)

Reg: the 'fat comments about the navigation_provider_id', let me try to explain
why we need it and why this patch tries to propagate the ID to the renderer
along with the navigation request.

In short: what we need to achieve is to set up pair objects in the browser
process and renderer process (i.e. ServiceWorkerProviderHost in browser and
ServiceWorkerProvider in renderer) for a page, and we use an ID called
'provider_id' to identify the pair.  The renderer-side object (i.e.
ServiceWorkerProvider) needs to be associated to the page’s DocumentLoader too,
and the pair objects cooperatively work on all resource loading requests for the
page.

In the current world where all loading requests are initiated from renderer
process we set-up a pair of ServiceWorkerProvider(Host) in a following way:

------------
1) Loading a document always starts in the renderer, and we create a new
ServiceWorkerProvider object for the document when a document loader is created.
 We generate a process-unique 'provider_id' here and assign it to the
ServiceWorkerProvider.

2) We send this 'provider_id' to the browser's SW code where we create
corresponding ServiceWorkerProviderHost.  We maintain a map from 'provider_id'
to associated ServiceWorkerProviderHost, so that we can find out corresponding
ServiceWorkerProviderHost from a given 'provider_id' in the browser process.

3) In the renderer process we attach the 'provider_id' created in 1) to every
resource request initiated by the document (including the document loading
request itself).

4) In the browser process we intercept all loading requests to look up
corresponding ServiceWorkerProviderHost using the 'provider_id' embedded in the
request in 3), and the ServiceWorkerProviderHost will handle those requests.
------------

However the sequence doesn't work well in browser-initiated navigation since
step 1) can't happen before we create loading request in the browser process
(i.e. in BeginNavigationRequest).  So this patch introduce another ID to
identify the pair, i.e. ‘navigation_provider_id’, which is created in the
browser process first where we create ServiceWorkerProviderHost, and then
propagate the ID to the renderer process along with the navigation commit IPC to
set up a corresponding ServiceWorkerProvider in the renderer process.

If we focus only on the ServiceWorker flow, the sequence for renderer-initiated
loading is (note: the capital B and R indicates browser process and renderer
process):

[R] SWProvider is created with a new unique ‘provider_id’
[B] Corresponding SWProviderHost is created with the same ‘provider_id’
[R] Renderer sends a resource load request with ‘provider_id’
[B] SWProviderHost handles requests which has the same ‘provider_id’

While this patch tries to achieve the same in the opposite order:

[B] SWProviderHost is created with a new unique ‘navigation_provider_id’ (in
BeginNavigationRequest)
[B] SWProviderHost handles the loading request for the navigation
[R] ‘navigation_provider_id’ is sent to the renderer when the navigation’s
committed, which is embedded in the loading request in NavigateInternal
[R] Corresponding SWProvider is created with the same ‘navigation_provider_id’
(and with a new ‘provider_id’) when the navigation’s committed (i.e. in
NavigateInternal).  'The navigation_provider_id’ and newly created ‘provider_id’
are sent to the browser.
[B] SWProviderHost with the same ‘navigation_provider_id’ is fully initialized
with the ‘provider_id’ created in 3, so that after this point all resource
requests are handled in the same way as before using the renderer-created
‘provider_id’.

So the ‘navigation_provider_id’ is used to identify SWProviderHost for the
navigation until we fully set-up the pair between the SWProviderHost and
corresponding SWProvider in the renderer process.  As michaeln@ noted we might
be able to remove some of these ID plumbing by utilizing existing request_id_,
since all we need to do is to identify the same request in the browser and in
the renderer.

I'm not sure if I'm explaining this well, but hope this helps.  I can also draft
a brief doc with some figures to describe the design if it helps.  As you
already see horo@'s doc https://goo.gl/dFLaAa also has a lot of details about
the implementation.

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

https://codereview.chromium.org/1257553002/diff/120001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4473: common_params, stream_params.Pass(),
navigation_provider_id,
On 2015/08/07 17:51:12, nasko (very slow) wrote:
> Why do we need to pass this? The stream is the result of what ServiceWorker
> returned to us (if it kicked in), so adding it again will likely not do
> anything. It will be even worse if SW is engaged on requesting the response
from
> a SW.

This is mainly to associate browser-side SW object (i.e. SWProviderHost, which
is identified by nav_provider_id) and renderer-side SW object (i.e. SWProvider,
which is created later in RenderFrameImpl::didCreateDataSource).

We embed nav_provider_id here in the request so that later in
RenderFrameImpl::didCreateDataSource (which is called when the document loader
is created in blink) we can create SWProvider with the same nav_provider_id.

[kinuko, 2015-08-12 03:15:07]

Hi, by the way once we all agreed on the basic design would it be possible
for some of you (in PlzNavigate) to take over this patch?  We're willing to
continue design discussion and following up as much as possible, but we're
getting a bit short of hands in terms of actual coding.


On Tue, Aug 11, 2015 at 11:43 PM, <kinuko@chromium.org> wrote:

> Hi,
>
> Looks like most are on vacation now :)  Anyways since horo's bit busy for
> other
> high-priority tasks let me respond to some of the questions instead.  (By
> the
> way this patch's intended to be a 'proof-of-concept' as its title implies,
> there're several hacks exist where we just wanted to let tests pass or
> where we
> aren’t sure what to do)
>
> Reg: the 'fat comments about the navigation_provider_id', let me try to
> explain
> why we need it and why this patch tries to propagate the ID to the renderer
> along with the navigation request.
>
> In short: what we need to achieve is to set up pair objects in the browser
> process and renderer process (i.e. ServiceWorkerProviderHost in browser and
> ServiceWorkerProvider in renderer) for a page, and we use an ID called
> 'provider_id' to identify the pair.  The renderer-side object (i.e.
> ServiceWorkerProvider) needs to be associated to the page’s DocumentLoader
> too,
> and the pair objects cooperatively work on all resource loading requests
> for the
> page.
>
> In the current world where all loading requests are initiated from renderer
> process we set-up a pair of ServiceWorkerProvider(Host) in a following way:
>
> ------------
> 1) Loading a document always starts in the renderer, and we create a new
> ServiceWorkerProvider object for the document when a document loader is
> created.
>  We generate a process-unique 'provider_id' here and assign it to the
> ServiceWorkerProvider.
>
> 2) We send this 'provider_id' to the browser's SW code where we create
> corresponding ServiceWorkerProviderHost.  We maintain a map from
> 'provider_id'
> to associated ServiceWorkerProviderHost, so that we can find out
> corresponding
> ServiceWorkerProviderHost from a given 'provider_id' in the browser
> process.
>
> 3) In the renderer process we attach the 'provider_id' created in 1) to
> every
> resource request initiated by the document (including the document loading
> request itself).
>
> 4) In the browser process we intercept all loading requests to look up
> corresponding ServiceWorkerProviderHost using the 'provider_id' embedded
> in the
> request in 3), and the ServiceWorkerProviderHost will handle those
> requests.
> ------------
>
> However the sequence doesn't work well in browser-initiated navigation
> since
> step 1) can't happen before we create loading request in the browser
> process
> (i.e. in BeginNavigationRequest).  So this patch introduce another ID to
> identify the pair, i.e. ‘navigation_provider_id’, which is created in the
> browser process first where we create ServiceWorkerProviderHost, and then
> propagate the ID to the renderer process along with the navigation commit
> IPC to
> set up a corresponding ServiceWorkerProvider in the renderer process.
>
> If we focus only on the ServiceWorker flow, the sequence for
> renderer-initiated
> loading is (note: the capital B and R indicates browser process and
> renderer
> process):
>
> [R] SWProvider is created with a new unique ‘provider_id’
> [B] Corresponding SWProviderHost is created with the same ‘provider_id’
> [R] Renderer sends a resource load request with ‘provider_id’
> [B] SWProviderHost handles requests which has the same ‘provider_id’
>
> While this patch tries to achieve the same in the opposite order:
>
> [B] SWProviderHost is created with a new unique ‘navigation_provider_id’
> (in
> BeginNavigationRequest)
> [B] SWProviderHost handles the loading request for the navigation
> [R] ‘navigation_provider_id’ is sent to the renderer when the navigation’s
> committed, which is embedded in the loading request in NavigateInternal
> [R] Corresponding SWProvider is created with the same
> ‘navigation_provider_id’
> (and with a new ‘provider_id’) when the navigation’s committed (i.e. in
> NavigateInternal).  'The navigation_provider_id’ and newly created
> ‘provider_id’
> are sent to the browser.
> [B] SWProviderHost with the same ‘navigation_provider_id’ is fully
> initialized
> with the ‘provider_id’ created in 3, so that after this point all resource
> requests are handled in the same way as before using the renderer-created
> ‘provider_id’.
>
> So the ‘navigation_provider_id’ is used to identify SWProviderHost for the
> navigation until we fully set-up the pair between the SWProviderHost and
> corresponding SWProvider in the renderer process.  As michaeln@ noted we
> might
> be able to remove some of these ID plumbing by utilizing existing
> request_id_,
> since all we need to do is to identify the same request in the browser and
> in
> the renderer.
>
> I'm not sure if I'm explaining this well, but hope this helps.  I can also
> draft
> a brief doc with some figures to describe the design if it helps.  As you
> already see horo@'s doc https://goo.gl/dFLaAa also has a lot of details
> about
> the implementation.
>
>
>
>
https://codereview.chromium.org/1257553002/diff/120001/content/renderer/rende...
> File content/renderer/render_frame_impl.cc (right):
>
>
>
https://codereview.chromium.org/1257553002/diff/120001/content/renderer/rende...
> content/renderer/render_frame_impl.cc:4473: common_params,
> stream_params.Pass(), navigation_provider_id,
> On 2015/08/07 17:51:12, nasko (very slow) wrote:
>
>> Why do we need to pass this? The stream is the result of what
>>
> ServiceWorker
>
>> returned to us (if it kicked in), so adding it again will likely not
>>
> do
>
>> anything. It will be even worse if SW is engaged on requesting the
>>
> response from
>
>> a SW.
>>
>
> This is mainly to associate browser-side SW object (i.e. SWProviderHost,
> which is identified by nav_provider_id) and renderer-side SW object
> (i.e. SWProvider, which is created later in
> RenderFrameImpl::didCreateDataSource).
>
> We embed nav_provider_id here in the request so that later in
> RenderFrameImpl::didCreateDataSource (which is called when the document
> loader is created in blink) we can create SWProvider with the same
> nav_provider_id.
>
> https://codereview.chromium.org/1257553002/
>

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

[Fabrice (no longer in Chrome), 2015-08-17 15:20:24]

On 2015/08/11 14:43:05, kinuko wrote:
> Hi, 
> 
> Looks like most are on vacation now :)  Anyways since horo's bit busy for
other
> high-priority tasks let me respond to some of the questions instead.  (By the
> way this patch's intended to be a 'proof-of-concept' as its title implies,
> there're several hacks exist where we just wanted to let tests pass or where
we
> aren’t sure what to do)
> 
> Reg: the 'fat comments about the navigation_provider_id', let me try to
explain
> why we need it and why this patch tries to propagate the ID to the renderer
> along with the navigation request.
> 
> In short: what we need to achieve is to set up pair objects in the browser
> process and renderer process (i.e. ServiceWorkerProviderHost in browser and
> ServiceWorkerProvider in renderer) for a page, and we use an ID called
> 'provider_id' to identify the pair.  The renderer-side object (i.e.
> ServiceWorkerProvider) needs to be associated to the page’s DocumentLoader
too,
> and the pair objects cooperatively work on all resource loading requests for
the
> page.
> 
> In the current world where all loading requests are initiated from renderer
> process we set-up a pair of ServiceWorkerProvider(Host) in a following way:
> 
> ------------
> 1) Loading a document always starts in the renderer, and we create a new
> ServiceWorkerProvider object for the document when a document loader is
created.
>  We generate a process-unique 'provider_id' here and assign it to the
> ServiceWorkerProvider.
> 
> 2) We send this 'provider_id' to the browser's SW code where we create
> corresponding ServiceWorkerProviderHost.  We maintain a map from 'provider_id'
> to associated ServiceWorkerProviderHost, so that we can find out corresponding
> ServiceWorkerProviderHost from a given 'provider_id' in the browser process.
> 
> 3) In the renderer process we attach the 'provider_id' created in 1) to every
> resource request initiated by the document (including the document loading
> request itself).
> 
> 4) In the browser process we intercept all loading requests to look up
> corresponding ServiceWorkerProviderHost using the 'provider_id' embedded in
the
> request in 3), and the ServiceWorkerProviderHost will handle those requests.
> ------------
> 
> However the sequence doesn't work well in browser-initiated navigation since
> step 1) can't happen before we create loading request in the browser process
> (i.e. in BeginNavigationRequest).  So this patch introduce another ID to
> identify the pair, i.e. ‘navigation_provider_id’, which is created in the
> browser process first where we create ServiceWorkerProviderHost, and then
> propagate the ID to the renderer process along with the navigation commit IPC
to
> set up a corresponding ServiceWorkerProvider in the renderer process.
> 
> If we focus only on the ServiceWorker flow, the sequence for
renderer-initiated
> loading is (note: the capital B and R indicates browser process and renderer
> process):
> 
> [R] SWProvider is created with a new unique ‘provider_id’
> [B] Corresponding SWProviderHost is created with the same ‘provider_id’
> [R] Renderer sends a resource load request with ‘provider_id’
> [B] SWProviderHost handles requests which has the same ‘provider_id’
> 
> While this patch tries to achieve the same in the opposite order:
> 
> [B] SWProviderHost is created with a new unique ‘navigation_provider_id’ (in
> BeginNavigationRequest)
> [B] SWProviderHost handles the loading request for the navigation
> [R] ‘navigation_provider_id’ is sent to the renderer when the navigation’s
> committed, which is embedded in the loading request in NavigateInternal
> [R] Corresponding SWProvider is created with the same ‘navigation_provider_id’
> (and with a new ‘provider_id’) when the navigation’s committed (i.e. in
> NavigateInternal).  'The navigation_provider_id’ and newly created
‘provider_id’
> are sent to the browser.
> [B] SWProviderHost with the same ‘navigation_provider_id’ is fully initialized
> with the ‘provider_id’ created in 3, so that after this point all resource
> requests are handled in the same way as before using the renderer-created
> ‘provider_id’.
> 
> So the ‘navigation_provider_id’ is used to identify SWProviderHost for the
> navigation until we fully set-up the pair between the SWProviderHost and
> corresponding SWProvider in the renderer process.  As michaeln@ noted we might
> be able to remove some of these ID plumbing by utilizing existing request_id_,
> since all we need to do is to identify the same request in the browser and in
> the renderer.
> 
> I'm not sure if I'm explaining this well, but hope this helps.  I can also
draft
> a brief doc with some figures to describe the design if it helps.  As you
> already see horo@'s doc https://goo.gl/dFLaAa also has a lot of details about
> the implementation.

Thanks for the detailed answer! Sorry I haven't been able to reply earlier.
I understand what you are trying to achieve now. I suggest an alternative
implementation. Could we re-use the existing provider_id and initialize it with
negative values for browser-initiated navigations? We'd avoid a lot of
argument-adding around. The value gets changed when the SWProvider is actually
created in the renderer. It then notifies the browser of the old and new values.
WDYT?
Regarding taking over the patch, I can do that but I am going to need you to
help review it and get it to a commitable state.

[Fabrice (no longer in Chrome), 2015-08-17 15:41:53]

On 2015/08/17 15:20:24, Fabrice wrote:
> [SNIP]
> 
> Thanks for the detailed answer! Sorry I haven't been able to reply earlier.
> I understand what you are trying to achieve now. I suggest an alternative
> implementation. Could we re-use the existing provider_id and initialize it
with
> negative values for browser-initiated navigations? We'd avoid a lot of
> argument-adding around. The value gets changed when the SWProvider is actually
> created in the renderer. It then notifies the browser of the old and new
values.
> WDYT?
> Regarding taking over the patch, I can do that but I am going to need you to
> help review it and get it to a commitable state.

I had an offline conversation with carlosk@ about that, he suggested we could
simply keep the negative ID around instead of updating it. As I understand it,
that should not be a problem. provider_id would still be unique per renderer. In
fact, in this case, negative IDs would be globally unique too.

[michaeln, 2015-08-17 20:40:35]

On 2015/08/17 15:41:53, Fabrice wrote:
> On 2015/08/17 15:20:24, Fabrice wrote:
> > [SNIP]
> > 
> > Thanks for the detailed answer! Sorry I haven't been able to reply earlier.
> > I understand what you are trying to achieve now. I suggest an alternative
> > implementation. Could we re-use the existing provider_id and initialize it
> with
> > negative values for browser-initiated navigations? We'd avoid a lot of
> > argument-adding around. The value gets changed when the SWProvider is
actually
> > created in the renderer. It then notifies the browser of the old and new
> values.
> > WDYT?
> > Regarding taking over the patch, I can do that but I am going to need you to
> > help review it and get it to a commitable state.
> 
> I had an offline conversation with carlosk@ about that, he suggested we could
> simply keep the negative ID around instead of updating it. As I understand it,
> that should not be a problem. provider_id would still be unique per renderer.
In
> fact, in this case, negative IDs would be globally unique too.

Appcache has a very similar structure and will need to identify a pre-existing
AppCacheHost in some way similar to how a pre-existing ServiceWorkerProviderHost
is identified. It'd be nice to pass a single id that identifies the 'navigation'
instead of passing extra serviceworker and appcache specific ids around in the
set of (common?) navigation params. I think the Both the sw and appcache could
key their pre-existing collection of <foo>Hosts created for browser initiated
navs on that value and the navivation params would not need to carry any sw or
appcache specific values. Based on this comment, it looks like
ResourceDispatcherHostImpl.request_id_ could serve this purpose.

  // Request ID for browser initiated requests. request_ids generated by
  // child processes are counted up from 0, while browser created requests
  // start at -2 and go down from there. (We need to start at -2 because -1 is
  // used as a special value all over the resource_dispatcher_host for
  // uninitialized variables.) This way, we no longer have the unlikely (but
  // observed in the real world!) event where we have two requests with the same
  // request_id_.
  int request_id_;

[nasko, 2015-08-17 22:46:14]

On 2015/08/17 20:40:35, michaeln wrote:
> On 2015/08/17 15:41:53, Fabrice wrote:
> > On 2015/08/17 15:20:24, Fabrice wrote:
> > > [SNIP]
> > > 
> > > Thanks for the detailed answer! Sorry I haven't been able to reply
earlier.
> > > I understand what you are trying to achieve now. I suggest an alternative
> > > implementation. Could we re-use the existing provider_id and initialize it
> > with
> > > negative values for browser-initiated navigations? We'd avoid a lot of
> > > argument-adding around. The value gets changed when the SWProvider is
> actually
> > > created in the renderer. It then notifies the browser of the old and new
> > values.
> > > WDYT?
> > > Regarding taking over the patch, I can do that but I am going to need you
to
> > > help review it and get it to a commitable state.
> > 
> > I had an offline conversation with carlosk@ about that, he suggested we
could
> > simply keep the negative ID around instead of updating it. As I understand
it,
> > that should not be a problem. provider_id would still be unique per
renderer.
> In
> > fact, in this case, negative IDs would be globally unique too.
> 
> Appcache has a very similar structure and will need to identify a pre-existing
> AppCacheHost in some way similar to how a pre-existing
ServiceWorkerProviderHost
> is identified. It'd be nice to pass a single id that identifies the
'navigation'
> instead of passing extra serviceworker and appcache specific ids around in the
> set of (common?) navigation params. I think the Both the sw and appcache could
> key their pre-existing collection of <foo>Hosts created for browser initiated
> navs on that value and the navivation params would not need to carry any sw or
> appcache specific values. Based on this comment, it looks like
> ResourceDispatcherHostImpl.request_id_ could serve this purpose.
> 
>   // Request ID for browser initiated requests. request_ids generated by
>   // child processes are counted up from 0, while browser created requests
>   // start at -2 and go down from there. (We need to start at -2 because -1 is
>   // used as a special value all over the resource_dispatcher_host for
>   // uninitialized variables.) This way, we no longer have the unlikely (but
>   // observed in the real world!) event where we have two requests with the
same
>   // request_id_.
>   int request_id_;

I will be much happier with approach that doesn't end up adding yet more IDs to
already very complex code. If we can actually reuse the request_id_, that will
be ideal, though I haven't looked whether it is feasible.

Is there a reason why we can't have the browser process be responsible for
allocating those IDs? This is already the case today for a bunch of ids,
including subframes and window.open() created main frames. Since
ServiceWorkerHost mostly lives on IO thread (correct?), we most likely won't
even need a hop to the UI thread. In the browser-initiated case, the ID will be
passed down as part of the navigation commit.

[kinuko, 2015-08-18 13:37:54]

On 2015/08/17 22:46:14, nasko wrote:
> On 2015/08/17 20:40:35, michaeln wrote:
> > On 2015/08/17 15:41:53, Fabrice wrote:
> > > On 2015/08/17 15:20:24, Fabrice wrote:
> > > > [SNIP]
> > > > 
> > > > Thanks for the detailed answer! Sorry I haven't been able to reply
> earlier.
> > > > I understand what you are trying to achieve now. I suggest an
alternative
> > > > implementation. Could we re-use the existing provider_id and initialize
it
> > > with
> > > > negative values for browser-initiated navigations? We'd avoid a lot of
> > > > argument-adding around. The value gets changed when the SWProvider is
> > actually
> > > > created in the renderer. It then notifies the browser of the old and new
> > > values.
> > > > WDYT?
> > > > Regarding taking over the patch, I can do that but I am going to need
you
> to
> > > > help review it and get it to a commitable state.

Thanks Fabrice! And yes I'd love to continue discussing the design.

> > > I had an offline conversation with carlosk@ about that, he suggested we
> could
> > > simply keep the negative ID around instead of updating it. As I understand
> it,
> > > that should not be a problem. provider_id would still be unique per
> renderer.
> > In
> > > fact, in this case, negative IDs would be globally unique too.
> > 
> > Appcache has a very similar structure and will need to identify a
pre-existing
> > AppCacheHost in some way similar to how a pre-existing
> ServiceWorkerProviderHost
> > is identified. It'd be nice to pass a single id that identifies the
> 'navigation'
> > instead of passing extra serviceworker and appcache specific ids around in
the
> > set of (common?) navigation params. I think the Both the sw and appcache
could
> > key their pre-existing collection of <foo>Hosts created for browser
initiated
> > navs on that value and the navivation params would not need to carry any sw
or
> > appcache specific values. Based on this comment, it looks like
> > ResourceDispatcherHostImpl.request_id_ could serve this purpose.
> > 
> >   // Request ID for browser initiated requests. request_ids generated by
> >   // child processes are counted up from 0, while browser created requests
> >   // start at -2 and go down from there. (We need to start at -2 because -1
is
> >   // used as a special value all over the resource_dispatcher_host for
> >   // uninitialized variables.) This way, we no longer have the unlikely (but
> >   // observed in the real world!) event where we have two requests with the
> same
> >   // request_id_.
> >   int request_id_;
> 
> I will be much happier with approach that doesn't end up adding yet more IDs
to
> already very complex code. If we can actually reuse the request_id_, that will
> be ideal, though I haven't looked whether it is feasible.

I'm all for using less IDs too, as the idea sounds promising let's go for it.

> Is there a reason why we can't have the browser process be responsible for
> allocating those IDs? This is already the case today for a bunch of ids,
> including subframes and window.open() created main frames. Since
> ServiceWorkerHost mostly lives on IO thread (correct?), we most likely won't
> even need a hop to the UI thread. In the browser-initiated case, the ID will
be
> passed down as part of the navigation commit.

I don't think it is/was 'impossible', but in the existing architecture where
loading almost always starts from the renderer, the current choice is/was more
reasonable, as in this way we could just pick an ID in the renderer when a
loading is initiated and could use the same ID in the browser process from the
beginning of the main resource loading.

[Fabrice (no longer in Chrome), 2015-08-18 18:52:48]

On 2015/08/17 22:46:14, nasko wrote:
> On 2015/08/17 20:40:35, michaeln wrote:
> > On 2015/08/17 15:41:53, Fabrice wrote:
> > > On 2015/08/17 15:20:24, Fabrice wrote:
> > > > [SNIP]
> > > > 
> > > > Thanks for the detailed answer! Sorry I haven't been able to reply
> earlier.
> > > > I understand what you are trying to achieve now. I suggest an
alternative
> > > > implementation. Could we re-use the existing provider_id and initialize
it
> > > with
> > > > negative values for browser-initiated navigations? We'd avoid a lot of
> > > > argument-adding around. The value gets changed when the SWProvider is
> > actually
> > > > created in the renderer. It then notifies the browser of the old and new
> > > values.
> > > > WDYT?
> > > > Regarding taking over the patch, I can do that but I am going to need
you
> to
> > > > help review it and get it to a commitable state.
> > > 
> > > I had an offline conversation with carlosk@ about that, he suggested we
> could
> > > simply keep the negative ID around instead of updating it. As I understand
> it,
> > > that should not be a problem. provider_id would still be unique per
> renderer.
> > In
> > > fact, in this case, negative IDs would be globally unique too.
> > 
> > Appcache has a very similar structure and will need to identify a
pre-existing
> > AppCacheHost in some way similar to how a pre-existing
> ServiceWorkerProviderHost
> > is identified. It'd be nice to pass a single id that identifies the
> 'navigation'
> > instead of passing extra serviceworker and appcache specific ids around in
the
> > set of (common?) navigation params. I think the Both the sw and appcache
could
> > key their pre-existing collection of <foo>Hosts created for browser
initiated
> > navs on that value and the navivation params would not need to carry any sw
or
> > appcache specific values. Based on this comment, it looks like
> > ResourceDispatcherHostImpl.request_id_ could serve this purpose.
> > 
> >   // Request ID for browser initiated requests. request_ids generated by
> >   // child processes are counted up from 0, while browser created requests
> >   // start at -2 and go down from there. (We need to start at -2 because -1
is
> >   // used as a special value all over the resource_dispatcher_host for
> >   // uninitialized variables.) This way, we no longer have the unlikely (but
> >   // observed in the real world!) event where we have two requests with the
> same
> >   // request_id_.
> >   int request_id_;
> 
> I will be much happier with approach that doesn't end up adding yet more IDs
to
> already very complex code. If we can actually reuse the request_id_, that will
> be ideal, though I haven't looked whether it is feasible.

Quick update.
I started taking over this patch. We cannot reuse request_id_ because the
request_id_ is created after we create RequestNavigationParams /
CommonNavigationParams.

> Is there a reason why we can't have the browser process be responsible for
> allocating those IDs? This is already the case today for a bunch of ids,
> including subframes and window.open() created main frames. Since
> ServiceWorkerHost mostly lives on IO thread (correct?), we most likely won't
> even need a hop to the UI thread. In the browser-initiated case, the ID will
be
> passed down as part of the navigation commit.

I suppose that would be better in the long term. For now, I think we can get
away with the sw_provider_id being created by the browser in the case of
browser-initiated navigation in PlzNavigate. That's the approach I am currently
going with.

[Fabrice (no longer in Chrome), 2015-08-20 16:21:49]

New patch is here: https://codereview.chromium.org/1294243004/