Revert "Bug 1999691 - When a DNS cache entry is used, take it out and readd it to the eviction queue, r=necko-reviewers,valentin" for causing a top crash tracked under Bug 2004939.

CosminSabou · CosminSabou · commit 7496c8515212 · 2025-12-09T14:30:17.000Z
This reverts commit ffb7bd5.
diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml
@@ -15666,13 +15666,6 @@
   value: ""
   mirror: never
 
-# When true, placing the most recent used cache entry
-# to the tail of the EvictionQ.
-- name: network.dns.mru_to_tail
-  type: RelaxedAtomicBool
-  value: @IS_NIGHTLY_BUILD@
-  mirror: always
-
 # Whether to add additional record IPs to the cache
 - name: network.trr.add_additional_records
   type: RelaxedAtomicBool
diff --git a/netwerk/dns/HostRecordQueue.cpp b/netwerk/dns/HostRecordQueue.cpp
@@ -92,21 +92,6 @@ void HostRecordQueue::AddToEvictionQ(
   }
 }
 
-void HostRecordQueue::MoveToEvictionQueueTail(
-    nsHostRecord* aRec, const MutexAutoLock& aProofOfLock) {
-  bool inEvictionQ = mEvictionQ.contains(aRec);
-  if (!inEvictionQ) {
-    // Note: this function can be called when the record isn’t in the
-    // mEvictionQ. For example, if we immediately start a TTL lookup (see
-    // nsHostResolver::CompleteLookupLocked), the record may not be in
-    // mEvictionQ.
-    return;
-  }
-
-  aRec->remove();
-  mEvictionQ.insertBack(aRec);
-}
-
 void HostRecordQueue::MaybeRenewHostRecord(nsHostRecord* aRec,
                                            const MutexAutoLock& aProofOfLock) {
   if (!aRec->isInList()) {
diff --git a/netwerk/dns/HostRecordQueue.h b/netwerk/dns/HostRecordQueue.h
@@ -34,11 +34,6 @@ class HostRecordQueue final {
       nsHostRecord* aRec, uint32_t aMaxCacheEntries,
       nsRefPtrHashtable<nsGenericHashKey<nsHostKey>, nsHostRecord>& aDB,
       const MutexAutoLock& aProofOfLock);
-
-  // Move aRec to the tail of mEvictionQ (the most-recently-used end).
-  void MoveToEvictionQueueTail(nsHostRecord* aRec,
-                               const MutexAutoLock& aProofOfLock);
-
   // Called for removing the record from mEvictionQ. When this function is
   // called, the record should be either in mEvictionQ or not in any queue.
   void MaybeRenewHostRecord(nsHostRecord* aRec,
diff --git a/netwerk/dns/nsHostResolver.cpp b/netwerk/dns/nsHostResolver.cpp
@@ -591,7 +591,7 @@ nsresult nsHostResolver::ResolveHost(const nsACString& aHost,
     } else if (!rec->mResolving) {
       result =
           FromUnspecEntry(rec, host, aTrrServer, originSuffix, type, flags, af,
-                          aOriginAttributes.IsPrivateBrowsing(), status, lock);
+                          aOriginAttributes.IsPrivateBrowsing(), status);
       // If this is a by-type request or if no valid record was found
       // in the cache or this is an AF_UNSPEC request, then start a
       // new lookup.
@@ -675,14 +675,12 @@ already_AddRefed<nsHostRecord> nsHostResolver::FromCache(
   // but start a new lookup in the background.
   //
   // Also records telemetry for type of cache hit (HIT/NEGATIVE_HIT/RENEWAL).
-  bool refresh = ConditionallyRefreshRecord(aRec, aHost, aLock);
+  ConditionallyRefreshRecord(aRec, aHost, aLock);
 
   if (aRec->negative) {
     LOG(("  Negative cache entry for host [%s].\n",
          nsPromiseFlatCString(aHost).get()));
     aStatus = NS_ERROR_UNKNOWN_HOST;
-  } else if (StaticPrefs::network_dns_mru_to_tail() && !refresh) {
-    mQueue.MoveToEvictionQueueTail(aRec, aLock);
   }
 
   return result.forget();
@@ -709,8 +707,7 @@ already_AddRefed<nsHostRecord> nsHostResolver::FromIPLiteral(
 already_AddRefed<nsHostRecord> nsHostResolver::FromUnspecEntry(
     nsHostRecord* aRec, const nsACString& aHost, const nsACString& aTrrServer,
     const nsACString& aOriginSuffix, uint16_t aType,
-    nsIDNSService::DNSFlags aFlags, uint16_t af, bool aPb, nsresult& aStatus,
-    const MutexAutoLock& aLock) {
+    nsIDNSService::DNSFlags aFlags, uint16_t af, bool aPb, nsresult& aStatus) {
   RefPtr<nsHostRecord> result = nullptr;
   // If this is an IPV4 or IPV6 specific request, check if there is
   // an AF_UNSPEC entry we can use. Otherwise, hit the resolver...
@@ -776,10 +773,7 @@ already_AddRefed<nsHostRecord> nsHostResolver::FromUnspecEntry(
         if (aRec->negative) {
           aStatus = NS_ERROR_UNKNOWN_HOST;
         }
-        bool refresh = ConditionallyRefreshRecord(aRec, aHost, lock);
-        if (!refresh) {
-          AddToEvictionQ(result, aLock);
-        }
+        ConditionallyRefreshRecord(aRec, aHost, lock);
       } else if (af == PR_AF_INET6) {
         // For AF_INET6, a new lookup means another AF_UNSPEC
         // lookup. We have already iterated through the
@@ -1189,15 +1183,14 @@ nsresult nsHostResolver::NameLookup(nsHostRecord* rec,
   return rv;
 }
 
-bool nsHostResolver::ConditionallyRefreshRecord(nsHostRecord* rec,
-                                                const nsACString& host,
-                                                const MutexAutoLock& aLock) {
+nsresult nsHostResolver::ConditionallyRefreshRecord(
+    nsHostRecord* rec, const nsACString& host, const MutexAutoLock& aLock) {
   if ((rec->CheckExpiration(TimeStamp::NowLoRes()) == nsHostRecord::EXP_GRACE ||
        rec->negative) &&
       !rec->mResolving && rec->RefreshForNegativeResponse()) {
     LOG(("  Using %s cache entry for host [%s] but starting async renewal.",
          rec->negative ? "negative" : "positive", host.BeginReading()));
-    nsresult rv = NameLookup(rec, aLock);
+    NameLookup(rec, aLock);
 
     if (rec->IsAddrRecord()) {
       if (!rec->negative) {
@@ -1206,8 +1199,6 @@ bool nsHostResolver::ConditionallyRefreshRecord(nsHostRecord* rec,
         glean::dns::lookup_method.AccumulateSingleSample(METHOD_NEGATIVE_HIT);
       }
     }
-
-    return NS_SUCCEEDED(rv);
   } else if (rec->IsAddrRecord()) {
     // it could be that the record is negative, but we haven't entered the above
     // if condition due to the second expression being false. In that case we
@@ -1219,7 +1210,7 @@ bool nsHostResolver::ConditionallyRefreshRecord(nsHostRecord* rec,
     }
   }
 
-  return false;
+  return NS_OK;
 }
 
 bool nsHostResolver::GetHostToLookup(nsHostRecord** result) {
diff --git a/netwerk/dns/nsHostResolver.h b/netwerk/dns/nsHostResolver.h
@@ -234,13 +234,12 @@ class nsHostResolver : public nsISupports, public AHostResolver {
 
   /**
    * Starts a new lookup in the background for cached entries that are in the
-   * grace period or that are are negative. Return true when the new lookup
-   * starts.
+   * grace period or that are are negative.
    *
    * Also records telemetry for type of cache hit (HIT/NEGATIVE_HIT/RENEWAL).
    */
-  bool ConditionallyRefreshRecord(nsHostRecord* rec, const nsACString& host,
-                                  const mozilla::MutexAutoLock& aLock)
+  nsresult ConditionallyRefreshRecord(nsHostRecord* rec, const nsACString& host,
+                                      const mozilla::MutexAutoLock& aLock)
       MOZ_REQUIRES(mLock);
 
   void OnResolveComplete(nsHostRecord* aRec,
@@ -267,8 +266,8 @@ class nsHostResolver : public nsISupports, public AHostResolver {
   already_AddRefed<nsHostRecord> FromUnspecEntry(
       nsHostRecord* aRec, const nsACString& aHost, const nsACString& aTrrServer,
       const nsACString& aOriginSuffix, uint16_t aType,
-      nsIDNSService::DNSFlags aFlags, uint16_t af, bool aPb, nsresult& aStatus,
-      const mozilla::MutexAutoLock& aLock) MOZ_REQUIRES(mLock);
+      nsIDNSService::DNSFlags aFlags, uint16_t af, bool aPb, nsresult& aStatus)
+      MOZ_REQUIRES(mLock);
 
   enum {
     METHOD_HIT = 1,
diff --git a/netwerk/test/gtest/TestHostRecordQueue.cpp b/netwerk/test/gtest/TestHostRecordQueue.cpp
diff --git a/netwerk/test/gtest/moz.build b/netwerk/test/gtest/moz.build
@@ -16,7 +16,6 @@ UNIFIED_SOURCES += [
     "TestDNSPacket.cpp",
     "TestEtcHostsParsing.cpp",
     "TestHeaders.cpp",
-    "TestHostRecordQueue.cpp",
     "TestHttp2WebTransport.cpp",
     "TestHttp3ConnectUDPStream.cpp",
     "TestHttpAtom.cpp",
@@ -70,7 +69,6 @@ LOCAL_INCLUDES += [
     "/netwerk/base",
     "/netwerk/cache2",
     "/netwerk/cookie",
-    "/netwerk/dns",
     "/netwerk/protocol/http",
     "/toolkit/components/jsoncpp/include",
     "/xpcom/tests/gtest",
