ServiceWorker: Support SWRegistration.unregister() in SWGlobalScope [1/2]

ServiceWorker: Support SWRegistration.unregister() in SWGlobalScope [1/2]

SWRegistration.unregister() depends on WebSWProvider, but it's not available
in SWGlobalScope because the provider isn't supplied on the worker startup
sequence. This series of CLs make it available and support unregister() in
the service worker context.

This CL implements createServiceWorkerProvider() to supply a provider and
sets the document URL of its provider host to the worker script URL in
order to allow unregister() call on SWGlobalScope.

[1] Chromium: THIS PATCH
[2] Blink: https://codereview.chromium.org/900793002/

BUG=452910
TEST=compile

Committed: https://crrev.com/3d7ea50b10067d64dfc3d075b2c5ba0e47c9e982
Cr-Commit-Position: refs/heads/master@{#314979}

[nhiroki, 2015-02-04 04:02:21]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-02-04 04:46:16]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-02-04 04:58:37]

Patchset #1 (id:30004) has been deleted

[nhiroki, 2015-02-04 05:16:45]

nhiroki@chromium.org changed reviewers:
+ falken@chromium.org, kinuko@chromium.org

[nhiroki, 2015-02-04 05:18:27]

Hi kinuko@ and falken@, can you review this? Thanks!

https://codereview.chromium.org/893363003/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/893363003/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:566:
provider_host->SetDocumentUrl(version->script_url());
I'm not really sure whether we have to call SetTopMostFrameUrl() for service
worker. falken@, do you know anything about it?

[kinuko, 2015-02-04 07:12:52]

Hmm, I have a bit mixed impression on this patch.  Creating provider client
object only in order to perform unregister feels a bit sidestepping, and I feel
your previous patch (implementing .unregister on SWRegistrationImpl) was more
straightforward, mod the provider_host lifetime issue. Now looking into the code
more that seems to be the problem that exists today, except that it doesn't
crash but just errors out [1]. If this is the ok behavior I suppose we could
consider the direction of your previous patch, e.g. by changing BadMessage with
returning error. Doing so has security implications though, and if that's
considered dangerous we could even remember what provider IDs were valid in the
dispatcher host... I think I want to ask other's opinions too (falken?).

[1]
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

https://codereview.chromium.org/893363003/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/893363003/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:566:
provider_host->SetDocumentUrl(version->script_url());
On 2015/02/04 05:18:27, nhiroki (slow) wrote:
> I'm not really sure whether we have to call SetTopMostFrameUrl() for service
> worker. falken@, do you know anything about it?

Basically topmost_frame_url is used to lookup cookies settings to see if using
SW is allowed for the document, I'm not 100% sure but for ServiceWorkers we
probably can omit this.

[nhiroki, 2015-02-04 11:13:18]

Just in case, these are my previous CLs:

 (1) Blink: https://codereview.chromium.org/897573002/
 (2) Chromium: https://codereview.chromium.org/896533004/
 (3) Blink: https://codereview.chromium.org/896433005/


On 2015/02/04 07:12:52, kinuko wrote:
> Hmm, I have a bit mixed impression on this patch.  Creating provider client
> object only in order to perform unregister feels a bit sidestepping,

I might be missing something... I think the provider would be necessary to
support WorkerNavigator.ServiceWorkerContainer, so maybe we'll need to implement
it at some point anyway?

> and I feel
> your previous patch (implementing .unregister on SWRegistrationImpl) was more
> straightforward, mod the provider_host lifetime issue. Now looking into the
code
> more that seems to be the problem that exists today, except that it doesn't
> crash but just errors out [1]. If this is the ok behavior I suppose we could
> consider the direction of your previous patch, e.g. by changing BadMessage
with
> returning error. Doing so has security implications though, and if that's
> considered dangerous we could even remember what provider IDs were valid in
the
> dispatcher host... I think I want to ask other's opinions too (falken?).

Okay, let's wait for his feedback :)

> [1]
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[kinuko, 2015-02-04 15:31:57]

On 2015/02/04 11:13:18, nhiroki (slow) wrote:
> Just in case, these are my previous CLs:
> 
>  (1) Blink: https://codereview.chromium.org/897573002/
>  (2) Chromium: https://codereview.chromium.org/896533004/
>  (3) Blink: https://codereview.chromium.org/896433005/
> 
> 
> On 2015/02/04 07:12:52, kinuko wrote:
> > Hmm, I have a bit mixed impression on this patch.  Creating provider client
> > object only in order to perform unregister feels a bit sidestepping,
> 
> I might be missing something... I think the provider would be necessary to
> support WorkerNavigator.ServiceWorkerContainer, so maybe we'll need to
implement
> it at some point anyway?

