Make mojo::Callback safe to copy across threads.

Make mojo::Callback safe to copy across threads and destroy on any thread.

This brings the behaviour of mojo::Callback close to that of base::Callback.

BUG=595939
Committed: https://crrev.com/314a150a7a17639873137ba079a4a26785242273
Cr-Commit-Position: refs/heads/master@{#382349}

[Anand Mistry (off Chromium), 2016-03-18 20:31:21]

Description was changed from

==========
Make mojo::Callback safe to copy across threads.

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

to

==========
Make mojo::Callback safe to copy across threads and destroy on any thread.

This brings the behaviour of mojo::Callback close to that of base::Callback.

BUG=595939
==========

[Anand Mistry (off Chromium), 2016-03-18 20:52:42]

amistry@chromium.org changed reviewers:
+ rockot@chromium.org, yzshen@chromium.org

[Anand Mistry (off Chromium), 2016-03-18 20:52:42]

WDYT?

[Ken Rockot(use gerrit already), 2016-03-18 21:56:23]

lgtm

[Anand Mistry (off Chromium), 2016-03-18 22:05:11]

amistry@chromium.org changed reviewers:
+ sammc@chromium.org

[Anand Mistry (off Chromium), 2016-03-18 22:05:12]

sam: If you have any insights on this issue.

[yzshen1, 2016-03-18 23:47:04]

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/router.cc (right):

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:26: const std::string message) {
"const &"?

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:28: NOTREACHED() << message;
nit: maybe consider DCHECK(!the_current_condition)?

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:46: router_->RaiseError();
In interface_endpoint_client.cc (which is used for the associated interface case
-- I really need to unify the code!), I post a task to raise error even on the
same thread. That way it won't break the case where the user abandons the
interface ptr/binding soon after he/she abandons the callback. For example, if
the callback and the interface ptr/binding are both destroyed by a class'
destructor, the user doesn't have to worry about which is destroyed first.

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:75: void DCheckValid(const std::string&
message) override {
Please do the same thing to the ResponderThunk in interface_endpoint_client.cc

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/message.h (right):

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/message.h:139: virtual bool IsValid() = 0;
Is this method still needed?

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/message.h:142: // pipe has not been closed, and the
pipe has not encountered an error.
Could you please comment that this may be called from a different thread?

[Anand Mistry (off Chromium), 2016-03-19 01:02:14]

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/router.cc (right):

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:26: const std::string message) {
On 2016/03/18 23:47:04, yzshen1 wrote:
> "const &"?

Done.

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:28: NOTREACHED() << message;
On 2016/03/18 23:47:04, yzshen1 wrote:
> nit: maybe consider DCHECK(!the_current_condition)?

Done.

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:46: router_->RaiseError();
On 2016/03/18 23:47:04, yzshen1 wrote:
> In interface_endpoint_client.cc (which is used for the associated interface
case
> -- I really need to unify the code!), I post a task to raise error even on the
> same thread. That way it won't break the case where the user abandons the
> interface ptr/binding soon after he/she abandons the callback. For example, if
> the callback and the interface ptr/binding are both destroyed by a class'
> destructor, the user doesn't have to worry about which is destroyed first.

I traced the code down to Connector and noticed that Connector::RaiseError()
calls Connector::HandleError() with force_async_handler = true. This should
force any error handler to be posted instead of run synchronously. You're
concerned about error handler callbacks, right?

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:75: void DCheckValid(const std::string&
message) override {
On 2016/03/18 23:47:04, yzshen1 wrote:
> Please do the same thing to the ResponderThunk in interface_endpoint_client.cc

Done.

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/message.h (right):

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/message.h:139: virtual bool IsValid() = 0;
On 2016/03/18 23:47:04, yzshen1 wrote:
> Is this method still needed?

Yes... for unittests. I'll leave it in for now and take a look at the tests
separately. I think we'll want to keep it so we can do tests without them
crashing.

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/message.h:142: // pipe has not been closed, and the
pipe has not encountered an error.
On 2016/03/18 23:47:04, yzshen1 wrote:
> Could you please comment that this may be called from a different thread?

Done.

[Sam McNally, 2016-03-19 01:03:53]

lgtm

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/callback.h (right):

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/callback.h:134: friend class
base::RefCountedThreadSafe<RunnableHolder>;
Friends before destructors.

[Anand Mistry (off Chromium), 2016-03-19 01:09:36]

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/callback.h (right):

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/callback.h:134: friend class
base::RefCountedThreadSafe<RunnableHolder>;
On 2016/03/19 01:03:53, Sam McNally wrote:
> Friends before destructors.

Done.

[yzshen1, 2016-03-19 01:29:54]

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/router.cc (right):

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:46: router_->RaiseError();
On 2016/03/19 01:02:13, Anand Mistry wrote:
> On 2016/03/18 23:47:04, yzshen1 wrote:
> > In interface_endpoint_client.cc (which is used for the associated interface
> case
> > -- I really need to unify the code!), I post a task to raise error even on
the
> > same thread. That way it won't break the case where the user abandons the
> > interface ptr/binding soon after he/she abandons the callback. For example,
if
> > the callback and the interface ptr/binding are both destroyed by a class'
> > destructor, the user doesn't have to worry about which is destroyed first.
> 
> I traced the code down to Connector and noticed that Connector::RaiseError()
> calls Connector::HandleError() with force_async_handler = true. This should
> force any error handler to be posted instead of run synchronously. You're
> concerned about error handler callbacks, right?

I think you are right. :)
So maybe we should change InterfaceEndpointClient to avoid posting task when it
is on the same thread.

Thanks for pointing this out.

https://codereview.chromium.org/1819463002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/interface_endpoint_client.cc (right):

https://codereview.chromium.org/1819463002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:30: DCHECK(!is_valid)
<< message;
So this function and ResponderThunk::DCheckValid should probably be named
DCheck*Invalid*, right?

[Anand Mistry (off Chromium), 2016-03-19 03:56:54]

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/router.cc (right):

https://codereview.chromium.org/1819463002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/router.cc:46: router_->RaiseError();
On 2016/03/19 01:29:54, yzshen1 wrote:
> On 2016/03/19 01:02:13, Anand Mistry wrote:
> > On 2016/03/18 23:47:04, yzshen1 wrote:
> > > In interface_endpoint_client.cc (which is used for the associated
interface
> > case
> > > -- I really need to unify the code!), I post a task to raise error even on
> the
> > > same thread. That way it won't break the case where the user abandons the
> > > interface ptr/binding soon after he/she abandons the callback. For
example,
> if
> > > the callback and the interface ptr/binding are both destroyed by a class'
> > > destructor, the user doesn't have to worry about which is destroyed first.
> > 
> > I traced the code down to Connector and noticed that Connector::RaiseError()
> > calls Connector::HandleError() with force_async_handler = true. This should
> > force any error handler to be posted instead of run synchronously. You're
> > concerned about error handler callbacks, right?
> 
> I think you are right. :)
> So maybe we should change InterfaceEndpointClient to avoid posting task when
it
> is on the same thread.
> 
> Thanks for pointing this out.

Done.

https://codereview.chromium.org/1819463002/diff/80001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/interface_endpoint_client.cc (right):

https://codereview.chromium.org/1819463002/diff/80001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/interface_endpoint_client.cc:30: DCHECK(!is_valid)
<< message;
On 2016/03/19 01:29:54, yzshen1 wrote:
> So this function and ResponderThunk::DCheckValid should probably be named
> DCheck*Invalid*, right?

Done.

[yzshen1, 2016-03-21 05:42:19]

lgtm

[Anand Mistry (off Chromium), 2016-03-21 16:57:01]

The CQ bit was checked by amistry@chromium.org

[Anand Mistry (off Chromium), 2016-03-21 16:57:02]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
sammc@chromium.org
Link to the patchset: https://codereview.chromium.org/1819463002/#ps100001
(title: "review comments")

[commit-bot: I haz the power, 2016-03-21 16:57:30]

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

[commit-bot: I haz the power, 2016-03-21 19:16:50]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-03-21 19:17:49]

Description was changed from

==========
Make mojo::Callback safe to copy across threads and destroy on any thread.

This brings the behaviour of mojo::Callback close to that of base::Callback.

BUG=595939
==========

to

==========
Make mojo::Callback safe to copy across threads and destroy on any thread.

This brings the behaviour of mojo::Callback close to that of base::Callback.

BUG=595939

Committed: https://crrev.com/314a150a7a17639873137ba079a4a26785242273
Cr-Commit-Position: refs/heads/master@{#382349}
==========

[commit-bot: I haz the power, 2016-03-21 19:17:50]

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