ServiceWorker: Release a reference when it fails to dispatch ExtendableMessageEvent

ServiceWorker: Release a reference when it fails to dispatch ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this have not happened in release builds.

BUG=543198
Committed: https://crrev.com/f6da9a0a7eb3a919aa29749017321c9bae8bfa8b
Cr-Commit-Position: refs/heads/master@{#382455}

[nhiroki, 2016-03-17 04:28:42]

Patchset #2 (id:20001) has been deleted

[nhiroki, 2016-03-17 04:28:48]

Patchset #1 (id:1) has been deleted

[nhiroki, 2016-03-17 04:29:35]

Patchset #1 (id:40001) has been deleted

[nhiroki, 2016-03-17 04:36:30]

Description was changed from

==========
ServiceWorker: Decrement refcounts to ServiceWorkerVersion when message
dispatching fails

BUG=543198
==========

to

==========
ServiceWorker: Clean up resources when it fails to dispatch
ExtendableMessageEvent

BUG=543198
==========

[nhiroki, 2016-03-17 04:39:37]

Description was changed from

==========
ServiceWorker: Clean up resources when it fails to dispatch
ExtendableMessageEvent

BUG=543198
==========

to

==========
ServiceWorker: Clean up resources when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

BUG=543198
==========

[nhiroki, 2016-03-17 04:40:32]

Description was changed from

==========
ServiceWorker: Clean up resources when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

BUG=543198
==========

to

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

BUG=543198
==========

[nhiroki, 2016-03-17 04:55:52]

Description was changed from

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

BUG=543198
==========

to

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this don't happen in a real world.

BUG=543198
==========

[nhiroki, 2016-03-17 04:56:42]

Description was changed from

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this don't happen in a real world.

BUG=543198
==========

to

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this have not happened in a real world.

BUG=543198
==========

[nhiroki, 2016-03-17 04:58:33]

Description was changed from

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this have not happened in a real world.

BUG=543198
==========

to

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this have not happened in a real world.

BUG=543198
==========

[nhiroki, 2016-03-17 04:58:34]

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

[nhiroki, 2016-03-17 04:59:12]

Description was changed from

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this have not happened in a real world.

BUG=543198
==========

to

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this have not happened in release builds.

BUG=543198
==========

[nhiroki, 2016-03-17 04:59:26]

PTAL, thanks!

[falken, 2016-03-17 05:10:49]

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

https://codereview.chromium.org/1799413002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:951: void
ServiceWorkerDispatcherHost::DidFailToDispatchExtendableMessageEvent(
I'm kind of confused why there are two
ServiceWorkerDispatcherHost::DidFailToDispatchExtendableMessageEvent's. Can you
help me understand?

[nhiroki, 2016-03-17 06:01:12]

Patchset #2 (id:80001) has been deleted

[nhiroki, 2016-03-17 06:05:30]

Thank you for your comment.

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

https://codereview.chromium.org/1799413002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:951: void
ServiceWorkerDispatcherHost::DidFailToDispatchExtendableMessageEvent(
On 2016/03/17 05:10:49, falken wrote:
> I'm kind of confused why there are two
> ServiceWorkerDispatcherHost::DidFailToDispatchExtendableMessageEvent's. Can
you
> help me understand?

"typename SourceInfo" is resolved to ServiceWorkerObejectInfo or
ServiceWorkerClientInfo.

As for ServiceWorkerObjectInfo, it implicitly has a reference to
ServiceWorkerHandle and needs a special care to release the reference when
sending a message fails.

As for ServiceWorkerClientInfo, it's a kind of snapshot of Client states and
does not have a reference to anything, so the special care is not necessary.

To handle the both cases, I made these two functions. On the second patchset, I
tried to reduce duplicate lines. This could be more explicit about my intention?

[nhiroki, 2016-03-17 06:33:28]

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

https://codereview.chromium.org/1799413002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_dispatcher_host.cc:951: void
ServiceWorkerDispatcherHost::DidFailToDispatchExtendableMessageEvent(
On 2016/03/17 06:05:30, nhiroki wrote:
> On 2016/03/17 05:10:49, falken wrote:
> > I'm kind of confused why there are two
> > ServiceWorkerDispatcherHost::DidFailToDispatchExtendableMessageEvent's. Can
> you
> > help me understand?
> 
> "typename SourceInfo" is resolved to ServiceWorkerObejectInfo or
> ServiceWorkerClientInfo.
> 
> As for ServiceWorkerObjectInfo, it implicitly has a reference to
> ServiceWorkerHandle and needs a special care to release the reference when
> sending a message fails.
> 
> As for ServiceWorkerClientInfo, it's a kind of snapshot of Client states and
> does not have a reference to anything, so the special care is not necessary.
> 
> To handle the both cases, I made these two functions. On the second patchset,
I
> tried to reduce duplicate lines. This could be more explicit about my
intention?

FYI: I tried to use overloaded functions instead of C++ template, but it didn't
work because base::Bind() couldn't determine the functor on compile time.

[falken, 2016-03-17 06:44:36]

lgtm

https://codereview.chromium.org/1799413002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/1799413002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:641:
GURL scope = GURL("http://www.example.com/service_worker.js");
script_url

https://codereview.chromium.org/1799413002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:673:
ASSERT_EQ(SERVICE_WORKER_OK, status);
Maybe it's time to refactor lines 640-673 and the similar ones in
CleanupOnRendererCrash and DispatchExtendableMessageEvent_Fail into a helper
function.

https://codereview.chromium.org/1799413002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:713:
GURL scope = GURL("http://www.example.com/service_worker.js");
script_url

[nhiroki, 2016-03-17 07:58:18]

Updated. Thank you!

https://codereview.chromium.org/1799413002/diff/100001/content/browser/servic...
File content/browser/service_worker/service_worker_dispatcher_host_unittest.cc
(right):

https://codereview.chromium.org/1799413002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:641:
GURL scope = GURL("http://www.example.com/service_worker.js");
On 2016/03/17 06:44:36, falken wrote:
> script_url

Good eye. I wrote this line while I was thinking about whether I should replace
'pattern' with 'scope'. That's the reason why I made a mistake :p

https://codereview.chromium.org/1799413002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:673:
ASSERT_EQ(SERVICE_WORKER_OK, status);
On 2016/03/17 06:44:36, falken wrote:
> Maybe it's time to refactor lines 640-673 and the similar ones in
> CleanupOnRendererCrash and DispatchExtendableMessageEvent_Fail into a helper
> function.

Done.

https://codereview.chromium.org/1799413002/diff/100001/content/browser/servic...
content/browser/service_worker/service_worker_dispatcher_host_unittest.cc:713:
GURL scope = GURL("http://www.example.com/service_worker.js");
On 2016/03/17 06:44:36, falken wrote:
> script_url

Done.

[nhiroki, 2016-03-17 07:59:25]

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

[nhiroki, 2016-03-17 07:59:26]

+kinuko@ for content/browser/message_port_service.{cc.h}

PTAL, thanks!

[nhiroki, 2016-03-18 09:16:25]

On 2016/03/17 07:59:26, nhiroki wrote:
> +kinuko@ for content/browser/message_port_service.{cc.h}
> 
> PTAL, thanks!

ping?

[kinuko, 2016-03-19 06:35:05]

lgtm

[nhiroki, 2016-03-19 06:51:14]

The CQ bit was checked by nhiroki@chromium.org

[nhiroki, 2016-03-19 06:51:15]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org
Link to the patchset: https://codereview.chromium.org/1799413002/#ps140001
(title: "fix build failures")

[commit-bot: I haz the power, 2016-03-19 06:51:27]

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

[commit-bot: I haz the power, 2016-03-19 09:56:50]

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

[commit-bot: I haz the power, 2016-03-19 09:56:51]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[nhiroki, 2016-03-21 23:18:02]

The CQ bit was checked by nhiroki@chromium.org

[commit-bot: I haz the power, 2016-03-21 23:18:22]

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

[commit-bot: I haz the power, 2016-03-22 00:26:41]

Description was changed from

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this have not happened in release builds.

BUG=543198
==========

to

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this have not happened in release builds.

BUG=543198
==========

[commit-bot: I haz the power, 2016-03-22 00:26:42]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2016-03-22 00:28:51]

Description was changed from

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this have not happened in release builds.

BUG=543198
==========

to

==========
ServiceWorker: Release a reference when it fails to dispatch
ExtendableMessageEvent

This CL makes sure to release a reference to ServiceWorkerHandle that was
supposed to be sent to the destination.

Note: ExtendableMessageEvent is still behind a flag and resource leaks due to
this have not happened in release builds.

BUG=543198

Committed: https://crrev.com/f6da9a0a7eb3a919aa29749017321c9bae8bfa8b
Cr-Commit-Position: refs/heads/master@{#382455}
==========

[commit-bot: I haz the power, 2016-03-22 00:28:52]

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