Bug 1552428 - LSNG: Datastore::PrivateBrowsingClear uses wrong delta for usage updating; r=asuth

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

dom/localstorage/ActorsParent.cpp

@@ -1678,9 +1678,7 @@ class Datastore final
   int64_t mSizeOfKeys;
   int64_t mSizeOfItems;
   bool mClosed;
-#ifdef DEBUG
   bool mInUpdateBatch;
-#endif
 
  public:
   // Created by PrepareDatastoreOp.
@@ -1759,8 +1757,6 @@ class Datastore final
 
   void Clear(Database* aDatabase);
 
-  void PrivateBrowsingClear();
-
   void BeginUpdateBatch(int64_t aSnapshotInitialUsage);
 
   int64_t EndUpdateBatch(int64_t aSnapshotPeakUsage);
@@ -1795,8 +1791,6 @@ class Datastore final
 
   void NotifySnapshots(Database* aDatabase, const nsAString& aKey,
                        const LSValue& aOldValue, bool aAffectsOrder);
-
-  void MarkSnapshotsDirty();
 };
 
 class PreparedDatastore {
@@ -3658,7 +3652,7 @@ bool RecvLSClearPrivateBrowsing() {
       MOZ_ASSERT(datastore);
 
       if (datastore->PrivateBrowsingId()) {
-        datastore->PrivateBrowsingClear();
+        datastore->Clear(nullptr);
       }
     }
   }
