From 05c83e62763c49dc415e7bff4c908954f7368b1b Mon Sep 17 00:00:00 2001 From: Jan Varga Date: Thu, 17 Feb 2022 15:09:58 +0000 Subject: [PATCH] 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 | 12 ++++++++++++ dom/localstorage/LSDatabase.h | 8 ++------ dom/localstorage/LSObject.cpp | 17 +++++++++-------- dom/localstorage/LSObject.h | 4 ++-- dom/localstorage/test/unit/test_largeItems.js | 2 +- dom/storage/Storage.h | 4 ++-- dom/webidl/Storage.webidl | 7 ++++--- 7 files changed, 32 insertions(+), 22 deletions(-) diff --git a/dom/localstorage/LSDatabase.cpp b/dom/localstorage/LSDatabase.cpp index cc8b5252b018..46b3b96bfb5d 100644 --- a/dom/localstorage/LSDatabase.cpp +++ b/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); diff --git a/dom/localstorage/LSDatabase.h b/dom/localstorage/LSDatabase.h index b22ffdab8feb..25cd1c610049 100644 --- a/dom/localstorage/LSDatabase.h +++ b/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(); diff --git a/dom/localstorage/LSObject.cpp b/dom/localstorage/LSObject.cpp index 54267d9660f6..8b78b4e623e2 100644 --- a/dom/localstorage/LSObject.cpp +++ b/dom/localstorage/LSObject.cpp @@ -860,22 +860,23 @@ void LSObject::EndExplicitSnapshot(nsIPrincipal& aSubjectPrincipal, mInExplicitSnapshot = false; } -bool LSObject::GetHasActiveSnapshot(nsIPrincipal& aSubjectPrincipal, - ErrorResult& aError) { +bool LSObject::GetHasSnapshot(nsIPrincipal& aSubjectPrincipal, + ErrorResult& aError) { AssertIsOnOwningThread(); if (!CanUseStorage(aSubjectPrincipal)) { aError.Throw(NS_ERROR_DOM_SECURITY_ERR); - return 0; + return false; } - if (mDatabase && mDatabase->HasActiveSnapshot()) { - MOZ_ASSERT(!mDatabase->IsAllowedToClose()); - - return true; + // 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 false; + return mDatabase->HasSnapshot(); } NS_IMPL_ADDREF_INHERITED(LSObject, Storage) diff --git a/dom/localstorage/LSObject.h b/dom/localstorage/LSObject.h index cbe9a536cac3..726b180a1f82 100644 --- a/dom/localstorage/LSObject.h +++ b/dom/localstorage/LSObject.h @@ -182,8 +182,8 @@ class LSObject final : public Storage { void EndExplicitSnapshot(nsIPrincipal& aSubjectPrincipal, ErrorResult& aError) override; - bool GetHasActiveSnapshot(nsIPrincipal& aSubjectPrincipal, - ErrorResult& aError) override; + bool GetHasSnapshot(nsIPrincipal& aSubjectPrincipal, + ErrorResult& aError) override; ////////////////////////////////////////////////////////////////////////////// diff --git a/dom/localstorage/test/unit/test_largeItems.js b/dom/localstorage/test/unit/test_largeItems.js index f274724a88b7..c60f6ba3dba6 100644 --- a/dom/localstorage/test/unit/test_largeItems.js +++ b/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"); } diff --git a/dom/storage/Storage.h b/dom/storage/Storage.h index 533b870f7cfd..0648e70fb929 100644 --- a/dom/storage/Storage.h +++ b/dom/storage/Storage.h @@ -121,8 +121,8 @@ class Storage : public nsISupports, public nsWrapperCache { virtual void EndExplicitSnapshot(nsIPrincipal& aSubjectPrincipal, ErrorResult& aRv) {} - virtual bool GetHasActiveSnapshot(nsIPrincipal& aSubjectPrincipal, - ErrorResult& aRv) { + virtual bool GetHasSnapshot(nsIPrincipal& aSubjectPrincipal, + ErrorResult& aRv) { return false; } diff --git a/dom/webidl/Storage.webidl b/dom/webidl/Storage.webidl index 56e76ff38af8..fd587021098f 100644 --- a/dom/webidl/Storage.webidl +++ b/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 - * active snapshot (initialized implicitly or explicitly). + * Returns true if the underlying database has been opened, the database is + * 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; };