SyncFS: store disabled origins in DriveMetadataStore

chrome/browser/sync_file_system/drive_file_sync_service.cc

Index: chrome/browser/sync_file_system/drive_file_sync_service.cc
diff --git a/chrome/browser/sync_file_system/drive_file_sync_service.cc b/chrome/browser/sync_file_system/drive_file_sync_service.cc
index 12c883719a3003e1fedf8c6af7191bb3ff286aab..dc10d72b46997afb645272c4a4ba8adb74d2a8f6 100644
--- a/chrome/browser/sync_file_system/drive_file_sync_service.cc
+++ b/chrome/browser/sync_file_system/drive_file_sync_service.cc
@@ -422,7 +422,49 @@ void DriveFileSyncService::UnregisterOriginForTrackingChanges(
   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));
 }
 
@@ -855,8 +897,7 @@ void DriveFileSyncService::DidInitializeMetadataStore(
     return;
   }
 
-  // Remove any origins that are not installed or enabled.
-  UnregisterInactiveExtensionsIds();
+  UpdateRegisteredOrigins();
 
   largest_fetched_changestamp_ = metadata_store_->GetLargestChangeStamp();
 
@@ -888,29 +929,61 @@ void DriveFileSyncService::DidInitializeMetadataStore(
   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();
+    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 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();
-    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 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.

 }
 
@@ -990,7 +1063,8 @@ void DriveFileSyncService::GetDriveDirectoryForOrigin(
   }
 
   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);
@@ -1019,8 +1093,10 @@ void DriveFileSyncService::DidGetDriveDirectoryForOrigin(
     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);
@@ -1096,6 +1172,7 @@ void DriveFileSyncService::DidGetDirectoryContentForBatchSync(
 
   // 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);
   }
@@ -1111,7 +1188,7 @@ void DriveFileSyncService::DidGetDirectoryContentForBatchSync(
   NotifyTaskDone(SYNC_STATUS_OK, token.Pass());
 }
 
-void DriveFileSyncService::DidRemoveOriginOnMetadataStore(
+void DriveFileSyncService::DidChangeOriginOnMetadataStore(
     scoped_ptr<TaskToken> token,
     const SyncStatusCallback& callback,
     SyncStatusCode status) {
@@ -1991,6 +2068,7 @@ void DriveFileSyncService::RemoveRemoteChange(
     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());
   }