Sync: Avoid deadlock in SyncBackendRegistrar / ModelSafeWorker on sync backend shutdown.

Sync: Avoid deadlock in SyncBackendRegistrar / ModelSafeWorker on sync backend shutdown.

This is a resubmit of the previous fix that has been reverted due to the
flaky unittest that would hang under the load:
https://codereview.chromium.org/637413003

Please see delta between patch sets 1 and 2 for the
unit test fix.
The test would hang due to a race condition between
MessageLoop::RunUntilIdle call for the main UI thread and
submitting a task on the UI thread from SyncBackendRegistrar running on the sync thread.
The new test implementation runs the UI thread loop continuously
until the expected result is reached so that eliminates the
race condition and makes the test simpler.

I verified the fix by running the test while running
Chromium build in background. Using this method I reliably
reproduced the hang with the first implementation of the test.
This version of the test works reliably under the same condition.

BUG=423078
Committed: https://crrev.com/df486a3cbdad2025fff98fa497edb1261e83435c
Cr-Commit-Position: refs/heads/master@{#301056}

[stanisc, 2014-10-22 05:39:36]

stanisc@chromium.org changed reviewers:
+ zea@chromium.org

[stanisc, 2014-10-22 05:39:36]

Please review. This is pretty much the same as the previous patch that you've
already reviewed with the exception of the unit test. You can see what I've
changed by looking at the delta between patches 1 and 2.
There is a number of trybot failures but all of the seem to be unrelated. I'll
try rerunning failed trybots tomorrow.

[Nicolas Zea, 2014-10-22 17:06:44]

https://codereview.chromium.org/640853008/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_registrar_unittest.cc (right):

https://codereview.chromium.org/640853008/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_registrar_unittest.cc:336:
base::MessageLoop::current()->Run();
nit: Run() is deprecated. The preferred approach these days is to create a
RunLoop, and pass it's quit closure to the TestRegistrar to invoke.

[stanisc, 2014-10-23 17:51:13]

Please see the updated version of the test that uses RunLoop.

I couldn't get a completely green output from trybots but as far as I see all
the failures are unrelated.
I tried the following trybots that included the new test:
1) linux_clang_tsan - green.
2) linux_valgrind - one unrelated failure, unit_tests was green.

https://codereview.chromium.org/640853008/diff/20001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_registrar_unittest.cc (right):

https://codereview.chromium.org/640853008/diff/20001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_registrar_unittest.cc:336:
base::MessageLoop::current()->Run();
On 2014/10/22 17:06:44, Nicolas Zea wrote:
> nit: Run() is deprecated. The preferred approach these days is to create a
> RunLoop, and pass it's quit closure to the TestRegistrar to invoke.

Done.

[Nicolas Zea, 2014-10-23 18:07:17]

LGTM with some nits

https://codereview.chromium.org/640853008/diff/60001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_registrar_unittest.cc (right):

https://codereview.chromium.org/640853008/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_registrar_unittest.cc:281: base::AutoLock
l(lock_);
given that you control the lifetime of the registrar in the test (and this is
only triggered after destruction), it might be cleaner to just assert that the
lock is not acquired here, rather than attempting to acquire.

https://codereview.chromium.org/640853008/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_registrar_unittest.cc:290: base::Lock
lock_;
nit: perhaps name this db_thread_lock_?

https://codereview.chromium.org/640853008/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_registrar_unittest.cc:315: quit_closure_ =
run_loop_.QuitClosure();
nit: may as well just put this in the constructor for the test

https://codereview.chromium.org/640853008/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_registrar_unittest.cc:346:
sync_thread->message_loop()->RunUntilIdle();
Could you sanity check that this is necessary? SyncBackendRegistrar is calling
Thread::Start, which means the thread is actually running in parallel. I don't
think you need to call RunUntilIdle here.

[stanisc, 2014-10-23 19:21:57]

https://codereview.chromium.org/640853008/diff/60001/chrome/browser/sync/glue...
File chrome/browser/sync/glue/sync_backend_registrar_unittest.cc (right):

https://codereview.chromium.org/640853008/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_registrar_unittest.cc:281: base::AutoLock
l(lock_);
On 2014/10/23 18:07:16, Nicolas Zea wrote:
> given that you control the lifetime of the registrar in the test (and this is
> only triggered after destruction), it might be cleaner to just assert that the
> lock is not acquired here, rather than attempting to acquire.
I've done this based on the thread sanitizer output. I'll see if moving
quit_closure to the constructor would make this unnecessary.

https://codereview.chromium.org/640853008/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_registrar_unittest.cc:290: base::Lock
lock_;
On 2014/10/23 18:07:16, Nicolas Zea wrote:
> nit: perhaps name this db_thread_lock_?

I renamed it because its purpose got extended to protect quit_closure_ to make
thread sanitizer happy. I'll test test again whether is that necessary. If not,
I'll rename it back to db_thread_lock_.

https://codereview.chromium.org/640853008/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_registrar_unittest.cc:315: quit_closure_ =
run_loop_.QuitClosure();
On 2014/10/23 18:07:16, Nicolas Zea wrote:
> nit: may as well just put this in the constructor for the test

Done.

https://codereview.chromium.org/640853008/diff/60001/chrome/browser/sync/glue...
chrome/browser/sync/glue/sync_backend_registrar_unittest.cc:346:
sync_thread->message_loop()->RunUntilIdle();
On 2014/10/23 18:07:16, Nicolas Zea wrote:
> Could you sanity check that this is necessary? SyncBackendRegistrar is calling
> Thread::Start, which means the thread is actually running in parallel. I don't
> think you need to call RunUntilIdle here.

I think this is needed to ensure that the task posted above (running Shutdown)
gets executed and finishes without blocking. With the old implementation
RunUntilIdle would block.

[stanisc, 2014-10-24 04:36:39]

The CQ bit was checked by stanisc@chromium.org

[commit-bot: I haz the power, 2014-10-24 04:37:59]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/640853008/100001

[commit-bot: I haz the power, 2014-10-24 05:17:56]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2014-10-24 05:18:46]

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