WebApp: Implement basic Copy-On-Write for the batched writes API.

Previously we did in-place immediate updates for existing apps.
This was not consistent with the commit ideology since effects were becoming
visible immediately. It didn't match the creation and deletion of apps: they
were deferred until commit.

With this CL we implement basic copy-on-write for WebApp object updates.
We commit the created copy into the original for real, during the commit call.

In next CLs: Fix UpdateSync algorithm to avoid excessive change_processor
Put and Delete calls.

TBR=alancutter@chromium.org

Bug: 860583
Change-Id: I26eb53b668f74ce0c606a60af9016aa8a4cca1c6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1851792
Commit-Queue: Alexey Baskakov <loyso@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705007}

chrome/browser/web_applications/web_app.cc

@@ -19,6 +19,10 @@ WebApp::WebApp(const AppId& app_id)
 
 WebApp::~WebApp() = default;
 
+WebApp::WebApp(const WebApp& web_app) = default;
+
+WebApp& WebApp::operator=(WebApp&& web_app) = default;
+
 void WebApp::AddSource(Source::Type source) {
   sources_[source] = true;
 }
@@ -74,14 +78,18 @@ void WebApp::SetIcons(Icons icons) {
   icons_ = std::move(icons);
 }
 
-void WebApp::SetSyncData(const SyncData& sync_data) {
-  sync_data_ = sync_data;
+void WebApp::SetSyncData(SyncData sync_data) {
+  sync_data_ = std::move(sync_data);
 }
 
 WebApp::SyncData::SyncData() = default;
 
 WebApp::SyncData::~SyncData() = default;
 
+WebApp::SyncData::SyncData(const SyncData& sync_data) = default;
+
+WebApp::SyncData& WebApp::SyncData::operator=(SyncData&& sync_data) = default;
+
 std::ostream& operator<<(std::ostream& out, const WebApp& app) {
   const std::string theme_color =
       app.theme_color_.has_value()

chrome/browser/web_applications/web_app.h

@@ -10,7 +10,6 @@
 #include <string>
 #include <vector>
 
-#include "base/macros.h"
 #include "base/optional.h"
 #include "chrome/browser/web_applications/components/web_app_constants.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
@@ -25,6 +24,14 @@ class WebApp {
   explicit WebApp(const AppId& app_id);
   ~WebApp();
 
+  // Copyable and move-assignable to support Copy-on-Write with Commit.
+  WebApp(const WebApp& web_app);
+  WebApp& operator=(WebApp&& web_app);
+
+  // Explicitly disallow other copy ctors and assign operators.
+  WebApp(WebApp&&) = delete;
+  WebApp& operator=(const WebApp&) = delete;
+
   const AppId& app_id() const { return app_id_; }
 
   const std::string& name() const { return name_; }
@@ -58,6 +65,10 @@ class WebApp {
   struct SyncData {
     SyncData();
     ~SyncData();
+    // Copyable and move-assignable to support Copy-on-Write with Commit.
+    SyncData(const SyncData& sync_data);
+    SyncData& operator=(SyncData&& sync_data);
+
     std::string name;
     base::Optional<SkColor> theme_color;
   };
@@ -81,14 +92,14 @@ class WebApp {
   void SetIsSyncPlaceholder(bool is_sync_placeholder);
   void SetIcons(Icons icons);
 
-  void SetSyncData(const SyncData& sync_data);
+  void SetSyncData(SyncData sync_data);
 
  private:
   friend class WebAppDatabase;
   friend bool operator==(const WebApp&, const WebApp&);
   friend std::ostream& operator<<(std::ostream&, const WebApp&);
 
-  const AppId app_id_;
+  AppId app_id_;
 
   // This set always contains at least one source.
   using Sources = std::bitset<Source::kMaxValue>;
@@ -107,8 +118,6 @@ class WebApp {
   Icons icons_;
 
   SyncData sync_data_;
-
-  DISALLOW_COPY_AND_ASSIGN(WebApp);
 };
 
 // For logging and debug purposes.

chrome/browser/web_applications/web_app_database.cc

@@ -62,14 +62,14 @@ void WebAppDatabase::Write(
     write_batch->WriteData(web_app->app_id(), proto->SerializeAsString());
   }
 
-  for (const AppId& app_id : update_data.apps_to_delete)
-    write_batch->DeleteData(app_id);
-
-  for (const WebApp* web_app : update_data.apps_to_update) {
+  for (const std::unique_ptr<WebApp>& web_app : update_data.apps_to_update) {
     auto proto = CreateWebAppProto(*web_app);
     write_batch->WriteData(web_app->app_id(), proto->SerializeAsString());
   }
 
+  for (const AppId& app_id : update_data.apps_to_delete)
+    write_batch->DeleteData(app_id);
+
   store_->CommitWriteBatch(
       std::move(write_batch),
       base::BindOnce(&WebAppDatabase::OnDataWritten,
@@ -216,7 +216,7 @@ std::unique_ptr<WebApp> WebAppDatabase::CreateWebApp(
     parsed_sync_data.name = sync_data.name();
   if (sync_data.has_theme_color())
     parsed_sync_data.theme_color = sync_data.theme_color();
-  web_app->SetSyncData(parsed_sync_data);
+  web_app->SetSyncData(std::move(parsed_sync_data));
 
   WebApp::Icons icons;
   for (int i = 0; i < local_data.icons_size(); ++i) {

chrome/browser/web_applications/web_app_database_unittest.cc

@@ -87,7 +87,7 @@ class WebAppDatabaseTest : public WebAppTest {
     WebApp::SyncData sync_data;
     sync_data.name = "Sync" + name;
     sync_data.theme_color = synced_theme_color;
-    app->SetSyncData(sync_data);
+    app->SetSyncData(std::move(sync_data));
 
     return app;
   }
@@ -185,8 +185,8 @@ TEST_F(WebAppDatabaseTest, WriteAndDeleteAppsWithCallbacks) {
   const int num_apps = 10;
   const std::string base_url = "https://example.com/path";
 
-  RegistryUpdateData::AppsToCreate apps_to_create;
-  RegistryUpdateData::AppsToDelete apps_to_delete;
+  RegistryUpdateData::Apps apps_to_create;
+  std::vector<AppId> apps_to_delete;
   Registry expected_registry;
 
   for (int i = 0; i < num_apps; ++i) {

chrome/browser/web_applications/web_app_install_finalizer.cc

@@ -134,7 +134,7 @@ void WebAppInstallFinalizer::FinalizeInstall(
   WebApp::SyncData sync_data;
   sync_data.name = base::UTF16ToUTF8(web_app_info.title);
   sync_data.theme_color = web_app_info.theme_color;
-  web_app->SetSyncData(sync_data);
+  web_app->SetSyncData(std::move(sync_data));
 
   icon_manager_->WriteData(
       std::move(app_id), std::make_unique<WebApplicationInfo>(web_app_info),

chrome/browser/web_applications/web_app_registrar_unittest.cc

@@ -629,4 +629,47 @@ TEST_F(WebAppRegistrarTest, ScopedRegistryUpdate) {
   EXPECT_TRUE(ids.empty());
 }
 
+TEST_F(WebAppRegistrarTest, CopyOnWrite) {
+  controller().Init();
+
+  const GURL launch_url("https://example.com");
+  const AppId app_id = GenerateAppIdFromURL(launch_url);
+  const WebApp* app = nullptr;
+  {
+    auto new_app = CreateWebApp(launch_url.spec());
+    app = new_app.get();
+    RegisterApp(std::move(new_app));
+  }
+
+  {
+    std::unique_ptr<WebAppRegistryUpdate> update = sync_bridge().BeginUpdate();
+
+    WebApp* app_copy = update->UpdateApp(app_id);
+    EXPECT_TRUE(app_copy);
+    EXPECT_NE(app_copy, app);
+
+    app_copy->SetName("New Name");
+    EXPECT_EQ(app_copy->name(), "New Name");
+    EXPECT_EQ(app->name(), "Name");
+
+    app_copy->AddSource(Source::kPolicy);
+    app_copy->RemoveSource(Source::kSync);
+
+    EXPECT_FALSE(app_copy->IsSynced());
+    EXPECT_TRUE(app_copy->HasAnySources());
+
+    EXPECT_TRUE(app->IsSynced());
+    EXPECT_TRUE(app->HasAnySources());
+
+    SyncBridgeCommitUpdate(std::move(update));
+  }
+
+  // Pointer value stays the same.
+  EXPECT_EQ(app, registrar().GetAppById(app_id));
+
+  EXPECT_EQ(app->name(), "New Name");
+  EXPECT_FALSE(app->IsSynced());
+  EXPECT_TRUE(app->HasAnySources());
+}
+
 }  // namespace web_app

chrome/browser/web_applications/web_app_registry_update.cc

@@ -20,10 +20,10 @@ bool RegistryUpdateData::IsEmpty() const {
          apps_to_update.empty();
 }
 
-WebAppRegistryUpdate::WebAppRegistryUpdate(
-    WebAppRegistrarMutable* mutable_registrar)
-    : mutable_registrar_(mutable_registrar) {
-  DCHECK(mutable_registrar_);
+WebAppRegistryUpdate::WebAppRegistryUpdate(const WebAppRegistrar* registrar,
+                                           util::PassKey<WebAppSyncBridge>)
+    : registrar_(registrar) {
+  DCHECK(registrar_);
   update_data_ = std::make_unique<RegistryUpdateData>();
 }
 
@@ -32,7 +32,7 @@ WebAppRegistryUpdate::~WebAppRegistryUpdate() = default;
 void WebAppRegistryUpdate::CreateApp(std::unique_ptr<WebApp> web_app) {
   DCHECK(update_data_);
   DCHECK(!web_app->app_id().empty());
-  DCHECK(!mutable_registrar_->GetAppById(web_app->app_id()));
+  DCHECK(!registrar_->GetAppById(web_app->app_id()));
   DCHECK(!base::Contains(update_data_->apps_to_create, web_app));
 
   update_data_->apps_to_create.push_back(std::move(web_app));
@@ -41,7 +41,7 @@ void WebAppRegistryUpdate::CreateApp(std::unique_ptr<WebApp> web_app) {
 void WebAppRegistryUpdate::DeleteApp(const AppId& app_id) {
   DCHECK(update_data_);
   DCHECK(!app_id.empty());
-  DCHECK(mutable_registrar_->GetAppById(app_id));
+  DCHECK(registrar_->GetAppById(app_id));
   DCHECK(!base::Contains(update_data_->apps_to_delete, app_id));
 
   update_data_->apps_to_delete.push_back(app_id);
@@ -49,11 +49,21 @@ void WebAppRegistryUpdate::DeleteApp(const AppId& app_id) {
 
 WebApp* WebAppRegistryUpdate::UpdateApp(const AppId& app_id) {
   DCHECK(update_data_);
-  WebApp* app = mutable_registrar_->GetAppByIdMutable(app_id);
-  if (app)
-    update_data_->apps_to_update.insert(app);
+  const WebApp* original_app = registrar_->GetAppById(app_id);
+  if (!original_app)
+    return nullptr;
 
-  return app;
+  for (auto& app_to_update : update_data_->apps_to_update) {
+    if (app_to_update->app_id() == app_id)
+      return app_to_update.get();
+  }
+
+  // Make a copy on write.
+  auto app_copy = std::make_unique<WebApp>(*original_app);
+  WebApp* app_copy_ptr = app_copy.get();
+  update_data_->apps_to_update.push_back(std::move(app_copy));
+
+  return app_copy_ptr;
 }
 
 std::unique_ptr<RegistryUpdateData> WebAppRegistryUpdate::TakeUpdateData() {

chrome/browser/web_applications/web_app_registry_update.h

@@ -9,14 +9,14 @@
 #include <vector>
 
 #include "base/callback.h"
-#include "base/containers/flat_set.h"
 #include "base/macros.h"
+#include "base/util/type_safety/pass_key.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 
 namespace web_app {
 
 class WebApp;
-class WebAppRegistrarMutable;
+class WebAppRegistrar;
 class WebAppSyncBridge;
 
 // A raw registry update data.
@@ -24,14 +24,11 @@ struct RegistryUpdateData {
   RegistryUpdateData();
   ~RegistryUpdateData();
 
-  using AppsToCreate = std::vector<std::unique_ptr<WebApp>>;
-  AppsToCreate apps_to_create;
+  using Apps = std::vector<std::unique_ptr<WebApp>>;
+  Apps apps_to_create;
+  Apps apps_to_update;
 
-  using AppsToDelete = std::vector<AppId>;
-  AppsToDelete apps_to_delete;
-
-  using AppsToUpdate = base::flat_set<const WebApp*>;
-  AppsToUpdate apps_to_update;
+  std::vector<AppId> apps_to_delete;
 
   bool IsEmpty() const;
 
@@ -43,7 +40,8 @@ struct RegistryUpdateData {
 // WebAppRegistryUpdate is a part of WebAppSyncBridge class.
 class WebAppRegistryUpdate {
  public:
-  explicit WebAppRegistryUpdate(WebAppRegistrarMutable* mutable_registrar);
+  WebAppRegistryUpdate(const WebAppRegistrar* registrar,
+                       util::PassKey<WebAppSyncBridge>);
   ~WebAppRegistryUpdate();
 
   // Register a new app.
@@ -58,7 +56,7 @@ class WebAppRegistryUpdate {
 
  private:
   std::unique_ptr<RegistryUpdateData> update_data_;
-  WebAppRegistrarMutable* const mutable_registrar_;
+  const WebAppRegistrar* const registrar_;
 
   DISALLOW_COPY_AND_ASSIGN(WebAppRegistryUpdate);
 };

chrome/browser/web_applications/web_app_sync_bridge.cc

@@ -13,6 +13,7 @@
 #include "base/containers/flat_set.h"
 #include "base/logging.h"
 #include "base/optional.h"
+#include "base/util/type_safety/pass_key.h"
 #include "chrome/browser/web_applications/components/web_app_helpers.h"
 #include "chrome/browser/web_applications/web_app.h"
 #include "chrome/browser/web_applications/web_app_database.h"
@@ -81,7 +82,7 @@ void ApplySyncDataToApp(const sync_pb::WebAppSpecifics& sync_data,
   parsed_sync_data.name = sync_data.name();
   if (sync_data.has_theme_color())
     parsed_sync_data.theme_color = sync_data.theme_color();
-  app->SetSyncData(parsed_sync_data);
+  app->SetSyncData(std::move(parsed_sync_data));
 }
 
 bool AreAppsLocallyInstalledByDefault() {
@@ -133,7 +134,8 @@ WebAppSyncBridge::~WebAppSyncBridge() = default;
 std::unique_ptr<WebAppRegistryUpdate> WebAppSyncBridge::BeginUpdate() {
   DCHECK(!is_in_update_);
   is_in_update_ = true;
-  return std::make_unique<WebAppRegistryUpdate>(registrar_);
+  return std::make_unique<WebAppRegistryUpdate>(
+      registrar_, util::PassKey<WebAppSyncBridge>());
 }
 
 void WebAppSyncBridge::CommitUpdate(
@@ -197,11 +199,11 @@ void WebAppSyncBridge::CheckRegistryUpdateData(
   for (const std::unique_ptr<WebApp>& web_app : update_data.apps_to_create)
     DCHECK(!registrar_->GetAppById(web_app->app_id()));
 
+  for (const std::unique_ptr<WebApp>& web_app : update_data.apps_to_update)
+    DCHECK(registrar_->GetAppById(web_app->app_id()));
+
   for (const AppId& app_id : update_data.apps_to_delete)
     DCHECK(registrar_->GetAppById(app_id));
-
-  for (const WebApp* web_app : update_data.apps_to_update)
-    DCHECK(registrar_->GetAppById(web_app->app_id()));
 #endif
 }
 
@@ -217,8 +219,13 @@ std::vector<std::unique_ptr<WebApp>> WebAppSyncBridge::UpdateRegistrar(
     registrar_->registry().emplace(std::move(app_id), std::move(web_app));
   }
 
-  // update_data->apps_to_update are ignored here because we do in-place
-  // updates.
+  for (std::unique_ptr<WebApp>& web_app : update_data->apps_to_update) {
+    WebApp* original_web_app = registrar_->GetAppByIdMutable(web_app->app_id());
+    DCHECK(original_web_app);
+    // Commit previously created copy into original. Preserve original web_app
+    // object pointer value (the object's identity) to support stored pointers.
+    *original_web_app = std::move(*web_app);
+  }
 
   for (const AppId& app_id : update_data->apps_to_delete) {
     auto it = registrar_->registry().find(app_id);
@@ -252,11 +259,12 @@ void WebAppSyncBridge::UpdateSync(
 
   // An app may obtain or may loose IsSynced flag without being deleted. We
   // should conservatively include or exclude the app from the synced apps
-  // subset.
+  // subset. TODO(loyso): Use previous state of the app (and CoW) to detect
+  // if IsSynced flag changed.
   //
   // TODO(loyso): Send an update to sync server only if any sync-specific
   // data was changed. Implement some dirty flags in WebApp setter methods.
-  for (const WebApp* app : update_data.apps_to_update) {
+  for (const std::unique_ptr<WebApp>& app : update_data.apps_to_update) {
     if (app->IsSynced()) {
       change_processor()->Put(app->app_id(), CreateSyncEntityData(*app),
                               metadata_change_list);
@@ -266,7 +274,8 @@ void WebAppSyncBridge::UpdateSync(
   }
 
   // We should unconditionally delete from sync (in case IsSynced flag was
-  // removed during the update).
+  // removed during the update). TODO(loyso): Use previous state of the app (and
+  // CoW) to detect if IsSynced flag changed.
   for (const AppId& app_id : update_data.apps_to_delete) {
     const WebApp* app = registrar_->GetAppById(app_id);
     DCHECK(app);
@@ -328,7 +337,7 @@ void WebAppSyncBridge::ApplySyncDataChange(
   // app_id is storage key.
   const AppId& app_id = change.storage_key();
 
-  WebApp* existing_web_app = registrar_->GetAppByIdMutable(app_id);
+  const WebApp* existing_web_app = registrar_->GetAppByIdMutable(app_id);
 
   // Handle deletion first.
   if (change.type() == syncer::EntityChange::ACTION_DELETE) {
@@ -336,10 +345,12 @@ void WebAppSyncBridge::ApplySyncDataChange(
       DLOG(ERROR) << "ApplySyncDataChange error: no app to delete";
       return;
     }
-    existing_web_app->RemoveSource(Source::kSync);
+    // Do copy on write:
+    auto app_copy = std::make_unique<WebApp>(*existing_web_app);
+    app_copy->RemoveSource(Source::kSync);
 
-    if (existing_web_app->HasAnySources())
-      update_local_data->apps_to_update.insert(existing_web_app);
+    if (app_copy->HasAnySources())
+      update_local_data->apps_to_update.push_back(std::move(app_copy));
     else
       update_local_data->apps_to_delete.push_back(app_id);
 
@@ -352,10 +363,12 @@ void WebAppSyncBridge::ApplySyncDataChange(
 
   if (existing_web_app) {
     // Any entities that appear in both sets must be merged.
-    ApplySyncDataToApp(specifics, existing_web_app);
+    // Do copy on write:
+    auto app_copy = std::make_unique<WebApp>(*existing_web_app);
+    ApplySyncDataToApp(specifics, app_copy.get());
     // Preserve web_app->is_locally_installed user's choice here.
 
-    update_local_data->apps_to_update.insert(existing_web_app);
+    update_local_data->apps_to_update.push_back(std::move(app_copy));
   } else {
     // Any remote entities that don’t exist locally must be written to local
     // storage.