Mojo Bindings: Fix lock-order inversion in associated controllers

Mojo Bindings: Fix lock-order inversion in associated controllers

The two AssociatedGroupController implementations we have both own a lock
which is acquired during message dispatch, among other operations.
In the EDK layer a Watcher lock is also acquired further up the stack.

Because sending a message may indirectly require notifying the same
Watcher lock, it must never be true that the AssociatedGroupController's
lock is held while its pipe is written to.

This fixes the lock-order inversion resulting from the
fact that pipe control messages were being sent under lock, and removes
the associated TSAN suppression.

BUG=663557
TBR=glider@chromium.org

R=yzshen@chromium.org

Committed: https://crrev.com/58909542197b704efe10526c169e4b502f799b57
Cr-Commit-Position: refs/heads/master@{#431331}

[Ken Rockot(use gerrit already), 2016-11-10 00:45:31]

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

[commit-bot: I haz the power, 2016-11-10 00:45:56]

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

[Ken Rockot(use gerrit already), 2016-11-10 00:51:33]

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

[Ken Rockot(use gerrit already), 2016-11-10 00:51:51]

PTAL. Thanks!

[commit-bot: I haz the power, 2016-11-10 00:52:06]

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, 2016-11-10 01:53:02]

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

[commit-bot: I haz the power, 2016-11-10 01:53:03]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-11-10 18:02:34]

LGTM with one comment

https://codereview.chromium.org/2494483003/diff/20001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2494483003/diff/20001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:174:
control_message_proxy_.NotifyEndpointClosedBeforeSent(id);
PipeControlMessageProxy has a |context_| member which is not thread safe. For
those control messages, we actually don't use it in anyway (we don't have
associated interfaces, handles or custom types). So it shouldn't cause trouble.
But it seems nice to not rely on that. Instead, we could create a new context
object as local variable whenever we need to do serialization/deserialization.

And maybe it makes sense to add a comment to PipeControlMessageProxy saying that
the class could be used from multiple threads.

[Ken Rockot(use gerrit already), 2016-11-10 18:22:38]

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

[commit-bot: I haz the power, 2016-11-10 18:23:17]

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

[Ken Rockot(use gerrit already), 2016-11-10 18:26:24]

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

[Ken Rockot(use gerrit already), 2016-11-10 18:26:38]

Description was changed from

==========
Mojo Bindings: Fix lock-order inversion in associated controllers

The two AssociatedGroupController implementations we have both own a lock
which is acquired during message dispatch, among other operations.
In the EDK layer a Watcher lock is also acquired further up the stack.

Because sending a message may indirectly require notifying the same
Watcher lock, it must never be true that the AssociatedGroupController's
lock is held while its pipe is written to.

This fixes the lock-order inversion resulting from the
fact that pipe control messages were being sent under lock.

BUG=663557
R=yzshen@chromium.org
==========

to

==========
Mojo Bindings: Fix lock-order inversion in associated controllers

The two AssociatedGroupController implementations we have both own a lock
which is acquired during message dispatch, among other operations.
In the EDK layer a Watcher lock is also acquired further up the stack.

Because sending a message may indirectly require notifying the same
Watcher lock, it must never be true that the AssociatedGroupController's
lock is held while its pipe is written to.

This fixes the lock-order inversion resulting from the
fact that pipe control messages were being sent under lock, and removes
the associated TSAN suppression.

BUG=663557
TBR=glider@chromium.org

R=yzshen@chromium.org
==========

[commit-bot: I haz the power, 2016-11-10 18:26:40]

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

[Ken Rockot(use gerrit already), 2016-11-10 18:26:44]

https://codereview.chromium.org/2494483003/diff/20001/ipc/ipc_mojo_bootstrap.cc
File ipc/ipc_mojo_bootstrap.cc (right):

https://codereview.chromium.org/2494483003/diff/20001/ipc/ipc_mojo_bootstrap....
ipc/ipc_mojo_bootstrap.cc:174:
control_message_proxy_.NotifyEndpointClosedBeforeSent(id);
On 2016/11/10 at 18:02:34, yzshen1 wrote:
> PipeControlMessageProxy has a |context_| member which is not thread safe. For
those control messages, we actually don't use it in anyway (we don't have
associated interfaces, handles or custom types). So it shouldn't cause trouble.
But it seems nice to not rely on that. Instead, we could create a new context
object as local variable whenever we need to do serialization/deserialization.
> 
> And maybe it makes sense to add a comment to PipeControlMessageProxy saying
that the class could be used from multiple threads.

Done

[Ken Rockot(use gerrit already), 2016-11-10 18:27:19]

The CQ bit was unchecked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-11-10 18:27:24]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-11-10 18:27:24]

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/2494483003/#ps60001
(title: ".")

[commit-bot: I haz the power, 2016-11-10 18:28:00]

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-10 20:06:14]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-10 20:14:24]

Description was changed from

==========
Mojo Bindings: Fix lock-order inversion in associated controllers

The two AssociatedGroupController implementations we have both own a lock
which is acquired during message dispatch, among other operations.
In the EDK layer a Watcher lock is also acquired further up the stack.

Because sending a message may indirectly require notifying the same
Watcher lock, it must never be true that the AssociatedGroupController's
lock is held while its pipe is written to.

This fixes the lock-order inversion resulting from the
fact that pipe control messages were being sent under lock, and removes
the associated TSAN suppression.

BUG=663557
TBR=glider@chromium.org

R=yzshen@chromium.org
==========

to

==========
Mojo Bindings: Fix lock-order inversion in associated controllers

The two AssociatedGroupController implementations we have both own a lock
which is acquired during message dispatch, among other operations.
In the EDK layer a Watcher lock is also acquired further up the stack.

Because sending a message may indirectly require notifying the same
Watcher lock, it must never be true that the AssociatedGroupController's
lock is held while its pipe is written to.

This fixes the lock-order inversion resulting from the
fact that pipe control messages were being sent under lock, and removes
the associated TSAN suppression.

BUG=663557
TBR=glider@chromium.org

R=yzshen@chromium.org

Committed: https://crrev.com/58909542197b704efe10526c169e4b502f799b57
Cr-Commit-Position: refs/heads/master@{#431331}
==========

[commit-bot: I haz the power, 2016-11-10 20:14:25]

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