Bug 337418: added a generation count for the addr_info field of nsHostRecord to detect the change of addr_info and invalidate addr_info's iterator, and added a lock to protect access to the addr_info and addr_info_gencnt fields. r=cbiesinger,sr=bzbarsky blocking1.9+ Modified files: nsDNSService2.cpp nsHostResolver.cpp nsHostResolver.h

netwerk/dns/src/nsDNSService2.cpp

@@ -73,6 +73,7 @@ public:
     nsDNSRecord(nsHostRecord *hostRecord)
         : mHostRecord(hostRecord)
         , mIter(nsnull)
+        , mIterGenCnt(-1)
         , mDone(PR_FALSE) {}
 
 private:
@@ -80,6 +81,9 @@ private:
 
     nsRefPtr<nsHostRecord>  mHostRecord;
     void                   *mIter;
+    int                     mIterGenCnt; // the generation count of
+                                         // mHostRecord->addr_info when we
+                                         // start iterating
     PRBool                  mDone;
 };
 
@@ -95,11 +99,13 @@ nsDNSRecord::GetCanonicalName(nsACString &result)
     // if the record is for an IP address literal, then the canonical
     // host name is the IP address literal.
     const char *cname;
+    PR_Lock(mHostRecord->addr_info_lock);
     if (mHostRecord->addr_info)
         cname = PR_GetCanonNameFromAddrInfo(mHostRecord->addr_info);
     else
         cname = mHostRecord->host;
     result.Assign(cname);
+    PR_Unlock(mHostRecord->addr_info_lock);
     return NS_OK;
 }
 
@@ -112,16 +118,26 @@ nsDNSRecord::GetNextAddr(PRUint16 port, PRNetAddr *addr)
     if (mDone)
         return NS_ERROR_NOT_AVAILABLE;
 
+    PR_Lock(mHostRecord->addr_info_lock);
     if (mHostRecord->addr_info) {
-        mIter = PR_EnumerateAddrInfo(mIter, mHostRecord->addr_info, port, addr);
         if (!mIter)
+            mIterGenCnt = mHostRecord->addr_info_gencnt;
+        else if (mIterGenCnt != mHostRecord->addr_info_gencnt) {
+            // mHostRecord->addr_info has changed, so mIter is invalid.
+            // Restart the iteration.  Alternatively, we could just fail.
+            mIter = nsnull;
+            mIterGenCnt = mHostRecord->addr_info_gencnt;
+        }
+        mIter = PR_EnumerateAddrInfo(mIter, mHostRecord->addr_info, port, addr);
+        PR_Unlock(mHostRecord->addr_info_lock);
+        if (!mIter) {
+            mDone = PR_TRUE;
             return NS_ERROR_NOT_AVAILABLE;
+        }
     }
     else {
-        // This should never be null (but see bug 290190) :-(
-        NS_ENSURE_STATE(mHostRecord->addr);
-
-        mIter = nsnull; // no iterations
+        PR_Unlock(mHostRecord->addr_info_lock);
+        NS_ASSERTION(mHostRecord->addr, "no addr");
         memcpy(addr, mHostRecord->addr, sizeof(PRNetAddr));
         // set given port
         port = PR_htons(port);
@@ -129,9 +145,9 @@ nsDNSRecord::GetNextAddr(PRUint16 port, PRNetAddr *addr)
             addr->inet.port = port;
         else
             addr->ipv6.port = port;
+        mDone = PR_TRUE; // no iterations
     }
         
-    mDone = !mIter;
     return NS_OK; 
 }
 
@@ -172,6 +188,7 @@ NS_IMETHODIMP
 nsDNSRecord::Rewind()
 {
     mIter = nsnull;
+    mIterGenCnt = -1;
     mDone = PR_FALSE;
     return NS_OK;
 }

netwerk/dns/src/nsHostResolver.cpp

@@ -159,12 +159,18 @@ private:
 nsresult
 nsHostRecord::Create(const nsHostKey *key, nsHostRecord **result)
 {
+    PRLock *lock = PR_NewLock();
+    if (!lock)
+        return NS_ERROR_OUT_OF_MEMORY;
+
     size_t hostLen = strlen(key->host) + 1;
     size_t size = hostLen + sizeof(nsHostRecord);
 
     nsHostRecord *rec = (nsHostRecord*) ::operator new(size);
-    if (!rec)
+    if (!rec) {
+        PR_DestroyLock(lock);
         return NS_ERROR_OUT_OF_MEMORY;
+    }
 
     rec->host = ((char *) rec) + sizeof(nsHostRecord);
     rec->flags = RES_KEY_FLAGS(key->flags);
@@ -172,7 +178,9 @@ nsHostRecord::Create(const nsHostKey *key, nsHostRecord **result)
 
     rec->_refc = 1; // addref
     NS_LOG_ADDREF(rec, 1, "nsHostRecord", sizeof(nsHostRecord));
+    rec->addr_info_lock = lock;
     rec->addr_info = nsnull;
+    rec->addr_info_gencnt = 0;
     rec->addr = nsnull;
     rec->expiration = NowInMinutes();
     rec->resolving = PR_FALSE;
@@ -186,6 +194,8 @@ nsHostRecord::Create(const nsHostKey *key, nsHostRecord **result)
 
 nsHostRecord::~nsHostRecord()
 {
+    if (addr_info_lock)
+        PR_DestroyLock(addr_info_lock);
     if (addr_info)
         PR_FreeAddrInfo(addr_info);
     if (addr)
@@ -614,9 +624,14 @@ nsHostResolver::OnLookupComplete(nsHostRecord *rec, nsresult status, PRAddrInfo
 
         // update record fields.  We might have a rec->addr_info already if a
         // previous lookup result expired and we're reresolving it..
-	if (rec->addr_info)
-	  PR_FreeAddrInfo(rec->addr_info);
+        PRAddrInfo  *old_addr_info;
+        PR_Lock(rec->addr_info_lock);
+        old_addr_info = rec->addr_info;
         rec->addr_info = result;
+        rec->addr_info_gencnt++;
+        PR_Unlock(rec->addr_info_lock);
+        if (old_addr_info)
+            PR_FreeAddrInfo(old_addr_info);
         rec->expiration = NowInMinutes() + mMaxCacheLifetime;
         rec->resolving = PR_FALSE;
         

netwerk/dns/src/nsHostResolver.h

@@ -95,7 +95,17 @@ public:
      * |af| is the address family of the record we are querying for.
      */
 
+    /* the lock protects |addr_info| and |addr_info_gencnt| because they
+     * are mutable and accessed by the resolver worker thread and the
+     * nsDNSService2 class.  |addr| doesn't change after it has been
+     * assigned a value.  only the resolver worker thread modifies
+     * nsHostRecord (and only in nsHostResolver::OnLookupComplete);
+     * the other threads just read it.  therefore the resolver worker
+     * thread doesn't need to lock when reading |addr_info|.
+     */
+    PRLock      *addr_info_lock;
     PRAddrInfo  *addr_info;
+    int          addr_info_gencnt; /* generation count of |addr_info| */
     PRNetAddr   *addr;
     PRUint32     expiration; /* measured in minutes since epoch */