commit 7496c8515212669451d7e775a00c2be07da38ca5
parent a2586e4723b725d111ec72b3f090d1ecd62a04c7
Author: Cosmin Sabou <csabou@mozilla.com>
Date: Tue, 9 Dec 2025 16:28:55 +0200
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.
This reverts commit ffb7bd5a17789fa1b0961865b6e9b945f8babde2.
Diffstat:
7 files changed, 13 insertions(+), 172 deletions(-)
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
@@ -1,120 +0,0 @@
-#include "gtest/gtest.h"
-
-#include "HostRecordQueue.h"
-#include "TRRQuery.h"
-#include "TRR.h"
-
-using namespace mozilla;
-using namespace mozilla::net;
-
-class MockHostRecord : public nsHostRecord {
- public:
- NS_DECL_ISUPPORTS_INHERITED
- explicit MockHostRecord(const nsHostKey& aKey) : nsHostRecord(aKey) {
- negative = true;
- }
-
- void ResolveComplete() override {}
-
- bool HasUsableResultInternal(
- const mozilla::TimeStamp& now,
- nsIDNSService::DNSFlags queryFlags) const override {
- return true;
- }
-
- private:
- ~MockHostRecord() = default;
-};
-
-NS_IMPL_ISUPPORTS_INHERITED(MockHostRecord, nsHostRecord, MockHostRecord)
-
-class HostRecordQueueTest : public ::testing::Test {
- protected:
- void SetUp() override {}
-
- HostRecordQueue queue;
- Mutex mMutex{"HostRecordQueueTest"};
- nsRefPtrHashtable<nsGenericHashKey<nsHostKey>, nsHostRecord> mDB;
-};
-
-static RefPtr<nsHostRecord> CreateAndInsertMockRecord(
- nsRefPtrHashtable<nsGenericHashKey<nsHostKey>, nsHostRecord>& aDB,
- const char* hostName) {
- nsHostKey key(nsCString(hostName), ""_ns, 0,
- nsIDNSService::RESOLVE_DEFAULT_FLAGS, 0, false, ""_ns);
- return aDB.LookupOrInsertWith(key, [&] { return new MockHostRecord(key); });
-}
-
-TEST_F(HostRecordQueueTest, AddToEvictionQ_BelowMax) {
- RefPtr<nsHostRecord> rec = CreateAndInsertMockRecord(mDB, "A.com");
-
- MutexAutoLock lock(mMutex);
-
- queue.AddToEvictionQ(rec.get(), 100, mDB, lock);
-
- ASSERT_EQ(1u, queue.EvictionQSize());
- ASSERT_TRUE(rec->isInList());
-
- // Cleanup
- rec->remove();
-}
-
-// When the eviction queue is at capacity, adding a new entry evicts the
-// oldest (head) entry.
-TEST_F(HostRecordQueueTest, AddToEvictionQ_AtMax_EvictsOldest) {
- const uint32_t MAX_ENTRIES = 3;
-
- RefPtr<nsHostRecord> rec1 = CreateAndInsertMockRecord(mDB, "A.com");
- RefPtr<nsHostRecord> rec2 = CreateAndInsertMockRecord(mDB, "B.com");
- RefPtr<nsHostRecord> rec3 = CreateAndInsertMockRecord(mDB, "C.com");
-
- MutexAutoLock lock(mMutex);
- queue.AddToEvictionQ(rec1, MAX_ENTRIES, mDB, lock);
- queue.AddToEvictionQ(rec2, MAX_ENTRIES, mDB, lock);
- queue.AddToEvictionQ(rec3, MAX_ENTRIES, mDB, lock);
-
- ASSERT_EQ(MAX_ENTRIES, queue.EvictionQSize());
-
- RefPtr<nsHostRecord> rec4 = CreateAndInsertMockRecord(mDB, "New.com");
- queue.AddToEvictionQ(rec4, MAX_ENTRIES, mDB, lock);
-
- ASSERT_TRUE(rec2->isInList());
- ASSERT_TRUE(rec3->isInList());
- ASSERT_TRUE(rec4->isInList());
- ASSERT_FALSE(rec1->isInList());
-
- rec2->remove();
- rec3->remove();
- rec4->remove();
-}
-
-// After adding a new record, the touched entry
-// remains, and the oldest untouched entry is evicted.
-TEST_F(HostRecordQueueTest, MoveToEvictionQueueTail) {
- const uint32_t MAX_ENTRIES = 3;
-
- RefPtr<nsHostRecord> rec1 = CreateAndInsertMockRecord(mDB, "A.com");
- RefPtr<nsHostRecord> rec2 = CreateAndInsertMockRecord(mDB, "B.com");
- RefPtr<nsHostRecord> rec3 = CreateAndInsertMockRecord(mDB, "C.com");
-
- MutexAutoLock lock(mMutex);
- queue.AddToEvictionQ(rec1, MAX_ENTRIES, mDB, lock);
- queue.AddToEvictionQ(rec2, MAX_ENTRIES, mDB, lock);
- queue.AddToEvictionQ(rec3, MAX_ENTRIES, mDB, lock);
-
- ASSERT_EQ(MAX_ENTRIES, queue.EvictionQSize());
-
- queue.MoveToEvictionQueueTail(rec1, lock);
-
- RefPtr<nsHostRecord> rec4 = CreateAndInsertMockRecord(mDB, "New.com");
- queue.AddToEvictionQ(rec4, MAX_ENTRIES, mDB, lock);
-
- ASSERT_TRUE(rec1->isInList());
- ASSERT_TRUE(rec3->isInList());
- ASSERT_TRUE(rec4->isInList());
- ASSERT_FALSE(rec2->isInList());
-
- rec1->remove();
- rec3->remove();
- rec4->remove();
-}
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",
