Make HostController/DeviceListener deletion fully synchronous.

Make HostController/DeviceListener deletion fully synchronous.

These classes are responsible for creating Forwarder instances that operate on
their own thread. The Forwarder class used to self-delete independently of its
creator (HostController/DeviceListener) lifecycle. This could have led to
Forwarder instances still operating although HostController/DeviceListener were
deleted.

r229931 recently added support for adb port unmapping once there is no
HostController instance anymore operating for a specific device. This change
assumed (very reasonably) that ~HostController() was fully synchronous which
was unfortunately not the case.

This CL makes the Forwarder instances now explicitly owned by their creator.

This is not as trivial as it sounds since the Forwarder instance creators
(HostController and DeviceListener) live on a thread other than the one
Forwarder instances are created on.
This complexity is encapsulated into the new ForwardersManager class that
creates and manages the lifecycle (e.g. explicit destruction or destruction on
error) of a set of Forwarder instances. This class is used both by
HostController and DeviceListener.

BUG=313809
R=digit@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245895

[Philippe, 2013-11-13 10:17:40]

FYI, this should be ready for review now. Not that it depends on
https://codereview.chromium.org/68893013/.

I had a hard time investigating a dead lock that show up very rarely. It turns
out Marcus that your initial intuitions a few months ago were right I believe
when investigating the deadlock in Telemetry.

[digit1, 2013-11-14 09:50:04]

https://chromiumcodereview.appspot.com/60033002/diff/560001/tools/android/for...
File tools/android/forwarder2/device_listener.cc (right):

https://chromiumcodereview.appspot.com/60033002/diff/560001/tools/android/for...
tools/android/forwarder2/device_listener.cc:134:
AssertIsRunningOnInternalThread();
Following of face-to-face discussion, please add a comment here explaining
what's happening. Example:

  This function can be called in two different cases:

    1) When a network error occurs, we need to ask for deletion
       of the Listener object on the controller/listener message
       loop. Use a weak pointer in the callback to ensure that
       MaybeSelfDelete() is not called if the listener is
       destroyed before the task's callback is run.

    2) When the controller explictly deletes the Listener object,
       ~DeviceListener() will perform the following actions in
       this specific order:

           1) Notify()
           2) Thread.join()
           3) destroy weak_ptr_factory.

       The notify will force the internal thread to call this
       function, which will create a new task before 2) above
       that is posted with a weak pointer to the DeviceListener's
       message loop.

       The function returns, the internal thread message loop is
       empty, so Thread.join() returns.

       Destroying the weak pointer factory nullifies the weak
       pointer.
 
       When the destructor returns, control ultimately goes to
       the message loop implementation, which will see the null
       weak pointer is the queued task, and ignore it entirely.

It looks like this scheme is also use for other classes. Not sure if this
shouldn't be in a technical note instead.

https://chromiumcodereview.appspot.com/60033002/diff/560001/tools/android/for...
File tools/android/forwarder2/device_listener.h (right):

https://chromiumcodereview.appspot.com/60033002/diff/560001/tools/android/for...
tools/android/forwarder2/device_listener.h:105: base::Thread thread_;
Add a comment like:
  // IMPORTANT: thread_ must appear after weak_ptr_factory_.

[Philippe, 2013-11-14 16:22:01]

FYI David, I rebased this on ToT + your patch (being CQ'ed I believe).

[Philippe, 2014-01-17 15:12:43]

FYI David, I've just rebased this CL and it's also adding the PipeNotifier back
(to the ForwardersManager this time so that we don't consume more than a couple
of FDs).

I would like to use this new ForwardersManager class to also have all the
forwarder instances operate on a single thread (that would be owned by
ForwardersManager).

[digit1, 2014-01-17 15:47:02]

rubberstamp lgtm :-) Thanks.

[Philippe, 2014-01-17 16:35:58]

Thanks David! Monday would be a more appropriate time to submit this though
:)


On Fri, Jan 17, 2014 at 4:47 PM, <digit@chromium.org> wrote:

> rubberstamp lgtm :-) Thanks.
>
> https://chromiumcodereview.appspot.com/60033002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2014-01-20 08:38:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/pliard@chromium.org/60033002/760001

[commit-bot: I haz the power, 2014-01-20 08:54:45]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[Philippe, 2014-01-20 10:38:29]

Committed patchset #16 manually as r245895.