sync: reland scheduler ownership refactoring, now with less crash

sync/engine/sync_scheduler_impl.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "sync/engine/sync_scheduler_impl.h"
 
 #include <algorithm>
 #include <cstring>
 
 #include "base/auto_reset.h"
@@ skipping 1003 matching lines @@
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   SDVLOG(2) << "Do canary job";
 
   // Only set canary privileges here, when we are about to run the job. This
   // avoids confusion in managing canary bits during scheduling, when you
   // consider that mode switches (e.g., to config) can "pre-empt" a NUDGE that
   // was scheduled as canary, and send it to an "unscheduled" state.
   to_be_canary->GrantCanaryPrivilege();
 
   if (to_be_canary->purpose() == SyncSessionJob::NUDGE) {
+    DCHECK_EQ(pending_nudge_->session(), to_be_canary->session());
     // TODO(tim): We should be able to remove this...
     scoped_ptr<SyncSession> temp = CreateSyncSession(
         to_be_canary->session()->source()).Pass();
     // The routing info might have been changed since we cached the
     // pending nudge. Update it by coalescing to the latest.
     to_be_canary->mutable_session()->Coalesce(*(temp));
   }
   DoSyncSessionJob(to_be_canary.Pass());
 }
 
 scoped_ptr<SyncSessionJob> SyncSchedulerImpl::TakePendingJobForCurrentMode() {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   // If we find a scheduled pending_ job, abandon the old one and return a
   // a clone.  If unscheduled, just hand over ownership.
   scoped_ptr<SyncSessionJob> candidate;
   if (mode_ == CONFIGURATION_MODE && wait_interval_.get()
       && wait_interval_->pending_configure_job) {
     SDVLOG(2) << "Found pending configure job";
     candidate =
         wait_interval_->pending_configure_job->CloneAndAbandon().Pass();
     wait_interval_->pending_configure_job = candidate.get();
   } else if (mode_ == NORMAL_MODE && pending_nudge_) {
     SDVLOG(2) << "Found pending nudge job";
     candidate = pending_nudge_->CloneAndAbandon();
     pending_nudge_ = candidate.get();
     unscheduled_nudge_storage_.reset();
   }
+  // If we took a job and there's a wait interval, we took the pending canary.
+  if (candidate && wait_interval_)
+    wait_interval_->timer.Stop();

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

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:29]

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. 

   return candidate.Pass();
 }
 
 scoped_ptr<SyncSession> SyncSchedulerImpl::CreateSyncSession(
     const SyncSourceInfo& source) {
   DCHECK_EQ(MessageLoop::current(), sync_loop_);
   DVLOG(2) << "Creating sync session with routes "
            << ModelSafeRoutingInfoToString(session_context_->routing_info());
 
   SyncSourceInfo info(source);
@@ skipping 113 matching lines @@
 
 #undef SDVLOG_LOC
 
 #undef SDVLOG
 
 #undef SLOG
 
 #undef ENUM_CASE
 
 }  // namespace syncer