Fix unnecessary errors returned when unique db is missing

When unique db is missing and migration state says it should, the
database could have been deleted by other means. In this case just
return an empty shared db instead of error.

Change-Id: I5b15955662405b306583ed1942d8591703ebf942
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2130685Reviewed-by: Salvador Guerrero <salg@google.com>
Commit-Queue: ssid <ssid@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756947}

components/leveldb_proto/internal/proto_database_impl_unittest.cc

@@ -12,6 +12,7 @@
 #include "base/test/scoped_feature_list.h"
 #include "base/test/task_environment.h"
 #include "components/leveldb_proto/internal/leveldb_proto_feature_list.h"
+#include "components/leveldb_proto/internal/proto_database_selector.h"
 #include "components/leveldb_proto/internal/shared_proto_database_provider.h"
 #include "components/leveldb_proto/public/proto_database_provider.h"
 #include "components/leveldb_proto/testing/proto/test_db.pb.h"
@@ -108,9 +109,11 @@ class TestSharedProtoDatabaseClient : public SharedProtoDatabaseClient {
   using SharedProtoDatabaseClient::set_migration_status;
   using SharedProtoDatabaseClient::SharedProtoDatabaseClient;
 
-  TestSharedProtoDatabaseClient(scoped_refptr<SharedProtoDatabase> shared_db)
+  explicit TestSharedProtoDatabaseClient(
+      scoped_refptr<SharedProtoDatabase> shared_db)
       : SharedProtoDatabaseClient::SharedProtoDatabaseClient(
-            nullptr,
+            std::make_unique<ProtoLevelDBWrapper>(shared_db->task_runner_,
+                                                  shared_db->db_.get()),
             ProtoDbType::TEST_DATABASE1,
             shared_db) {}
 
@@ -118,11 +121,18 @@ class TestSharedProtoDatabaseClient : public SharedProtoDatabaseClient {
       SharedDBMetadataProto::MigrationStatus migration_status) override {
     set_migration_status(migration_status);
   }
+
+  void UpdateEntriesWithRemoveFilter(
+      std::unique_ptr<KeyValueVector> entries_to_save,
+      const KeyFilter& delete_key_filter,
+      Callbacks::UpdateCallback callback) override {
+    std::move(callback).Run(true);
+  }
 };
 
 class TestProtoDatabaseProvider : public ProtoDatabaseProvider {
  public:
-  TestProtoDatabaseProvider(const base::FilePath& profile_dir)
+  explicit TestProtoDatabaseProvider(const base::FilePath& profile_dir)
       : ProtoDatabaseProvider(profile_dir) {}
   TestProtoDatabaseProvider(const base::FilePath& profile_dir,
                             const scoped_refptr<SharedProtoDatabase>& shared_db)
@@ -156,10 +166,9 @@ class ProtoDatabaseImplTest : public testing::Test {
   void SetUp() override {
     ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
     ASSERT_TRUE(shared_db_temp_dir_.CreateUniqueTempDir());
-    test_task_runner_ =
-        base::ThreadPool::CreateSequencedTaskRunner({base::MayBlock()});
     shared_db_ = base::WrapRefCounted(new SharedProtoDatabase(
         kDefaultClientName, shared_db_temp_dir_.GetPath()));
+    test_task_runner_ = shared_db_->database_task_runner_for_testing();
   }
 
   void TearDown() override { shared_db_->Shutdown(); }
@@ -275,7 +284,10 @@ class ProtoDatabaseImplTest : public testing::Test {
     scoped_refptr<ProtoDatabaseSelector> selector(new ProtoDatabaseSelector(
         ProtoDbType::TEST_DATABASE1, GetTestThreadTaskRunner(), nullptr));
 
-    selector->OnGetSharedDBClient(
+    GetTestThreadTaskRunner()->PostTask(
+        FROM_HERE,
+        base::BindOnce(
+            &ProtoDatabaseSelector::OnGetSharedDBClient, selector,
             std::move(unique_db), unique_db_status, use_shared_db,
             base::BindOnce(
                 [](base::OnceClosure closure, Enums::InitStatus expect_status,
@@ -284,7 +296,7 @@ class ProtoDatabaseImplTest : public testing::Test {
                   std::move(closure).Run();
                 },
                 init_loop.QuitClosure(), expect_status),
-        std::move(shared_db_client), shared_db_status);
+            std::move(shared_db_client), shared_db_status));
 
     init_loop.Run();
 
@@ -522,6 +534,42 @@ TYPED_TEST(ProtoDatabaseImplTest,
       Enums::InitStatus::kOK);  // Then the DB impl should use the shared DB.
 }
 
+TYPED_TEST(ProtoDatabaseImplTest,
+           SucceedsWithShared_UseShared_HasSharedDB_MigratedUniqueWasDeleted) {
+  auto shared_db_client = this->GetSharedClient();
+
+  // Database has been migrated to Unique.
+  shared_db_client->set_migration_status(
+      SharedDBMetadataProto::MIGRATE_TO_UNIQUE_SUCCESSFUL);
+
+  // If we request a shared DB, the unique DB fails does not exist and
+  // the data has been migrated to the unique DB then we return shared db.
+  this->CallOnGetSharedDBClientAndWait(
+      nullptr,                               // Unique DB fails to open.
+      Enums::InitStatus::kInvalidOperation,  // Unique DB missing.
+      true,                                  // We should be using a shared DB.
+      std::move(shared_db_client),           // Shared DB opens fine.
+      Enums::InitStatus::kOK,
+      Enums::InitStatus::kOK);  // Returns shared db.
+
+  shared_db_client = this->GetSharedClient();
+
+  // Data has been migrated to Unique, but data still exists in Shared DB that
+  // should be removed.
+  shared_db_client->set_migration_status(
+      SharedDBMetadataProto::MIGRATE_TO_UNIQUE_SHARED_TO_BE_DELETED);
+
+  // This second scenario occurs when the Shared DB still contains data, we
+  // should still clear shared and return it.
+  this->CallOnGetSharedDBClientAndWait(
+      nullptr,                               // Unique DB fails to open.
+      Enums::InitStatus::kInvalidOperation,  // Unique DB missing.
+      true,                                  // We should be using a shared DB.
+      std::move(shared_db_client),           // Shared DB opens fine.
+      Enums::InitStatus::kOK,
+      Enums::InitStatus::kOK);  // Returns shared db
+}
+
 TYPED_TEST(ProtoDatabaseImplTest,
            Fails_UseShared_HasSharedDB_DataWasMigratedToUnique) {
   auto shared_db_client = this->GetSharedClient();
@@ -535,7 +583,7 @@ TYPED_TEST(ProtoDatabaseImplTest,
   // unique database may contain data.
   this->CallOnGetSharedDBClientAndWait(
       nullptr,                      // Unique DB fails to open.
-      Enums::InitStatus::kInvalidOperation,  // Unique DB doesn't exist.
+      Enums::InitStatus::kError,    // Unique DB failure.
       true,                         // We should be using a shared DB.
       std::move(shared_db_client),  // Shared DB opens fine.
       Enums::InitStatus::kOK,

components/leveldb_proto/internal/proto_database_selector.cc

@@ -258,13 +258,24 @@ void ProtoDatabaseSelector::OnGetSharedDBClient(
         break;
       case SharedDBMetadataProto::MIGRATE_TO_UNIQUE_SUCCESSFUL:
       case SharedDBMetadataProto::MIGRATE_TO_UNIQUE_SHARED_TO_BE_DELETED:
+        if (unique_db_status == Enums::kInvalidOperation) {
+          // If unique db does not exist and migration state expects it, reset
+          // the migration state since this is not recoverable, and return the
+          // shared db. Clear the shared db since it might contain stale data.
+          SharedProtoDatabaseClient* client_ptr = client.get();
+          client_ptr->UpdateEntriesWithRemoveFilter(
+              std::make_unique<KeyValueVector>(),
+              base::BindRepeating([](const std::string& key) { return true; }),
+              base::BindOnce(&ProtoDatabaseSelector::
+                                 InvokeInitUniqueDbMissingSharedCleared,
+                             this, std::move(client), std::move(callback)));
+          return;
+        }
         // If the unique DB failed to open, and the data is located on it then
-        // we throw an error. We ignore the deletion flag because we want both
-        // databases to be open before we delete the shared DB.
+        // we throw an error.
         std::move(callback).Run(Enums::InitStatus::kError);
         OnInitDone(ProtoDatabaseInitState::kUniqueDbOpenFailed);
         return;
-        break;
     }
   }
 
@@ -604,6 +615,24 @@ void ProtoDatabaseSelector::RemoveKeysForTesting(
   db_->RemoveKeysForTesting(key_filter, target_prefix, std::move(callback));
 }
 
+void ProtoDatabaseSelector::InvokeInitUniqueDbMissingSharedCleared(
+    std::unique_ptr<SharedProtoDatabaseClient> client,
+    Callbacks::InitStatusCallback callback,
+    bool shared_cleared) {
+  if (!shared_cleared) {
+    OnInitDone(
+        ProtoDatabaseInitState::kFailureUniqueDbMissingClearSharedFailed);
+    std::move(callback).Run(Enums::InitStatus::kError);
+    return;
+  }
+  // Reset state to migrated to shared since unique db is missing.
+  client->UpdateClientInitMetadata(
+      SharedDBMetadataProto::MIGRATE_TO_SHARED_SUCCESSFUL);
+  db_ = std::move(client);
+  OnInitDone(ProtoDatabaseInitState::kUniqueDbMissingSharedReturned);
+  std::move(callback).Run(Enums::InitStatus::kOK);
+}
+
 void ProtoDatabaseSelector::OnInitDone(
     ProtoDatabaseSelector::ProtoDatabaseInitState state) {
   RecordInitState(state);

components/leveldb_proto/internal/proto_database_selector.h

@@ -61,7 +61,8 @@ class COMPONENT_EXPORT(LEVELDB_PROTO) ProtoDatabaseSelector
     kSharedDbClientMissing = 25,
     kFailureNoSharedDBProviderUniqueFailed = 26,
     kSuccessNoSharedDBProviderUniqueSucceeded = 27,
-    kMaxValue = kSuccessNoSharedDBProviderUniqueSucceeded,
+    kFailureUniqueDbMissingClearSharedFailed = 28,
+    kMaxValue = kFailureUniqueDbMissingClearSharedFailed,
   };
 
   static void RecordInitState(ProtoDatabaseInitState state);
@@ -185,6 +186,10 @@ class COMPONENT_EXPORT(LEVELDB_PROTO) ProtoDatabaseSelector
       Callbacks::InitStatusCallback callback,
       bool success);
   void OnInitDone(ProtoDatabaseInitState state);
+  void InvokeInitUniqueDbMissingSharedCleared(
+      std::unique_ptr<SharedProtoDatabaseClient> client,
+      Callbacks::InitStatusCallback,
+      bool shared_cleared);
 
   ProtoDbType db_type_;
   const scoped_refptr<base::SequencedTaskRunner> task_runner_;

components/leveldb_proto/internal/shared_proto_database.h

@@ -68,6 +68,7 @@ class COMPONENT_EXPORT(LEVELDB_PROTO) SharedProtoDatabase
   friend class SharedProtoDatabaseTest;
   friend class SharedProtoDatabaseClientTest;
   friend class TestSharedProtoDatabase;
+  friend class TestSharedProtoDatabaseClient;
   FRIEND_TEST_ALL_PREFIXES(SharedProtoDatabaseTest,
                            CancelDeleteObsoleteClients);
   FRIEND_TEST_ALL_PREFIXES(SharedProtoDatabaseTest, DeleteObsoleteClients);

tools/metrics/histograms/enums.xml

@@ -54496,6 +54496,7 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf
       label="Failed: No shared DB provider provided, unique DB failed"/>
   <int value="27"
       label="Success: No shared DB provider provided, using unique"/>
+  <int value="28" label="Failed: Unique DB missing, shared deletion failed"/>
 </enum>
 
 <enum name="ProvisionalLoadEvent">