Backed out changeset c7911efe7aad (bug 1708673) from Fx93 for causing perf bug 1718267 DEVEDITION_93_0b1_BUILD1 DEVEDITION_93_0b1_RELEASE FIREFOX_93_0b1_BUILD1 FIREFOX_93_0b1_RELEASE
authorPascal Chevrel <pchevrel@mozilla.com>
Mon, 06 Sep 2021 18:36:34 +0200
changeset 659878 65591fce10de05bfd1d6d3d829f8cad140113138
parent 659877 b0ad30f14d10b21fa4c1470c40209c688d9ef82d
child 659879 fb4b4d8f59f3945b20319e30fe1503ef26190ca1
push id15796
push userpchevrel@mozilla.com
push dateMon, 06 Sep 2021 16:43:01 +0000
treeherdermozilla-beta@65591fce10de [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1708673, 1718267
milestone93.0
backs outc7911efe7aad186df5f2737d9c7799c25e31afbd
Backed out changeset c7911efe7aad (bug 1708673) from Fx93 for causing perf bug 1718267
modules/libpref/init/StaticPrefList.yaml
netwerk/base/Predictor.cpp
netwerk/cache2/CacheEntry.cpp
netwerk/cache2/CacheStorage.cpp
netwerk/cache2/CacheStorageService.cpp
netwerk/cache2/CacheStorageService.h
netwerk/test/unit/test_cache2-02b-open-non-existing-and-doom.js
netwerk/test/unit/test_predictor.js
netwerk/test/unit/xpcshell.ini
--- a/modules/libpref/init/StaticPrefList.yaml
+++ b/modules/libpref/init/StaticPrefList.yaml
@@ -10037,24 +10037,16 @@
   value: 300   # 5 minutes (in seconds)
   mirror: always
 
 - name: network.cache.frecency_array_check_enabled
   type: RelaxedAtomicBool
   value: @IS_EARLY_BETA_OR_EARLIER@
   mirror: always
 
-# When this pref is true, AddStorageEntry will return an error if the
-# OPEN_READONLY flag is passed and no entry exists. If no regressions occur
-# this pref should be removed at some point in the future.
-- name: network.cache.bug1708673
-  type: RelaxedAtomicBool
-  value: true
-  mirror: always
-
 #  This is used for a temporary workaround for a web-compat issue. If pref is
 # true CORS preflight requests are allowed to send client certificates.
 - name: network.cors_preflight.allow_client_cert
   type: RelaxedAtomicBool
   value: false
   mirror: always
 
 # Whether to record the telemetry event when a JAR channel is failed to load.
--- a/netwerk/base/Predictor.cpp
+++ b/netwerk/base/Predictor.cpp
@@ -1881,28 +1881,21 @@ Predictor::Resetter::OnCacheEntryVisitCo
     nsCString u;
     nsCOMPtr<nsICacheStorage> cacheDiskStorage;
 
     rv = mPredictor->mCacheStorageService->DiskCacheStorage(
         infosToVisit[i], getter_AddRefs(cacheDiskStorage));
     NS_ENSURE_SUCCESS(rv, rv);
 
     urisToVisit[i]->GetAsciiSpec(u);
-    rv = cacheDiskStorage->AsyncOpenURI(
-        urisToVisit[i], ""_ns,
-        nsICacheStorage::OPEN_READONLY | nsICacheStorage::OPEN_SECRETLY |
-            nsICacheStorage::CHECK_MULTITHREADED,
-        this);
-    if (NS_FAILED(rv)) {
-      mEntriesToVisit--;
-      if (!mEntriesToVisit) {
-        Complete();
-        return NS_OK;
-      }
-    }
+    cacheDiskStorage->AsyncOpenURI(urisToVisit[i], ""_ns,
+                                   nsICacheStorage::OPEN_READONLY |
+                                       nsICacheStorage::OPEN_SECRETLY |
+                                       nsICacheStorage::CHECK_MULTITHREADED,
+                                   this);
   }
 
   return NS_OK;
 }
 
 void Predictor::Resetter::Complete() {
   MOZ_ASSERT(NS_IsMainThread());
 
--- a/netwerk/cache2/CacheEntry.cpp
+++ b/netwerk/cache2/CacheEntry.cpp
@@ -520,17 +520,17 @@ already_AddRefed<CacheEntryHandle> Cache
     }
 
     mozilla::MutexAutoUnlock unlock(mLock);
 
     // The following call dooms this entry (calls DoomAlreadyRemoved on us)
     nsresult rv = CacheStorageService::Self()->AddStorageEntry(
         GetStorageID(), GetURI(), GetEnhanceID(), mUseDisk && !aMemoryOnly,
         mSkipSizeCheck, mPinned,
-        nsICacheStorage::OPEN_TRUNCATE,  // truncate existing (this one)
+        true,  // truncate existing (this one)
         getter_AddRefs(handle));
 
     if (NS_SUCCEEDED(rv)) {
       newEntry = handle->Entry();
       LOG(("  exchanged entry %p by entry %p, rv=0x%08" PRIx32, this,
            newEntry.get(), static_cast<uint32_t>(rv)));
       newEntry->AsyncOpen(aCallback, nsICacheStorage::OPEN_TRUNCATE);
     } else {
--- a/netwerk/cache2/CacheStorage.cpp
+++ b/netwerk/cache2/CacheStorage.cpp
@@ -44,31 +44,32 @@ NS_IMETHODIMP CacheStorage::AsyncOpenURI
     return NS_OK;
   }
 
   NS_ENSURE_ARG(aURI);
   NS_ENSURE_ARG(aCallback);
 
   nsresult rv;
 
