ServiceWorker: Add a check if error handler runs on the right thread

ServiceWorker: Add a check if error handler runs on the right thread

As observation in https://crbug.com/658702, I found the mojo error handler
(EWInstance::Detach in this case) is called on the UI thread when the message
loop on the UI thread gets destructed though EWInstance is on the IO
thread. This patch is to check the running thread before the error handling to
avoid unexpected threading issues.

BUG=658702, 604762
Committed: https://crrev.com/be675544ee93b8f940230861a8a4ae1dbbf2ebc7
Cr-Commit-Position: refs/heads/master@{#430208}

[shimazu, 2016-11-02 05:25:38]

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

[commit-bot: I haz the power, 2016-11-02 05:25:45]

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

[shimazu, 2016-11-02 05:26:52]

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

[shimazu, 2016-11-02 05:26:53]

ptal

[falken, 2016-11-02 05:31:02]

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

[falken, 2016-11-02 05:31:02]

drive-by comment: The code change is hard to understand from the CL description.
It's removing an if() statement but the CL description is "add a check". I'm
also not sure how "error handler" relates to the function being changed
EmbeddedWorkerInstance::OnDetach. Can this be explained more? :)

[commit-bot: I haz the power, 2016-11-02 06:29:52]

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

[commit-bot: I haz the power, 2016-11-02 06:29:52]

Dry run: This issue passed the CQ dry run.

[shimazu, 2016-11-02 07:32:21]

Description was changed from

==========
ServiceWorker: Add a check if error handler runs on the right thread

As observation in https://crbug.com/658702, I found the error handler
gets called on the UI thread though EWInstance is on the IO thread. This
patch is to check the running thread before the error handling to
terminate the shutdown sequence correctly.

BUG=658702,604762
==========

to

==========
ServiceWorker: Add a check if error handler runs on the right thread

As observation in https://crbug.com/658702, I found the mojo error handler
(EWInstance::Detach in this case) is called on the UI thread when the message
loop on the UI thread gets destructed though EWInstance is on the IO
thread. This patch is to check the running thread before the error handling to
avoid unexpected threading issues.

BUG=658702,604762
==========

[shimazu, 2016-11-02 07:34:34]

On 2016/11/02 05:31:02, falken wrote:
> drive-by comment: The code change is hard to understand from the CL
description.
> It's removing an if() statement but the CL description is "add a check". I'm
> also not sure how "error handler" relates to the function being changed
> EmbeddedWorkerInstance::OnDetach. Can this be explained more? :)


Sorry for lack of information. I tried to describe more. PTAL again.

[falken, 2016-11-02 07:43:14]

On 2016/11/02 07:34:34, shimazu wrote:
> On 2016/11/02 05:31:02, falken wrote:
> > drive-by comment: The code change is hard to understand from the CL
> description.
> > It's removing an if() statement but the CL description is "add a check". I'm
> > also not sure how "error handler" relates to the function being changed
> > EmbeddedWorkerInstance::OnDetach. Can this be explained more? :)
> 
> 
> Sorry for lack of information. I tried to describe more. PTAL again.

Sorry, I still don't understand how removing the if() statement == adding a
check. Does the check happen in one of next functions?
821   registry_->DetachWorker(process_id(), embedded_worker_id());
822   OnDetached();

Is the check a DCHECK?

[horo, 2016-11-02 07:49:13]

https://codereview.chromium.org/2467223002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2467223002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:820: return;
Please add DCHECK for mojo sw.

if (!BrowserThread::CurrentlyOn(BrowserThread::IO)) {
  DCHECK(ServiceWorkerUtils::IsMojoForServiceWorkerEnabled());
  return;
}

[falken, 2016-11-02 07:49:39]

Um, sorry I totally misread the patch. Somehow I thought it's deleting the code
but it's adding the code. So please disregard my last comment.

[nhiroki, 2016-11-02 07:59:50]

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

[nhiroki, 2016-11-02 07:59:50]

https://codereview.chromium.org/2467223002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2467223002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:817: // message loop
on the UI thread gets destructed.
Making an indirection could be saner than directly calling
EmbeddedWorkerInstance's method on an unexpected thread. For example,

namespace {
void CallDetach(EmbeddedWorkerInstance* instance) {
  // This could be...
  if (!BrowserThread::CurrentlyOn(BrowserThread::IO))
    return;
  instance->Detach();
}
}  // namespace

then

client_.set_connection_error_handler(base::Bind(&CallDetach,
base::Unretained(this)));

[horo, 2016-11-02 08:22:54]

https://codereview.chromium.org/2467223002/diff/1/content/browser/service_wor...
File content/browser/service_worker/embedded_worker_instance.cc (right):

https://codereview.chromium.org/2467223002/diff/1/content/browser/service_wor...
content/browser/service_worker/embedded_worker_instance.cc:817: // message loop
on the UI thread gets destructed.
On 2016/11/02 07:59:50, nhiroki wrote:
> Making an indirection could be saner than directly calling
> EmbeddedWorkerInstance's method on an unexpected thread. For example,
> 
> namespace {
> void CallDetach(EmbeddedWorkerInstance* instance) {
>   // This could be...
>   if (!BrowserThread::CurrentlyOn(BrowserThread::IO))
>     return;
>   instance->Detach();
> }
> }  // namespace
> 
> then
> 
> client_.set_connection_error_handler(base::Bind(&CallDetach,
> base::Unretained(this)));

