Transfer serviceworker state during cross site navigations too.

Transfer serviceworker state during cross site navigations too.

The ServiceWorkerProviderHost is an object that persist across the lifetime of a document. It's created prior to the document and prior to the initiation of the resource load for the documents main resource. Today, when a cross-site xfer occurs the host is deleted and a new host is created that knows nothing of whats going on... so not surprisingly things are broken.

The change is to retain the original ServiceworkerProviderHost and transfer ownership of it to the process in which the document finally lands.

BUG=438005
Committed: https://crrev.com/bfea6ecef56cba8beb9816e8e9674a9dc456b20e
Cr-Commit-Position: refs/heads/master@{#307579}

[michaeln, 2014-11-11 01:55:40]

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

[michaeln, 2014-11-11 01:58:18]

i need to do testing yet, but here's the change

[michaeln, 2014-11-11 02:01:16]

and here's some history about why a change like this is needed

https://code.google.com/p/chromium/issues/detail?id=331270

[falken, 2014-11-11 08:38:57]

Code looks good though I'm not totally sure I understand the bug being fixed
(even after reading the CL description and appcache bug)...

Currently, if there's a redirect, Service Worker just doesn't work? What state
does SWProviderHost hold that we need to carry on to the redirected site... I'd
assume the redirect happens before we populate
controlling/active/waiting/installing version (or is that not the case for a
META redirect)? Is it correct behavior to carry the versions over in case of a
redirect?

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_context_core.cc:410: return
scoped_ptr<ServiceWorkerProviderHost>();
This pattern of gracefully handling a DCHECK()/NOTREACHED() failure is generally
disallowed by the style guide:
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

Just DCHECK() and continue is preferred. Is there a reason to make an exception
here?

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.cc:219: process_id_
= 0;
EmbeddedWorkerInstance uses -1 for "invalid process id", maybe it's time to add
a kInvalidProcessId constant?

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.h:139:
ServiceWorkerDispatcherHost* displatcher_host);
spelling: displatcher (sounds painful :)

[michaeln, 2014-11-11 21:14:10]

> Currently, if there's a redirect, Service Worker just doesn't work? What state
> does SWProviderHost hold that we need to carry on to the redirected site...

Sure normal redirection just works, but this isn't normal redirection. It's not
easy to see how it works, but for cross-site-navs, a Request is started in
processA and the resumed in processB. The 'handler' created for the request when
its started weakpoints to the 'providerhost' living in processA. When the
request is resumed in processB (see how UpdateRequestForTransfer fits in), the
existing request and existing 'handler' are used, but it weakpoints to a
providerhost that will have already been deleted at resume time w/o these
changes. These changes keep the providerhost from processA alive and carries it
over to the new processB.

I have to do some more testing to look more closely at what state maintained
within swph needs to make the leap (if a sw generated the redirect which caused
the cross-site-nav, we may have an associated sw at this time, not sure). But I
think not much. Another approach could be to update the handler to point to a
new host created for processB. I chose to xfer the host though mostly for
similarity with how cross-site-navs work for appcache.

[michaeln, 2014-11-13 00:03:55]

> I have to do some more testing to look more closely at what state maintained
> within swph needs to make the leap (if a sw generated the redirect which
caused
> the cross-site-nav, we may have an associated sw at this time, not sure). But
I
> think not much. Another approach could be to update the handler to point to a
> new host created for processB. I chose to xfer the host though mostly for
> similarity with how cross-site-navs work for appcache.

Earlier i said, "i think not much". I should have said "probably more than i
expect".

The StartCrossSiteTransition control flow is initiated upon receipt of the
response head of the final destination of a redirect chain. If a SW is the
agency that's producing the response, the document is associated to that SW and
the script could be pumping bytes into the 'stream' (if/when scripts can stream
response bodies back in that fashion). I'll need to account for that state at
UpdateRequestForTransfer time when the new process resumes receipt of the
response.

[michaeln, 2014-11-13 00:05:08]