+  bool truncate = aFlags & nsICacheStorage::OPEN_TRUNCATE;
+
   nsCOMPtr<nsIURI> noRefURI;
   rv = NS_GetURIWithoutRef(aURI, getter_AddRefs(noRefURI));
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsAutoCString asciiSpec;
   rv = noRefURI->GetAsciiSpec(asciiSpec);
   NS_ENSURE_SUCCESS(rv, rv);
 
   RefPtr<CacheEntryHandle> entry;
   rv = CacheStorageService::Self()->AddStorageEntry(
-      this, asciiSpec, aIdExtension, aFlags, getter_AddRefs(entry));
-  if (NS_FAILED(rv)) {
-    aCallback->OnCacheEntryAvailable(nullptr, false, rv);
-    return NS_OK;
-  }
+      this, asciiSpec, aIdExtension,
+      truncate,  // replace any existing one?
+      getter_AddRefs(entry));
+  NS_ENSURE_SUCCESS(rv, rv);
 
   // May invoke the callback synchronously
   entry->Entry()->AsyncOpen(aCallback, aFlags);
 
   return NS_OK;
 }
 
 NS_IMETHODIMP CacheStorage::OpenTruncate(nsIURI* aURI,
@@ -84,17 +85,17 @@ NS_IMETHODIMP CacheStorage::OpenTruncate
 
   nsAutoCString asciiSpec;
   rv = noRefURI->GetAsciiSpec(asciiSpec);
   NS_ENSURE_SUCCESS(rv, rv);
 
   RefPtr<CacheEntryHandle> handle;
   rv = CacheStorageService::Self()->AddStorageEntry(
       this, asciiSpec, aIdExtension,
-      nsICacheStorage::OPEN_TRUNCATE,  // replace any existing one
+      true,  // replace any existing one
       getter_AddRefs(handle));
   NS_ENSURE_SUCCESS(rv, rv);
 
   // Just open w/o callback, similar to nsICacheEntry.recreate().
   handle->Entry()->AsyncOpen(nullptr, OPEN_TRUNCATE);
 
   // Return a write handler, consumer is supposed to fill in the entry.
   RefPtr<CacheEntryHandle> writeHandle = handle->Entry()->NewWriteHandle();
--- a/netwerk/cache2/CacheStorageService.cpp
+++ b/netwerk/cache2/CacheStorageService.cpp
@@ -28,17 +28,16 @@
 #include "nsXULAppAPI.h"
 #include "mozilla/AtomicBitfields.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/DebugOnly.h"
 #include "mozilla/Services.h"
 #include "mozilla/StoragePrincipalHelper.h"
 #include "mozilla/IntegerPrintfMacros.h"
 #include "mozilla/Telemetry.h"
-#include "mozilla/StaticPrefs_network.h"
 
 namespace mozilla::net {
 
 namespace {
 
 void AppendMemoryStorageTag(nsAutoCString& key) {
   // Using DEL as the very last ascii-7 character we can use in the list of
   // attributes
@@ -1562,34 +1561,34 @@ void CacheStorageService::MemoryPool::Pu
   }
 }
 
 // Methods exposed to and used by CacheStorage.
 
 nsresult CacheStorageService::AddStorageEntry(CacheStorage const* aStorage,
                                               const nsACString& aURI,
                                               const nsACString& aIdExtension,
-                                              uint32_t aFlags,
+                                              bool aReplace,
                                               CacheEntryHandle** aResult) {
   NS_ENSURE_FALSE(mShutdown, NS_ERROR_NOT_INITIALIZED);
 
   NS_ENSURE_ARG(aStorage);
 
   nsAutoCString contextKey;
   CacheFileUtils::AppendKeyPrefix(aStorage->LoadInfo(), contextKey);
 
   return AddStorageEntry(contextKey, aURI, aIdExtension,
                          aStorage->WriteToDisk(), aStorage->SkipSizeCheck(),
-                         aStorage->Pinning(), aFlags, aResult);
+                         aStorage->Pinning(), aReplace, aResult);
 }
 
 nsresult CacheStorageService::AddStorageEntry(
     const nsACString& aContextKey, const nsACString& aURI,
     const nsACString& aIdExtension, bool aWriteToDisk, bool aSkipSizeCheck,
-    bool aPin, uint32_t aFlags, CacheEntryHandle** aResult) {
+    bool aPin, bool aReplace, CacheEntryHandle** aResult) {
   nsresult rv;
 
   nsAutoCString entryKey;
   rv = CacheEntry::HashingKey(""_ns, aIdExtension, aURI, entryKey);
   NS_ENSURE_SUCCESS(rv, rv);
 
   LOG(("CacheStorageService::AddStorageEntry [entryKey=%s, contextKey=%s]",
        entryKey.get(), aContextKey.BeginReading()));
@@ -1611,55 +1610,49 @@ nsresult CacheStorageService::AddStorage
                   LOG(("  new storage entries table for context '%s'",
                        aContextKey.BeginReading()));
                   return MakeUnique<CacheEntryTable>(
                       CacheEntryTable::ALL_ENTRIES);
                 })
             .get();
 
     bool entryExists = entries->Get(entryKey, getter_AddRefs(entry));