Hm, ok that may be true.

> > and I feel
> > your previous patch (implementing .unregister on SWRegistrationImpl) was
more
> > straightforward, mod the provider_host lifetime issue. Now looking into the
> code
> > more that seems to be the problem that exists today, except that it doesn't
> > crash but just errors out [1]. If this is the ok behavior I suppose we could
> > consider the direction of your previous patch, e.g. by changing BadMessage
> with
> > returning error. Doing so has security implications though, and if that's
> > considered dangerous we could even remember what provider IDs were valid in
> the
> > dispatcher host... I think I want to ask other's opinions too (falken?).
> 
> Okay, let's wait for his feedback :)
> 
> > [1]
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

[kinuko, 2015-02-04 15:39:14]

I think this lgtm

https://codereview.chromium.org/893363003/diff/50001/content/renderer/service...
File content/renderer/service_worker/embedded_worker_context_client.h (right):

https://codereview.chromium.org/893363003/diff/50001/content/renderer/service...
content/renderer/service_worker/embedded_worker_context_client.h:40: // are
called on the worker thread.
Please update this comment.

[nhiroki, 2015-02-05 05:07:28]

New patchsets have been uploaded after l-g-t-m from kinuko@chromium.org

[nhiroki, 2015-02-05 05:28:40]

Thank you! Updated.

https://codereview.chromium.org/893363003/diff/50001/content/browser/service_...
File content/browser/service_worker/service_worker_dispatcher_host.cc (right):

https://codereview.chromium.org/893363003/diff/50001/content/browser/service_...
content/browser/service_worker/service_worker_dispatcher_host.cc:566:
provider_host->SetDocumentUrl(version->script_url());
On 2015/02/04 07:12:52, kinuko wrote:
> On 2015/02/04 05:18:27, nhiroki (slow) wrote:
> > I'm not really sure whether we have to call SetTopMostFrameUrl() for service
> > worker. falken@, do you know anything about it?
> 
> Basically topmost_frame_url is used to lookup cookies settings to see if using
> SW is allowed for the document, I'm not 100% sure but for ServiceWorkers we
> probably can omit this.

I see. Already running worker wouldn't have to take care of the preference. I'm
going to keep this as is.

https://codereview.chromium.org/893363003/diff/50001/content/renderer/service...
File content/renderer/service_worker/embedded_worker_context_client.h (right):

https://codereview.chromium.org/893363003/diff/50001/content/renderer/service...
content/renderer/service_worker/embedded_worker_context_client.h:40: // are
called on the worker thread.
On 2015/02/04 15:39:14, kinuko wrote:
> Please update this comment.

Done.

[nhiroki, 2015-02-05 08:31:09]

Patchset #4 (id:110001) has been deleted

[falken, 2015-02-05 12:07:18]

lgtm2

https://codereview.chromium.org/893363003/diff/90001/content/renderer/service...
File content/renderer/service_worker/embedded_worker_context_client.h (right):

https://codereview.chromium.org/893363003/diff/90001/content/renderer/service...
content/renderer/service_worker/embedded_worker_context_client.h:40: // on the
worker thread.
Seems better to keep track of by saying "Unless otherwise noted, all methods are
called on the worker thread". And then note the exceptions above the individual
functions.

[nhiroki, 2015-02-06 01:57:31]

Thank you!

https://codereview.chromium.org/893363003/diff/90001/content/renderer/service...
File content/renderer/service_worker/embedded_worker_context_client.h (right):

https://codereview.chromium.org/893363003/diff/90001/content/renderer/service...
content/renderer/service_worker/embedded_worker_context_client.h:40: // on the
worker thread.
On 2015/02/05 12:07:18, falken wrote:
> Seems better to keep track of by saying "Unless otherwise noted, all methods
are
> called on the worker thread". And then note the exceptions above the
individual
> functions.

Done.

[nhiroki, 2015-02-06 01:57:38]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-02-06 01:59:15]

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

[nhiroki, 2015-02-06 03:40:12]

The CQ bit was unchecked by nhiroki@chromium.org

[nhiroki, 2015-02-06 03:44:06]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-02-06 03:45:24]

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

[commit-bot: I haz the power, 2015-02-06 04:56:28]

Committed patchset #6 (id:170001)

[commit-bot: I haz the power, 2015-02-06 04:57:26]

Patchset 6 (id:??) landed as
https://crrev.com/3d7ea50b10067d64dfc3d075b2c5ba0e47c9e982
Cr-Commit-Position: refs/heads/master@{#314979}