ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed handle references (1)

ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed handle references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should surely be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references at
the beginning of message handlers related to registration association. The
rest of the handlers will be treated in the subsequent CL.

BUG=557655
Committed: https://crrev.com/4524454748a8676fdd6f1beae30810e4b0c13cac
Cr-Commit-Position: refs/heads/master@{#360762}

[nhiroki, 2015-11-18 11:38:47]

Patchset #1 (id:1) has been deleted

[nhiroki, 2015-11-18 11:48:35]

Patchset #1 (id:20001) has been deleted

[nhiroki, 2015-11-18 11:49:31]

Description was changed from

==========
WIP: ServiceWorker: Ensure that ServiceWorkerDispatcher adopts passed handle
references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should appropriately be
adopted by the dispatcher but not in some early-return cases. This means the
handles in the browser process are leaks (Note: handles are managed per-process
and cleaned up when renderer shutdown).

This CL ensures that the dispatcher always adopts passed handle references on
message handlers related to registration association. The rest of the message
handlers will be treated in the subsequent CL.

BUG=
==========

to

==========
WIP: ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed
handle references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should appropriately be
adopted by the dispatcher but not in some early-return cases. This means the
handles in the browser process are leaks (Note: handles are managed per-process
and cleaned up when renderer shutdown).

This CL ensures that the dispatcher always adopts passed handle references on
message handlers related to registration association. The rest of the message
handlers will be treated in the subsequent CL.

BUG=
==========

[nhiroki, 2015-11-18 11:53:37]

Description was changed from

==========
WIP: ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed
handle references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should appropriately be
adopted by the dispatcher but not in some early-return cases. This means the
handles in the browser process are leaks (Note: handles are managed per-process
and cleaned up when renderer shutdown).

This CL ensures that the dispatcher always adopts passed handle references on
message handlers related to registration association. The rest of the message
handlers will be treated in the subsequent CL.

BUG=
==========

to

==========
WIP: ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed
handle references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should appropriately be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references on
message handlers related to registration association. The rest of the message
handlers will be treated in the subsequent CL.

BUG=
==========

[nhiroki, 2015-11-18 11:54:42]

Description was changed from

==========
WIP: ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed
handle references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should appropriately be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references on
message handlers related to registration association. The rest of the message
handlers will be treated in the subsequent CL.

BUG=
==========

to

==========
WIP: ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed
handle references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should surely be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references on
message handlers related to registration association. The rest of the message
handlers will be treated in the subsequent CL.

BUG=
==========

[nhiroki, 2015-11-18 12:06:13]

Description was changed from

==========
WIP: ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed
handle references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should surely be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references on
message handlers related to registration association. The rest of the message
handlers will be treated in the subsequent CL.

BUG=
==========

to

==========
WIP: ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed
handle references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should surely be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references on
message handlers related to registration association. The rest of the message
handlers will be treated in the subsequent CL.

BUG=557655
==========

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

Description was changed from

==========
WIP: ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed
handle references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should surely be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references on
message handlers related to registration association. The rest of the message
handlers will be treated in the subsequent CL.

BUG=557655
==========

to

==========
ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed handle
references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should surely be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references on
message handlers related to registration association. The rest of the message
handlers will be treated in the subsequent CL.

BUG=557655
==========

[nhiroki, 2015-11-18 15:45:11]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2015-11-19 07:46:11]

Description was changed from

==========
ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed handle
references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should surely be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references on
message handlers related to registration association. The rest of the message
handlers will be treated in the subsequent CL.

BUG=557655
==========

to

==========
ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed handle
references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should surely be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references at
the beginning of message handlers related to registration association. The
rest of the message handlers will be treated in the subsequent CL.

BUG=557655
==========

[nhiroki, 2015-11-19 07:46:46]

Description was changed from

==========
ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed handle
references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should surely be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references at
the beginning of message handlers related to registration association. The
rest of the message handlers will be treated in the subsequent CL.

BUG=557655
==========

to

==========
ServiceWorker: Ensure that ServiceWorkerDispatcher always adopts passed handle
references (1)