@@ -4798,12 +4792,8 @@ Datastore::Datastore(const nsACString& aOrigin, uint32_t aPrivateBrowsingId,
       mUpdateBatchUsage(-1),
       mSizeOfKeys(aSizeOfKeys),
       mSizeOfItems(aSizeOfItems),
-      mClosed(false)
-#ifdef DEBUG
-      ,
-      mInUpdateBatch(false)
-#endif
-{
+      mClosed(false),
+      mInUpdateBatch(false) {
   AssertIsOnBackgroundThread();
 
   mValues.SwapElements(aValues);
@@ -5264,9 +5254,7 @@ void Datastore::RemoveItem(Database* aDatabase, const nsString& aKey) {
 
 void Datastore::Clear(Database* aDatabase) {
   AssertIsOnBackgroundThread();
-  MOZ_ASSERT(aDatabase);
   MOZ_ASSERT(!mClosed);
-  MOZ_ASSERT(mInUpdateBatch);
 
   if (mValues.Count()) {
     int64_t delta = 0;
@@ -5282,9 +5270,16 @@ void Datastore::Clear(Database* aDatabase) {
 
     mValues.Clear();
 
-    mWriteOptimizer.Truncate();
+    if (mInUpdateBatch) {
+      mWriteOptimizer.Truncate();
 
-    mUpdateBatchUsage += delta;
+      mUpdateBatchUsage += delta;
+    } else {
+      mOrderedItems.Clear();
+
+      DebugOnly<bool> ok = UpdateUsage(delta);
+      MOZ_ASSERT(ok);
+    }
 
     mSizeOfKeys = 0;
     mSizeOfItems = 0;
@@ -5295,27 +5290,6 @@ void Datastore::Clear(Database* aDatabase) {
   }
 }
 
-void Datastore::PrivateBrowsingClear() {
-  AssertIsOnBackgroundThread();
-  MOZ_ASSERT(mPrivateBrowsingId);
-  MOZ_ASSERT(!mClosed);
-  MOZ_ASSERT(!mInUpdateBatch);
-
-  if (mValues.Count()) {
-    MarkSnapshotsDirty();
-
-    mValues.Clear();
-
-    mOrderedItems.Clear();
-
-    DebugOnly<bool> ok = UpdateUsage(-mSizeOfItems);
-    MOZ_ASSERT(ok);
-
-    mSizeOfKeys = 0;
-    mSizeOfItems = 0;
-  }
-}
-
 void Datastore::BeginUpdateBatch(int64_t aSnapshotInitialUsage) {
   AssertIsOnBackgroundThread();
   MOZ_ASSERT(aSnapshotInitialUsage >= 0);
@@ -5329,9 +5303,7 @@ void Datastore::BeginUpdateBatch(int64_t aSnapshotInitialUsage) {
     mConnection->BeginUpdateBatch();
   }
 
-#ifdef DEBUG
   mInUpdateBatch = true;
-#endif
 }
 
 int64_t Datastore::EndUpdateBatch(int64_t aSnapshotPeakUsage) {
@@ -5369,9 +5341,7 @@ int64_t Datastore::EndUpdateBatch(int64_t aSnapshotPeakUsage) {
     mConnection->EndUpdateBatch();
   }
 
-#ifdef DEBUG
   mInUpdateBatch = false;
-#endif
 
   return result;
 }
@@ -5554,10 +5524,12 @@ void Datastore::CleanupMetadata() {
 void Datastore::NotifySnapshots(Database* aDatabase, const nsAString& aKey,
                                 const LSValue& aOldValue, bool aAffectsOrder) {
   AssertIsOnBackgroundThread();
-  MOZ_ASSERT(aDatabase);
 
   for (auto iter = mDatabases.ConstIter(); !iter.Done(); iter.Next()) {
     Database* database = iter.Get()->GetKey();
+
+    MOZ_ASSERT(database);
+
     if (database == aDatabase) {
       continue;
     }
@@ -5569,19 +5541,6 @@ void Datastore::NotifySnapshots(Database* aDatabase, const nsAString& aKey,
   }
 }
 
-void Datastore::MarkSnapshotsDirty() {
-  AssertIsOnBackgroundThread();
-
-  for (auto iter = mDatabases.ConstIter(); !iter.Done(); iter.Next()) {
-    Database* database = iter.Get()->GetKey();
-
-    Snapshot* snapshot = database->GetSnapshot();
-    if (snapshot) {
-      snapshot->MarkDirty();
-    }
-  }
-}
-
 /*******************************************************************************
  * PreparedDatastore
  ******************************************************************************/

dom/localstorage/moz.build

@@ -7,6 +7,8 @@
 with Files("**"):
     BUG_COMPONENT = ("Core", "DOM: Web Storage")
 
+BROWSER_CHROME_MANIFESTS += ['test/browser.ini']
+
 XPCSHELL_TESTS_MANIFESTS += [
     'test/unit/xpcshell.ini'
 ]

dom/localstorage/test/browser.ini

@@ -0,0 +1,6 @@
+[DEFAULT]
+skip-if = (buildapp != "browser")
+support-files =
+  page_private_ls.html
+
+[browser_private_ls.js]

dom/localstorage/test/browser_private_ls.js

@@ -0,0 +1,39 @@
+/**
+ * This test is mainly to verify that datastores are cleared when the last
+ * private browsing context exited.
+ */
+
+async function lsCheckFunc() {
+  let storage = content.localStorage;
+
+  if (storage.length) {
+    return false;
+  }
+
+  // Store non-ASCII value to verify bug 1552428.
+  storage.setItem("foo", "úžasné");
+
+  return true;
+}
+
+function checkTabWindowLS(tab) {
+  return ContentTask.spawn(tab.linkedBrowser, null, lsCheckFunc);
+}
+
+add_task(async function() {
+  const pageUrl =
+    "http://example.com/browser/dom/localstorage/test/page_private_ls.html";
+
+  for (let i = 0; i < 2; i++) {
+    let privateWin =
+      await BrowserTestUtils.openNewBrowserWindow({private: true});
+
+    let privateTab =
+      await BrowserTestUtils.openNewForegroundTab(privateWin.gBrowser, pageUrl);
+
+    ok(await checkTabWindowLS(privateTab),
+       "LS works correctly in a private-browsing page.");
+
+    await BrowserTestUtils.closeWindow(privateWin);
+  }
+});

dom/localstorage/test/page_private_ls.html

@@ -0,0 +1,9 @@
+<!doctype html>
+<html>
+<head>
+  <meta charset="utf-8">
+</head>
+<body>
+  All the interesting stuff happens in ContentTask.spawn() calls.
+</body>
+</html>