Protect RefCount manipulation of InterfaceEndpoint with MultiplexRouter lock

Protect RefCount manipulation of InterfaceEndpoint with MultiplexRouter lock

mojo::internal::InterfaceEndpoint has non-thread-safe ref count.
Most of the ref count manipulations are protected by MultiplexRouter::lock_,
but one in InterfaceEndpoint::OnHandleReady is not protected. That may cause
data race on the ref count.

Review-Url: https://codereview.chromium.org/2684213002
Cr-Commit-Position: refs/heads/master@{#449470}
Committed: https://chromium.googlesource.com/chromium/src/+/47ee91d0dc7bfc1afed32d6ce1d7c5311859d1d2

[tzik, 2017-02-09 09:12:23]

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

[commit-bot: I haz the power, 2017-02-09 09:12:38]

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

[tzik, 2017-02-09 09:32:10]

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

[commit-bot: I haz the power, 2017-02-09 09:32:42]

Dry run: 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, 2017-02-09 10:28:29]

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

[commit-bot: I haz the power, 2017-02-09 10:28:29]

Dry run: This issue passed the CQ dry run.

[tzik, 2017-02-09 10:41:41]

Description was changed from

==========
Protect RefCount manipulation of InterfaceEndpoint with MultiplexRouter lock
==========

to

==========
Protect RefCount manipulation of InterfaceEndpoint with MultiplexRouter lock

mojo::internal::InterfaceEndpoint has non-thread-safe ref count.
Most of the ref count manipulations are protected by MultiplexRouter::lock_,
but one in InterfaceEndpoint::OnHandleReady is not protected. That may cause
data race on the ref count.
==========

[tzik, 2017-02-09 10:41:41]

tzik@chromium.org changed reviewers:
+ yzshen@chromium.org

[tzik, 2017-02-09 10:45:59]

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

[tzik, 2017-02-09 10:46:04]

PTAL

[commit-bot: I haz the power, 2017-02-09 10:46:12]

Dry run: 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, 2017-02-09 12:04:23]

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

[commit-bot: I haz the power, 2017-02-09 12:04:24]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2017-02-09 17:16:51]

Thanks for finding out this issue! :)

I also checked that ipc_mojo_bootstrap.cc (the other AssociatedGroupController
impl) doesn't have the same problem, it uses RefCountedThreadSafe for its
endpoints.

LGTM with one nit.

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

https://codereview.chromium.org/2684213002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/multiplex_router.cc:197: MayAutoUnlock
unlocker(router_->lock_.get());
|sync_watcher_| is used exclusively on the |task_runner_|'s thread, so it is
safe to use inside/outside of lock. In this case, we may just directly reset
while holding the lock to save unlock/re-lock cost. Lock contention shouldn't be
an issue here.

[tzik, 2017-02-09 22:41:22]

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

https://codereview.chromium.org/2684213002/diff/40001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/multiplex_router.cc:197: MayAutoUnlock
unlocker(router_->lock_.get());
On 2017/02/09 17:16:50, yzshen1 wrote:
> |sync_watcher_| is used exclusively on the |task_runner_|'s thread, so it is
> safe to use inside/outside of lock. In this case, we may just directly reset
> while holding the lock to save unlock/re-lock cost. Lock contention shouldn't
be
> an issue here.

Done.

[tzik, 2017-02-09 22:41:30]

The CQ bit was checked by tzik@chromium.org

[tzik, 2017-02-09 22:41:30]

The patchset sent to the CQ was uploaded after l-g-t-m from yzshen@chromium.org
Link to the patchset: https://codereview.chromium.org/2684213002/#ps60001
(title: "-MayAutoUnlock")

[commit-bot: I haz the power, 2017-02-09 22:41:59]

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, 2017-02-09 23:51:26]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486680090521240,
"parent_rev": "284df53db5c6f01b9f41838cfa89f42672ed406a", "commit_rev":
"47ee91d0dc7bfc1afed32d6ce1d7c5311859d1d2"}

[commit-bot: I haz the power, 2017-02-09 23:52:02]

Description was changed from

==========
Protect RefCount manipulation of InterfaceEndpoint with MultiplexRouter lock

mojo::internal::InterfaceEndpoint has non-thread-safe ref count.
Most of the ref count manipulations are protected by MultiplexRouter::lock_,
but one in InterfaceEndpoint::OnHandleReady is not protected. That may cause
data race on the ref count.
==========

to

==========
Protect RefCount manipulation of InterfaceEndpoint with MultiplexRouter lock

mojo::internal::InterfaceEndpoint has non-thread-safe ref count.
Most of the ref count manipulations are protected by MultiplexRouter::lock_,
but one in InterfaceEndpoint::OnHandleReady is not protected. That may cause
data race on the ref count.

Review-Url: https://codereview.chromium.org/2684213002
Cr-Commit-Position: refs/heads/master@{#449470}
Committed:
https://chromium.googlesource.com/chromium/src/+/47ee91d0dc7bfc1afed32d6ce1d7...
==========

[commit-bot: I haz the power, 2017-02-09 23:52:02]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/47ee91d0dc7bfc1afed32d6ce1d7...