no new upload yet, but done locally

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_context_core.cc:410: return
scoped_ptr<ServiceWorkerProviderHost>();
On 2014/11/11 08:38:57, falken wrote:
> This pattern of gracefully handling a DCHECK()/NOTREACHED() failure is
generally
> disallowed by the style guide:
>
http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-
> 
> Just DCHECK() and continue is preferred. Is there a reason to make an
exception
> here?

Done.

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.cc:219: process_id_
= 0;
Done

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_provider_host.h (right):

https://codereview.chromium.org/702843004/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_provider_host.h:139:
ServiceWorkerDispatcherHost* displatcher_host);
On 2014/11/11 08:38:57, falken wrote:
> spelling: displatcher (sounds painful :)

lol... but i absolutely prefer my spelling!

[michaeln, 2014-11-13 01:51:22]

https://codereview.chromium.org/702843004/diff/20001/content/browser/cross_si...
File content/browser/cross_site_transfer_browsertest.cc (right):

https://codereview.chromium.org/702843004/diff/20001/content/browser/cross_si...
content/browser/cross_site_transfer_browsertest.cc:463: // TODO: write me
I think i have two cases to test

1. The SW generates the redirect to a final cross-site location.

2. The SW vends the response for the final cross-site location.

[michaeln, 2014-11-13 01:53:25]

new upload, but missing tests

[falken, 2014-11-14 04:15:26]

On 2014/11/13 01:53:25, michaeln wrote:
> new upload, but missing tests

Thanks for the updates.

> but this isn't normal redirection. It's not
> easy to see how it works, but for cross-site-navs, a Request is started in
> processA and the resumed in processB

To help me understand, when does this cross-site navigation as opposed to a
normal redirection occur?

[michaeln1, 2014-11-14 20:46:03]

> To help me understand, when does this cross-site navigation as opposed to a
> normal redirection occur?

I'm somewhat familiar with what happens, but I don't fully grok when/why this is
employed, also i think the policies about when to use this change from time to
time. Roughly speaking i think if a renderer process is hosting an app with
elevated privileges, navigtions to pages w less privs are forced into other
processes, and vice versa. Charlies (cries) could give you a better answer.

As a concrete example, if you have the GDrive app installed when random.com
redirects to a gdrive.com, that will trigger a cross-site-nav.

[falken, 2014-11-17 03:19:00]

On 2014/11/14 20:46:03, michaeln1 wrote:
> > To help me understand, when does this cross-site navigation as opposed to a
> > normal redirection occur?
> 
> I'm somewhat familiar with what happens, but I don't fully grok when/why this
is
> employed, also i think the policies about when to use this change from time to
> time. Roughly speaking i think if a renderer process is hosting an app with
> elevated privileges, navigtions to pages w less privs are forced into other
> processes, and vice versa. Charlies (cries) could give you a better answer.
> 
> As a concrete example, if you have the GDrive app installed when
http://random.com
> redirects to a http://gdrive.com, that will trigger a cross-site-nav.

Complicated! OK thanks.

[falken, 2014-11-27 05:00:13]

Sorry if you were blocked on feedback. The code looks good to me, just needs
tests.

https://codereview.chromium.org/702843004/diff/40001/content/browser/cross_si...
File content/browser/cross_site_transfer_browsertest.cc (right):

