Implement Invalidator::Acknowledge

chrome/browser/sync/profile_sync_service.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 "chrome/browser/sync/profile_sync_service.h"
 
 #include <cstddef>
 #include <map>
 #include <set>
 #include <utility>
@@ skipping 480 matching lines @@
   InitializeBackend(!HasSyncSetupCompleted());
 
   // |backend_| may end up being NULL here in tests (in synchronous
   // initialization mode).
   //
   // TODO(akalin): Fix this horribly non-intuitive behavior (see
   // http://crbug.com/140354).
   if (backend_.get()) {
     backend_->UpdateRegisteredInvalidationIds(
         invalidator_registrar_->GetAllRegisteredIds());
+    for (AckHandleReplayQueue::const_iterator it = ack_replay_queue_.begin();

[rlarocque, 2013/02/12 21:48:04]

I don't like the way this code (or the line above it, for that matter) are
communicating with the sync thread.  

InitializeBackend() will start the sync thread and post a task to it to
initialize the sync manager.  Eventually that task will end up in
SyncBackendHost::Core::OnInitializationComplete(), which may decide to destroy
the SyncManager if it detects that initialization failed.  

The tasks posted by UpdateRegisteredInvalidationIds and AcknowledgeInvalidation
will be queued up on the sync thread behind the initialization task.  They must
(and do) support the sync_manager_ being NULL, but not for the right reasons. 
The PSS shouldn't be trying to communicate with the backend until we receive the
OnBackendInitialized() callback.  

Would it be possible to do this from OnBackendInitialized() instead?

=====

On the other hand, I intend to move this code out of here as we transition
towards an independent InvalidationService.  So it's probably OK to leave it as
is.  

[dcheng, 2013/02/12 22:39:28]

I don't mind fixing this, but perhaps it would be better done in a separate CL
(since the existing code is busted already in this respect).

+         it != ack_replay_queue_.end(); ++it) {
+      backend_->AcknowledgeInvalidation(it->first, it->second);
+    }
+    ack_replay_queue_.clear();
   }
 
   if (!sync_global_error_.get()) {
 #if !defined(OS_ANDROID)
     sync_global_error_.reset(new SyncGlobalError(this, signin()));
 #endif
     GlobalErrorServiceFactory::GetForProfile(profile_)->AddGlobalError(
         sync_global_error_.get());
     AddObserver(sync_global_error_.get());
   }
@@ skipping 15 matching lines @@
     backend_->UpdateRegisteredInvalidationIds(
         invalidator_registrar_->GetAllRegisteredIds());
   }
 }
 
 void ProfileSyncService::UnregisterInvalidationHandler(
     syncer::InvalidationHandler* handler) {
   invalidator_registrar_->UnregisterHandler(handler);
 }
 
+void ProfileSyncService::AcknowledgeInvalidation(
+    const invalidation::ObjectId& id,
+    const syncer::AckHandle& ack_handle) {
+  if (backend_.get()) {
+    backend_->AcknowledgeInvalidation(id, ack_handle);
+  } else {
+    // If |backend_| is NULL, save the acknowledgements to replay when
+    // it's created and initialized.
+    ack_replay_queue_.push_back(std::make_pair(id, ack_handle));
+  }
+}
+
 syncer::InvalidatorState ProfileSyncService::GetInvalidatorState() const {
   return invalidator_registrar_->GetInvalidatorState();
 }
 
 void ProfileSyncService::EmitInvalidationForTest(
     const invalidation::ObjectId& id,
     const std::string& payload) {
   syncer::ObjectIdSet notify_ids;
   notify_ids.insert(id);
 
@@ skipping 1420 matching lines @@
   // See http://stackoverflow.com/questions/6224121/is-new-this-myclass-undefined-behaviour-after-directly-calling-the-destru.
   ProfileSyncService* old_this = this;
   this->~ProfileSyncService();
   new(old_this) ProfileSyncService(
       new ProfileSyncComponentsFactoryImpl(profile,
                                            CommandLine::ForCurrentProcess()),
       profile,
       signin,
       behavior);
 }