Eliminate deferred destruction of AssociatedGroupController

Eliminate deferred destruction of AssociatedGroupController

Moves AssociatedGroupController from base::RefCountedDeleteOnMessageLoop
to a simple base::RefCountedThreadSafe. Updates MultiplexRouter
and ChannelAssociatedGroupController expectations accordingly.

BUG=631491
R=yzshen@chromium.org

Committed: https://crrev.com/b01ef6ae6f781753a82e0d2857aaed43323bceae
Cr-Commit-Position: refs/heads/master@{#408032}

[Ken Rockot(use gerrit already), 2016-07-26 17:13:35]

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

[commit-bot: I haz the power, 2016-07-26 17:13:49]

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-07-26 17:15:03]

Please take a look. I opted to introduce a separate lock and flag for
Connector's connected state, so that we only need to synchronize access in
CloseMessagePipe and the destructor. WDYT?

[commit-bot: I haz the power, 2016-07-26 18:19:58]

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

[commit-bot: I haz the power, 2016-07-26 18:19:58]

Dry run: This issue passed the CQ dry run.

[yzshen1, 2016-07-26 21:21:16]

https://codereview.chromium.org/2185723002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/2185723002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/connector.cc:88: return std::move(message_pipe_);
When the message pipe is transferred, we need to set connected_ to false too.

https://codereview.chromium.org/2185723002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/multiplex_router.cc (left):

https://codereview.chromium.org/2185723002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/multiplex_router.cc:308: base::Unretained(this)));
It seems safe to use base::Unretained() here. The master interface endpoint will
hold a ref to the multiplex router and it will call CloseMessagePipe or
PassMessagePipe before it releases that ref.

If we let the connector holds a ref it is easier to leak. (I think it already
leaks if we call PassMessagePipe() because it doesn't reset the connection error
handler.)

So maybe it makes sense to keep base::Unretained() and add comments to
MultiplexRouter header file saying that CloseMessagePipe or PassMessageMessage
must be called on the master thread before destruction?

[Ken Rockot(use gerrit already), 2016-07-26 22:05:58]

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

[commit-bot: I haz the power, 2016-07-26 22:06:50]

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-07-26 22:07:00]

https://codereview.chromium.org/2185723002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/2185723002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/connector.cc:88: return std::move(message_pipe_);
On 2016/07/26 at 21:21:15, yzshen1 wrote:
> When the message pipe is transferred, we need to set connected_ to false too.

Done

https://codereview.chromium.org/2185723002/diff/1/mojo/public/cpp/bindings/li...
File mojo/public/cpp/bindings/lib/multiplex_router.cc (left):

https://codereview.chromium.org/2185723002/diff/1/mojo/public/cpp/bindings/li...
mojo/public/cpp/bindings/lib/multiplex_router.cc:308: base::Unretained(this)));
On 2016/07/26 at 21:21:15, yzshen1 wrote:
> It seems safe to use base::Unretained() here. The master interface endpoint
will hold a ref to the multiplex router and it will call CloseMessagePipe or
PassMessagePipe before it releases that ref.
> 
> If we let the connector holds a ref it is easier to leak. (I think it already
leaks if we call PassMessagePipe() because it doesn't reset the connection error
handler.)
> 
> So maybe it makes sense to keep base::Unretained() and add comments to
MultiplexRouter header file saying that CloseMessagePipe or PassMessageMessage
must be called on the master thread before destruction?

Done

[yzshen1, 2016-07-26 22:11:30]

One more thing before LGTM

https://codereview.chromium.org/2185723002/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/lib/connector.cc (right):

https://codereview.chromium.org/2185723002/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/lib/connector.cc:88: ScopedMessagePipeHandle
message_pipe = std::move(message_pipe_);
this should be returned.

[Ken Rockot(use gerrit already), 2016-07-26 22:12:10]

Oops. Haha, I should have actually compiled...

[commit-bot: I haz the power, 2016-07-26 22:14:18]

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

[commit-bot: I haz the power, 2016-07-26 22:14:20]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[Ken Rockot(use gerrit already), 2016-07-26 22:17:29]

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

[commit-bot: I haz the power, 2016-07-26 22:17:53]

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-07-26 23:33:38]

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

[commit-bot: I haz the power, 2016-07-26 23:33:38]

Dry run: This issue passed the CQ dry run.

[Ken Rockot(use gerrit already), 2016-07-27 03:20:12]

The CQ bit was checked by rockot@chromium.org

[Ken Rockot(use gerrit already), 2016-07-27 03:20:13]

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

[commit-bot: I haz the power, 2016-07-27 03:20:32]

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-07-27 03:25:25]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-27 03:27:23]

Description was changed from

==========
Eliminate deferred destruction of AssociatedGroupController

Moves AssociatedGroupController from base::RefCountedDeleteOnMessageLoop
to a simple base::RefCountedThreadSafe. Updates MultiplexRouter
and ChannelAssociatedGroupController expectations accordingly.

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

to

==========
Eliminate deferred destruction of AssociatedGroupController

Moves AssociatedGroupController from base::RefCountedDeleteOnMessageLoop
to a simple base::RefCountedThreadSafe. Updates MultiplexRouter
and ChannelAssociatedGroupController expectations accordingly.

BUG=631491
R=yzshen@chromium.org

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

[commit-bot: I haz the power, 2016-07-27 03:27:24]

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