Skip to content

Commit

Permalink
Bug 1424834 - Replace nsHostRecord.callbacks with LinkedList<RefPtr<n…
Browse files Browse the repository at this point in the history 
…sResolveHostCallback>> r=mayhemer

* nsResolveHostCallback extends nsISupports (for addref-ing and because nsDNSAsyncRequest implements nsICancelable)
* nsResolveHostCallback extends LinkedListElement<RefPtr<nsResolveHostCallback>> 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 : d8c5c89c35e455c5d8e6556a140a0ef119b95e86
  • Loading branch information
@valenting
valenting committed Dec 15, 2017
1 parent dbb3111 commit d2697a4
Show file tree
Hide file tree
Showing 3 changed files with 77 additions and 84 deletions.
57 changes: 26 additions & 31 deletions netwerk/dns/nsDNSService2.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -331,8 +329,12 @@ class nsDNSAsyncRequest final : public nsResolveHostCallback
uint16_t mFlags;
uint16_t mAF;
nsCString mNetworkInterface;
private:
virtual ~nsDNSAsyncRequest() = default;
};

NS_IMPL_ISUPPORTS(nsDNSAsyncRequest, nsICancelable)

void
nsDNSAsyncRequest::OnResolveHostComplete(nsHostResolver *resolver,
nsHostRecord *hostRecord,
Expand All @@ -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
Expand Down Expand Up @@ -380,8 +378,6 @@ nsDNSAsyncRequest::SizeOfIncludingThis(MallocSizeOf mallocSizeOf) const
return n;
}

NS_IMPL_ISUPPORTS(nsDNSAsyncRequest, nsICancelable)

NS_IMETHODIMP
nsDNSAsyncRequest::Cancel(nsresult reason)
{
Expand All @@ -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;
Expand All @@ -411,9 +408,13 @@ class nsDNSSyncRequest : public nsResolveHostCallback
RefPtr<nsHostRecord> mHostRecord;

private:
virtual ~nsDNSSyncRequest() = default;

PRMonitor *mMonitor;
};

NS_IMPL_ISUPPORTS0(nsDNSSyncRequest)

void
nsDNSSyncRequest::OnResolveHostComplete(nsHostResolver *resolver,
nsHostRecord *hostRecord,
Expand Down Expand Up @@ -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)
Expand All @@ -808,6 +809,7 @@ nsDNSService::AsyncResolveExtendedNative(const nsACString &aHostname,
RefPtr<nsHostResolver> res;
nsCOMPtr<nsIIDNService> idn;
nsCOMPtr<nsIEventTarget> target = target_;
nsCOMPtr<nsIDNSListener> listener = aListener;
bool localDomain = false;
{
MutexAutoLock lock(mLock);
Expand Down Expand Up @@ -850,21 +852,16 @@ nsDNSService::AsyncResolveExtendedNative(const nsACString &aHostname,

uint16_t af = GetAFForLookup(hostname, flags);

auto *req =
MOZ_ASSERT(listener);
RefPtr<nsDNSAsyncRequest> 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;
}

Expand Down Expand Up @@ -1055,25 +1052,23 @@ nsDNSService::ResolveInternal(const nsACString &aHostname,
return NS_ERROR_OUT_OF_MEMORY;

PR_EnterMonitor(mon);
nsDNSSyncRequest syncReq(mon);
RefPtr<nsDNSSyncRequest> 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<nsDNSRecord> rec = new nsDNSRecord(syncReq->mHostRecord);
rec.forget(result);
}
}

Expand Down
89 changes: 41 additions & 48 deletions netwerk/dns/nsHostResolver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -230,6 +229,8 @@ nsHostRecord::CopyExpirationTimesAndFlagsFrom(const nsHostRecord *aFromHostRecor

nsHostRecord::~nsHostRecord()
{
mCallbacks.clear();

Telemetry::Accumulate(Telemetry::DNS_BLACKLIST_COUNT, mBlacklistedCount);
delete addr_info;
}
Expand Down Expand Up @@ -326,17 +327,15 @@ nsHostRecord::HasUsableResult(const mozilla::TimeStamp& now, uint16_t queryFlags
}

static size_t
SizeOfResolveHostCallbackListExcludingHead(const PRCList *head,
SizeOfResolveHostCallbackListExcludingHead(const mozilla::LinkedList<RefPtr<nsResolveHostCallback>>& aCallbacks,
MallocSizeOf mallocSizeOf)
{
size_t n = 0;
PRCList *curr = head->next;
while (curr != head) {
nsResolveHostCallback *callback =
static_cast<nsResolveHostCallback*>(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;
}

Expand All @@ -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());

Expand Down Expand Up @@ -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);
Expand All @@ -746,6 +745,7 @@ nsHostResolver::ResolveHost(const char *host,
if (!net_IsValidHostName(nsDependentCString(host)))
return NS_ERROR_UNKNOWN_HOST;

RefPtr<nsResolveHostCallback> callback(aCallback);
// if result is set inside the lock, then we need to issue the
// callback before returning.
RefPtr<nsHostRecord> result;
Expand Down Expand Up @@ -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);
Expand All @@ -976,7 +976,11 @@ nsHostResolver::ResolveHost(const char *host,
}
}
}

if (result) {
if (callback->isInList()) {
callback->remove();
}
callback->OnResolveHostComplete(this, result, status);
}

Expand All @@ -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<nsHostRecord> rec;
RefPtr<nsResolveHostCallback> callback(aCallback);

{
MutexAutoLock lock(mLock);

Expand All @@ -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<nsResolveHostCallback *>(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
Expand Down Expand Up @@ -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<RefPtr<nsResolveHostCallback>> cbs;

{
MutexAutoLock lock(mLock);

Expand All @@ -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..
Expand Down Expand Up @@ -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<nsResolveHostCallback *>(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);
Expand Down Expand Up @@ -1410,24 +1409,18 @@ nsHostResolver::CancelAsyncRequest(const char *host,
auto he = static_cast<nsHostDBEnt*>(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<nsResolveHostCallback *>(node);
if (callback && (callback->EqualsAsyncListener(aListener))) {
// Remove from the list of callbacks
PR_REMOVE_LINK(callback);

for (RefPtr<nsResolveHostCallback> 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) {
Expand Down
Loading

0 comments on commit d2697a4

Please sign in to comment.