DOM Storage: Simplify metadata fetch

This removes the generalized GetMany API from LevelDBDatabaseImpl,
which was only used by session storage to fetch specific metadata
fields. The database logic has been migrated fully into
SessionStorageContextMojo.

Bug: 1000959
Change-Id: Ie972684a5ccedfc879997540ea2b3d4ac6698dde
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1859587Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Reviewed-by: Robert Sesek <rsesek@chromium.org>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#706180}

components/services/leveldb/leveldb_database_impl.cc

@@ -176,51 +176,6 @@ void LevelDBDatabaseImpl::Get(const std::vector<uint8_t>& key,
                       std::move(callback)));
 }
 
-void LevelDBDatabaseImpl::GetMany(
-    std::vector<mojom::GetManyRequestPtr> keys_or_prefixes,
-    GetManyCallback callback) {
-  RunDatabaseTask(
-      base::BindOnce(
-          [](std::vector<mojom::GetManyRequestPtr> keys_or_prefixes,
-             const storage::DomStorageDatabase& db) {
-            std::vector<mojom::GetManyResultPtr> data;
-            for (const auto& request : keys_or_prefixes) {
-              mojom::GetManyResultPtr result = mojom::GetManyResult::New();
-              Status status;
-
-              if (request->is_key()) {
-                const std::vector<uint8_t>& key = request->get_key();
-                storage::DomStorageDatabase::Value value;
-                status = db.Get(key, &value);
-                if (status.ok())
-                  result->set_key_value(value);
-                else
-                  result->set_status(LeveldbStatusToError(status));
-              } else {
-                const std::vector<uint8_t>& key_prefix =
-                    request->get_key_prefix();
-                std::vector<storage::DomStorageDatabase::KeyValuePair> entries;
-                status = db.GetPrefixed(key_prefix, &entries);
-                if (status.ok()) {
-                  std::vector<mojom::KeyValuePtr> out_entries;
-                  for (auto& entry : entries) {
-                    out_entries.push_back(
-                        mojom::KeyValue::New(entry.key, entry.value));
-                  }
-                  result->set_key_prefix_values(std::move(out_entries));
-                } else {
-                  result->set_status(LeveldbStatusToError(status));
-                }
-              }
-
-              data.push_back(std::move(result));
-            }
-            return data;
-          },
-          std::move(keys_or_prefixes)),
-      std::move(callback));
-}
-
 void LevelDBDatabaseImpl::GetPrefixed(const std::vector<uint8_t>& key_prefix,
                                       GetPrefixedCallback callback) {
   struct GetPrefixedResult {

components/services/leveldb/leveldb_database_impl.h

@@ -79,26 +79,10 @@ class LevelDBDatabaseImpl {
   void GetPrefixed(const std::vector<uint8_t>& key_prefix,
                    GetPrefixedCallback callback);
 
-  using GetManyCallback =
-      base::OnceCallback<void(std::vector<mojom::GetManyResultPtr>)>;
-  void GetMany(std::vector<mojom::GetManyRequestPtr> keys_or_prefixes,
-               GetManyCallback callback);
-
   void CopyPrefixed(const std::vector<uint8_t>& source_key_prefix,
                     const std::vector<uint8_t>& destination_key_prefix,
                     StatusCallback callback);
 
- private:
-  using StatusAndKeyValues =
-      std::tuple<Status, std::vector<mojom::KeyValuePtr>>;
-
-  void OnDatabaseOpened(
-      StatusCallback callback,
-      base::SequenceBound<storage::DomStorageDatabase> database,
-      leveldb::Status status);
-
-  explicit LevelDBDatabaseImpl();
-
   template <typename ResultType>
   void RunDatabaseTask(
       base::OnceCallback<ResultType(const storage::DomStorageDatabase&)> task,
@@ -122,6 +106,17 @@ class LevelDBDatabaseImpl {
     }
   }
 
+ private:
+  using StatusAndKeyValues =
+      std::tuple<Status, std::vector<mojom::KeyValuePtr>>;
+
+  void OnDatabaseOpened(
+      StatusCallback callback,
+      base::SequenceBound<storage::DomStorageDatabase> database,
+      leveldb::Status status);
+
+  explicit LevelDBDatabaseImpl();
+
   base::SequenceBound<storage::DomStorageDatabase> database_;
 
   using DatabaseTask =

components/services/leveldb/public/mojom/leveldb.mojom

@@ -33,15 +33,3 @@ struct KeyValue {
   array<uint8> key;
   array<uint8> value;
 };
-
-union GetManyRequest {
-  array<uint8> key_prefix;
-  array<uint8> key;
-};
-
-union GetManyResult {
-  DatabaseError status;
-  array<KeyValue> key_prefix_values;
-  array<uint8> key_value;
-};
-

content/browser/dom_storage/session_storage_context_mojo.cc

@@ -13,6 +13,7 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/compiler_specific.h"
+#include "base/containers/span.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_functions.h"
 #include "base/metrics/histogram_macros.h"
@@ -768,73 +769,70 @@ void SessionStorageContextMojo::OnDatabaseOpened(
     return;
   }
 
-  std::vector<uint8_t> database_version(
-      SessionStorageMetadata::kDatabaseVersionBytes,
-      std::end(SessionStorageMetadata::kDatabaseVersionBytes));
-  std::vector<uint8_t> namespace_prefix(
-      SessionStorageMetadata::kNamespacePrefixBytes,
-      std::end(SessionStorageMetadata::kNamespacePrefixBytes));
-  std::vector<uint8_t> next_map_id_key(
-      SessionStorageMetadata::kNextMapIdKeyBytes,
-      std::end(SessionStorageMetadata::kNextMapIdKeyBytes));
-
-  std::vector<leveldb::mojom::GetManyRequestPtr> requests;
-  requests.emplace_back(
-      leveldb::mojom::GetManyRequest::NewKey(std::move(database_version)));
-  requests.emplace_back(leveldb::mojom::GetManyRequest::NewKeyPrefix(
-      std::move(namespace_prefix)));
-  requests.emplace_back(
-      leveldb::mojom::GetManyRequest::NewKey(std::move(next_map_id_key)));
-
-  database_->GetMany(
-      std::move(requests),
+  database_->RunDatabaseTask(
+      base::BindOnce([](const storage::DomStorageDatabase& db) {
+        DatabaseMetadataResult result;
+        result.version_status = db.Get(
+            base::make_span(SessionStorageMetadata::kDatabaseVersionBytes),
+            &result.version);
+        result.next_map_id_status =
+            db.Get(base::make_span(SessionStorageMetadata::kNextMapIdKeyBytes),
+                   &result.next_map_id);
+        result.namespaces_status = db.GetPrefixed(
+            base::make_span(SessionStorageMetadata::kNamespacePrefixBytes),
+            &result.namespaces);
+        return result;
+      }),
       base::BindOnce(&SessionStorageContextMojo::OnGotDatabaseMetadata,
                      weak_ptr_factory_.GetWeakPtr()));
 }
 
-void SessionStorageContextMojo::OnGotDatabaseMetadata(
-    std::vector<leveldb::mojom::GetManyResultPtr> results) {
-  DCHECK_EQ(results.size(), 3U);
+SessionStorageContextMojo::DatabaseMetadataResult::DatabaseMetadataResult() =
+    default;
 
-  std::vector<leveldb::mojom::BatchedOperationPtr> migration_operations;
+SessionStorageContextMojo::DatabaseMetadataResult::DatabaseMetadataResult(
+    DatabaseMetadataResult&&) = default;
 
-  OpenResult open_result;
-  const char* histogram_name;
+SessionStorageContextMojo::DatabaseMetadataResult::~DatabaseMetadataResult() =
+    default;
+
+void SessionStorageContextMojo::OnGotDatabaseMetadata(
+    DatabaseMetadataResult result) {
+  std::vector<leveldb::mojom::BatchedOperationPtr> migration_operations;
 
-  std::tie(open_result, histogram_name) =
-      ParseDatabaseVersion(results[0], &migration_operations);
-  if (open_result != OpenResult::kSuccess) {
-    LogDatabaseOpenResult(open_result);
-    DeleteAndRecreateDatabase(histogram_name);
+  MetadataParseResult version_parse =
+      ParseDatabaseVersion(&result, &migration_operations);
+  if (version_parse.open_result != OpenResult::kSuccess) {
+    LogDatabaseOpenResult(version_parse.open_result);
+    DeleteAndRecreateDatabase(version_parse.histogram_name);
     return;
   }
 
-  std::tie(open_result, histogram_name) =
-      ParseNamespaces(results[1], std::move(migration_operations));
-  if (open_result != OpenResult::kSuccess) {
-    LogDatabaseOpenResult(open_result);
-    DeleteAndRecreateDatabase(histogram_name);
+  MetadataParseResult namespaces_parse =
+      ParseNamespaces(&result, std::move(migration_operations));
+  if (namespaces_parse.open_result != OpenResult::kSuccess) {
+    LogDatabaseOpenResult(namespaces_parse.open_result);
+    DeleteAndRecreateDatabase(namespaces_parse.histogram_name);
     return;
   }
 
-  std::tie(open_result, histogram_name) = ParseNextMapId(results[2]);
-  if (open_result != OpenResult::kSuccess) {
-    LogDatabaseOpenResult(open_result);
-    DeleteAndRecreateDatabase(histogram_name);
+  MetadataParseResult next_map_id_parse = ParseNextMapId(&result);
+  if (next_map_id_parse.open_result != OpenResult::kSuccess) {
+    LogDatabaseOpenResult(next_map_id_parse.open_result);
+    DeleteAndRecreateDatabase(next_map_id_parse.histogram_name);
     return;
   }
 
   OnConnectionFinished();
 }
 
-SessionStorageContextMojo::OpenResultAndHistogramName
+SessionStorageContextMojo::MetadataParseResult
 SessionStorageContextMojo::ParseDatabaseVersion(
-    const leveldb::mojom::GetManyResultPtr& result,
+    DatabaseMetadataResult* result,
     std::vector<leveldb::mojom::BatchedOperationPtr>* migration_operations) {
-  if (result->is_key_value()) {
-    const std::vector<uint8_t>& value = result->get_key_value();
-
-    if (!metadata_.ParseDatabaseVersion(value, migration_operations)) {
+  if (result->version_status.ok()) {
+    if (!metadata_.ParseDatabaseVersion(std::move(result->version),
+                                        migration_operations)) {
       return {OpenResult::kInvalidVersion,
               "SessionStorageContext.OpenResultAfterInvalidVersion"};
     }
@@ -842,47 +840,43 @@ SessionStorageContextMojo::ParseDatabaseVersion(
     return {OpenResult::kSuccess, ""};
   }
 
-  // Failed to get DatabaseVersion, |result| contains error status
-  leveldb::mojom::DatabaseError status = result->get_status();
-
-  if (status == leveldb::mojom::DatabaseError::NOT_FOUND) {
+  if (result->version_status.IsNotFound()) {
     // treat as v0 or new database
     metadata_.ParseDatabaseVersion(base::nullopt, migration_operations);
     return {OpenResult::kSuccess, ""};
   }
 
   // Other read error, Possibly database corruption
-  UMA_HISTOGRAM_ENUMERATION("SessionStorageContext.ReadVersionError",
-                            leveldb::GetLevelDBStatusUMAValue(status),
-                            leveldb_env::LEVELDB_STATUS_MAX);
+  UMA_HISTOGRAM_ENUMERATION(
+      "SessionStorageContext.ReadVersionError",
+      leveldb_env::GetLevelDBStatusUMAValue(result->version_status),
+      leveldb_env::LEVELDB_STATUS_MAX);
   return {OpenResult::kVersionReadError,
           "SessionStorageContext.OpenResultAfterReadVersionError"};
 }
 
-SessionStorageContextMojo::OpenResultAndHistogramName
+SessionStorageContextMojo::MetadataParseResult
 SessionStorageContextMojo::ParseNamespaces(
-    const leveldb::mojom::GetManyResultPtr& result,
+    DatabaseMetadataResult* result,
     std::vector<leveldb::mojom::BatchedOperationPtr> migration_operations) {
   DCHECK_EQ(connection_state_, CONNECTION_IN_PROGRESS);
 
-  if (result->is_status()) {
+  if (!result->namespaces_status.ok()) {
     UMA_HISTOGRAM_ENUMERATION(
         "SessionStorageContext.ReadNamespacesError",
-        leveldb::GetLevelDBStatusUMAValue(result->get_status()),
+        leveldb_env::GetLevelDBStatusUMAValue(result->namespaces_status),
         leveldb_env::LEVELDB_STATUS_MAX);
     return {OpenResult::kNamespacesReadError,
             "SessionStorageContext.OpenResultAfterReadNamespacesError"};
   }
 
-  DCHECK(result->is_key_prefix_values());
-
   bool parsing_success = metadata_.ParseNamespaces(
-      std::move(result->get_key_prefix_values()), &migration_operations);
+      std::move(result->namespaces), &migration_operations);
 
   if (!parsing_success) {
     UMA_HISTOGRAM_ENUMERATION(
         "SessionStorageContext.ReadNamespacesError",
-        leveldb::GetLevelDBStatusUMAValue(leveldb::mojom::DatabaseError::OK),
+        leveldb_env::GetLevelDBStatusUMAValue(leveldb::Status::OK()),
         leveldb_env::LEVELDB_STATUS_MAX);
     return {OpenResult::kNamespacesReadError,
             "SessionStorageContext.OpenResultAfterReadNamespacesError"};
@@ -910,26 +904,22 @@ SessionStorageContextMojo::ParseNamespaces(
   return {OpenResult::kSuccess, ""};
 }
 
-SessionStorageContextMojo::OpenResultAndHistogramName
-SessionStorageContextMojo::ParseNextMapId(
-    const leveldb::mojom::GetManyResultPtr& result) {
-  if (result->is_status()) {
-    leveldb::mojom::DatabaseError status = result->get_status();
-
-    if (status == leveldb::mojom::DatabaseError::NOT_FOUND) {
+SessionStorageContextMojo::MetadataParseResult
+SessionStorageContextMojo::ParseNextMapId(DatabaseMetadataResult* result) {
+  if (!result->next_map_id_status.ok()) {
+    if (result->next_map_id_status.IsNotFound())
       return {OpenResult::kSuccess, ""};
-    }
 
     // Other read error. Possibly database corruption.
-    UMA_HISTOGRAM_ENUMERATION("SessionStorageContext.ReadNextMapIdError",
-                              leveldb::GetLevelDBStatusUMAValue(status),
-                              leveldb_env::LEVELDB_STATUS_MAX);
+    UMA_HISTOGRAM_ENUMERATION(
+        "SessionStorageContext.ReadNextMapIdError",
+        leveldb_env::GetLevelDBStatusUMAValue(result->next_map_id_status),
+        leveldb_env::LEVELDB_STATUS_MAX);
     return {OpenResult::kNamespacesReadError,
             "SessionStorageContext.OpenResultAfterReadNextMapIdError"};
   }
 
-  DCHECK(result->is_key_value());
-  metadata_.ParseNextMapId(result->get_key_value());
+  metadata_.ParseNextMapId(std::move(result->next_map_id));
   return {OpenResult::kSuccess, ""};
 }
 

content/browser/dom_storage/session_storage_context_mojo.h

@@ -208,18 +208,34 @@ class CONTENT_EXPORT SessionStorageContextMojo
   void InitiateConnection(bool in_memory_only = false);
   void OnDatabaseOpened(leveldb::mojom::DatabaseError status);
 
-  void OnGotDatabaseMetadata(
-      std::vector<leveldb::mojom::GetManyResultPtr> results);
+  struct DatabaseMetadataResult {
+    DatabaseMetadataResult();
+    DatabaseMetadataResult(DatabaseMetadataResult&&);
+    DatabaseMetadataResult(const DatabaseMetadataResult&) = delete;
+    ~DatabaseMetadataResult();
 
-  using OpenResultAndHistogramName = std::tuple<OpenResult, const char*>;
-  OpenResultAndHistogramName ParseDatabaseVersion(
-      const leveldb::mojom::GetManyResultPtr& result,
+    storage::DomStorageDatabase::Value version;
+    leveldb::Status version_status;
+
+    storage::DomStorageDatabase::Value next_map_id;
+    leveldb::Status next_map_id_status;
+
+    std::vector<storage::DomStorageDatabase::KeyValuePair> namespaces;
+    leveldb::Status namespaces_status;
+  };
+  void OnGotDatabaseMetadata(DatabaseMetadataResult result);
+
+  struct MetadataParseResult {
+    OpenResult open_result;
+    const char* histogram_name;
+  };
+  MetadataParseResult ParseDatabaseVersion(
+      DatabaseMetadataResult* result,
       std::vector<leveldb::mojom::BatchedOperationPtr>* migration_operations);
-  OpenResultAndHistogramName ParseNamespaces(
-      const leveldb::mojom::GetManyResultPtr& result,
+  MetadataParseResult ParseNamespaces(
+      DatabaseMetadataResult* result,
       std::vector<leveldb::mojom::BatchedOperationPtr> migration_operations);
-  OpenResultAndHistogramName ParseNextMapId(
-      const leveldb::mojom::GetManyResultPtr& result);
+  MetadataParseResult ParseNextMapId(DatabaseMetadataResult* result);
 
   void OnConnectionFinished();
   void DeleteAndRecreateDatabase(const char* histogram_name);

content/browser/dom_storage/session_storage_metadata.cc

@@ -121,7 +121,7 @@ bool SessionStorageMetadata::ParseDatabaseVersion(
 }
 
 bool SessionStorageMetadata::ParseNamespaces(
-    std::vector<leveldb::mojom::KeyValuePtr> values,
+    std::vector<storage::DomStorageDatabase::KeyValuePair> values,
     std::vector<leveldb::mojom::BatchedOperationPtr>* upgrade_operations) {
   namespace_origin_map_.clear();
   next_map_id_from_namespaces_ = 0;
@@ -131,11 +131,11 @@ bool SessionStorageMetadata::ParseNamespaces(
   std::map<url::Origin, scoped_refptr<MapData>>* last_namespace = nullptr;
   std::map<int64_t, scoped_refptr<MapData>> maps;
   bool error = false;
-  for (const leveldb::mojom::KeyValuePtr& key_value : values) {
-    size_t key_size = key_value->key.size();
+  for (const storage::DomStorageDatabase::KeyValuePair& key_value : values) {
+    size_t key_size = key_value.key.size();
 
     base::StringPiece key_as_string =
-        leveldb::Uint8VectorToStringPiece(key_value->key);
+        leveldb::Uint8VectorToStringPiece(key_value.key);
 
     if (key_size < kNamespacePrefixLength) {
       LOG(ERROR) << "Key size is less than prefix length: " << key_as_string;
@@ -176,10 +176,10 @@ bool SessionStorageMetadata::ParseNamespaces(
         key_as_string.substr(kPrefixBeforeOriginLength);
 
     int64_t map_number;
-    if (!ValueToNumber(key_value->value, &map_number)) {
+    if (!ValueToNumber(key_value.value, &map_number)) {
       error = true;
       LOG(ERROR) << "Could not parse map number "
-                 << leveldb::Uint8VectorToStringPiece(key_value->value);
+                 << leveldb::Uint8VectorToStringPiece(key_value.value);
       break;
     }
 

content/browser/dom_storage/session_storage_metadata.h

@@ -11,6 +11,7 @@
 
 #include "base/memory/ref_counted.h"
 #include "components/services/leveldb/public/mojom/leveldb.mojom.h"
+#include "components/services/storage/dom_storage/dom_storage_database.h"
 #include "content/common/content_export.h"
 #include "url/origin.h"
 
@@ -106,7 +107,7 @@ class CONTENT_EXPORT SessionStorageMetadata {
   // will be populated in |upgrade_operations|. This call is not necessary on
   // new databases.
   bool ParseNamespaces(
-      std::vector<leveldb::mojom::KeyValuePtr> values,
+      std::vector<storage::DomStorageDatabase::KeyValuePair> values,
       std::vector<leveldb::mojom::BatchedOperationPtr>* upgrade_operations);
 
   // Parses the next map id from the given bytes. If that fails, then it uses

content/browser/dom_storage/session_storage_metadata_unittest.cc

@@ -17,6 +17,7 @@
 #include "base/test/task_environment.h"
 #include "components/services/leveldb/leveldb_database_impl.h"
 #include "components/services/leveldb/public/cpp/util.h"
+#include "components/services/storage/dom_storage/dom_storage_database.h"
 #include "content/browser/dom_storage/dom_storage_types.h"
 #include "content/browser/dom_storage/session_storage_database.h"
 #include "content/browser/indexed_db/leveldb/leveldb_env.h"
@@ -92,12 +93,7 @@ class SessionStorageMetadataTest : public testing::Test {
 
     metadata->ParseNextMapId(next_map_id_value);
 
-    std::vector<leveldb::mojom::KeyValuePtr> namespace_values;
-    for (auto& entry : namespace_entries) {
-      namespace_values.push_back(
-          leveldb::mojom::KeyValue::New(entry.key, entry.value));
-    }
-    EXPECT_TRUE(metadata->ParseNamespaces(std::move(namespace_values),
+    EXPECT_TRUE(metadata->ParseNamespaces(std::move(namespace_entries),
                                           &migration_operations));
     EXPECT_TRUE(migration_operations.empty());
   }
@@ -487,7 +483,7 @@ TEST_F(SessionStorageMetadataMigrationTest, MigrateV0ToV1) {
   metadata.ParseNextMapId(leveldb::StdStringToUint8Vector(db_value));
 
   // Get all keys-value pairs with the given key prefix
-  std::vector<leveldb::mojom::KeyValuePtr> values;
+  std::vector<storage::DomStorageDatabase::KeyValuePair> values;
   {
     std::unique_ptr<leveldb::Iterator> it(db()->NewIterator(options));
     it->Seek(leveldb::Slice("namespace-"));
@@ -495,9 +491,8 @@ TEST_F(SessionStorageMetadataMigrationTest, MigrateV0ToV1) {
       if (!it->key().starts_with(leveldb::Slice("namespace-")))
         break;
       leveldb::mojom::KeyValuePtr kv = leveldb::mojom::KeyValue::New();
-      kv->key = leveldb::GetVectorFor(it->key());
-      kv->value = leveldb::GetVectorFor(it->value());
-      values.push_back(std::move(kv));
+      values.emplace_back(leveldb::GetVectorFor(it->key()),
+                          leveldb::GetVectorFor(it->value()));
     }
     EXPECT_TRUE(it->status().ok());
   }