Bug 1546723 - Part 2: Make it more clear that checkpointing also notifies observers; r=asuth draft
authorJan Varga <jan.varga@gmail.com>
Wed, 15 May 2019 06:11:10 +0200
changeset 2008604 76848afcc69e13a36965c6a60eccc66e4fc13e1f
parent 2008603 d4e0a949f8c6481169ee72d241778e4903b1d1f9
child 2008605 12ef1daf509831e5d4a0e372a09cbdb1840780d8
push id363926
push userjvarga@mozilla.com
push dateSat, 18 May 2019 08:19:45 +0000
treeherdertry@e89baae86dd1 [default view] [failures only]
reviewersasuth
bugs1546723
milestone68.0a1
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
dom/localstorage/ActorsParent.cpp
dom/localstorage/LSSnapshot.cpp
dom/localstorage/LSSnapshot.h
dom/localstorage/PBackgroundLSSnapshot.ipdl
--- a/dom/localstorage/ActorsParent.cpp
+++ b/dom/localstorage/ActorsParent.cpp
@@ -1745,33 +1745,35 @@ class Datastore final
   // Mutation Methods
   //
   // These are only called during Snapshot::RecvCheckpoint
 
   /**
    * 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 Clear(Database* aDatabase, const nsString& aDocumentURI);
+  void RemoveItem(Database* aDatabase, const nsString& aKey);
+
+  void Clear(Database* aDatabase);
 
   void PrivateBrowsingClear();
 
   void BeginUpdateBatch(int64_t aSnapshotInitialUsage);
 
   int64_t EndUpdateBatch(int64_t aSnapshotPeakUsage);
 
   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:
   // Reference counted.
   ~Datastore();
 
   bool UpdateUsage(int64_t aDelta);
 
@@ -1780,20 +1782,16 @@ class Datastore final
   void ConnectionClosedCallback();
 
   void CleanupMetadata();
 
   void NotifySnapshots(Database* aDatabase, const nsAString& aKey,
                        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 {
   RefPtr<Datastore> mDatastore;
   nsCOMPtr<nsITimer> mTimer;
   const Maybe<ContentParentId> mContentParentId;
   // Strings share buffers if possible, so it's not a problem to duplicate the
   // origin here.
@@ -2118,18 +2116,18 @@ class Snapshot final : public PBackgroun
 
   void Finish();
 
   // IPDL methods are only called by IPDL.
   void ActorDestroy(ActorDestroyReason aWhy) override;
 
   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;
 
   mozilla::ipc::IPCResult RecvLoaded() override;
 
   mozilla::ipc::IPCResult RecvLoadValueAndMoreItems(
       const nsString& aKey, LSValue* aValue,
       nsTArray<LSItemInfo>* aItemInfos) override;
@@ -5114,18 +5112,17 @@ void Datastore::GetKeys(nsTArray<nsStrin
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(!mClosed);
 
   for (auto item : mOrderedItems) {
     aKeys.AppendElement(item.key());
   }
 }
 
-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);
   MOZ_ASSERT(!mClosed);
   MOZ_ASSERT(mInUpdateBatch);
 
   LSValue oldValue;
   GetItem(aKey, oldValue);
@@ -5161,22 +5158,19 @@ void Datastore::SetItem(Database* aDatab
       mSizeOfItems += static_cast<int64_t>(aValue.Length()) -
                       static_cast<int64_t>(oldValue.Length());
     }
 
     if (IsPersistent()) {
       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);
   MOZ_ASSERT(mInUpdateBatch);
 
   LSValue oldValue;
   GetItem(aKey, oldValue);
 
@@ -5195,21 +5189,19 @@ void Datastore::RemoveItem(Database* aDa
 
     mSizeOfKeys -= sizeOfKey;
     mSizeOfItems -= sizeOfKey + static_cast<int64_t>(oldValue.Length());
 
     if (IsPersistent()) {
       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);
   MOZ_ASSERT(mInUpdateBatch);
 
   if (mValues.Count()) {
     int64_t delta = 0;
     for (auto iter = mValues.ConstIter(); !iter.Done(); iter.Next()) {
@@ -5230,19 +5222,16 @@ void Datastore::Clear(Database* aDatabas
 
     mSizeOfKeys = 0;
     mSizeOfItems = 0;
 
     if (IsPersistent()) {
       mConnection->Clear(delta);
     }
   }
-
-  NotifyObservers(aDatabase, aDocumentURI, VoidString(), VoidLSValue(),
-                  VoidLSValue());
 }
 
 void Datastore::PrivateBrowsingClear() {
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(mPrivateBrowsingId);
   MOZ_ASSERT(!mClosed);
   MOZ_ASSERT(!mInUpdateBatch);
 
@@ -5333,16 +5322,45 @@ int64_t Datastore::RequestUpdateUsage(in
 
   if (UpdateUsage(aMinSize)) {
     return aMinSize;
   }
 
   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();
 
   // Check internal LocalStorage origin limit.
   int64_t newUsage = mUsage + aDelta;
 
   MOZ_ASSERT(newUsage >= 0);
 
@@ -5435,45 +5453,16 @@ void Datastore::MarkSnapshotsDirty() {
 
     Snapshot* snapshot = database->GetSnapshot();
     if (snapshot) {
       snapshot->MarkDirty();
     }
   }
 }
 
-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
  ******************************************************************************/
 
 void PreparedDatastore::Destroy() {
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(gPreparedDatastores);
   MOZ_ASSERT(gPreparedDatastores->Get(mDatastoreId));
@@ -5819,52 +5808,63 @@ mozilla::ipc::IPCResult Snapshot::RecvDe
 
   IProtocol* mgr = Manager();
   if (!PBackgroundLSSnapshotParent::Send__delete__(this)) {
     return IPC_FAIL_NO_REASON(mgr);
   }
   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_DIAGNOSTIC_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();
-
-        mDatastore->SetItem(mDatabase, mDocumentURI, info.key(),
-                            info.oldValue(), info.value());
+  for (uint32_t index = 0; index < aWriteAndNotifyInfos.Length(); index++) {
+    const LSWriteAndNotifyInfo& writeAndNotifyInfo =
+        aWriteAndNotifyInfos[index];
+
+    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();
-
-        mDatastore->RemoveItem(mDatabase, mDocumentURI, info.key(),
-                               info.oldValue());
+      case LSWriteAndNotifyInfo::TLSRemoveItemAndNotifyInfo: {
+        const LSRemoveItemAndNotifyInfo& info =
+            writeAndNotifyInfo.get_LSRemoveItemAndNotifyInfo();
+
+        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;
       }
 
       default:
         MOZ_CRASH("Should never get here!");
     }
   }