https://codereview.chromium.org/702843004/diff/40001/content/browser/cross_si...
content/browser/cross_site_transfer_browsertest.cc:462:
IN_PROC_BROWSER_TEST_F(CrossSiteTransferTest, ServicedByServceWorkerWorks) {
typo: ServiceWorker (probably can just remove "Works" too)

[michaeln, 2014-12-03 00:44:27]

On 2014/11/27 05:00:13, falken wrote:
> Sorry if you were blocked on feedback. The code looks good to me, just needs
> tests.

Wasn't block, but I learned something new about cross site transfers while
looking into adding a test. They might not really xfer at all.

[michaeln, 2014-12-05 01:55:14]

Ok, added a browser tests for this now.

@mmenke, can you take a look at the /loader changes

[michaeln, 2014-12-05 02:21:48]

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

[mmenke, 2014-12-05 15:17:10]

On 2014/12/05 01:55:14, michaeln wrote:
> Ok, added a browser tests for this now.
> 
> @mmenke, can you take a look at the /loader changes

loader changes LGTM.

Note that I didn't get an email with your review request, just saw this CL on my
pending review list.

[michaeln, 2014-12-05 20:42:28]

ping: falken ptal

> Note that I didn't get an email with your review request, just saw this CL on
my
> pending review list.

hmmm, i may have added you to the list after making the comment, thnx for
noticing it

[michaeln, 2014-12-05 23:46:55]

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

[michaeln, 2014-12-05 23:48:18]

@rvargas, ptal at the new Replace() method on IDMap

[rvargas (doing something else), 2014-12-06 00:37:03]

base lgtm

https://codereview.chromium.org/702843004/diff/120001/base/id_map.h
File base/id_map.h (right):

https://codereview.chromium.org/702843004/diff/120001/base/id_map.h#newcode100
base/id_map.h:100: // nullptr is returned. The OwnershipSemantics of the map
have no affect on
nit: s/affect/effect

https://codereview.chromium.org/702843004/diff/120001/base/id_map.h#newcode105
base/id_map.h:105: CHECK(!check_on_null_data_ || new_data);
Do you mind changing this (and the other instances) to DCHECK. It looks like it
was introduced years ago to track a specific crash, but never moved to DCHECK
after the crash was fixed.

[falken, 2014-12-08 07:32:16]

i think this looks good, just have one question

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

https://codereview.chromium.org/702843004/diff/120001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:18: #include
"content/browser/service_worker/service_worker_request_handler.h"
not needed?

https://codereview.chromium.org/702843004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/702843004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:427:
ServiceWorkerProviderHost* temp = map->Lookup(new_provider_id);
I have a question, how do we know no one like ServiceWorkerVersion has a pointer
to |temp|? Is it possible to do DCHECK(!temp->controlling_version() &&
!temp->active_version() ...)? Or did that get taken care of at some point during
the Prepare/CompleteCrossSiteTransfer sequence?

https://codereview.chromium.org/702843004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/702843004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:246:
render_frame_id_ = 0;
nit: other code uses MSG_ROUTING_NONE as the invalid value for render frame
ids... would it be appropriate here too?

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
File content/test/data/service_worker/cross_site_xfer.html (right):

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
content/test/data/service_worker/cross_site_xfer.html:30: document.title =
'OK_1';  // Titlewatcher looks for this.
nitty nit: TitleWatcher

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
File content/test/data/service_worker/cross_site_xfer.js (right):

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
content/test/data/service_worker/cross_site_xfer.js:14: }
nit: add semi-colon and remove the empty line after

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
File content/test/data/service_worker/cross_site_xfer_confirm.html (right):

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
content/test/data/service_worker/cross_site_xfer_confirm.html:21: 
nit: extra empty lines

[michaeln, 2014-12-08 23:56:44]

https://codereview.chromium.org/702843004/diff/120001/base/id_map.h
File base/id_map.h (right):

https://codereview.chromium.org/702843004/diff/120001/base/id_map.h#newcode100
base/id_map.h:100: // nullptr is returned. The OwnershipSemantics of the map
have no affect on
On 2014/12/06 00:37:03, rvargas wrote:
> nit: s/affect/effect

Done.

https://codereview.chromium.org/702843004/diff/120001/base/id_map.h#newcode105
base/id_map.h:105: CHECK(!check_on_null_data_ || new_data);
On 2014/12/06 00:37:03, rvargas wrote:
> Do you mind changing this (and the other instances) to DCHECK. It looks like
it
> was introduced years ago to track a specific crash, but never moved to DCHECK
> after the crash was fixed.

Done.

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

