ServiceWorker: Mojofication of EWInstance::StopWorker

ServiceWorker: Mojofication of EWInstance::StopWorker

In this patch, EWInstance will send StopWorker IPC using mojo. Mojo has
their own error handling mechanism, and in this case errors will be handled as
detach.

BUG=629701
Committed: https://crrev.com/6ea839368fd4dbff868df10ef18fc3a6acdda2be
Cr-Commit-Position: refs/heads/master@{#424707}

[shimazu, 2016-09-02 03:16:05]

shimazu@chromium.org changed reviewers:
+ dcheng@chromium.org, horo@chromium.org

[shimazu, 2016-09-02 03:16:06]

ptal

[horo, 2016-09-05 08:05:57]

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:788:
registry_->DetachWorker(process_id(), embedded_worker_id());
Could you please explain why you don't erase the EmbeddedWorker from
worker_map_?

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_mojo_unittest.cc
(right):

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_mojo_unittest.cc:67:
class MojoEmbeddedWorkerInstanceTest : public testing::Test,
You can use TEST_P instead of duplicating tests.
See CacheStorageCacheTestP for example.

https://codereview.chromium.org/2307543002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2307543002/diff/20001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:28:
DCHECK(embedded_worker_id_);
DCHECK(stop_callback_);

https://codereview.chromium.org/2307543002/diff/20001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:77: if
(stop_callback_)
Is it possible to call StopWorker() twice?
If not, this should be DCHECK(!stop_callback_).

[shimazu, 2016-09-06 08:53:59]

Updated. PTAL again.

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:788:
registry_->DetachWorker(process_id(), embedded_worker_id());
On 2016/09/05 08:05:57, horo wrote:
> Could you please explain why you don't erase the EmbeddedWorker from
> worker_map_?

If I understand correctly, EmbeddedWorkerInstance is alive though it's stopped
or failing to send a StartWorker message except for Detach. 
The only user of Detach() is SWVersion::OnTimeoutTimer [1], but I think
OnTimeoutTimer doesn't intend to kill the instance, but intend to annotate
EWInstance as "detached".

Plus, I'd like to use Detach() as the error handler of mojo in this patch
because connection failures should be handled as "detach". However, if Detach()
removes the instance by calling worker_map_.erase(), the object failing IPC will
get deleted. I think this doesn't seem expected behavior because an unit test
[2] calls worker->Start() after failing IPC.

I don't have 100% confidence of this observation, so please tell me if my
understanding is incorrect.


[1]
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w...

[2]
https://cs.chromium.org/chromium/src/content/browser/service_worker/embedded_...

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_mojo_unittest.cc
(right):

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_mojo_unittest.cc:67:
class MojoEmbeddedWorkerInstanceTest : public testing::Test,
On 2016/09/05 08:05:57, horo wrote:
> You can use TEST_P instead of duplicating tests.
> See CacheStorageCacheTestP for example.

I didn't know that. Tried it.

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:483:
EXPECT_EQ(EmbeddedWorkerStatus::STARTING, events_[0].status);
I removed this expectation because this is actually not what we expect. Is this
right?

https://codereview.chromium.org/2307543002/diff/20001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_instance_client_impl.cc
(right):

https://codereview.chromium.org/2307543002/diff/20001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:28:
DCHECK(embedded_worker_id_);
On 2016/09/05 08:05:57, horo wrote:
> DCHECK(stop_callback_);

Done.

https://codereview.chromium.org/2307543002/diff/20001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_instance_client_impl.cc:77: if
(stop_callback_)
On 2016/09/05 08:05:57, horo wrote:
> Is it possible to call StopWorker() twice?
> If not, this should be DCHECK(!stop_callback_).

No, is not. Changed it.

[horo, 2016-09-13 08:25:33]

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:788:
registry_->DetachWorker(process_id(), embedded_worker_id());
On 2016/09/06 08:53:59, shimazu wrote:
> On 2016/09/05 08:05:57, horo wrote:
> > Could you please explain why you don't erase the EmbeddedWorker from
> > worker_map_?
> 
> If I understand correctly, EmbeddedWorkerInstance is alive though it's stopped
> or failing to send a StartWorker message except for Detach. 
> The only user of Detach() is SWVersion::OnTimeoutTimer [1], but I think
> OnTimeoutTimer doesn't intend to kill the instance, but intend to annotate
> EWInstance as "detached".


> Plus, I'd like to use Detach() as the error handler of mojo in this patch
> because connection failures should be handled as "detach". However, if
Detach()
> removes the instance by calling worker_map_.erase(), the object failing IPC
will
> get deleted. I think this doesn't seem expected behavior because an unit test
> [2] calls worker->Start() after failing IPC.



> I don't have 100% confidence of this observation, so please tell me if my
> understanding is incorrect.
> 
> 
> [1]
>
https://cs.chromium.org/chromium/src/content/browser/service_worker/service_w...
> 
> [2]
>
https://cs.chromium.org/chromium/src/content/browser/service_worker/embedded_...

I see.

But I think the name of the error handler should be OnConnectionError().

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:483:
EXPECT_EQ(EmbeddedWorkerStatus::STARTING, events_[0].status);
On 2016/09/06 08:53:59, shimazu wrote:
> I removed this expectation because this is actually not what we expect. Is
this
> right?

  if (is_mojo_enabled()) {
    // EmbeddedWorkerInstance must be detached while executing RunUntilIdle.
    EXPECT_EQ(EmbeddedWorkerStatus::STOPPING, events_[0].status);
  } else {
    EXPECT_EQ(EmbeddedWorkerStatus::STARTING, events_[0].status);
  }

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

https://codereview.chromium.org/2307543002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:481: if (!client_) {
How this happens?

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

https://codereview.chromium.org/2307543002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:237:
ASSERT_TRUE(
Keep this comment.

// StopWorker should be sent in either case.

https://codereview.chromium.org/2307543002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_dispatcher.cc (right):

https://codereview.chromium.org/2307543002/diff/60001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_dispatcher.cc:176: LOG(WARNING)
<< "Got OnStopWorker for nonexistent worker";
Is it possible?

https://codereview.chromium.org/2307543002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_dispatcher.h (right):

https://codereview.chromium.org/2307543002/diff/60001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_dispatcher.h:81: bool
SetStopWorkerTimer(int embedded_worker_id);
It doesn't set a timer.

RecordStopWorkerTime is better.

[shimazu, 2016-09-20 04:56:21]

Updated. PTAL again:)

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

https://codereview.chromium.org/2307543002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:481: if (!client_) {
On 2016/09/13 08:25:32, horo(ooo until 11) wrote:
> How this happens?

This will happen when calling Stop before Start, but this doesn't happen.
Removed.

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

https://codereview.chromium.org/2307543002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:237:
ASSERT_TRUE(
On 2016/09/13 08:25:33, horo(ooo until 11) wrote:
> Keep this comment.
> 
> // StopWorker should be sent in either case.

The comment means StopWorker massage should be sent via chromium IPC before
mojofication. Now it's mojofied, so I think it's okay to remove the comment.

but this might be confusing. Updated the comment.

https://codereview.chromium.org/2307543002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_dispatcher.cc (right):

https://codereview.chromium.org/2307543002/diff/60001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_dispatcher.cc:176: LOG(WARNING)
<< "Got OnStopWorker for nonexistent worker";
On 2016/09/13 08:25:33, horo wrote:
> Is it possible?

I think it's not possible because EmbeddedWorkerInstance can't fire StopWorker
twice.
Can I remove this branch? (and I guess the branch in OnStopWorker can be also
removed?)

https://codereview.chromium.org/2307543002/diff/60001/content/renderer/servic...
File content/renderer/service_worker/embedded_worker_dispatcher.h (right):

https://codereview.chromium.org/2307543002/diff/60001/content/renderer/servic...
content/renderer/service_worker/embedded_worker_dispatcher.h:81: bool
SetStopWorkerTimer(int embedded_worker_id);
On 2016/09/13 08:25:33, horo wrote:
> It doesn't set a timer.
> 
> RecordStopWorkerTime is better.

Done.

[horo, 2016-09-27 01:59:23]

Sorry for the delay.

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:483:
EXPECT_EQ(EmbeddedWorkerStatus::STARTING, events_[0].status);
On 2016/09/13 08:25:32, horo wrote:
> On 2016/09/06 08:53:59, shimazu wrote:
> > I removed this expectation because this is actually not what we expect. Is
> this
> > right?
> 
>   if (is_mojo_enabled()) {
>     // EmbeddedWorkerInstance must be detached while executing RunUntilIdle.
>     EXPECT_EQ(EmbeddedWorkerStatus::STOPPING, events_[0].status);
>   } else {
>     EXPECT_EQ(EmbeddedWorkerStatus::STARTING, events_[0].status);
>   }

How about this?

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

https://codereview.chromium.org/2307543002/diff/100001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:32: void
NoOpStatusCallback(ServiceWorkerStatusCode code) {}
You can use ServiceWorkerUtils::NoOpStatusCallback

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:448: if
(embedded_worker_client_) {
if (ServiceWorkerUtils::IsMojoForServiceWorkerEnabled()) {
  DCHECK(embedded_worker_client_);
  ...
}

[dcheng, 2016-09-30 08:25:29]

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

https://codereview.chromium.org/2307543002/diff/100001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:463:
base::Bind(&EmbeddedWorkerInstance::Detach, AsWeakPtr()));
It should be unnecessary to use AsWeakPtr() here.

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.h
(right):

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.h:32: //
Implementation of mojo interface
Nit: This is usually worded as:
// mojom::EmbeddedWorkerInstanceClient implementation:

(It's nice to refer to the exact type, sinceit's possible for one interface to
implement multiple mojo bindings)

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.h:45:
std::unique_ptr<EmbeddedWorkerInstanceClientImpl> temporal_self_;
This is a bit too clever. Let me try to think if there is a better way...

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:452:
base::Passed(std::move(embedded_worker_client_))));
base::Passed(&embedded_worker_client_)