+1

[shimazu, 2016-11-02 08:30:39]

On 2016/11/02 08:22:54, horo wrote:
>
https://codereview.chromium.org/2467223002/diff/1/content/browser/service_wor...
> File content/browser/service_worker/embedded_worker_instance.cc (right):
> 
>
https://codereview.chromium.org/2467223002/diff/1/content/browser/service_wor...
> content/browser/service_worker/embedded_worker_instance.cc:817: // message
loop
> on the UI thread gets destructed.
> On 2016/11/02 07:59:50, nhiroki wrote:
> > Making an indirection could be saner than directly calling
> > EmbeddedWorkerInstance's method on an unexpected thread. For example,
> > 
> > namespace {
> > void CallDetach(EmbeddedWorkerInstance* instance) {
> >   // This could be...
> >   if (!BrowserThread::CurrentlyOn(BrowserThread::IO))
> >     return;
> >   instance->Detach();
> > }
> > }  // namespace
> > 
> > then
> > 
> > client_.set_connection_error_handler(base::Bind(&CallDetach,
> > base::Unretained(this)));
> 
> +1

+1 too. Updated.

[nhiroki, 2016-11-02 08:42:40]

lgtm

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

https://codereview.chromium.org/2467223002/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:124: void
CallDetach(EmbeddedWorkerInstance* instance) {
Just to confirm: A raw pointer is safe here because a callback bound with
CallDetach() is owned by EmbeddedWorkerInstance::client_ that has the same
lifetime with EmbeddedWorkerInstance.

[shimazu, 2016-11-02 08:48:05]

On 2016/11/02 08:42:40, nhiroki wrote:
> lgtm
> 
>
https://codereview.chromium.org/2467223002/diff/40001/content/browser/service...
> File content/browser/service_worker/embedded_worker_instance.cc (right):
> 
>
https://codereview.chromium.org/2467223002/diff/40001/content/browser/service...
> content/browser/service_worker/embedded_worker_instance.cc:124: void
> CallDetach(EmbeddedWorkerInstance* instance) {
> Just to confirm: A raw pointer is safe here because a callback bound with
> CallDetach() is owned by EmbeddedWorkerInstance::client_ that has the same
> lifetime with EmbeddedWorkerInstance.

Thanks.
Yes, I think so.

[horo, 2016-11-04 01:06:21]

lgtm

[shimazu, 2016-11-04 01:09:22]

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

https://codereview.chromium.org/2467223002/diff/40001/content/browser/service...
content/browser/service_worker/embedded_worker_instance.cc:124: void
CallDetach(EmbeddedWorkerInstance* instance) {
On 2016/11/02 08:42:39, nhiroki (OOO until Nov 4) wrote:
> Just to confirm: A raw pointer is safe here because a callback bound with
> CallDetach() is owned by EmbeddedWorkerInstance::client_ that has the same
> lifetime with EmbeddedWorkerInstance.

Done.

[shimazu, 2016-11-04 01:09:30]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2016-11-04 01:10:09]

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

[commit-bot: I haz the power, 2016-11-04 04:11:54]

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

[commit-bot: I haz the power, 2016-11-04 04:11:55]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[shimazu, 2016-11-04 05:04:41]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2016-11-04 05:05:07]

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

[commit-bot: I haz the power, 2016-11-04 08:34:46]

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

[commit-bot: I haz the power, 2016-11-04 08:34:47]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[shimazu, 2016-11-06 23:53:11]

The CQ bit was checked by shimazu@chromium.org

[commit-bot: I haz the power, 2016-11-06 23:53:21]

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

[commit-bot: I haz the power, 2016-11-07 01:38:41]

Description was changed from

==========
ServiceWorker: Add a check if error handler runs on the right thread

As observation in https://crbug.com/658702, I found the mojo error handler
(EWInstance::Detach in this case) is called on the UI thread when the message
loop on the UI thread gets destructed though EWInstance is on the IO
thread. This patch is to check the running thread before the error handling to
avoid unexpected threading issues.

BUG=658702,604762
==========

to

==========
ServiceWorker: Add a check if error handler runs on the right thread

As observation in https://crbug.com/658702, I found the mojo error handler
(EWInstance::Detach in this case) is called on the UI thread when the message
loop on the UI thread gets destructed though EWInstance is on the IO
thread. This patch is to check the running thread before the error handling to
avoid unexpected threading issues.

BUG=658702,604762
==========

[commit-bot: I haz the power, 2016-11-07 01:38:42]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-07 01:47:55]

Description was changed from

==========
ServiceWorker: Add a check if error handler runs on the right thread

As observation in https://crbug.com/658702, I found the mojo error handler
(EWInstance::Detach in this case) is called on the UI thread when the message
loop on the UI thread gets destructed though EWInstance is on the IO
thread. This patch is to check the running thread before the error handling to
avoid unexpected threading issues.

BUG=658702,604762
==========

to

==========
ServiceWorker: Add a check if error handler runs on the right thread

As observation in https://crbug.com/658702, I found the mojo error handler
(EWInstance::Detach in this case) is called on the UI thread when the message
loop on the UI thread gets destructed though EWInstance is on the IO
thread. This patch is to check the running thread before the error handling to
avoid unexpected threading issues.

BUG=658702,604762

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

[commit-bot: I haz the power, 2016-11-07 01:47:56]

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