SyncFS: store disabled origins in DriveMetadataStore

chrome/browser/sync_file_system/drive_file_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_file_system/drive_file_sync_service.h"
 
 #include <algorithm>
 #include <string>
 #include <utility>
 
@@ skipping 404 matching lines @@
   OriginToChangesMap::iterator found = origin_to_changes_map_.find(origin);
   if (found != origin_to_changes_map_.end()) {
     for (PathToChangeMap::iterator itr = found->second.begin();
          itr != found->second.end(); ++itr)
       pending_changes_.erase(itr->second.position_in_queue);
     origin_to_changes_map_.erase(found);
   }
   pending_batch_sync_origins_.erase(origin);
 
   metadata_store_->RemoveOrigin(origin, base::Bind(
-      &DriveFileSyncService::DidRemoveOriginOnMetadataStore,
+      &DriveFileSyncService::DidChangeOriginOnMetadataStore,
       AsWeakPtr(), base::Passed(&token), callback));
 }
 
+void DriveFileSyncService::EnableOriginForTrackingChanges(
+    const GURL& origin,
+    const SyncStatusCallback& callback) {

[kinuko, 2013/03/18 18:09:43]

I think we can likely early-return here if the origin's not in disabled list?

[nhiroki, 2013/03/19 02:35:42]

Done.

+  scoped_ptr<TaskToken> token(GetToken(FROM_HERE, TASK_TYPE_DATABASE, ""));
+  if (!token) {
+    pending_tasks_.push_back(base::Bind(
+        &DriveFileSyncService::EnableOriginForTrackingChanges,
+        AsWeakPtr(), origin, callback));
+    return;
+  }
+
+  metadata_store_->EnableOrigin(origin, base::Bind(
+      &DriveFileSyncService::DidChangeOriginOnMetadataStore,
+      AsWeakPtr(), base::Passed(&token), callback));
+  pending_batch_sync_origins_.insert(origin);
+}
+
+void DriveFileSyncService::DisableOriginForTrackingChanges(
+    const GURL& origin,
+    const SyncStatusCallback& callback) {

[kinuko, 2013/03/18 18:09:43]

ditto. I guess we can likely early-return here if the origin's not in
incremental_sync or batch_sync?

[nhiroki, 2013/03/19 02:35:42]

Done.

+  scoped_ptr<TaskToken> token(GetToken(FROM_HERE, TASK_TYPE_DATABASE, ""));
+  if (!token) {
+    pending_tasks_.push_back(base::Bind(
+        &DriveFileSyncService::DisableOriginForTrackingChanges,
+        AsWeakPtr(), origin, callback));
+    return;
+  }
+
+  OriginToChangesMap::iterator found = origin_to_changes_map_.find(origin);
+  if (found != origin_to_changes_map_.end()) {
+    for (PathToChangeMap::iterator itr = found->second.begin();
+         itr != found->second.end(); ++itr)
+      pending_changes_.erase(itr->second.position_in_queue);
+    origin_to_changes_map_.erase(found);
+  }
+  pending_batch_sync_origins_.erase(origin);
+
+  metadata_store_->DisableOrigin(origin, base::Bind(
+      &DriveFileSyncService::DidChangeOriginOnMetadataStore,
+      AsWeakPtr(), base::Passed(&token), callback));
+}
+
 void DriveFileSyncService::DeleteOriginDirectory(
     const GURL& origin,
     const SyncStatusCallback& callback) {
   scoped_ptr<TaskToken> token(GetToken(
       FROM_HERE, TASK_TYPE_DATABASE, "Delete Origin Directory"));
   if (!token) {
     pending_tasks_.push_back(base::Bind(
         &DriveFileSyncService::DeleteOriginDirectory,
         AsWeakPtr(), origin, callback));
     return;
@@ skipping 409 matching lines @@
 
 void DriveFileSyncService::DidInitializeMetadataStore(
     scoped_ptr<TaskToken> token,
     SyncStatusCode status,
     bool created) {
   if (status != SYNC_STATUS_OK) {
     NotifyTaskDone(status, token.Pass());
     return;
   }
 
-  // Remove any origins that are not installed or enabled.
-  UnregisterInactiveExtensionsIds();
+  UpdateRegisteredOrigins();
 
   largest_fetched_changestamp_ = metadata_store_->GetLargestChangeStamp();
 
   // Mark all the batch sync origins as 'pending' so that we can start
   // batch sync when we're ready.
   DCHECK(pending_batch_sync_origins_.empty());
   for (std::map<GURL, std::string>::const_iterator itr =
           metadata_store_->batch_sync_origins().begin();
         itr != metadata_store_->batch_sync_origins().end();
         ++itr) {
@@ skipping 11 matching lines @@
     const std::string& resource_id = itr->second;
     AppendFetchChange(url.origin(), url.path(), resource_id);
   }
 
   // It's ok to fail, so we pass EmptyResourceIdCallback here.
   GetSyncRootDirectory(token.Pass(), base::Bind(&EmptyResourceIdCallback));
 
   RegisterDriveNotifications();
 }
 
-void DriveFileSyncService::UnregisterInactiveExtensionsIds() {
+void DriveFileSyncService::UpdateRegisteredOrigins() {
   ExtensionService* extension_service =
       extensions::ExtensionSystem::Get(profile_)->extension_service();
   if (!extension_service)
     return;
-  std::vector<GURL> tracked_origins;
-  metadata_store_->GetAllOrigins(&tracked_origins);
 
-  for (std::vector<GURL>::const_iterator itr = tracked_origins.begin();
-       itr != tracked_origins.end();
-       ++itr) {
-    // Make sure the registered extension is installed and enabled.
+  std::vector<GURL> disabled_origins;
+  metadata_store_->GetDisabledOrigins(&disabled_origins);
+  for (std::vector<GURL>::const_iterator itr = disabled_origins.begin();
+       itr != disabled_origins.end(); ++itr) {
     std::string extension_id = itr->host();
-    const extensions::Extension* installed_extension =
-        extension_service->GetExtensionById(extension_id,
-                                            false /* include_disabled */);
-    if (installed_extension != NULL)
+    GURL origin =
+        extensions::Extension::GetBaseURLFromExtensionId(extension_id);
+
+    if (extension_service->GetInstalledExtension(extension_id) == NULL) {
+      // Extension is uninstalled. Unregister origin.
+      metadata_store_->RemoveOrigin(
+          origin, base::Bind(&DidRemoveOrigin, origin));
       continue;
+    }
 
-    // Extension is either disabled or uninstalled. Unregister origin.
-    GURL origin = extensions::Extension::GetBaseURLFromExtensionId(
-        extension_id);
-    metadata_store_->RemoveOrigin(origin, base::Bind(&DidRemoveOrigin, origin));
+    if (extension_service->IsExtensionEnabled(extension_id)) {
+      // Extension is enabled. Enable origin.
+      metadata_store_->EnableOrigin(
+          origin, base::Bind(&EmptyStatusCallback));
+      pending_batch_sync_origins_.insert(origin);
+      continue;
+    }
+
+    // Extension is still disabled.
+  }
+
+  std::vector<GURL> enabled_origins;
+  metadata_store_->GetEnabledOrigins(&enabled_origins);
+  for (std::vector<GURL>::const_iterator itr = enabled_origins.begin();
+       itr != enabled_origins.end(); ++itr) {
+    std::string extension_id = itr->host();
+    GURL origin =
+        extensions::Extension::GetBaseURLFromExtensionId(extension_id);
+
+    if (extension_service->GetInstalledExtension(extension_id) == NULL) {
+      // Extension is uninstalled. Unregister origin.
+      metadata_store_->RemoveOrigin(
+          origin, base::Bind(&DidRemoveOrigin, origin));
+      continue;
+    }
+
+    if (!extension_service->IsExtensionEnabled(extension_id)) {
+      // Extension is disabled. Disable origin.
+      metadata_store_->DisableOrigin(
+          origin, base::Bind(&EmptyStatusCallback));
+      continue;
+    }
+
+    // Extension is still enabled.
   }

[kinuko, 2013/03/18 18:09:43]

Hmm, having two loops with similar body looks a bit awkward.. but I hope we can
clean this up when we migrate to use type enum.

[nhiroki, 2013/03/19 02:35:42]

Okay, for now just add TODO comment.

 }
 
 void DriveFileSyncService::GetSyncRootDirectory(
     scoped_ptr<TaskToken> token,
     const ResourceIdCallback& callback) {
   token->UpdateTask(FROM_HERE, TASK_TYPE_DRIVE, "Retrieving drive root");
   sync_client_->GetDriveDirectoryForSyncRoot(
       base::Bind(&DriveFileSyncService::DidGetSyncRootDirectory,
                  AsWeakPtr(), base::Passed(&token), callback));
 }
@@ skipping 34 matching lines @@
   GetDriveDirectoryForOrigin(origin, callback, sync_root_resource_id);
 }
 
 void DriveFileSyncService::StartBatchSyncForOrigin(
     const GURL& origin,
     const std::string& resource_id) {
   scoped_ptr<TaskToken> token(
       GetToken(FROM_HERE, TASK_TYPE_DRIVE, "Retrieving largest changestamp"));
   DCHECK(token);
   DCHECK(GetCurrentState() == REMOTE_SERVICE_OK || may_have_unfetched_changes_);
 

[kinuko, 2013/03/18 18:09:43]

Can we have another DCHECK to check if the origin's not disabled?

[nhiroki, 2013/03/19 02:35:42]

Done.

   DVLOG(1) << "Start batch sync for:" << origin.spec();
 
   sync_client_->GetLargestChangeStamp(
       base::Bind(&DriveFileSyncService::DidGetLargestChangeStampForBatchSync,
                  AsWeakPtr(), base::Passed(&token), origin, resource_id));
 
   may_have_unfetched_changes_ = false;
 }
 
 void DriveFileSyncService::GetDriveDirectoryForOrigin(
     const GURL& origin,
     const SyncStatusCallback& callback,
     const std::string& sync_root_resource_id) {
   scoped_ptr<TaskToken> token(GetToken(
       FROM_HERE, TASK_TYPE_DRIVE, "Retrieving origin metadata"));
   if (!token) {
     pending_tasks_.push_back(base::Bind(
         &DriveFileSyncService::GetDriveDirectoryForOrigin,
         AsWeakPtr(), origin, callback, sync_root_resource_id));
     return;
   }
 
   if (metadata_store_->IsIncrementalSyncOrigin(origin) ||
-      metadata_store_->IsBatchSyncOrigin(origin)) {
+      metadata_store_->IsBatchSyncOrigin(origin) ||
+      metadata_store_->IsOriginDisabled(origin)) {

[kinuko, 2013/03/18 18:09:43]

If we came here from RegisterOrigin I think we need to re-enable the origin.

[nhiroki, 2013/03/19 02:35:42]

hmm... if we came here from RegisterOrigin, |origin| hasn't belonged to any
origin maps (batch, incremental and disabled) yet, so I think we can pass here.

[kinuko, 2013/03/19 02:44:44]

That sounds right... wait, couldn't we be in a situation like: an app is
disabled, and then enabled, chrome crashes before we change the db, and the app
is launched after restart

[nhiroki, 2013/03/19 03:43:52]

I see... in your situation, an origin is disabled until an app is reloaded or
disabled/enabled.

I'm not sure whether a true-positive case (extension and origin are disabled)
can reach here. If such case exists, I think we cannot enable an origin easily
(we need to know an actual extension state?).

Anyway, I'll take a look into it (sorry, I won't be able to use a network a few
hours.).

[kinuko, 2013/03/19 03:51:39]

Nope, I just realized that we do perform the enabled/disabled check at startup
in UpdateRegisteredOrigins, so I guess we don't need to worry about this case.

[nhiroki, 2013/03/19 08:47:38]

Thanks! Done.

[kinuko, 2013/03/19 09:56:30]

I meant I don't think we need to worry about disabled origin case here either,
since if it's disabled in metadata but extension service says it's enabled we
enable them in UpdateRegisteredOrigins.  Can you check the behavior again?

     token->ResetTask(FROM_HERE);
     NotifyTaskDone(SYNC_STATUS_OK, token.Pass());
     callback.Run(SYNC_STATUS_OK);
     return;
   }
 
   DCHECK(!sync_root_resource_id.empty());
   sync_client_->GetDriveDirectoryForOrigin(
       sync_root_resource_id, origin,
       base::Bind(&DriveFileSyncService::DidGetDriveDirectoryForOrigin,
                  AsWeakPtr(), base::Passed(&token), origin, callback));
 }
 
 void DriveFileSyncService::DidGetDriveDirectoryForOrigin(
     scoped_ptr<TaskToken> token,
     const GURL& origin,
     const SyncStatusCallback& callback,
     google_apis::GDataErrorCode error,
     const std::string& resource_id) {
   if (error != google_apis::HTTP_SUCCESS &&
       error != google_apis::HTTP_CREATED) {
     SyncStatusCode status =
         GDataErrorCodeToSyncStatusCodeWrapper(error);
     NotifyTaskDone(status, token.Pass());
     callback.Run(status);
     return;
   }
 
-  metadata_store_->AddBatchSyncOrigin(origin, resource_id);
-  pending_batch_sync_origins_.insert(origin);
+  if (!metadata_store_->IsOriginDisabled(origin)) {

[kinuko, 2013/03/18 18:09:43]

We've gotten token in GetDriveDirectoryForOrigin and there we've checked if
origin is disabled or not, haven't we?  Could this happen?

[nhiroki, 2013/03/19 02:35:42]

Yes, we've checked it on line 1067, so we don't have to do here. Removed.

+    metadata_store_->AddBatchSyncOrigin(origin, resource_id);
+    pending_batch_sync_origins_.insert(origin);
+  }
 
   NotifyTaskDone(SYNC_STATUS_OK, token.Pass());
   callback.Run(SYNC_STATUS_OK);
 }
 
 void DriveFileSyncService::DidDeleteOriginDirectory(
     scoped_ptr<TaskToken> token,
     const SyncStatusCallback& callback,
     google_apis::GDataErrorCode error) {
   SyncStatusCode status = GDataErrorCodeToSyncStatusCodeWrapper(error);
@@ skipping 55 matching lines @@
     sync_client_->ContinueListing(
         next_feed_url,
         base::Bind(
             &DriveFileSyncService::DidGetDirectoryContentForBatchSync,
             AsWeakPtr(), base::Passed(&token), origin, largest_changestamp));
     return;
   }
 
   // Move |origin| to the incremental sync origin set if the origin has no file.
   if (metadata_store_->IsBatchSyncOrigin(origin) &&
+      !metadata_store_->IsOriginDisabled(origin) &&

[kinuko, 2013/03/18 18:09:43]

Could this (origin is batch-sync and disabled) happen?

[nhiroki, 2013/03/19 02:35:42]

I thought this may happen in situations like you mentioned... 

"If we had a crash after an extension is disabled (and the
status change is persisted in extension system) but before the changes are
successfully written into the metadata store, ..."

, but now I'm thinking this never happen since enabled(batch or
incremental)/disabled are completely toggled by EnableOrigin()/DisableOrigin()
(i.e. there is no intermediate state), so I think we don't have to check it
here.

Removed.

[kinuko, 2013/03/19 02:44:44]

Yup, as far as I understand the only possible inconsistency we need to care is
the state inconsistency between the extension service and our metadata store.

       !ContainsKey(origin_to_changes_map_, origin)) {
     metadata_store_->MoveBatchSyncOriginToIncremental(origin);
   }
 
   // If this was the last batch sync origin and push_notification is enabled
   // (indicates that we may have longer polling cycle), trigger the first
   // incremental sync on next task cycle.
   if (pending_batch_sync_origins_.empty() &&
       push_notification_enabled_) {
     may_have_unfetched_changes_ = true;
   }
 
   NotifyTaskDone(SYNC_STATUS_OK, token.Pass());
 }
 