[shimazu, 2016-10-06 03:50:52]

Thanks for your reviews!
PTAL again.

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

https://codereview.chromium.org/2307543002/diff/100001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance.cc:463:
base::Bind(&EmbeddedWorkerInstance::Detach, AsWeakPtr()));
On 2016/09/30 08:25:28, dcheng wrote:
> It should be unnecessary to use AsWeakPtr() here.

Done.

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

https://codereview.chromium.org/2307543002/diff/100001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:32: void
NoOpStatusCallback(ServiceWorkerStatusCode code) {}
On 2016/09/27 01:59:23, horo wrote:
> You can use ServiceWorkerUtils::NoOpStatusCallback

Done.

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
File content/renderer/service_worker/embedded_worker_instance_client_impl.h
(right):

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.h:32: //
Implementation of mojo interface
On 2016/09/30 08:25:28, dcheng wrote:
> Nit: This is usually worded as:
> // mojom::EmbeddedWorkerInstanceClient implementation:
> 
> (It's nice to refer to the exact type, sinceit's possible for one interface to
> implement multiple mojo bindings)

Done.

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
content/renderer/service_worker/embedded_worker_instance_client_impl.h:45:
std::unique_ptr<EmbeddedWorkerInstanceClientImpl> temporal_self_;
On 2016/09/30 08:25:28, dcheng wrote:
> This is a bit too clever. Let me try to think if there is a better way...

