ServiceWorker: Add support for .skipWaiting and controllerchange event(1/3)

ServiceWorker: Add support for .skipWaiting and controllerchange event(1/3)

ServiceWorkerGlobalScope will pass .skipWaiting promise resolver
callback to worker client which will resolve it after Chrome handles it.

Make ServiceWorkerContainer inherits EventTarget in order to receive
"controllerchange" event triggered by .skipWaiting.

Spec:
http://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-global-scope-skipwaiting
https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#service-worker-container-controllerchange-event

(1/3): This
(2/3): https://codereview.chromium.org/717353004/
(3/3): https://codereview.chromium.org/765323002/

BUG=370742
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=186387

[xiang, 2014-11-13 07:27:33]

xiang.long@intel.com changed reviewers:
+ dominicc@chromium.org, falken@chromium.org, jsbell@chromium.org

[xiang, 2014-11-13 07:28:23]

Hi, would you have a look of this? Thanks.

[xiang, 2014-11-13 07:28:57]

PTAL, thanks!

[jsbell, 2014-11-13 23:07:51]

Overall, looks pretty good. The tests are necessarily complex, so anything you
can do to make them more readable (factoring out common patterns) would be
helpful. I would suggest splitting it into two or three separate workers and
using service_worker_test/promise_test where possible. Even the more complex
cases could share more code so that it's very clear what the test is doing.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/resources/skip-waiting.js (right):

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:1: function
onMessageDefault(e) {
Please rename file to skip-waiting-worker.js, to match other worker scripts.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:4: if (self.done)
The way this worker is designed, if anything fails (e.g. a promise rejects
instead of resolves, or something starts throwing e.g. method is removed leading
to an exception) then the test will unhelpfully timeout instead of failing with
an error.

Can you add .catch clauses to the promise chains that (with some changes here)
make the message handler send back 'fail' ?

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:12: skipWaiting()
Just as a style thing, we tend to use self.onmessage, self.skipWaiting() and so
on when referring to properties on the worker's global object, rather than
defined in the script.

(The functionality is identical, of course, it's just more readable for those
unfamiliar with what's on the global.)

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:14: if (result
=== undefined)
otherwise, throw so the promise rejects? (see comment above)

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:33: var sate =
'starting';
This line doesn't look like it does anything? lines below assume state is a
global.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:51: state =
'installing';
Be consistent with tests above (i.e. use self.state)

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:59: if
(scope.match('blank')) {
Use /blank/ to make it clear the match is being done against a regexp.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:61: } else if
(scope.match('multiple-skip-waiting')) {
Use /multiple-skip-waiting/

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/skip-waiting.html (right):

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/skip-waiting.html:8: async_test(function(t)
{
Can you use the service_worker_test() helper to remove some of this boilerplate,
at least for these first two tests?

You may need to modify that to take an optional scope argument or introduce
service_worker_with_scope_test(). Or just use three different workers, which
would probably make this easier to understand.

That would also simplify the test cases in the worker since they can use
promise_test()

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/skip-waiting.html:12: var message = e.data;
Only need 2 more spaces of indent here (6 total)

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/skip-waiting.html:35: 'multiple skipWaiting
promises should be resolved');
This message would be a lot clearer if it was in the test case itself (see note
above about using service_worker_test/promise_test)

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/skip-waiting.html:48: }, 'Test multiple
skipWaiting be called');
How about: 'skipWaiting() called multiple times' ?

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/skip-waiting.html:77: return
navigator.serviceWorker.register(url2, { scope : scope });
Nit: no space after {, after :, or before } in object literals. i.e. just
{scope: scope}, here and below

https://codereview.chromium.org/723923002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl (right):

https://codereview.chromium.org/723923002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerGlobalScope.idl:48:
[CallWith=ScriptState] Promise skipWaiting();
Make this Promise<void> to match the spec.

The type is currently ignored, but useful extra documentation. I have a CL in
review to add promise types to the other SW IDL files.

[michaeln, 2014-11-14 22:53:54]

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

[michaeln, 2014-11-14 22:53:55]

https://codereview.chromium.org/723923002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/723923002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:227:
dispatchEvent(Event::create(EventTypeNames::controllerchange));
This event should only be raised in the skipWaiting case specifically, as coded
i think it's being raised in other cases too.

[xiang, 2014-11-28 07:57:28]

Patchset #2 (id:20001) has been deleted

[xiang, 2014-11-28 08:00:48]

Thanks for the review, I have rebased the patch and rewrite some of the tests.
Please take a look again.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/resources/skip-waiting.js (right):

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:1: function
onMessageDefault(e) {
On 2014/11/13 23:07:50, jsbell wrote:
> Please rename file to skip-waiting-worker.js, to match other worker scripts.

Done.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:4: if (self.done)
On 2014/11/13 23:07:50, jsbell wrote:
> The way this worker is designed, if anything fails (e.g. a promise rejects
> instead of resolves, or something starts throwing e.g. method is removed
leading
> to an exception) then the test will unhelpfully timeout instead of failing
with
> an error.
> 
> Can you add .catch clauses to the promise chains that (with some changes here)
> make the message handler send back 'fail' ?

Done.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:12: skipWaiting()
On 2014/11/13 23:07:50, jsbell wrote:
> Just as a style thing, we tend to use self.onmessage, self.skipWaiting() and
so
> on when referring to properties on the worker's global object, rather than
> defined in the script.
> 
> (The functionality is identical, of course, it's just more readable for those
> unfamiliar with what's on the global.)

Done.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:14: if (result
=== undefined)
On 2014/11/13 23:07:50, jsbell wrote:
> otherwise, throw so the promise rejects? (see comment above)

Done.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:33: var sate =
'starting';
On 2014/11/13 23:07:50, jsbell wrote:
> This line doesn't look like it does anything? lines below assume state is a
> global.

Done.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:51: state =
'installing';
On 2014/11/13 23:07:50, jsbell wrote:
> Be consistent with tests above (i.e. use self.state)

Done.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/resources/skip-waiting.js:61: } else if
(scope.match('multiple-skip-waiting')) {
On 2014/11/13 23:07:50, jsbell wrote:
> Use /multiple-skip-waiting/

don't need them anymore.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
File LayoutTests/http/tests/serviceworker/skip-waiting.html (right):

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/skip-waiting.html:8: async_test(function(t)
{
On 2014/11/13 23:07:51, jsbell wrote:
> Can you use the service_worker_test() helper to remove some of this
boilerplate,
> at least for these first two tests?
> 
> You may need to modify that to take an optional scope argument or introduce
> service_worker_with_scope_test(). Or just use three different workers, which
> would probably make this easier to understand.
> 
> That would also simplify the test cases in the worker since they can use
> promise_test()

Thanks for the suggestion, I split them to smaller ones.

https://codereview.chromium.org/723923002/diff/1/LayoutTests/http/tests/servi...
LayoutTests/http/tests/serviceworker/skip-waiting.html:77: return
navigator.serviceWorker.register(url2, { scope : scope });
On 2014/11/13 23:07:50, jsbell wrote:
> Nit: no space after {, after :, or before } in object literals. i.e. just
> {scope: scope}, here and below

Done.

https://codereview.chromium.org/723923002/diff/1/Source/modules/serviceworker...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/723923002/diff/1/Source/modules/serviceworker...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:227:
dispatchEvent(Event::create(EventTypeNames::controllerchange));
On 2014/11/14 22:53:55, michaeln wrote:
> This event should only be raised in the skipWaiting case specifically, as
coded
> i think it's being raised in other cases too.

Done.

[falken, 2014-12-01 02:28:48]

looks good, I think you'll need to land as a three-sided patch though, right?

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/skip-waiting-installed-worker.js
(right):

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/skip-waiting-installed-worker.js:14:
port.postMessage('FAIL: Worker should waiting in installed state');
nit: "be waiting"?

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/skip-waiting-worker.js
(right):

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/skip-waiting-worker.js:17: 'All
promises should be resolved');
Remove this assert. It's just testing the generic behavior of Promise.all.

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/skip-waiting.html (right):

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/skip-waiting.html:12: var iframeContainer,
onControllerChanged, worker;
nit: variables should be snake_case (same with other files)

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/skip-waiting.html:29: return
with_iframe(scope);
nit: please remove this iframe when done, as per style guide (same with other
files)

[xiang, 2014-12-01 07:13:44]

Thanks for the review, I have split this CL to two parts.

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
File
LayoutTests/http/tests/serviceworker/resources/skip-waiting-installed-worker.js
(right):

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/skip-waiting-installed-worker.js:14:
port.postMessage('FAIL: Worker should waiting in installed state');
On 2014/12/01 02:28:48, falken wrote:
> nit: "be waiting"?

Done.

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/resources/skip-waiting-worker.js
(right):

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/resources/skip-waiting-worker.js:17: 'All
promises should be resolved');
On 2014/12/01 02:28:48, falken wrote:
> Remove this assert. It's just testing the generic behavior of Promise.all.

Done.

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
File LayoutTests/http/tests/serviceworker/skip-waiting.html (right):

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/skip-waiting.html:12: var iframeContainer,
onControllerChanged, worker;
On 2014/12/01 02:28:48, falken wrote:
> nit: variables should be snake_case (same with other files)

Done.

https://codereview.chromium.org/723923002/diff/40001/LayoutTests/http/tests/s...
LayoutTests/http/tests/serviceworker/skip-waiting.html:29: return
with_iframe(scope);
On 2014/12/01 02:28:48, falken wrote:
> nit: please remove this iframe when done, as per style guide (same with other
> files)

Done.

[falken, 2014-12-01 07:21:05]

lgtm

[xiang, 2014-12-01 07:44:42]

xiang.long@intel.com changed reviewers:
+ tkent@chromium.org

[xiang, 2014-12-01 07:46:16]

tkent@ would you review the public/ part? Thanks!

[tkent, 2014-12-01 07:57:14]

https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:246:
dispatchEvent(Event::create(EventTypeNames::controllerchange));
Is it possible to cause use-after-free by JavaScript code invoked by an event
handler?

https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.h:60: , public
HeapSupplementable<ServiceWorkerContainer> {
Why is this HeapSupplementable?

https://codereview.chromium.org/723923002/diff/60001/public/platform/WebServi...
File public/platform/WebServiceWorkerSkipWaitingCallbacks.h (right):

https://codereview.chromium.org/723923002/diff/60001/public/platform/WebServi...
public/platform/WebServiceWorkerSkipWaitingCallbacks.h:12: typedef
WebCallbacks<void, void> WebServiceWorkerSkipWaitingCallbacks;
we prefer |using| to |typedef|.

[xiang, 2014-12-02 02:31:48]

tkent, thanks for the review, would you take a look again?

https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:246:
dispatchEvent(Event::create(EventTypeNames::controllerchange));
On 2014/12/01 07:57:13, tkent wrote:
> Is it possible to cause use-after-free by JavaScript code invoked by an event
> handler?

I don't know in which case the event object will be use-after-free, would you
provide more info on this?

https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.h (right):

https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.h:60: , public
HeapSupplementable<ServiceWorkerContainer> {
On 2014/12/01 07:57:13, tkent wrote:
> Why is this HeapSupplementable?

It's not needed, my mistake.

https://codereview.chromium.org/723923002/diff/60001/public/platform/WebServi...
File public/platform/WebServiceWorkerSkipWaitingCallbacks.h (right):

https://codereview.chromium.org/723923002/diff/60001/public/platform/WebServi...
public/platform/WebServiceWorkerSkipWaitingCallbacks.h:12: typedef
WebCallbacks<void, void> WebServiceWorkerSkipWaitingCallbacks;
On 2014/12/01 07:57:13, tkent wrote:
> we prefer |using| to |typedef|.

Done.

[tkent, 2014-12-02 02:37:08]

https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):

https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/ServiceWorkerContainer.cpp:246:
dispatchEvent(Event::create(EventTypeNames::controllerchange));
On 2014/12/02 02:31:48, xiang wrote:
> On 2014/12/01 07:57:13, tkent wrote:
> > Is it possible to cause use-after-free by JavaScript code invoked by an
event
> > handler?
> 
> I don't know in which case the event object will be use-after-free, would you
> provide more info on this?

A web page can run any JavaScript code as a |controllerchange| event handler,
and the code can remove C++ objects referred from a JavaScript object.  I'm
afraid such code removes this ServiceWorkerContainer or a callsite object of
setController().

[xiang, 2014-12-02 03:20:55]

On 2014/12/02 02:37:08, tkent wrote:
>
https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
> File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):
> 
>
https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
> Source/modules/serviceworkers/ServiceWorkerContainer.cpp:246:
> dispatchEvent(Event::create(EventTypeNames::controllerchange));
> On 2014/12/02 02:31:48, xiang wrote:
> > On 2014/12/01 07:57:13, tkent wrote:
> > > Is it possible to cause use-after-free by JavaScript code invoked by an
> event
> > > handler?
> > 
> > I don't know in which case the event object will be use-after-free, would
you
> > provide more info on this?
> 
> A web page can run any JavaScript code as a |controllerchange| event handler,
> and the code can remove C++ objects referred from a JavaScript object.  I'm
> afraid such code removes this ServiceWorkerContainer or a callsite object of
> setController().

The ServiceWorkerContainer is referred by navigtor and other C++ objects, I
don't think it will be removed until document destroyed.

[tkent, 2014-12-02 04:41:11]

On 2014/12/02 03:20:55, xiang wrote:
> On 2014/12/02 02:37:08, tkent wrote:
> >
>
https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
> > File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):
> > 
> >
>
https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
> > Source/modules/serviceworkers/ServiceWorkerContainer.cpp:246:
> > dispatchEvent(Event::create(EventTypeNames::controllerchange));
> > On 2014/12/02 02:31:48, xiang wrote:
> > > On 2014/12/01 07:57:13, tkent wrote:
> > > > Is it possible to cause use-after-free by JavaScript code invoked by an
> > event
> > > > handler?
> > > 
> > > I don't know in which case the event object will be use-after-free, would
> you
> > > provide more info on this?
> > 
> > A web page can run any JavaScript code as a |controllerchange| event
handler,
> > and the code can remove C++ objects referred from a JavaScript object.  I'm
> > afraid such code removes this ServiceWorkerContainer or a callsite object of
> > setController().
> 
> The ServiceWorkerContainer is referred by navigtor and other C++ objects, I
> don't think it will be removed until document destroyed.

JavaScript code can delete a document.


BTW, I looked the specification.  It says "queue a task."  So, we shouldn't use
dispatchEvent() here, which dispatches an event synchronously.

> 9. Queue a task to fire a simple event named controllerchange at all the
ServiceWorkerContainer objects for all the service worker clients which use
registration.

[xiang, 2014-12-02 06:10:11]

On 2014/12/02 04:41:11, tkent wrote:
> On 2014/12/02 03:20:55, xiang wrote:
> > On 2014/12/02 02:37:08, tkent wrote:
> > >
> >
>
https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
> > > File Source/modules/serviceworkers/ServiceWorkerContainer.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/723923002/diff/60001/Source/modules/servicewo...
> > > Source/modules/serviceworkers/ServiceWorkerContainer.cpp:246:
> > > dispatchEvent(Event::create(EventTypeNames::controllerchange));
> > > On 2014/12/02 02:31:48, xiang wrote:
> > > > On 2014/12/01 07:57:13, tkent wrote:
> > > > > Is it possible to cause use-after-free by JavaScript code invoked by
an
> > > event
> > > > > handler?
> > > > 
> > > > I don't know in which case the event object will be use-after-free,
would
> > you
> > > > provide more info on this?
> > > 
> > > A web page can run any JavaScript code as a |controllerchange| event
> handler,
> > > and the code can remove C++ objects referred from a JavaScript object. 
I'm
> > > afraid such code removes this ServiceWorkerContainer or a callsite object
of
> > > setController().
> > 
> > The ServiceWorkerContainer is referred by navigtor and other C++ objects, I
> > don't think it will be removed until document destroyed.
> 
> JavaScript code can delete a document.
> 

ServiceWorkerContainer will keep alive if there's a reference to it.
ServiceWorkerContainer::willBeDetachedFromFrame will handle its associated
document unload.
We can still access the C++ object, however some actions will just throw
exception.

> 
> BTW, I looked the specification.  It says "queue a task."  So, we shouldn't
use
> dispatchEvent() here, which dispatches an event synchronously.
> 
> > 9. Queue a task to fire a simple event named controllerchange at all the
> ServiceWorkerContainer objects for all the service worker clients which use
> registration.

Currently the event is implemented by browser process sending corresponding msg
to ServiceWorkerContainer in renderer.
The renderer handles it in next event loop. It is already asynchronous, am I
right?

[tkent, 2014-12-02 06:14:09]

I don't know the detail of Service Worker implementation.
Service Worker team, please re-review this CL.

[falken, 2014-12-02 08:07:29]

Hi tkent,

> > JavaScript code can delete a document.
> > 
> 
> ServiceWorkerContainer will keep alive if there's a reference to it.
> ServiceWorkerContainer::willBeDetachedFromFrame will handle its associated
> document unload.
> We can still access the C++ object, however some actions will just throw
> exception.

In addition, ServiceWorkerContainer is
RefCountedGarbageCollectedWillBeGarbageCollectedFinalized. Doesn't this mean its
expected to be kept alive while there's a pointer to it?

In any case, the callsites don't access the object after the setController call,
so it should be safe. xiang@: Can you add a layout test in your third patch for
calling something like window.location.reload() in oncontrollerchange handler to
show we don't crash? As it doesn't assert a spec clause, it would be a chromium/
test.

> 
> > 
> > BTW, I looked the specification.  It says "queue a task."  So, we shouldn't
> use
> > dispatchEvent() here, which dispatches an event synchronously.
> > 
> > > 9. Queue a task to fire a simple event named controllerchange at all the
> > ServiceWorkerContainer objects for all the service worker clients which use
> > registration.
> 
> Currently the event is implemented by browser process sending corresponding
msg
> to ServiceWorkerContainer in renderer.
> The renderer handles it in next event loop. It is already asynchronous, am I
> right?

Yes I think this is fine since setController is invoked by IPC message, not
synchronously by a JavaScript call.

[tkent, 2014-12-02 09:55:59]

It sounds ok.  LGTM.

https://codereview.chromium.org/723923002/diff/80001/public/platform/WebServi...
File public/platform/WebServiceWorkerSkipWaitingCallbacks.h (right):

https://codereview.chromium.org/723923002/diff/80001/public/platform/WebServi...
public/platform/WebServiceWorkerSkipWaitingCallbacks.h:8: #include
"WebCallbacks.h"
"WebCallbacks.h" -> "public/platform/WebCallbacks.h"

[xiang, 2014-12-03 00:12:07]

A document close test during oncontrollerchange handler is added in the third
patch. Thanks for the reviews.

https://codereview.chromium.org/723923002/diff/80001/public/platform/WebServi...
File public/platform/WebServiceWorkerSkipWaitingCallbacks.h (right):

https://codereview.chromium.org/723923002/diff/80001/public/platform/WebServi...
public/platform/WebServiceWorkerSkipWaitingCallbacks.h:8: #include
"WebCallbacks.h"
On 2014/12/02 09:55:59, tkent wrote:
> "WebCallbacks.h" -> "public/platform/WebCallbacks.h"

Done.

[xiang, 2014-12-03 00:12:21]

The CQ bit was checked by xiang.long@intel.com

[commit-bot: I haz the power, 2014-12-03 00:12:43]

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

[commit-bot: I haz the power, 2014-12-03 01:24:38]

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

[commit-bot: I haz the power, 2014-12-03 01:24:39]

Try jobs failed on following builders:
  linux_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

[falken, 2014-12-03 04:17:42]

On 2014/12/03 01:24:39, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_blink_rel on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/3...)

Looks like you need to rebase these:
fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced.html
fast/dom/Window/property-access-on-cached-properties-after-frame-removed.html
fast/dom/Window/property-access-on-cached-window-after-frame-removed.html
fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced.html
fast/dom/Window/property-access-on-cached-properties-after-frame-navigated.html
fast/dom/Window/property-access-on-cached-window-after-frame-navigated.html

[xiang, 2014-12-03 05:16:49]

The CQ bit was checked by xiang.long@intel.com

[commit-bot: I haz the power, 2014-12-03 05:17:34]

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

[commit-bot: I haz the power, 2014-12-03 07:30:08]

Committed patchset #6 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=186387