Don't call Timer::Stop() off-sequence in SyncBrowserThreadModelWorkerTest

Don't call Timer::Stop() off-sequence in SyncBrowserThreadModelWorkerTest

Timer isn't thread-safe and this call is racy as highlighted by
https://codereview.chromium.org/2491613004/#ps420001

SyncBrowserThreadModelWorkerTest.DoesWorkOnDatabaseThread (run #1):
[ RUN      ] SyncBrowserThreadModelWorkerTest.DoesWorkOnDatabaseThread
[6916:2808:0123/195343.636:38250883:FATAL:timer.cc(150)] Check failed: origin_sequence_checker_.CalledOnValidSequence().
Backtrace:
base::debug::StackTrace::StackTrace [0x01DF27F7+23]
logging::LogMessage::~LogMessage [0x01D7F221+49]
base::Timer::Stop [0x01DD050B+91]
testing::internal::TestFactoryImpl<syncer::`anonymous namespace'::SyncBrowserThreadModelWorkerTest_DoesWorkOnDatabaseThread_Test>::CreateTest [0x00C4A22B+458]
base::internal::RunMixin<base::Callback<bool __cdecl(void),1,1> >::Run [0x0115609D+25]
base::internal::FunctorTraits<void (__thiscall syncer::BrowserThreadModelWorker::*)(base::Callback<enum syncer::SyncerError __cdecl(void),1,1> const &,syncer::ScopedEventSignal,enum syncer::SyncerError *),void>::Invoke<scoped_refptr<syncer::BrowserThreadM [0x025FBBD7+41]
base::internal::Invoker<base::internal::BindState<void (__thiscall syncer::BrowserThreadModelWorker::*)(base::Callback<enum syncer::SyncerError __cdecl(void),1,1> const &,syncer::ScopedEventSignal,enum syncer::SyncerError *),scoped_refptr<syncer::BrowserT [0x025FBC08+46]
base::internal::Invoker<base::internal::BindState<void (__thiscall syncer::BrowserThreadModelWorker::*)(base::Callback<enum syncer::SyncerError __cdecl(void),1,1> const &,syncer::ScopedEventSignal,enum syncer::SyncerError *),scoped_refptr<syncer::BrowserT [0x025FBF14+22]
base::debug::TaskAnnotator::RunTask [0x01E0136F+383]
base::MessageLoop::RunTask [0x01D839CC+1228]
base::MessageLoop::DoWork [0x01D82953+611]
base::MessagePumpDefault::Run [0x01E0327A+106]
base::MessageLoop::RunHandler [0x01D834F1+305]
base::RunLoop::Run [0x01D86D54+132]
base::Thread::Run [0x01D9F7BD+173]
base::Thread::ThreadMain [0x01DA02DE+622]
base::PlatformThread::Sleep [0x01D9E832+290]
BaseThreadInitThunk [0x7560337A+18]
RtlInitializeExceptionChain [0x775692B2+99]
RtlInitializeExceptionChain [0x77569285+54]

BUG=684640
Review-Url: https://codereview.chromium.org/2652673006
Cr-Commit-Position: refs/heads/master@{#446101}
Committed: https://chromium.googlesource.com/chromium/src/+/3c942aae9aaa0a5a04314090526d8f24560d65a6

[gab, 2017-01-24 22:12:46]

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

[commit-bot: I haz the power, 2017-01-24 22:13:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[gab, 2017-01-24 22:14:49]

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

[gab, 2017-01-24 22:14:50]

Stanis PTAL, FWIW this Timer is probably overkill (tests have a default timeout
and this is the first time I see a test self-manage its own timeout..).

If you agree I'll let you address this further though as this minimum change
unblocks https://codereview.chromium.org/2491613004 which is what I'm trying to
achieve right now.

[commit-bot: I haz the power, 2017-01-24 22:56:42]

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

[commit-bot: I haz the power, 2017-01-24 22:56:43]

Dry run: This issue passed the CQ dry run.

[stanisc, 2017-01-25 00:55:16]

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

[stanisc, 2017-01-25 00:55:16]

+ maxbogue@ since he has done a number of refactoring changes in this file
recently and might be more familiar with this code.

https://codereview.chromium.org/2652673006/diff/1/components/sync/engine/brow...
File components/sync/engine/browser_thread_model_worker_unittest.cc (left):

https://codereview.chromium.org/2652673006/diff/1/components/sync/engine/brow...
components/sync/engine/browser_thread_model_worker_unittest.cc:53:
timer_.Stop();  // Stop the failure timer so the test succeeds.
Would it make sense to remove timer_ completely and just rely on test timeout?

[gab, 2017-01-25 14:51:47]

https://codereview.chromium.org/2652673006/diff/1/components/sync/engine/brow...
File components/sync/engine/browser_thread_model_worker_unittest.cc (left):

https://codereview.chromium.org/2652673006/diff/1/components/sync/engine/brow...
components/sync/engine/browser_thread_model_worker_unittest.cc:53:
timer_.Stop();  // Stop the failure timer so the test succeeds.
On 2017/01/25 00:55:16, stanisc wrote:
> Would it make sense to remove timer_ completely and just rely on test timeout?

Yes it does but since there are many erroneous cases around I was hoping to just
do the minimal change to get things correct and have owners improve further on
their own timeline should they care

[maxbogue, 2017-01-25 20:01:00]

lgtm

[gab, 2017-01-25 20:02:00]

The CQ bit was checked by gab@chromium.org

[commit-bot: I haz the power, 2017-01-25 20:02:36]

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, 2017-01-25 20:10:12]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1485374520458480, "parent_rev":
"24439f5819c904b12f454103bd8c8c73c9d31cc3", "commit_rev":
"3c942aae9aaa0a5a04314090526d8f24560d65a6"}

[commit-bot: I haz the power, 2017-01-25 20:10:46]

Description was changed from

==========
Don't call Timer::Stop() off-sequence in SyncBrowserThreadModelWorkerTest

Timer isn't thread-safe and this call is racy as highlighted by
https://codereview.chromium.org/2491613004/#ps420001

SyncBrowserThreadModelWorkerTest.DoesWorkOnDatabaseThread (run #1):
[ RUN      ] SyncBrowserThreadModelWorkerTest.DoesWorkOnDatabaseThread
[6916:2808:0123/195343.636:38250883:FATAL:timer.cc(150)] Check failed:
origin_sequence_checker_.CalledOnValidSequence().
Backtrace:
base::debug::StackTrace::StackTrace [0x01DF27F7+23]
logging::LogMessage::~LogMessage [0x01D7F221+49]
base::Timer::Stop [0x01DD050B+91]
testing::internal::TestFactoryImpl<syncer::`anonymous
namespace'::SyncBrowserThreadModelWorkerTest_DoesWorkOnDatabaseThread_Test>::CreateTest
[0x00C4A22B+458]
base::internal::RunMixin<base::Callback<bool __cdecl(void),1,1> >::Run
[0x0115609D+25]
base::internal::FunctorTraits<void (__thiscall
syncer::BrowserThreadModelWorker::*)(base::Callback<enum syncer::SyncerError
__cdecl(void),1,1> const &,syncer::ScopedEventSignal,enum syncer::SyncerError
*),void>::Invoke<scoped_refptr<syncer::BrowserThreadM [0x025FBBD7+41]
base::internal::Invoker<base::internal::BindState<void (__thiscall
syncer::BrowserThreadModelWorker::*)(base::Callback<enum syncer::SyncerError
__cdecl(void),1,1> const &,syncer::ScopedEventSignal,enum syncer::SyncerError
*),scoped_refptr<syncer::BrowserT [0x025FBC08+46]
base::internal::Invoker<base::internal::BindState<void (__thiscall
syncer::BrowserThreadModelWorker::*)(base::Callback<enum syncer::SyncerError
__cdecl(void),1,1> const &,syncer::ScopedEventSignal,enum syncer::SyncerError
*),scoped_refptr<syncer::BrowserT [0x025FBF14+22]
base::debug::TaskAnnotator::RunTask [0x01E0136F+383]
base::MessageLoop::RunTask [0x01D839CC+1228]
base::MessageLoop::DoWork [0x01D82953+611]
base::MessagePumpDefault::Run [0x01E0327A+106]
base::MessageLoop::RunHandler [0x01D834F1+305]
base::RunLoop::Run [0x01D86D54+132]
base::Thread::Run [0x01D9F7BD+173]
base::Thread::ThreadMain [0x01DA02DE+622]
base::PlatformThread::Sleep [0x01D9E832+290]
BaseThreadInitThunk [0x7560337A+18]
RtlInitializeExceptionChain [0x775692B2+99]
RtlInitializeExceptionChain [0x77569285+54]

BUG=684640
==========

to

==========
Don't call Timer::Stop() off-sequence in SyncBrowserThreadModelWorkerTest

Timer isn't thread-safe and this call is racy as highlighted by
https://codereview.chromium.org/2491613004/#ps420001

SyncBrowserThreadModelWorkerTest.DoesWorkOnDatabaseThread (run #1):
[ RUN      ] SyncBrowserThreadModelWorkerTest.DoesWorkOnDatabaseThread
[6916:2808:0123/195343.636:38250883:FATAL:timer.cc(150)] Check failed:
origin_sequence_checker_.CalledOnValidSequence().
Backtrace:
base::debug::StackTrace::StackTrace [0x01DF27F7+23]
logging::LogMessage::~LogMessage [0x01D7F221+49]
base::Timer::Stop [0x01DD050B+91]
testing::internal::TestFactoryImpl<syncer::`anonymous
namespace'::SyncBrowserThreadModelWorkerTest_DoesWorkOnDatabaseThread_Test>::CreateTest
[0x00C4A22B+458]
base::internal::RunMixin<base::Callback<bool __cdecl(void),1,1> >::Run
[0x0115609D+25]
base::internal::FunctorTraits<void (__thiscall
syncer::BrowserThreadModelWorker::*)(base::Callback<enum syncer::SyncerError
__cdecl(void),1,1> const &,syncer::ScopedEventSignal,enum syncer::SyncerError
*),void>::Invoke<scoped_refptr<syncer::BrowserThreadM [0x025FBBD7+41]
base::internal::Invoker<base::internal::BindState<void (__thiscall
syncer::BrowserThreadModelWorker::*)(base::Callback<enum syncer::SyncerError
__cdecl(void),1,1> const &,syncer::ScopedEventSignal,enum syncer::SyncerError
*),scoped_refptr<syncer::BrowserT [0x025FBC08+46]
base::internal::Invoker<base::internal::BindState<void (__thiscall
syncer::BrowserThreadModelWorker::*)(base::Callback<enum syncer::SyncerError
__cdecl(void),1,1> const &,syncer::ScopedEventSignal,enum syncer::SyncerError
*),scoped_refptr<syncer::BrowserT [0x025FBF14+22]
base::debug::TaskAnnotator::RunTask [0x01E0136F+383]
base::MessageLoop::RunTask [0x01D839CC+1228]
base::MessageLoop::DoWork [0x01D82953+611]
base::MessagePumpDefault::Run [0x01E0327A+106]
base::MessageLoop::RunHandler [0x01D834F1+305]
base::RunLoop::Run [0x01D86D54+132]
base::Thread::Run [0x01D9F7BD+173]
base::Thread::ThreadMain [0x01DA02DE+622]
base::PlatformThread::Sleep [0x01D9E832+290]
BaseThreadInitThunk [0x7560337A+18]
RtlInitializeExceptionChain [0x775692B2+99]
RtlInitializeExceptionChain [0x77569285+54]

BUG=684640

Review-Url: https://codereview.chromium.org/2652673006
Cr-Commit-Position: refs/heads/master@{#446101}
Committed:
https://chromium.googlesource.com/chromium/src/+/3c942aae9aaa0a5a04314090526d...
==========

[commit-bot: I haz the power, 2017-01-25 20:10:48]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/3c942aae9aaa0a5a04314090526d...