Force TalkMediator::Logout to wait until the Login task has been destroyed an...

Change MediatorThreadImpl::Run to process messages forever and periodically call PumpAuxiliaryLoops to run the 3 (!!:(!!) other loops associated with this thread. There was previously a race where
quit would be called and set a cancellation flag before the posted task would 
get executed.

BUG=37771
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=41724

[tim (not reviewing), 2010-03-15 17:21:09]

Hey Fred, this fixes the crash we were seeing.  The only thing is we always wait
in Logout now, so if somehow DoDisconnect doesn't get called it could hang.  I
don't see a way to check for an "already logged out" sort of thing, so I added
the isQuitting check to make sure the owner thread calls logout before quit.
Since we're the only thread that calls in (I believe) and we'll be waiting on
the event, we should not be able to get into the quitting state in between. If
you know of another way that could work, let me know!

[tim (not reviewing), 2010-03-15 21:33:04]

changed to process messages forever on the libjingle loop with periodic
delayed-posts of a helper that pumps auxiliary message loops.

[akalin, 2010-03-15 21:46:31]

Some comments, also update CL description

http://codereview.chromium.org/984001/diff/8002/20001
File chrome/browser/sync/notifier/listener/mediator_thread_impl.cc (right):

http://codereview.chromium.org/984001/diff/8002/20001#newcode62
chrome/browser/sync/notifier/listener/mediator_thread_impl.cc:62:
PostDelayed(100, this, CMD_PUMP_AUXILIARY_LOOPS);
nit: post immediately to more closely match previous behavior

http://codereview.chromium.org/984001/diff/8002/20001#newcode83
chrome/browser/sync/notifier/listener/mediator_thread_impl.cc:83:
PostDelayed(100, this, CMD_PUMP_AUXILIARY_LOOPS);
add a comment with something like "we want to pump auxiiliary loops every 100 ms
until this thread is stopped, in which case this call will do nothing"

http://codereview.chromium.org/984001/diff/8002/20001#newcode91
chrome/browser/sync/notifier/listener/mediator_thread_impl.cc:91: if
(IsQuitting())
replace with:

CHECK(!IsQuitting()) << "Logout ...";

http://codereview.chromium.org/984001/diff/8002/20001#newcode94
chrome/browser/sync/notifier/listener/mediator_thread_impl.cc:94: Stop();
Consider overriding Stop() and checking that the disconnect did happen, i.e.
login_ and pump_ are NULL

the check has to happen after Thread::Stop(), though, as only then are we
guaranteed to be able to see changes to login_ and pump_ (since the Join() is a
memory barrier)

[tim (not reviewing), 2010-03-15 22:26:20]

thanks for the comments. made changes.

[akalin, 2010-03-15 22:42:56]

LGTM

http://codereview.chromium.org/984001/diff/15002/30001
File chrome/browser/sync/notifier/listener/mediator_thread_impl.cc (right):

http://codereview.chromium.org/984001/diff/15002/30001#newcode94
chrome/browser/sync/notifier/listener/mediator_thread_impl.cc:94:
CHECK(login_.get() == NULL);
nit: preferred style is CHECK(!foo.get())

perhaps:

CHECK(!login_.get() && !pump_.get()) << "Logout should be called prior to
message queue exit.";

[tim (not reviewing), 2010-03-16 15:11:55]

http://codereview.chromium.org/984001/diff/15002/30001
File chrome/browser/sync/notifier/listener/mediator_thread_impl.cc (right):

http://codereview.chromium.org/984001/diff/15002/30001#newcode94
chrome/browser/sync/notifier/listener/mediator_thread_impl.cc:94:
CHECK(login_.get() == NULL);
Ok, done.  Thanks, will submit shortly!