Bug 1124973 (part 3) - Use PL_DHashTableSearch() in nsHostResolver.cpp. r=froydnj,sworkman.
authorNicholas Nethercote <nnethercote@mozilla.com>
Thu, 22 Jan 2015 21:25:44 -0800
changeset 239565 ec10af0d21816723e3fbae6949dd2f12eb1e3092
parent 239564 9e2dbe7f144bd4ebcacf6c8ef05912cf9efbcf37
child 239566 fc7c269f2b7a7f2a9b55554c6961ccd156b475cf
push id500
push userjoshua.m.grant@gmail.com
push dateThu, 29 Jan 2015 01:48:36 +0000
reviewersfroydnj, sworkman
bugs1124973
milestone38.0a1
Bug 1124973 (part 3) - Use PL_DHashTableSearch() in nsHostResolver.cpp. r=froydnj,sworkman. Currently nsHostResolver.cpp uses PL_DHashTableLookup() and fails to use PL_DHASH_ENTRY_IS_{FREE,BUSY} on the result the way it should. However, I think it gets away with this because it always does this on the result: if (!he || !he->rec) { /* do stuff with |he->rec| */ } The |!he| test is useless and always fails, but the |!he->rec| does the right thing because (a) entry storage is always zeroed when the table is created, (b) HostDB_ClearEntry() zeroes the |rec| field (via NS_RELEASE). So unused entries always have a null |rec| field. Furthermore, |he->rec| is never zero in a used entry because HostDB_InitEntry always assigns it with a nsHostRecord assigned with infallible new in nsHostRecord::Create (and there are existing assertions to this effect). All this means that when this patch switches PL_DHashTableLookup to PL_DHashTableSearch it can drop the |!he->rec| test and just do this: if (!he) { /* do stuff with |he->rec| */ } Finally, there's a comment about HostDB_InitEntry failing which is bogus because HostDB_InitEntry cannot fail. This patch fixes that too.
netwerk/dns/nsHostResolver.cpp
--- a/netwerk/dns/nsHostResolver.cpp
+++ b/netwerk/dns/nsHostResolver.cpp
@@ -753,18 +753,18 @@ nsHostResolver::ResolveHost(const char  
             // any pending callbacks, then add to pending callbacks queue,
             // and return.  otherwise, add ourselves as first pending
             // callback, and proceed to do the lookup.
 
             nsHostKey key = { host, flags, af };
             nsHostDBEnt *he = static_cast<nsHostDBEnt *>
                                          (PL_DHashTableAdd(&mDB, &key));
 
-            // if the record is null, then HostDB_InitEntry failed.
-            if (!he || !he->rec) {
+            // if the record is null, the hash table OOM'd.
+            if (!he) {
                 LOG(("  Out of memory: no cache entry for [%s].\n", host));
                 rv = NS_ERROR_OUT_OF_MEMORY;
             }
             // do we have a cached result that we can reuse?
             else if (!(flags & RES_BYPASS_CACHE) &&
                      he->rec->HasUsableResult(TimeStamp::NowLoRes(), flags)) {
                 LOG(("  Using cached record for host [%s].\n", host));
                 // put reference to host record on stack...
@@ -829,17 +829,16 @@ nsHostResolver::ResolveHost(const char  
                     // First, search for an entry with AF_UNSPEC
                     const nsHostKey unspecKey = { host, flags, PR_AF_UNSPEC };
                     nsHostDBEnt *unspecHe = static_cast<nsHostDBEnt *>
                         (PL_DHashTableSearch(&mDB, &unspecKey));
                     NS_ASSERTION(!unspecHe ||
                                  (unspecHe && unspecHe->rec),
                                 "Valid host entries should contain a record");
                     if (unspecHe &&
-                        unspecHe->rec &&
                         unspecHe->rec->HasUsableResult(TimeStamp::NowLoRes(), flags)) {
 
                         MOZ_ASSERT(unspecHe->rec->addr_info || unspecHe->rec->negative,
                                    "Entry should be resolved or negative.");
 
                         LOG(("  Trying AF_UNSPEC entry for [%s] af: %s.\n",
                             host, (af == PR_AF_INET) ? "AF_INET" : "AF_INET6"));
 
@@ -951,18 +950,18 @@ nsHostResolver::DetachCallback(const cha
                                nsresult               status)
 {
     nsRefPtr<nsHostRecord> rec;
     {
         MutexAutoLock lock(mLock);
 
         nsHostKey key = { host, flags, af };
         nsHostDBEnt *he = static_cast<nsHostDBEnt *>
-                                     (PL_DHashTableLookup(&mDB, &key));
-        if (he && he->rec) {
+                                     (PL_DHashTableSearch(&mDB, &key));
+        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);
                     rec = he->rec;
                     break;
@@ -1299,18 +1298,18 @@ nsHostResolver::CancelAsyncRequest(const
                                    nsresult               status)
 
 {
     MutexAutoLock lock(mLock);
 
     // Lookup the host record associated with host, flags & address family
     nsHostKey key = { host, flags, af };
     nsHostDBEnt *he = static_cast<nsHostDBEnt *>
-                      (PL_DHashTableLookup(&mDB, &key));
-    if (he && he->rec) {
+                      (PL_DHashTableSearch(&mDB, &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))) {