--- a/dom/localstorage/LSSnapshot.cpp
+++ b/dom/localstorage/LSSnapshot.cpp
@@ -236,22 +236,22 @@ nsresult LSSnapshot::SetItem(const nsASt
       }
       return rv;
     }
 
     if (oldValue.IsVoid() && mLoadState == LoadState::Partial) {
       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;
   aNotifyInfo.oldValue() = oldValue;
 
   return NS_OK;
 }
 
@@ -282,21 +282,21 @@ nsresult LSSnapshot::RemoveItem(const ns
 
     DebugOnly<nsresult> rv = UpdateUsage(delta);
     MOZ_ASSERT(NS_SUCCEEDED(rv));
 
     if (mLoadState == LoadState::Partial) {
       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;
   aNotifyInfo.oldValue() = oldValue;
 
   return NS_OK;
 }
 
@@ -331,17 +331,17 @@ nsresult LSSnapshot::Clear(LSNotifyInfo&
 
     DebugOnly<nsresult> rv = UpdateUsage(-mExactUsage);
     MOZ_ASSERT(NS_SUCCEEDED(rv));
 
     mValues.Clear();
 
     LSClearInfo clearInfo;
 
-    mWriteInfos.AppendElement(std::move(clearInfo));
+    mWriteAndNotifyInfos.AppendElement(std::move(clearInfo));
   }
 
   aNotifyInfo.changed() = changed;
 
   return NS_OK;
 }
 
 void LSSnapshot::MarkDirty() {
@@ -588,29 +588,32 @@ nsresult LSSnapshot::EnsureAllKeys() {
   }
 
   nsDataHashtable<nsStringHashKey, nsString> newValues;
 
   for (auto key : keys) {
     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;
       }
 
       default:
         MOZ_CRASH("Should never get here!");
     }
   }
@@ -674,20 +677,20 @@ nsresult LSSnapshot::UpdateUsage(int64_t
 }
 
 nsresult LSSnapshot::Checkpoint() {
   AssertIsOnOwningThread();
   MOZ_ASSERT(mActor);
   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;
 }
 
 nsresult LSSnapshot::Finish() {
   AssertIsOnOwningThread();
   MOZ_ASSERT(mDatabase);
--- a/dom/localstorage/LSSnapshot.h
+++ b/dom/localstorage/LSSnapshot.h
@@ -11,17 +11,17 @@
 
 namespace mozilla {
 namespace dom {
 
 class LSDatabase;
 class LSNotifyInfo;
 class LSSnapshotChild;
 class LSSnapshotInitInfo;
-class LSWriteInfo;
+class LSWriteAndNotifyInfo;
 
 class LSSnapshot final : public nsIRunnable {
  public:
   /**
    * The LoadState expresses what subset of information a snapshot has from the
    * authoritative Datastore in the parent process.  The initial snapshot is
    * populated heuristically based on the size of the keys and size of the items
    * (inclusive of the key value; item is key+value, not just value) of the
@@ -74,17 +74,17 @@ class LSSnapshot final : public nsIRunna
 
   nsCOMPtr<nsITimer> mTimer;
 
   LSSnapshotChild* mActor;
 
   nsTHashtable<nsStringHashKey> mLoadedItems;
   nsTHashtable<nsStringHashKey> mUnknownItems;
   nsDataHashtable<nsStringHashKey, nsString> mValues;
-  nsTArray<LSWriteInfo> mWriteInfos;
+  nsTArray<LSWriteAndNotifyInfo> mWriteAndNotifyInfos;
 
   uint32_t mInitLength;
   uint32_t mLength;
   int64_t mExactUsage;
   int64_t mPeakUsage;
 
   LoadState mLoadState;
 
--- a/dom/localstorage/PBackgroundLSSnapshot.ipdl
+++ b/dom/localstorage/PBackgroundLSSnapshot.ipdl
@@ -10,51 +10,51 @@ include PBackgroundLSSharedTypes;
 include "mozilla/dom/localstorage/SerializationHelpers.h";
 
 using mozilla::dom::LSValue
   from "mozilla/dom/LSValue.h";
 
 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;
 };
 
 sync protocol PBackgroundLSSnapshot
 {
   manager PBackgroundLSDatabase;
 
 parent:
   async DeleteMe();
 
-  async Checkpoint(LSWriteInfo[] writeInfos);
+  async CheckpointAndNotify(LSWriteAndNotifyInfo[] writeAndNotifyInfos);
 
   async Finish();
 
   async Loaded();
 
   /**
    * Invoked on demand to load an item that didn't fit into the initial
    * snapshot prefill and also some additional key/value pairs to lower down