-    if (!entryExists && aFlags & nsICacheStorage::OPEN_READONLY &&
-        StaticPrefs::network_cache_bug1708673()) {
-      return NS_ERROR_CACHE_KEY_NOT_FOUND;
-    }
-
-    bool replace = aFlags & nsICacheStorage::OPEN_TRUNCATE;
-
-    if (entryExists && !replace) {
+
+    if (entryExists && !aReplace) {
       // check whether we want to turn this entry to a memory-only.
       if (MOZ_UNLIKELY(!aWriteToDisk) && MOZ_LIKELY(entry->IsUsingDisk())) {
         LOG(("  entry is persistent but we want mem-only, replacing it"));
-        replace = true;
+        aReplace = true;
       }
     }
 
     // If truncate is demanded, delete and doom the current entry
-    if (entryExists && replace) {
+    if (entryExists && aReplace) {
       entries->Remove(entryKey);
 
       LOG(("  dooming entry %p for %s because of OPEN_TRUNCATE", entry.get(),
            entryKey.get()));
       // On purpose called under the lock to prevent races of doom and open on
       // I/O thread No need to remove from both memory-only and all-entries
       // tables.  The new entry will overwrite the shadow entry in its ctor.
       entry->DoomAlreadyRemoved();
 
       entry = nullptr;
       entryExists = false;
 
       // Would only lead to deleting force-valid timestamp again.  We don't need
       // the replace information anymore after this point anyway.
-      replace = false;
+      aReplace = false;
     }
 
     // Ensure entry for the particular URL
     if (!entryExists) {
       // When replacing with a new entry, always remove the current force-valid
       // timestamp, this is the only place to do it.
-      if (replace) {
+      if (aReplace) {
         RemoveEntryForceValid(aContextKey, entryKey);
       }
 
       // Entry is not in the hashtable or has just been truncated...
       entry = new CacheEntry(aContextKey, aURI, aIdExtension, aWriteToDisk,
                              aSkipSizeCheck, aPin);
       entries->InsertOrUpdate(entryKey, RefPtr{entry});
       LOG(("  new entry %p for %s", entry.get(), entryKey.get()));
--- a/netwerk/cache2/CacheStorageService.h
+++ b/netwerk/cache2/CacheStorageService.h
@@ -214,17 +214,17 @@ class CacheStorageService final : public
   // Following methods are thread safe to call.
   friend class CacheStorage;
 
   /**
    * Get, or create when not existing and demanded, an entry for the storage
    * and uri+id extension.
    */
   nsresult AddStorageEntry(CacheStorage const* aStorage, const nsACString& aURI,
-                           const nsACString& aIdExtension, uint32_t aFlags,
+                           const nsACString& aIdExtension, bool aReplace,
                            CacheEntryHandle** aResult);
 
   /**
    * Check existance of an entry.  This may throw NS_ERROR_NOT_AVAILABLE
    * when the information cannot be obtained synchronously w/o blocking.
    */
   nsresult CheckStorageEntry(CacheStorage const* aStorage,
                              const nsACString& aURI,
@@ -302,17 +302,17 @@ class CacheStorageService final : public
 
  private:
   nsresult DoomStorageEntries(const nsACString& aContextKey,
                               nsILoadContextInfo* aContext, bool aDiskStorage,
                               bool aPin, nsICacheEntryDoomCallback* aCallback);
   nsresult AddStorageEntry(const nsACString& aContextKey,
                            const nsACString& aURI,
                            const nsACString& aIdExtension, bool aWriteToDisk,
-                           bool aSkipSizeCheck, bool aPin, uint32_t aFlags,
+                           bool aSkipSizeCheck, bool aPin, bool aReplace,
                            CacheEntryHandle** aResult);
 
   nsresult ClearOriginInternal(
       const nsAString& aOrigin,
       const mozilla::OriginAttributes& aOriginAttributes, bool aAnonymous);
 
   static CacheStorageService* sSelf;
 
deleted file mode 100644
--- a/netwerk/test/unit/test_cache2-02b-open-non-existing-and-doom.js
+++ /dev/null
@@ -1,172 +0,0 @@
-"use strict";
-
-add_task(async function test() {
-  do_get_profile();
-  do_test_pending();
-
-  await new Promise(resolve => {
-    // Open non-existing for read, should fail
-    asyncOpenCacheEntry(
-      "http://b/",
-      "disk",
-      Ci.nsICacheStorage.OPEN_READONLY,
-      null,
-      new OpenCallback(NOTFOUND, null, null, function(entry) {
-        resolve(entry);
-      })
-    );
-  });
-
-  await new Promise(resolve => {
-    // Open the same non-existing for read again, should fail second time
-    asyncOpenCacheEntry(
-      "http://b/",
-      "disk",
-      Ci.nsICacheStorage.OPEN_READONLY,
-      null,
-      new OpenCallback(NOTFOUND, null, null, function(entry) {
-        resolve(entry);
-      })
-    );
-  });
-
-  await new Promise(resolve => {
-    // Try it again normally, should go
-    asyncOpenCacheEntry(
-      "http://b/",
-      "disk",
-      Ci.nsICacheStorage.OPEN_NORMALLY,
-      null,
-      new OpenCallback(NEW, "b1m", "b1d", function(entry) {
-        resolve(entry);
-      })
-    );
-  });
-
-  await new Promise(resolve => {
-    // ...and check
-    asyncOpenCacheEntry(
-      "http://b/",
-      "disk",
-      Ci.nsICacheStorage.OPEN_NORMALLY,
-      null,
-      new OpenCallback(NORMAL, "b1m", "b1d", function(entry) {
-        resolve(entry);
-      })
-    );
-  });
-
-  let asyncDoomVisitor = new Promise(resolve => {
-    let doomTasks = [];
-    let visitor = {
-      onCacheStorageInfo() {},
-      async onCacheEntryInfo(
-        aURI,
-        aIdEnhance,
-        aDataSize,
-        aFetchCount,
-        aLastModifiedTime,
-        aExpirationTime,
-        aPinned,
-        aInfo
-      ) {
-        doomTasks.push(
-          new Promise(resolve => {
-            Services.cache2
-              .diskCacheStorage(aInfo, false)
-              .asyncDoomURI(aURI, aIdEnhance, {
-                onCacheEntryDoomed() {
-                  info("doomed");
-                  resolve();
-                },
-              });
-          })
-        );
-      },
-      onCacheEntryVisitCompleted() {
-        Promise.allSettled(doomTasks).then(resolve);
-      },
-      QueryInterface: ChromeUtils.generateQI(["nsICacheStorageVisitor"]),
-    };
-    Services.cache2.asyncVisitAllStorages(visitor, true);
-  });
-
-  let asyncOpenVisitor = new Promise(resolve => {
-    let openTasks = [];
-    let visitor = {
-      onCacheStorageInfo() {},
-      async onCacheEntryInfo(
-        aURI,
-        aIdEnhance,
-        aDataSize,
-        aFetchCount,
-        aLastModifiedTime,
-        aExpirationTime,
-        aPinned,
-        aInfo
-      ) {
-        info(`found ${aURI.spec}`);
-        openTasks.push(
-          new Promise(r2 => {
-            Services.cache2
-              .diskCacheStorage(aInfo, false)
-              .asyncOpenURI(
-                aURI,
-                "",
-                Ci.nsICacheStorage.OPEN_READONLY |
-                  Ci.nsICacheStorage.OPEN_SECRETLY,
-                {
-                  onCacheEntryCheck() {
-                    return Ci.nsICacheEntryOpenCallback.ENTRY_WANTED;
-                  },
-                  onCacheEntryAvailable(entry, isnew, status) {
-                    info("opened");
-                    r2();
-                  },
-                  QueryInterface: ChromeUtils.generateQI([
-                    "nsICacheEntryOpenCallback",
-                  ]),
-                }
-              );
-          })
-        );
-      },
-      onCacheEntryVisitCompleted() {
-        Promise.all(openTasks).then(resolve);
-      },
-      QueryInterface: ChromeUtils.generateQI(["nsICacheStorageVisitor"]),
-    };
-    Services.cache2.asyncVisitAllStorages(visitor, true);
-  });
-
-  await Promise.all([asyncDoomVisitor, asyncOpenVisitor]);
-
-  info("finished visiting");
-
-  await new Promise(resolve => {
-    let entryCount = 0;
-    let visitor = {
-      onCacheStorageInfo() {},
-      async onCacheEntryInfo(
-        aURI,
-        aIdEnhance,
-        aDataSize,
-        aFetchCount,
-        aLastModifiedTime,
-        aExpirationTime,
-        aPinned,
-        aInfo
-      ) {
-        entryCount++;
-      },
-      onCacheEntryVisitCompleted() {
-        Assert.equal(entryCount, 0);
-        resolve();
-      },
-      QueryInterface: ChromeUtils.generateQI(["nsICacheStorageVisitor"]),
-    };
-    Services.cache2.asyncVisitAllStorages(visitor, true);
-  });
-
-  finish_cache2_test();
-});
--- a/netwerk/test/unit/test_predictor.js
+++ b/netwerk/test/unit/test_predictor.js
@@ -672,90 +672,16 @@ function continue_test_prefetch() {
     prefetch_tluri,
     null,
     predictor.PREDICT_LOAD,
     origin_attributes,
     verifier
   );
 }
 
-function test_visitor_doom() {
-  // See bug 1708673
-  let p1 = new Promise(resolve => {
-    let doomTasks = [];
-    let visitor = {
-      onCacheStorageInfo() {},
-      async onCacheEntryInfo(
-        aURI,
-        aIdEnhance,
-        aDataSize,
-        aFetchCount,
-        aLastModifiedTime,
-        aExpirationTime,
-        aPinned,
-        aInfo
-      ) {
-        let storages = [
-          Services.cache2.memoryCacheStorage(aInfo),
-          Services.cache2.diskCacheStorage(aInfo, false),
-        ];
-        console.debug("asyncDoomURI", aURI.spec);
-        let doomTask = Promise.all(
-          storages.map(storage => {
-            return new Promise(resolve => {
-              storage.asyncDoomURI(aURI, aIdEnhance, {
-                onCacheEntryDoomed: resolve,
-              });
-            });
-          })
-        );
-        doomTasks.push(doomTask);
-      },
-      onCacheEntryVisitCompleted() {
-        Promise.allSettled(doomTasks).then(resolve);
-      },
-      QueryInterface: ChromeUtils.generateQI(["nsICacheStorageVisitor"]),
-    };
-    Services.cache2.asyncVisitAllStorages(visitor, true);
-  });
-
-  let p2 = new Promise(resolve => {
-    reset_predictor();
-    resolve();
-  });
-
-  do_test_pending();
-  Promise.allSettled([p1, p2]).then(() => {
-    return new Promise(resolve => {
-      let entryCount = 0;
-      let visitor = {
-        onCacheStorageInfo() {},
-        async onCacheEntryInfo(
-          aURI,
-          aIdEnhance,
-          aDataSize,
-          aFetchCount,
-          aLastModifiedTime,
-          aExpirationTime,
-          aPinned,
-          aInfo
-        ) {
-          entryCount++;
-        },
-        onCacheEntryVisitCompleted() {
-          Assert.equal(entryCount, 0);
-          resolve();
-        },
-        QueryInterface: ChromeUtils.generateQI(["nsICacheStorageVisitor"]),
-      };
-      Services.cache2.asyncVisitAllStorages(visitor, true);
-    }).then(run_next_test);
-  });
-}
-
 function cleanup() {
   observer.cleaningUp = true;
   if (running_single_process) {
     // The http server is required (and started) by the prefetch test, which
     // only runs in single-process mode, so don't try to shut it down if we're
     // in e10s mode.
     do_test_pending();
     httpserv.stop(do_test_finished);
@@ -773,17 +699,16 @@ var tests = [
   //test_startup,
   // END DISABLED TESTS
   test_origin,
   test_dns,
   test_prefetch_setup,
   test_prefetch_prime,
   test_prefetch_prime,
   test_prefetch,
-  test_visitor_doom,
   // This must ALWAYS come last, to ensure we clean up after ourselves
   cleanup,
 ];
 
 var observer = {
   cleaningUp: false,
 
   QueryInterface: ChromeUtils.generateQI(["nsIObserver"]),
--- a/netwerk/test/unit/xpcshell.ini
+++ b/netwerk/test/unit/xpcshell.ini
@@ -33,17 +33,16 @@ run-sequentially = node server exception
 [test_cache2-01-basic.js]
 [test_cache2-01a-basic-readonly.js]
 [test_cache2-01b-basic-datasize.js]
 [test_cache2-01c-basic-hasmeta-only.js]
 [test_cache2-01d-basic-not-wanted.js]
 [test_cache2-01e-basic-bypass-if-busy.js]
 [test_cache2-01f-basic-openTruncate.js]
 [test_cache2-02-open-non-existing.js]
-[test_cache2-02b-open-non-existing-and-doom.js]
 [test_cache2-03-oncacheentryavail-throws.js]
 [test_cache2-04-oncacheentryavail-throws2x.js]
 [test_cache2-05-visit.js]
 [test_cache2-06-pb-mode.js]
 [test_cache2-07-visit-memory.js]
 [test_cache2-07a-open-memory.js]
 [test_cache2-08-evict-disk-by-memory-storage.js]
 [test_cache2-09-evict-disk-by-uri.js]