From a7fe7a006d14e67351f2ff5cb24396b458b8eed8 Mon Sep 17 00:00:00 2001 From: Valentin Gosu Date: Wed, 20 Dec 2017 01:13:46 +0100 Subject: [PATCH] Bug 1424834 - Replace nsHostRecord.callbacks with LinkedList> r=mayhemer * nsResolveHostCallback extends nsISupports (for addref-ing and because nsDNSAsyncRequest implements nsICancelable) * nsResolveHostCallback extends LinkedListElement> so the list can properly manage references * nsDNSAsyncRequest and nsDNSSyncRequest properly implement nsISupports and use RefPtr to manage lifetimes MozReview-Commit-ID: 5NvBcWZzfyN --HG-- extra : rebase_source : 67adcbd20a29803e5abbb3d16324f9cac6771d28 --- netwerk/dns/nsDNSService2.cpp | 57 ++++++++++------------ netwerk/dns/nsHostResolver.cpp | 89 ++++++++++++++++------------------ netwerk/dns/nsHostResolver.h | 15 ++++-- 3 files changed, 77 insertions(+), 84 deletions(-) diff --git a/netwerk/dns/nsDNSService2.cpp b/netwerk/dns/nsDNSService2.cpp index 0da1384e0079..8afcdf267aa1 100644 --- a/netwerk/dns/nsDNSService2.cpp +++ b/netwerk/dns/nsDNSService2.cpp @@ -295,8 +295,6 @@ nsDNSRecord::ReportUnusable(uint16_t aPort) class nsDNSAsyncRequest final : public nsResolveHostCallback , public nsICancelable { - ~nsDNSAsyncRequest() = default; - public: NS_DECL_THREADSAFE_ISUPPORTS NS_DECL_NSICANCELABLE @@ -331,8 +329,12 @@ public: uint16_t mFlags; uint16_t mAF; nsCString mNetworkInterface; +private: + virtual ~nsDNSAsyncRequest() = default; }; +NS_IMPL_ISUPPORTS(nsDNSAsyncRequest, nsICancelable) + void nsDNSAsyncRequest::OnResolveHostComplete(nsHostResolver *resolver, nsHostRecord *hostRecord, @@ -349,10 +351,6 @@ nsDNSAsyncRequest::OnResolveHostComplete(nsHostResolver *resolver, mListener->OnLookupComplete(this, rec, status); mListener = nullptr; - - // release the reference to ourselves that was added before we were - // handed off to the host resolver. - NS_RELEASE_THIS(); } bool @@ -380,8 +378,6 @@ nsDNSAsyncRequest::SizeOfIncludingThis(MallocSizeOf mallocSizeOf) const return n; } -NS_IMPL_ISUPPORTS(nsDNSAsyncRequest, nsICancelable) - NS_IMETHODIMP nsDNSAsyncRequest::Cancel(nsresult reason) { @@ -393,14 +389,15 @@ nsDNSAsyncRequest::Cancel(nsresult reason) //----------------------------------------------------------------------------- -class nsDNSSyncRequest : public nsResolveHostCallback +class nsDNSSyncRequest + : public nsResolveHostCallback { + NS_DECL_THREADSAFE_ISUPPORTS public: explicit nsDNSSyncRequest(PRMonitor *mon) : mDone(false) , mStatus(NS_OK) , mMonitor(mon) {} - virtual ~nsDNSSyncRequest() = default; void OnResolveHostComplete(nsHostResolver *, nsHostRecord *, nsresult) override; bool EqualsAsyncListener(nsIDNSListener *aListener) override; @@ -411,9 +408,13 @@ public: RefPtr mHostRecord; private: + virtual ~nsDNSSyncRequest() = default; + PRMonitor *mMonitor; }; +NS_IMPL_ISUPPORTS0(nsDNSSyncRequest) + void nsDNSSyncRequest::OnResolveHostComplete(nsHostResolver *resolver, nsHostRecord *hostRecord, @@ -798,7 +799,7 @@ NS_IMETHODIMP nsDNSService::AsyncResolveExtendedNative(const nsACString &aHostname, uint32_t flags, const nsACString &aNetworkInterface, - nsIDNSListener *listener, + nsIDNSListener *aListener, nsIEventTarget *target_, const OriginAttributes &aOriginAttributes, nsICancelable **result) @@ -808,6 +809,7 @@ nsDNSService::AsyncResolveExtendedNative(const nsACString &aHostname, RefPtr res; nsCOMPtr idn; nsCOMPtr target = target_; + nsCOMPtr listener = aListener; bool localDomain = false; { MutexAutoLock lock(mLock); @@ -850,21 +852,16 @@ nsDNSService::AsyncResolveExtendedNative(const nsACString &aHostname, uint16_t af = GetAFForLookup(hostname, flags); - auto *req = + MOZ_ASSERT(listener); + RefPtr req = new nsDNSAsyncRequest(res, hostname, aOriginAttributes, listener, flags, af, aNetworkInterface); if (!req) return NS_ERROR_OUT_OF_MEMORY; - NS_ADDREF(*result = req); - // addref for resolver; will be released when OnResolveHostComplete is called. - NS_ADDREF(req); rv = res->ResolveHost(req->mHost.get(), req->mOriginAttributes, flags, af, req->mNetworkInterface.get(), req); - if (NS_FAILED(rv)) { - NS_RELEASE(req); - NS_RELEASE(*result); - } + req.forget(result); return rv; } @@ -1055,25 +1052,23 @@ nsDNSService::ResolveInternal(const nsACString &aHostname, return NS_ERROR_OUT_OF_MEMORY; PR_EnterMonitor(mon); - nsDNSSyncRequest syncReq(mon); + RefPtr syncReq = new nsDNSSyncRequest(mon); uint16_t af = GetAFForLookup(hostname, flags); - rv = res->ResolveHost(hostname.get(), aOriginAttributes, flags, af, "", &syncReq); + rv = res->ResolveHost(hostname.get(), aOriginAttributes, flags, af, "", syncReq); if (NS_SUCCEEDED(rv)) { // wait for result - while (!syncReq.mDone) + while (!syncReq->mDone) { PR_Wait(mon, PR_INTERVAL_NO_TIMEOUT); + } - if (NS_FAILED(syncReq.mStatus)) - rv = syncReq.mStatus; - else { - NS_ASSERTION(syncReq.mHostRecord, "no host record"); - auto *rec = new nsDNSRecord(syncReq.mHostRecord); - if (!rec) - rv = NS_ERROR_OUT_OF_MEMORY; - else - NS_ADDREF(*result = rec); + if (NS_FAILED(syncReq->mStatus)) { + rv = syncReq->mStatus; + } else { + NS_ASSERTION(syncReq->mHostRecord, "no host record"); + RefPtr rec = new nsDNSRecord(syncReq->mHostRecord); + rec.forget(result); } } diff --git a/netwerk/dns/nsHostResolver.cpp b/netwerk/dns/nsHostResolver.cpp index 1a55c7605ea3..67b01879e4b9 100644 --- a/netwerk/dns/nsHostResolver.cpp +++ b/netwerk/dns/nsHostResolver.cpp @@ -189,7 +189,6 @@ nsHostRecord::nsHostRecord(const nsHostKey *key) memcpy((char *) originSuffix, key->originSuffix, strlen(key->originSuffix) + 1); PR_INIT_CLIST(this); - PR_INIT_CLIST(&callbacks); } nsresult @@ -230,6 +229,8 @@ nsHostRecord::CopyExpirationTimesAndFlagsFrom(const nsHostRecord *aFromHostRecor nsHostRecord::~nsHostRecord() { + mCallbacks.clear(); + Telemetry::Accumulate(Telemetry::DNS_BLACKLIST_COUNT, mBlacklistedCount); delete addr_info; } @@ -326,17 +327,15 @@ nsHostRecord::HasUsableResult(const mozilla::TimeStamp& now, uint16_t queryFlags } static size_t -SizeOfResolveHostCallbackListExcludingHead(const PRCList *head, +SizeOfResolveHostCallbackListExcludingHead(const mozilla::LinkedList>& aCallbacks, MallocSizeOf mallocSizeOf) { - size_t n = 0; - PRCList *curr = head->next; - while (curr != head) { - nsResolveHostCallback *callback = - static_cast(curr); - n += callback->SizeOfIncludingThis(mallocSizeOf); - curr = curr->next; + size_t n = 0; // TODO: should be aCallbacks.sizeOfIncludingThis(mallocSizeOf); + + for (const nsResolveHostCallback* t = aCallbacks.getFirst(); t; t = t->getNext()) { + n += t->SizeOfIncludingThis(mallocSizeOf); } + return n; } @@ -350,7 +349,7 @@ nsHostRecord::SizeOfIncludingThis(MallocSizeOf mallocSizeOf) const // nsHostRecord::Create()). So it will be included in the // |mallocSizeOf(this)| call above. - n += SizeOfResolveHostCallbackListExcludingHead(&callbacks, mallocSizeOf); + n += SizeOfResolveHostCallbackListExcludingHead(mCallbacks, mallocSizeOf); n += addr_info ? addr_info->SizeOfIncludingThis(mallocSizeOf) : 0; n += mallocSizeOf(addr.get()); @@ -733,7 +732,7 @@ nsHostResolver::ResolveHost(const char *host, uint16_t flags, uint16_t af, const char *netInterface, - nsResolveHostCallback *callback) + nsResolveHostCallback *aCallback) { NS_ENSURE_TRUE(host && *host, NS_ERROR_UNEXPECTED); NS_ENSURE_TRUE(netInterface, NS_ERROR_UNEXPECTED); @@ -746,6 +745,7 @@ nsHostResolver::ResolveHost(const char *host, if (!net_IsValidHostName(nsDependentCString(host))) return NS_ERROR_UNKNOWN_HOST; + RefPtr callback(aCallback); // if result is set inside the lock, then we need to issue the // callback before returning. RefPtr result; @@ -931,26 +931,26 @@ nsHostResolver::ResolveHost(const char *host, LOG_HOST(host, netInterface))); // Add callback to the list of pending callbacks. - PR_APPEND_LINK(callback, &he->rec->callbacks); + he->rec->mCallbacks.insertBack(callback); he->rec->flags = flags; rv = IssueLookup(he->rec); Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2, METHOD_NETWORK_FIRST); - if (NS_FAILED(rv)) { - PR_REMOVE_AND_INIT_LINK(callback); + if (NS_FAILED(rv) && callback->isInList()) { + callback->remove(); } else { LOG((" DNS lookup for host [%s%s%s] blocking " "pending 'getaddrinfo' query: callback [%p]", - LOG_HOST(host, netInterface), callback)); + LOG_HOST(host, netInterface), callback.get())); } } } else { LOG((" Host [%s%s%s] is being resolved. Appending callback " - "[%p].", LOG_HOST(host, netInterface), callback)); + "[%p].", LOG_HOST(host, netInterface), callback.get())); - PR_APPEND_LINK(callback, &he->rec->callbacks); + he->rec->mCallbacks.insertBack(callback); if (he->rec->onQueue) { Telemetry::Accumulate(Telemetry::DNS_LOOKUP_METHOD2, METHOD_NETWORK_SHARED); @@ -976,7 +976,11 @@ nsHostResolver::ResolveHost(const char *host, } } } + if (result) { + if (callback->isInList()) { + callback->remove(); + } callback->OnResolveHostComplete(this, result, status); } @@ -989,10 +993,12 @@ nsHostResolver::DetachCallback(const char *host, uint16_t flags, uint16_t af, const char *netInterface, - nsResolveHostCallback *callback, + nsResolveHostCallback *aCallback, nsresult status) { RefPtr rec; + RefPtr callback(aCallback); + { MutexAutoLock lock(mLock); @@ -1004,22 +1010,22 @@ nsHostResolver::DetachCallback(const char *host, if (he) { // walk list looking for |callback|... we cannot assume // that it will be there! - PRCList *node = he->rec->callbacks.next; - while (node != &he->rec->callbacks) { - if (static_cast(node) == callback) { - PR_REMOVE_LINK(callback); + + for (nsResolveHostCallback* c: he->rec->mCallbacks) { + if (c == callback) { rec = he->rec; + c->remove(); break; } - node = node->next; } } } // complete callback with the given status code; this would only be done if // the record was in the process of being resolved. - if (rec) + if (rec) { callback->OnResolveHostComplete(this, rec, status); + } } nsresult @@ -1293,8 +1299,8 @@ nsHostResolver::CompleteLookup(nsHostRecord* rec, nsresult status, AddrInfo* new { // get the list of pending callbacks for this lookup, and notify // them that the lookup is complete. - PRCList cbs; - PR_INIT_CLIST(&cbs); + mozilla::LinkedList> cbs; + { MutexAutoLock lock(mLock); @@ -1306,7 +1312,7 @@ nsHostResolver::CompleteLookup(nsHostRecord* rec, nsresult status, AddrInfo* new } // grab list of callbacks to notify - MoveCList(rec->callbacks, cbs); + cbs = mozilla::Move(rec->mCallbacks); // update record fields. We might have a rec->addr_info already if a // previous lookup result expired and we're reresolving it.. @@ -1374,15 +1380,8 @@ nsHostResolver::CompleteLookup(nsHostRecord* rec, nsresult status, AddrInfo* new } } - if (!PR_CLIST_IS_EMPTY(&cbs)) { - PRCList *node = cbs.next; - while (node != &cbs) { - nsResolveHostCallback *callback = - static_cast(node); - node = node->next; - callback->OnResolveHostComplete(this, rec, status); - // NOTE: callback must not be dereferenced after this point!! - } + for (nsResolveHostCallback* c = cbs.getFirst(); c; c = c->removeAndGetNext()) { + c->OnResolveHostComplete(this, rec, status); } NS_RELEASE(rec); @@ -1410,24 +1409,18 @@ nsHostResolver::CancelAsyncRequest(const char *host, auto he = static_cast(mDB.Search(&key)); if (he) { nsHostRecord* recPtr = nullptr; - PRCList *node = he->rec->callbacks.next; - // Remove the first nsDNSAsyncRequest callback which matches the - // supplied listener object - while (node != &he->rec->callbacks) { - nsResolveHostCallback *callback - = static_cast(node); - if (callback && (callback->EqualsAsyncListener(aListener))) { - // Remove from the list of callbacks - PR_REMOVE_LINK(callback); + + for (RefPtr c : he->rec->mCallbacks) { + if (c->EqualsAsyncListener(aListener)) { + c->remove(); recPtr = he->rec; - callback->OnResolveHostComplete(this, recPtr, status); + c->OnResolveHostComplete(this, recPtr, status); break; } - node = node->next; } // If there are no more callbacks, remove the hash table entry - if (recPtr && PR_CLIST_IS_EMPTY(&recPtr->callbacks)) { + if (recPtr && recPtr->mCallbacks.isEmpty()) { mDB.Remove((nsHostKey *)recPtr); // If record is on a Queue, remove it and then deref it if (recPtr->next != recPtr) { diff --git a/netwerk/dns/nsHostResolver.h b/netwerk/dns/nsHostResolver.h index 979ffac8e7a1..f8083dc242ab 100644 --- a/netwerk/dns/nsHostResolver.h +++ b/netwerk/dns/nsHostResolver.h @@ -20,6 +20,7 @@ #include "GetAddrInfo.h" #include "mozilla/net/DNS.h" #include "mozilla/net/DashboardTypes.h" +#include "mozilla/LinkedList.h" #include "mozilla/TimeStamp.h" #include "mozilla/UniquePtr.h" @@ -130,8 +131,7 @@ public: private: friend class nsHostResolver; - - PRCList callbacks; /* list of callbacks */ + mozilla::LinkedList> mCallbacks; bool resolving; /* true if this record is being resolved, which means * that it is either on the pending queue or owned by @@ -163,10 +163,13 @@ private: }; /** - * ResolveHost callback object. It's PRCList members are used by - * the nsHostResolver and should not be used by anything else. + * This class is used to notify listeners when a ResolveHost operation is + * complete. Classes that derive it must implement threadsafe nsISupports + * to be able to use RefPtr with this class. */ -class NS_NO_VTABLE nsResolveHostCallback : public PRCList +class nsResolveHostCallback + : public mozilla::LinkedListElement> + , public nsISupports { public: /** @@ -205,6 +208,8 @@ public: virtual bool EqualsAsyncListener(nsIDNSListener *aListener) = 0; virtual size_t SizeOfIncludingThis(mozilla::MallocSizeOf) const = 0; +protected: + virtual ~nsResolveHostCallback() = default; }; /**