Bug 1754448 - Clean up the code for Storage::hasActiveSnapshot; r=dom-storage-reviewers,jari,webidl,smaug

Differential Revision: https://phabricator.services.mozilla.com/D138635

dom/localstorage/LSDatabase.cpp

@@ -143,6 +143,10 @@ void LSDatabase::NoteFinishedSnapshot(LSSnapshot* aSnapshot) {
   }
 }
 
+// All these methods assert `!mAllowedToClose` because they shoudn't be called
+// if the database is being closed. Callers should first check the state by
+// calling `IsAlloweToClose` and eventually obtain a new database.
+
 nsresult LSDatabase::GetLength(LSObject* aObject, uint32_t* aResult) {
   AssertIsOnOwningThread();
   MOZ_ASSERT(aObject);
@@ -311,6 +315,14 @@ nsresult LSDatabase::EndExplicitSnapshot() {
   return NS_OK;
 }
 
+bool LSDatabase::HasSnapshot() const {
+  AssertIsOnOwningThread();
+  MOZ_ASSERT(mActor);
+  MOZ_ASSERT(!mAllowedToClose);
+
+  return !!mSnapshot;
+}
+
 nsresult LSDatabase::EnsureSnapshot(LSObject* aObject, const nsAString& aKey,
                                     bool aExplicit) {
   MOZ_ASSERT(aObject);

dom/localstorage/LSDatabase.h

@@ -55,12 +55,6 @@ class LSDatabase final {
     mActor = nullptr;
   }
 
-  bool HasActiveSnapshot() const {
-    AssertIsOnOwningThread();
-
-    return !!mSnapshot;
-  }
-
   bool IsAllowedToClose() const {
     AssertIsOnOwningThread();
 
@@ -92,6 +86,8 @@ class LSDatabase final {
 
   nsresult EndExplicitSnapshot();
 
+  bool HasSnapshot() const;
+
  private:
   ~LSDatabase();
 

dom/localstorage/LSObject.cpp

@@ -860,24 +860,25 @@ void LSObject::EndExplicitSnapshot(nsIPrincipal& aSubjectPrincipal,
   mInExplicitSnapshot = false;
 }
 
-bool LSObject::GetHasActiveSnapshot(nsIPrincipal& aSubjectPrincipal,
+bool LSObject::GetHasSnapshot(nsIPrincipal& aSubjectPrincipal,
                               ErrorResult& aError) {
   AssertIsOnOwningThread();
 
   if (!CanUseStorage(aSubjectPrincipal)) {
     aError.Throw(NS_ERROR_DOM_SECURITY_ERR);
-    return 0;
-  }
-
-  if (mDatabase && mDatabase->HasActiveSnapshot()) {
-    MOZ_ASSERT(!mDatabase->IsAllowedToClose());
-
-    return true;
-  }
-
     return false;
   }
 
+  // We can't call `HasSnapshot` on the database if it's being closed, but we
+  // know that a database which is being closed can't have a snapshot, so we
+  // return false in that case directly here.
+  if (!mDatabase || mDatabase->IsAllowedToClose()) {
+    return false;
+  }
+
+  return mDatabase->HasSnapshot();
+}
+
 NS_IMPL_ADDREF_INHERITED(LSObject, Storage)
 NS_IMPL_RELEASE_INHERITED(LSObject, Storage)
 

dom/localstorage/LSObject.h

@@ -182,7 +182,7 @@ class LSObject final : public Storage {
   void EndExplicitSnapshot(nsIPrincipal& aSubjectPrincipal,
                            ErrorResult& aError) override;
 
-  bool GetHasActiveSnapshot(nsIPrincipal& aSubjectPrincipal,
+  bool GetHasSnapshot(nsIPrincipal& aSubjectPrincipal,
                       ErrorResult& aError) override;
 
   //////////////////////////////////////////////////////////////////////////////

dom/localstorage/test/unit/test_largeItems.js

@@ -84,5 +84,5 @@ async function testSteps() {
 
   await returnToEventLoop();
 
-  ok(!storage.hasActiveSnapshot, "Snapshot successfully finished");
+  ok(!storage.hasSnapshot, "Snapshot successfully finished");
 }

dom/storage/Storage.h

@@ -121,7 +121,7 @@ class Storage : public nsISupports, public nsWrapperCache {
   virtual void EndExplicitSnapshot(nsIPrincipal& aSubjectPrincipal,
                                    ErrorResult& aRv) {}
 
-  virtual bool GetHasActiveSnapshot(nsIPrincipal& aSubjectPrincipal,
+  virtual bool GetHasSnapshot(nsIPrincipal& aSubjectPrincipal,
                               ErrorResult& aRv) {
     return false;
   }

dom/webidl/Storage.webidl

@@ -73,9 +73,10 @@ partial interface Storage {
   void endExplicitSnapshot();
 
   /**
-   * Returns true if the underlying database has been opened and it has an
+   * Returns true if the underlying database has been opened, the database is
-   * active snapshot (initialized implicitly or explicitly).
+   * not being closed and it has a snapshot (initialized implicitly or
+   * explicitly).
    */
   [Throws, NeedsSubjectPrincipal, Pref="dom.storage.testing"]
-  readonly attribute boolean hasActiveSnapshot;
+  readonly attribute boolean hasSnapshot;
 };