I think this is needed because currently no browser-side param can be passed to
the constructor (Create). If it's solved, I can remove this by passing a
EmbeddedWorkerStartParams to Create() and launching the worker at the same time.

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
File content/renderer/service_worker/service_worker_context_client.cc (right):

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:448: if
(embedded_worker_client_) {
On 2016/09/27 01:59:23, horo wrote:
> if (ServiceWorkerUtils::IsMojoForServiceWorkerEnabled()) {
>   DCHECK(embedded_worker_client_);
>   ...
> }

Done.

https://codereview.chromium.org/2307543002/diff/100001/content/renderer/servi...
content/renderer/service_worker/service_worker_context_client.cc:452:
base::Passed(std::move(embedded_worker_client_))));
On 2016/09/30 08:25:28, dcheng wrote:
> base::Passed(&embedded_worker_client_)

Done.

[horo, 2016-10-06 07:38:29]

lgtm

https://codereview.chromium.org/2307543002/diff/120001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2307543002/diff/120001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:519:
events_.clear();
Please see
https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...

[shimazu, 2016-10-12 08:33:13]

Updated.
dcheng@: ptal again?

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...
content/browser/service_worker/embedded_worker_instance_unittest.cc:483:
EXPECT_EQ(EmbeddedWorkerStatus::STARTING, events_[0].status);
On 2016/09/27 01:59:23, horo wrote:
> On 2016/09/13 08:25:32, horo wrote:
> > On 2016/09/06 08:53:59, shimazu wrote:
> > > I removed this expectation because this is actually not what we expect. Is
> > this
> > > right?
> > 
> >   if (is_mojo_enabled()) {
> >     // EmbeddedWorkerInstance must be detached while executing RunUntilIdle.
> >     EXPECT_EQ(EmbeddedWorkerStatus::STOPPING, events_[0].status);
> >   } else {
> >     EXPECT_EQ(EmbeddedWorkerStatus::STARTING, events_[0].status);
> >   }
> 
> How about this?

