Bug 1546723 - Part 2: Make it more clear that checkpointing also notifies observers; r=asuth

This patch renames the Checkpoint IPC message to CheckpointAndNotify. Other structures used by checkpointing are renamed too. Datastore methods SetItem/RemoveItem/Clear no longer call NotifyObservers, it's now up to RecvCheckpointAndNotify to call it. Differential Revision: https://phabricator.services.mozilla.com/D31197
2019-05-15 06:11:10 +02:00 · 2019-05-15 06:11:10 +02:00 · 70f70b482a
--- a/dom/localstorage/ActorsParent.cpp
+++ b/dom/localstorage/ActorsParent.cpp
@ -1750,14 +1750,12 @@ class Datastore final
   * Used by Snapshot::RecvCheckpoint to set a key/value pair as part of a an
   * explicit batch.
   */
-  void SetItem(Database* aDatabase, const nsString& aDocumentURI,
-               const nsString& aKey, const LSValue& aOldValue,
+  void SetItem(Database* aDatabase, const nsString& aKey,
               const LSValue& aValue);

-  void RemoveItem(Database* aDatabase, const nsString& aDocumentURI,
-                  const nsString& aKey, const LSValue& aOldValue);
+  void RemoveItem(Database* aDatabase, const nsString& aKey);

-  void Clear(Database* aDatabase, const nsString& aDocumentURI);
+  void Clear(Database* aDatabase);

  void PrivateBrowsingClear();

@ -1767,6 +1765,10 @@ class Datastore final

  int64_t RequestUpdateUsage(int64_t aRequestedSize, int64_t aMinSize);

+  void NotifyObservers(Database* aDatabase, const nsString& aDocumentURI,
+                       const nsString& aKey, const LSValue& aOldValue,
+                       const LSValue& aNewValue);
+
  NS_INLINE_DECL_REFCOUNTING(Datastore)

 private:
@ -1785,10 +1787,6 @@ class Datastore final
                       const LSValue& aOldValue, bool aAffectsOrder);

  void MarkSnapshotsDirty();