https://codereview.chromium.org/702843004/diff/120001/content/browser/loader/...
content/browser/loader/cross_site_resource_handler.cc:18: #include
"content/browser/service_worker/service_worker_request_handler.h"
On 2014/12/08 07:32:15, falken wrote:
> not needed?

Done.

https://codereview.chromium.org/702843004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/702843004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:427:
ServiceWorkerProviderHost* temp = map->Lookup(new_provider_id);
On 2014/12/08 07:32:15, falken wrote:
> I have a question, how do we know no one like ServiceWorkerVersion has a
pointer
> to |temp|? Is it possible to do DCHECK(!temp->controlling_version() &&
> !temp->active_version() ...)? Or did that get taken care of at some point
during
> the Prepare/CompleteCrossSiteTransfer sequence?

The 'temp' host has just been created to conduct the new navigation in the new
process. Its in a newly constructed state just like the 'replacement' object in
the TransferProviderHostOut. I've added a DCHECK about having no docurl.

About SWVersions specifically, i think it only has ptrs the hosts its
controlling. There should be no controller for the temp host, but if somehow
there is, the hosts dtor will unplug that ptr.

https://codereview.chromium.org/702843004/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_provider_host.cc (right):

https://codereview.chromium.org/702843004/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_provider_host.cc:246:
render_frame_id_ = 0;
On 2014/12/08 07:32:15, falken wrote:
> nit: other code uses MSG_ROUTING_NONE as the invalid value for render frame
> ids... would it be appropriate here too?

Done.

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
File content/test/data/service_worker/cross_site_xfer.html (right):

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
content/test/data/service_worker/cross_site_xfer.html:30: document.title =
'OK_1';  // Titlewatcher looks for this.
On 2014/12/08 07:32:16, falken wrote:
> nitty nit: TitleWatcher

Done.

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
File content/test/data/service_worker/cross_site_xfer.js (right):

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
content/test/data/service_worker/cross_site_xfer.js:14: }
On 2014/12/08 07:32:16, falken wrote:
> nit: add semi-colon and remove the empty line after

Done.

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
File content/test/data/service_worker/cross_site_xfer_confirm.html (right):

https://codereview.chromium.org/702843004/diff/120001/content/test/data/servi...
content/test/data/service_worker/cross_site_xfer_confirm.html:21: 
On 2014/12/08 07:32:16, falken wrote:
> nit: extra empty lines

Done.

[falken, 2014-12-09 01:19:32]

LGTM

https://codereview.chromium.org/702843004/diff/140001/base/id_map.h
File base/id_map.h (right):

https://codereview.chromium.org/702843004/diff/140001/base/id_map.h#newcode56
base/id_map.h:56: // Sets whether Add and Replace should CHECK if passed in NULL
data.
Shouldn't this read "DCHECK" now?

[michaeln, 2014-12-09 02:26:42]

https://codereview.chromium.org/702843004/diff/140001/base/id_map.h
File base/id_map.h (right):

https://codereview.chromium.org/702843004/diff/140001/base/id_map.h#newcode56
base/id_map.h:56: // Sets whether Add and Replace should CHECK if passed in NULL
data.
On 2014/12/09 01:19:32, falken wrote:
> Shouldn't this read "DCHECK" now?

right, thnx

[michaeln, 2014-12-09 21:11:43]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-12-09 21:12:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/702843004/160001

[commit-bot: I haz the power, 2014-12-09 23:16:21]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-12-09 23:17:31]

Patchset 9 (id:??) landed as
https://crrev.com/bfea6ecef56cba8beb9816e8e9674a9dc456b20e
Cr-Commit-Position: refs/heads/master@{#307579}

[michaeln, 2014-12-10 00:39:27]

https://codereview.chromium.org/702843004/diff/160001/content/browser/service...
File content/browser/service_worker/service_worker_context_core.cc (right):

https://codereview.chromium.org/702843004/diff/160001/content/browser/service...
content/browser/service_worker/service_worker_context_core.cc:417:
transferee->dispatcher_host());
ooops, i see a bug here, dispatcher_host() will always be null here

uploading a small followup patch soon