New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 631491 link

Starred by 2 users
Status: Fixed
Owner:
█ roc...@chromium.org
please use my google.com address
Closed: Jul 2016
Cc:
█ roc...@chromium.org
█ yzshen@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

RefCountedDeleteOnMessageLoop + IPC::Channel = leaks

Project Member Reported by roc...@chromium.org, Jul 26 2016 
Particularly in unit tests which use an in-process ChildThreadImpl, this leads to racy leaks as the main thread is shut down before the IO thread and the task to free the unref'd ChannelAssocatiedGroupController may only be posted after the main thread's MessageLoop has completely quit.

We don't strictly need to use RefCountedDeleteOnMessageLoop since the only state which needs to be torn down on a specific thread is the Connector and its sink.

Given the potential for these races to crop up in many existing tests (much to the confusion of whichever unlucky developer/sheriff encounters them) I think we should get rid of this usage and find another way to safely clean up these objects. WDYT?

Comment 1 by roc...@chromium.org, Jul 26 2016

Cc: roc...@chromium.org
 Issue 630985  has been merged into this issue.

Comment 2 by roc...@chromium.org, Jul 26 2016

Status: Started (was: Assigned)

Comment 3 by yzshen@chromium.org, Jul 26 2016 

Yeah. I have similar idea that we should get rid of RefCountedDeleteOnMessageLoop. Is it sufficient that Connector should be closed on the receiving thread but it is okay to destruct on any thread after that?

Comment 4 by roc...@chromium.org, Jul 26 2016 

Yep, that's what I was going to do too
Project Member

Comment 5 by bugdroid1@chromium.org, Jul 27 2016 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/b01ef6ae6f781753a82e0d2857aaed43323bceae

commit b01ef6ae6f781753a82e0d2857aaed43323bceae
Author: rockot <rockot@chromium.org>
Date: Wed Jul 27 03:24:32 2016

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

Review-Url: https://codereview.chromium.org/2185723002
Cr-Commit-Position: refs/heads/master@{#408032}

[modify] https://crrev.com/b01ef6ae6f781753a82e0d2857aaed43323bceae/ipc/ipc_mojo_bootstrap.cc
[modify] https://crrev.com/b01ef6ae6f781753a82e0d2857aaed43323bceae/mojo/public/cpp/bindings/associated_group_controller.h
[modify] https://crrev.com/b01ef6ae6f781753a82e0d2857aaed43323bceae/mojo/public/cpp/bindings/connector.h
[modify] https://crrev.com/b01ef6ae6f781753a82e0d2857aaed43323bceae/mojo/public/cpp/bindings/lib/associated_group_controller.cc
[modify] https://crrev.com/b01ef6ae6f781753a82e0d2857aaed43323bceae/mojo/public/cpp/bindings/lib/connector.cc
[modify] https://crrev.com/b01ef6ae6f781753a82e0d2857aaed43323bceae/mojo/public/cpp/bindings/lib/multiplex_router.cc
[modify] https://crrev.com/b01ef6ae6f781753a82e0d2857aaed43323bceae/mojo/public/cpp/bindings/lib/multiplex_router.h

Comment 6 by roc...@chromium.org, Jul 27 2016

Status: Fixed (was: Started)
►

Sign in to add a comment