-
-  void NotifyObservers(Database* aDatabase, const nsString& aDocumentURI,
-                       const nsString& aKey, const LSValue& aOldValue,
-                       const LSValue& aNewValue);
 };

 class PreparedDatastore {
@ -2123,8 +2121,8 @@ class Snapshot final : public PBackgroundLSSnapshotParent {

  mozilla::ipc::IPCResult RecvDeleteMe() override;

-  mozilla::ipc::IPCResult RecvCheckpoint(
-      nsTArray<LSWriteInfo>&& aWriteInfos) override;
+  mozilla::ipc::IPCResult RecvCheckpointAndNotify(
+      nsTArray<LSWriteAndNotifyInfo>&& aWriteAndNotifyInfos) override;

  mozilla::ipc::IPCResult RecvFinish() override;

@ -5121,8 +5119,7 @@ void Datastore::GetKeys(nsTArray<nsString>& aKeys) const {
  }
 }

-void Datastore::SetItem(Database* aDatabase, const nsString& aDocumentURI,
-                        const nsString& aKey, const LSValue& aOldValue,
+void Datastore::SetItem(Database* aDatabase, const nsString& aKey,
                        const LSValue& aValue) {
  AssertIsOnBackgroundThread();
  MOZ_ASSERT(aDatabase);
@ -5168,12 +5165,9 @@ void Datastore::SetItem(Database* aDatabase, const nsString& aDocumentURI,
      mConnection->SetItem(aKey, aValue, delta, isNewItem);
    }
  }
-
-  NotifyObservers(aDatabase, aDocumentURI, aKey, aOldValue, aValue);
 }

-void Datastore::RemoveItem(Database* aDatabase, const nsString& aDocumentURI,
-                           const nsString& aKey, const LSValue& aOldValue) {
+void Datastore::RemoveItem(Database* aDatabase, const nsString& aKey) {
  AssertIsOnBackgroundThread();
  MOZ_ASSERT(aDatabase);
  MOZ_ASSERT(!mClosed);
@ -5202,11 +5196,9 @@ void Datastore::RemoveItem(Database* aDatabase, const nsString& aDocumentURI,
      mConnection->RemoveItem(aKey, delta);
    }
  }
-
-  NotifyObservers(aDatabase, aDocumentURI, aKey, aOldValue, VoidLSValue());
 }

-void Datastore::Clear(Database* aDatabase, const nsString& aDocumentURI) {
+void Datastore::Clear(Database* aDatabase) {
  AssertIsOnBackgroundThread();
  MOZ_ASSERT(aDatabase);
  MOZ_ASSERT(!mClosed);
@ -5237,9 +5229,6 @@ void Datastore::Clear(Database* aDatabase, const nsString& aDocumentURI) {
      mConnection->Clear(delta);
    }
  }
-
-  NotifyObservers(aDatabase, aDocumentURI, VoidString(), VoidLSValue(),
-                  VoidLSValue());
 }

 void Datastore::PrivateBrowsingClear() {
@ -5340,6 +5329,35 @@ int64_t Datastore::RequestUpdateUsage(int64_t aRequestedSize,
  return 0;
 }

+void Datastore::NotifyObservers(Database* aDatabase,
+                                const nsString& aDocumentURI,
+                                const nsString& aKey, const LSValue& aOldValue,
+                                const LSValue& aNewValue) {
+  AssertIsOnBackgroundThread();
+  MOZ_ASSERT(aDatabase);
+
+  if (!gObservers) {
+    return;
+  }
+
+  nsTArray<Observer*>* array;
+  if (!gObservers->Get(mOrigin, &array)) {
+    return;
+  }
+
+  MOZ_ASSERT(array);
+
+  // We do not want to send information about events back to the content process
+  // that caused the change.
+  PBackgroundParent* databaseBackgroundActor = aDatabase->Manager();
+
+  for (Observer* observer : *array) {
+    if (observer->Manager() != databaseBackgroundActor) {
+      observer->Observe(aDatabase, aDocumentURI, aKey, aOldValue, aNewValue);
+    }
+  }
+}
+
 bool Datastore::UpdateUsage(int64_t aDelta) {
  AssertIsOnBackgroundThread();

@ -5442,35 +5460,6 @@ void Datastore::MarkSnapshotsDirty() {
  }
 }

-void Datastore::NotifyObservers(Database* aDatabase,
-                                const nsString& aDocumentURI,
-                                const nsString& aKey, const LSValue& aOldValue,
-                                const LSValue& aNewValue) {
-  AssertIsOnBackgroundThread();
-  MOZ_ASSERT(aDatabase);
-
-  if (!gObservers) {
-    return;
-  }
-
-  nsTArray<Observer*>* array;
-  if (!gObservers->Get(mOrigin, &array)) {
-    return;
-  }
-
-  MOZ_ASSERT(array);
-
-  // We do not want to send information about events back to the content process
-  // that caused the change.
-  PBackgroundParent* databaseBackgroundActor = aDatabase->Manager();
-
-  for (Observer* observer : *array) {
-    if (observer->Manager() != databaseBackgroundActor) {
-      observer->Observe(aDatabase, aDocumentURI, aKey, aOldValue, aNewValue);
-    }
-  }
-}
-
 /*******************************************************************************
 * PreparedDatastore
 ******************************************************************************/
@ -5826,42 +5815,53 @@ mozilla::ipc::IPCResult Snapshot::RecvDeleteMe() {
  return IPC_OK();
 }

-mozilla::ipc::IPCResult Snapshot::RecvCheckpoint(
-    nsTArray<LSWriteInfo>&& aWriteInfos) {
+mozilla::ipc::IPCResult Snapshot::RecvCheckpointAndNotify(
+    nsTArray<LSWriteAndNotifyInfo>&& aWriteAndNotifyInfos) {
  AssertIsOnBackgroundThread();
  MOZ_ASSERT(mUsage >= 0);
  MOZ_ASSERT(mPeakUsage >= mUsage);

-  if (NS_WARN_IF(aWriteInfos.IsEmpty())) {
+  if (NS_WARN_IF(aWriteAndNotifyInfos.IsEmpty())) {
    ASSERT_UNLESS_FUZZING();
    return IPC_FAIL_NO_REASON(this);
  }

  mDatastore->BeginUpdateBatch(mUsage);

-  for (uint32_t index = 0; index < aWriteInfos.Length(); index++) {
-    const LSWriteInfo& writeInfo = aWriteInfos[index];
-    switch (writeInfo.type()) {
-      case LSWriteInfo::TLSSetItemInfo: {
-        const LSSetItemInfo& info = writeInfo.get_LSSetItemInfo();
+  for (uint32_t index = 0; index < aWriteAndNotifyInfos.Length(); index++) {
+    const LSWriteAndNotifyInfo& writeAndNotifyInfo =
+        aWriteAndNotifyInfos[index];

-        mDatastore->SetItem(mDatabase, mDocumentURI, info.key(),
-                            info.oldValue(), info.value());
+    switch (writeAndNotifyInfo.type()) {
+      case LSWriteAndNotifyInfo::TLSSetItemAndNotifyInfo: {
+        const LSSetItemAndNotifyInfo& info =
+            writeAndNotifyInfo.get_LSSetItemAndNotifyInfo();
+
+        mDatastore->SetItem(mDatabase, info.key(), info.value());
+
+        mDatastore->NotifyObservers(mDatabase, mDocumentURI, info.key(),
+                                    info.oldValue(), info.value());

        break;
      }

-      case LSWriteInfo::TLSRemoveItemInfo: {
-        const LSRemoveItemInfo& info = writeInfo.get_LSRemoveItemInfo();
+      case LSWriteAndNotifyInfo::TLSRemoveItemAndNotifyInfo: {
+        const LSRemoveItemAndNotifyInfo& info =
+            writeAndNotifyInfo.get_LSRemoveItemAndNotifyInfo();

-        mDatastore->RemoveItem(mDatabase, mDocumentURI, info.key(),
-                               info.oldValue());
+        mDatastore->RemoveItem(mDatabase, info.key());
+
+        mDatastore->NotifyObservers(mDatabase, mDocumentURI, info.key(),
+                                    info.oldValue(), VoidLSValue());

        break;
      }

-      case LSWriteInfo::TLSClearInfo: {
-        mDatastore->Clear(mDatabase, mDocumentURI);
+      case LSWriteAndNotifyInfo::TLSClearInfo: {
+        mDatastore->Clear(mDatabase);
+
+        mDatastore->NotifyObservers(mDatabase, mDocumentURI, VoidString(),
+                                    VoidLSValue(), VoidLSValue());

        break;
      }
--- a/dom/localstorage/LSSnapshot.cpp
+++ b/dom/localstorage/LSSnapshot.cpp
@ -241,12 +241,12 @@ nsresult LSSnapshot::SetItem(const nsAString& aKey, const nsAString& aValue,
      mLength++;
    }

-    LSSetItemInfo setItemInfo;
-    setItemInfo.key() = aKey;
-    setItemInfo.oldValue() = LSValue(oldValue);
-    setItemInfo.value() = LSValue(aValue);
+    LSSetItemAndNotifyInfo setItemAndNotifyInfo;
+    setItemAndNotifyInfo.key() = aKey;
+    setItemAndNotifyInfo.oldValue() = LSValue(oldValue);
+    setItemAndNotifyInfo.value() = LSValue(aValue);

-    mWriteInfos.AppendElement(std::move(setItemInfo));
+    mWriteAndNotifyInfos.AppendElement(std::move(setItemAndNotifyInfo));
  }

  aNotifyInfo.changed() = changed;
@ -287,11 +287,11 @@ nsresult LSSnapshot::RemoveItem(const nsAString& aKey,
      mLength--;
    }

-    LSRemoveItemInfo removeItemInfo;
-    removeItemInfo.key() = aKey;
-    removeItemInfo.oldValue() = LSValue(oldValue);
+    LSRemoveItemAndNotifyInfo removeItemAndNotifyInfo;
+    removeItemAndNotifyInfo.key() = aKey;
+    removeItemAndNotifyInfo.oldValue() = LSValue(oldValue);

-    mWriteInfos.AppendElement(std::move(removeItemInfo));
+    mWriteAndNotifyInfos.AppendElement(std::move(removeItemAndNotifyInfo));
  }

  aNotifyInfo.changed() = changed;
@ -336,7 +336,7 @@ nsresult LSSnapshot::Clear(LSNotifyInfo& aNotifyInfo) {

    LSClearInfo clearInfo;

-    mWriteInfos.AppendElement(std::move(clearInfo));
+    mWriteAndNotifyInfos.AppendElement(std::move(clearInfo));
  }

  aNotifyInfo.changed() = changed;
@ -593,19 +593,22 @@ nsresult LSSnapshot::EnsureAllKeys() {
    newValues.Put(key, VoidString());
  }

-  for (uint32_t index = 0; index < mWriteInfos.Length(); index++) {
-    const LSWriteInfo& writeInfo = mWriteInfos[index];
+  for (uint32_t index = 0; index < mWriteAndNotifyInfos.Length(); index++) {
+    const LSWriteAndNotifyInfo& writeAndNotifyInfo =
+        mWriteAndNotifyInfos[index];

-    switch (writeInfo.type()) {
-      case LSWriteInfo::TLSSetItemInfo: {
-        newValues.Put(writeInfo.get_LSSetItemInfo().key(), VoidString());
+    switch (writeAndNotifyInfo.type()) {
+      case LSWriteAndNotifyInfo::TLSSetItemAndNotifyInfo: {
+        newValues.Put(writeAndNotifyInfo.get_LSSetItemAndNotifyInfo().key(),
+                      VoidString());
        break;
      }
-      case LSWriteInfo::TLSRemoveItemInfo: {
-        newValues.Remove(writeInfo.get_LSRemoveItemInfo().key());
+      case LSWriteAndNotifyInfo::TLSRemoveItemAndNotifyInfo: {
+        newValues.Remove(
+            writeAndNotifyInfo.get_LSRemoveItemAndNotifyInfo().key());
        break;
      }
-      case LSWriteInfo::TLSClearInfo: {
+      case LSWriteAndNotifyInfo::TLSClearInfo: {
        newValues.Clear();
        break;
      }
@ -679,10 +682,10 @@ nsresult LSSnapshot::Checkpoint() {
  MOZ_ASSERT(mInitialized);
  MOZ_ASSERT(!mSentFinish);

-  if (!mWriteInfos.IsEmpty()) {
-    MOZ_ALWAYS_TRUE(mActor->SendCheckpoint(mWriteInfos));
+  if (!mWriteAndNotifyInfos.IsEmpty()) {
+    MOZ_ALWAYS_TRUE(mActor->SendCheckpointAndNotify(mWriteAndNotifyInfos));

-    mWriteInfos.Clear();
+    mWriteAndNotifyInfos.Clear();
  }

  return NS_OK;
--- a/dom/localstorage/LSSnapshot.h
+++ b/dom/localstorage/LSSnapshot.h
@ -16,7 +16,7 @@ class LSDatabase;
 class LSNotifyInfo;
 class LSSnapshotChild;
 class LSSnapshotInitInfo;
-class LSWriteInfo;
+class LSWriteAndNotifyInfo;

 class LSSnapshot final : public nsIRunnable {
 public:
@ -79,7 +79,7 @@ class LSSnapshot final : public nsIRunnable {
  nsTHashtable<nsStringHashKey> mLoadedItems;
  nsTHashtable<nsStringHashKey> mUnknownItems;
  nsDataHashtable<nsStringHashKey, nsString> mValues;
-  nsTArray<LSWriteInfo> mWriteInfos;
+  nsTArray<LSWriteAndNotifyInfo> mWriteAndNotifyInfos;

  uint32_t mInitLength;
  uint32_t mLength;
--- a/dom/localstorage/PBackgroundLSSnapshot.ipdl
+++ b/dom/localstorage/PBackgroundLSSnapshot.ipdl
@ -15,30 +15,30 @@ using mozilla::dom::LSValue
 namespace mozilla {
 namespace dom {

-struct LSSetItemInfo
+struct LSClearInfo
+{
+};
+
+struct LSSetItemAndNotifyInfo
 {
  nsString key;
  LSValue oldValue;
  LSValue value;
 };

-struct LSRemoveItemInfo
+struct LSRemoveItemAndNotifyInfo
 {
  nsString key;
  LSValue oldValue;
 };

-struct LSClearInfo
-{
-};
-
 /**
 * Union of LocalStorage mutation types.
 */
-union LSWriteInfo
+union LSWriteAndNotifyInfo
 {
-  LSSetItemInfo;
-  LSRemoveItemInfo;
+  LSSetItemAndNotifyInfo;
+  LSRemoveItemAndNotifyInfo;
  LSClearInfo;
 };

@ -49,7 +49,7 @@ sync protocol PBackgroundLSSnapshot
 parent:
  async DeleteMe();

-  async Checkpoint(LSWriteInfo[] writeInfos);
+  async CheckpointAndNotify(LSWriteAndNotifyInfo[] writeAndNotifyInfos);

  async Finish();