sync: reland scheduler ownership refactoring, now with less crash

sync: reland scheduler ownership refactoring, now with less crash

Prevent abandoned jobs from entering DoCanaryJob by stopping the timer
in TakePendingJobForCurrentMode.

Original review at https://codereview.chromium.org/10917234/

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

[tim (not reviewing), 2012-10-30 00:34:01]

Richard, please review.  This diff is based off of the previous patch verbatim,
which I reuploaded as http://codereview.chromium.org/11338025/.

I was planning to re-land the two together, but separating this out so you can
more easily review the actual changes.

[tim (not reviewing), 2012-10-30 00:36:29]

hmm.. I messed up the patch upload, this isn't the right code.  Hold on a
second.

[tim (not reviewing), 2012-10-30 16:50:38]

Ready for a look.

[rlarocque, 2012-10-30 17:50:20]

http://codereview.chromium.org/11341030/diff/15001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/11341030/diff/15001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:1054: wait_interval_->timer.Stop();
What if the timer fires somewhere between this line and the previous one? 
Should you use StopAndAbandon() here, or is that handled in some other way?

[tim (not reviewing), 2012-10-30 17:58:28]

http://codereview.chromium.org/11341030/diff/15001/sync/engine/sync_scheduler...
File sync/engine/sync_scheduler_impl.cc (right):

http://codereview.chromium.org/11341030/diff/15001/sync/engine/sync_scheduler...
sync/engine/sync_scheduler_impl.cc:1054: wait_interval_->timer.Stop();
On 2012/10/30 17:50:20, rlarocque wrote:
> What if the timer fires somewhere between this line and the previous one? 
> Should you use StopAndAbandon() here, or is that handled in some other way?

Everything happens on the same message loop, so if the time logically expires
between the calls the actual callback won't fire until the stack is unwound.  At
that point, the timer won't be running, so it will not Run() any user tasks.
Because we use a OneShotTimer, Stop() also synchronously deletes the user task
(our SyncSessionJob), which means, for example, we don't have to worry about
someone resetting the timer.

[rlarocque, 2012-10-30 18:07:36]

On 2012/10/30 17:58:28, timsteele wrote:
>
http://codereview.chromium.org/11341030/diff/15001/sync/engine/sync_scheduler...
> File sync/engine/sync_scheduler_impl.cc (right):
> 
>
http://codereview.chromium.org/11341030/diff/15001/sync/engine/sync_scheduler...
> sync/engine/sync_scheduler_impl.cc:1054: wait_interval_->timer.Stop();
> On 2012/10/30 17:50:20, rlarocque wrote:
> > What if the timer fires somewhere between this line and the previous one? 
> > Should you use StopAndAbandon() here, or is that handled in some other way?
> 
> Everything happens on the same message loop, so if the time logically expires
> between the calls the actual callback won't fire until the stack is unwound. 
At
> that point, the timer won't be running, so it will not Run() any user tasks.
> Because we use a OneShotTimer, Stop() also synchronously deletes the user task
> (our SyncSessionJob), which means, for example, we don't have to worry about
> someone resetting the timer.

You're right, I misread OneShotTimer's definition.  LGTM.

[commit-bot: I haz the power, 2012-10-30 18:12:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/11341030/6003

[commit-bot: I haz the power, 2012-10-30 20:31:19]

Change committed as 164992