Done.

https://codereview.chromium.org/2307543002/diff/120001/content/browser/servic...
File content/browser/service_worker/embedded_worker_instance_unittest.cc
(right):

https://codereview.chromium.org/2307543002/diff/120001/content/browser/servic...
content/browser/service_worker/embedded_worker_instance_unittest.cc:519:
events_.clear();
On 2016/10/06 07:38:29, horo wrote:
> Please see
>
https://codereview.chromium.org/2307543002/diff/20001/content/browser/service...

Sorry, I missed your comment;(
Done.

[shimazu, 2016-10-12 08:33:26]

The CQ bit was checked by shimazu@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 08:33:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dcheng, 2016-10-12 08:41:30]

Well, I really don't like the funny business with std::unique_ptr, but I guess
that shouldn't block this CL, since I can't think of anything better either =(

mojo LGTM

[shimazu, 2016-10-12 08:58:37]

The CQ bit was unchecked by shimazu@chromium.org

[shimazu, 2016-10-12 08:58:37]

The CQ bit was checked by shimazu@chromium.org

[shimazu, 2016-10-12 08:58:38]

The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org
Link to the patchset: https://codereview.chromium.org/2307543002/#ps140001
(title: "Added a check of the status when detached")

[commit-bot: I haz the power, 2016-10-12 08:58:52]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[shimazu, 2016-10-12 09:04:29]

On 2016/10/12 08:41:30, dcheng wrote:
> Well, I really don't like the funny business with std::unique_ptr, but I guess
> that shouldn't block this CL, since I can't think of anything better either =(
> 
> mojo LGTM

Thanks for your review.
If you come up with some good idea, please let me know! :)

[commit-bot: I haz the power, 2016-10-12 10:40:29]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-10-12 10:44:02]

Description was changed from

==========
ServiceWorker: Mojofication of EWInstance::StopWorker

In this patch, EWInstance will send StopWorker IPC using mojo. Mojo has
their own error handling mechanism, and in this case errors will be handled as
detach.

BUG=629701
==========

to

==========
ServiceWorker: Mojofication of EWInstance::StopWorker

In this patch, EWInstance will send StopWorker IPC using mojo. Mojo has
their own error handling mechanism, and in this case errors will be handled as
detach.

BUG=629701

Committed: https://crrev.com/6ea839368fd4dbff868df10ef18fc3a6acdda2be
Cr-Commit-Position: refs/heads/master@{#424707}
==========

[commit-bot: I haz the power, 2016-10-12 10:44:04]

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