Some message handlers on ServiceWorkerDispatcher receive registration/worker
handle references sent from the browser process. Those should surely be
adopted by the dispatcher but not in some early-return cases. This means that
handles in the browser process are leaked until renderer shutdown.

This CL ensures that the dispatcher always adopts passed handle references at
the beginning of message handlers related to registration association. The
rest of the handlers will be treated in the subsequent CL.

BUG=557655
==========

[nhiroki, 2015-11-19 10:02:12]

Patchset #1 (id:60001) has been deleted

[nhiroki, 2015-11-19 10:04:19]

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

[nhiroki, 2015-11-19 10:04:19]

PTAL, thanks!

[falken, 2015-11-20 03:35:54]

Nice fix!

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher_unittest.cc (right):

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher_unittest.cc:174: // The
passed references should be adoped and owned by the provider context.
nit: adopted (two places)

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher_unittest.cc:205: info,
attrs);
How does this test differ from the one above it? The names don't give much clue;
they are both calling OnAssociateRegistrationWithServiceWorker.

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
File content/child/service_worker/service_worker_provider_context.cc (right):

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_provider_context.cc:95: installing_
= installing.Pass();
I thought this was in another patch.

[nhiroki, 2015-11-20 05:14:28]

Patchset #3 (id:120001) has been deleted

[nhiroki, 2015-11-20 05:14:59]

Thank you! Updated.

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher_unittest.cc (right):

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher_unittest.cc:174: // The
passed references should be adoped and owned by the provider context.
On 2015/11/20 03:35:54, falken wrote:
> nit: adopted (two places)

Done.

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher_unittest.cc:205: info,
attrs);
On 2015/11/20 03:35:54, falken wrote:
> How does this test differ from the one above it? The names don't give much
clue;
> they are both calling OnAssociateRegistrationWithServiceWorker.

Good catch! That should be OnAssociateRegistration. Fixed.

By the way, OnAssociateRegistration is now equal to
OnAssociateRegistrationWithServiceWorker and an operation is dispatched into
different implementations (ControllerDelegate vs. ControlleeDelegate) in
ServiceWorkerProviderContext depending on its provider type (please see my
comment on line 220), so we could simply merge them. I'll try it in a separate
CL.

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher_unittest.cc:220:
SERVICE_WORKER_PROVIDER_FOR_WINDOW,
fyi: This provider type is 
different from one in the other test (FOR_WINDOW vs. FOR_CONTROLLER).

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
File content/child/service_worker/service_worker_provider_context.cc (right):

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_provider_context.cc:95: installing_
= installing.Pass();
On 2015/11/20 03:35:54, falken wrote:
> I thought this was in another patch.

Right. I rebased this patch on that patch.
(This patch depended on this fix to run trybots)

[falken, 2015-11-20 05:26:25]

lgtm

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
File content/child/service_worker/service_worker_dispatcher_unittest.cc (right):

https://codereview.chromium.org/1454963003/diff/80001/content/child/service_w...
content/child/service_worker/service_worker_dispatcher_unittest.cc:205: info,
attrs);
On 2015/11/20 05:14:59, nhiroki wrote:
> On 2015/11/20 03:35:54, falken wrote:
> > How does this test differ from the one above it? The names don't give much
> clue;
> > they are both calling OnAssociateRegistrationWithServiceWorker.
> 
> Good catch! That should be OnAssociateRegistration. Fixed.
> 
> By the way, OnAssociateRegistration is now equal to
> OnAssociateRegistrationWithServiceWorker and an operation is dispatched into
> different implementations (ControllerDelegate vs. ControlleeDelegate) in
> ServiceWorkerProviderContext depending on its provider type (please see my
> comment on line 220), so we could simply merge them. I'll try it in a separate
> CL.

Agree with merging them.

[nhiroki, 2015-11-20 05:30:20]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2015-11-20 05:30:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1454963003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1454963003/140001

[commit-bot: I haz the power, 2015-11-20 06:21:22]

Committed patchset #3 (id:140001)

[commit-bot: I haz the power, 2015-11-20 06:22:12]

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