-void DriveFileSyncService::DidRemoveOriginOnMetadataStore(
+void DriveFileSyncService::DidChangeOriginOnMetadataStore(
     scoped_ptr<TaskToken> token,
     const SyncStatusCallback& callback,
     SyncStatusCode status) {
   NotifyTaskDone(status, token.Pass());
   callback.Run(status);
 }
 
 void DriveFileSyncService::DidGetRemoteFileMetadata(
     const SyncFileMetadataCallback& callback,
     google_apis::GDataErrorCode error,
@@ skipping 859 matching lines @@
   PathToChangeMap::iterator found_change = path_to_change->find(url.path());
   if (found_change == path_to_change->end())
     return;
 
   pending_changes_.erase(found_change->second.position_in_queue);
   path_to_change->erase(found_change);
   if (path_to_change->empty())
     origin_to_changes_map_.erase(found_origin);
 
   if (metadata_store_->IsBatchSyncOrigin(url.origin()) &&
+      !metadata_store_->IsOriginDisabled(url.origin()) &&

[kinuko, 2013/03/18 18:09:43]

ditto.

[nhiroki, 2013/03/19 02:35:42]

Done.

       !ContainsKey(origin_to_changes_map_, url.origin())) {
     metadata_store_->MoveBatchSyncOriginToIncremental(url.origin());
   }
 }
 
 bool DriveFileSyncService::GetPendingChangeForFileSystemURL(
     const FileSystemURL& url,
     RemoteChange* change) const {
   DCHECK(change);
   OriginToChangesMap::const_iterator found_url =
@@ skipping 276 matching lines @@
     DCHECK_EQ(SYNC_ACTION_NONE, action_taken);
     DCHECK_EQ(SYNC_DIRECTION_NONE, direction);
   }
 
   FOR_EACH_OBSERVER(
       FileStatusObserver, file_status_observers_,
       OnFileStatusChanged(url, sync_status, action_taken, direction));
 }
 
 }  // namespace sync_file_system