Bug 641937 - Blacklist non-responding IP addresses in DNS r=bz

Blacklist non-responding IP addresses for a hostname so the next time
we access that hostname we don't have to wait for a timeout again

netwerk/base/src/nsSocketTransport2.cpp

@@ -1273,6 +1273,8 @@ nsSocketTransport::RecoverFromError()
 
     // try next ip address only if past the resolver stage...
     if (mState == STATE_CONNECTING && mDNSRecord) {
+        mDNSRecord->ReportUnusable(SocketPort());
+        
         nsresult rv = mDNSRecord->GetNextAddr(SocketPort(), &mNetAddr);
         if (NS_SUCCEEDED(rv)) {
             SOCKET_LOG(("  trying again with next ip address\n"));

netwerk/dns/nsDNSService2.cpp

@@ -80,6 +80,7 @@ public:
     nsDNSRecord(nsHostRecord *hostRecord)
         : mHostRecord(hostRecord)
         , mIter(nsnull)
+        , mLastIter(nsnull)
         , mIterGenCnt(-1)
         , mDone(PR_FALSE) {}
 
@@ -87,7 +88,9 @@ private:
     virtual ~nsDNSRecord() {}
 
     nsRefPtr<nsHostRecord>  mHostRecord;
-    void                   *mIter;
+    void                   *mIter;       // enum ptr for PR_EnumerateAddrInfo
+    void                   *mLastIter;   // previous enum ptr, for use in
+                                         // getting addrinfo in ReportUnusable
     int                     mIterGenCnt; // the generation count of
                                          // mHostRecord->addr_info when we
                                          // start iterating
@@ -107,7 +110,7 @@ nsDNSRecord::GetCanonicalName(nsACString &result)
     // host name is the IP address literal.
     const char *cname;
     {
-        MutexAutoLock lock(*mHostRecord->addr_info_lock);
+        MutexAutoLock lock(mHostRecord->addr_info_lock);
         if (mHostRecord->addr_info)
             cname = PR_GetCanonNameFromAddrInfo(mHostRecord->addr_info);
         else
@@ -126,7 +129,9 @@ nsDNSRecord::GetNextAddr(PRUint16 port, PRNetAddr *addr)
     if (mDone)
         return NS_ERROR_NOT_AVAILABLE;
 
-    mHostRecord->addr_info_lock->Lock();
+    mHostRecord->addr_info_lock.Lock();
+    PRBool startedFresh = !mIter;
+
     if (mHostRecord->addr_info) {
         if (!mIter)
             mIterGenCnt = mHostRecord->addr_info_gencnt;
@@ -135,16 +140,34 @@ nsDNSRecord::GetNextAddr(PRUint16 port, PRNetAddr *addr)
             // Restart the iteration.  Alternatively, we could just fail.
             mIter = nsnull;
             mIterGenCnt = mHostRecord->addr_info_gencnt;
+            startedFresh = PR_TRUE;
         }
-        mIter = PR_EnumerateAddrInfo(mIter, mHostRecord->addr_info, port, addr);
-        mHostRecord->addr_info_lock->Unlock();
+
+        do {
+            mLastIter = mIter;
+            mIter = PR_EnumerateAddrInfo(mIter, mHostRecord->addr_info,
+                                         port, addr);
+        }
+        while (mIter && mHostRecord->Blacklisted(addr));
+
+        if (startedFresh && !mIter) {
+            // if everything was blacklisted we want to reset the blacklist (and
+            // likely relearn it) and return the first address. That is better
+            // than nothing
+            mHostRecord->ResetBlacklist();
+            mLastIter = nsnull;
+            mIter = PR_EnumerateAddrInfo(nsnull, mHostRecord->addr_info,
+                                         port, addr);
+        }
+            
+        mHostRecord->addr_info_lock.Unlock();
         if (!mIter) {
             mDone = PR_TRUE;
             return NS_ERROR_NOT_AVAILABLE;
         }
     }
     else {
-        mHostRecord->addr_info_lock->Unlock();
+        mHostRecord->addr_info_lock.Unlock();
         if (!mHostRecord->addr) {
             // Both mHostRecord->addr_info and mHostRecord->addr are null.
             // This can happen if mHostRecord->addr_info expired and the
@@ -189,9 +212,11 @@ nsDNSRecord::HasMore(PRBool *result)
         // unfortunately, NSPR does not provide a way for us to determine if
         // there is another address other than to simply get the next address.
         void *iterCopy = mIter;
+        void *iterLastCopy = mLastIter;
         PRNetAddr addr;
         *result = NS_SUCCEEDED(GetNextAddr(0, &addr));
         mIter = iterCopy; // backup iterator
+        mLastIter = iterLastCopy; // backup iterator
         mDone = PR_FALSE;
     }
     return NS_OK;
@@ -201,11 +226,36 @@ NS_IMETHODIMP
 nsDNSRecord::Rewind()
 {
     mIter = nsnull;
+    mLastIter = nsnull;
     mIterGenCnt = -1;
     mDone = PR_FALSE;
     return NS_OK;
 }
 
+NS_IMETHODIMP
+nsDNSRecord::ReportUnusable(PRUint16 aPort)
+{
+    // right now we don't use the port in the blacklist
+
+    mHostRecord->addr_info_lock.Lock();
+
+    // Check that we are using a real addr_info (as opposed to a single
+    // constant address), and that the generation count is valid. Otherwise,
+    // ignore the report.
+
+    if (mHostRecord->addr_info &&
+        mIterGenCnt == mHostRecord->addr_info_gencnt) {
+        PRNetAddr addr;
+        void *id = PR_EnumerateAddrInfo(mLastIter, mHostRecord->addr_info,
+                                        aPort, &addr);
+        if (id)
+            mHostRecord->ReportUnusable(&addr);
+    }
+    
+    mHostRecord->addr_info_lock.Unlock();
+    return NS_OK;
+}
+
 //-----------------------------------------------------------------------------
 
 class nsDNSAsyncRequest : public nsResolveHostCallback

netwerk/dns/nsHostResolver.cpp

@@ -178,44 +178,95 @@ private:
 // host record (i.e., the flags that are passed down to PR_GetAddrInfoByName).
 #define RES_KEY_FLAGS(_f) ((_f) & nsHostResolver::RES_CANON_NAME)
 
+nsHostRecord::nsHostRecord(const nsHostKey *key)
+    : _refc(1)
+    , addr_info_lock("nsHostRecord.addr_info_lock")
+    , addr_info_gencnt(0)
+    , addr_info(nsnull)
+    , addr(nsnull)
+    , negative(PR_FALSE)
+    , resolving(PR_FALSE)
+    , onQueue(PR_FALSE)
+    , usingAnyThread(PR_FALSE)
+{
+    host = ((char *) this) + sizeof(nsHostRecord);
+    memcpy((char *) host, key->host, strlen(key->host) + 1);
+    flags = key->flags;
+    af = key->af;
+
+    NS_LOG_ADDREF(this, 1, "nsHostRecord", sizeof(nsHostRecord));
+    expiration = NowInMinutes();
+
+    PR_INIT_CLIST(this);
+    PR_INIT_CLIST(&callbacks);
+}
+
 nsresult
 nsHostRecord::Create(const nsHostKey *key, nsHostRecord **result)
 {
     size_t hostLen = strlen(key->host) + 1;
     size_t size = hostLen + sizeof(nsHostRecord);
 
-    nsHostRecord *rec = (nsHostRecord*) ::operator new(size);
-
-    rec->host = ((char *) rec) + sizeof(nsHostRecord);
-    rec->flags = key->flags;
-    rec->af = key->af;
-
-    rec->_refc = 1; // addref
-    NS_LOG_ADDREF(rec, 1, "nsHostRecord", sizeof(nsHostRecord));
-    rec->addr_info_lock = new Mutex("nsHostRecord.addr_info_lock");
-    rec->addr_info = nsnull;
-    rec->addr_info_gencnt = 0;
-    rec->addr = nsnull;
-    rec->expiration = NowInMinutes();
-    rec->resolving = PR_FALSE;
-    rec->onQueue = PR_FALSE;
-    rec->usingAnyThread = PR_FALSE;
-    PR_INIT_CLIST(rec);
-    PR_INIT_CLIST(&rec->callbacks);
-    rec->negative = PR_FALSE;
-    memcpy((char *) rec->host, key->host, hostLen);
-
-    *result = rec;
+    // Use placement new to create the object with room for the hostname
+    // allocated after it.
+    void *place = ::operator new(size);
+    *result = new(place) nsHostRecord(key);
     return NS_OK;
 }
 
 nsHostRecord::~nsHostRecord()
 {
-    delete addr_info_lock;
     if (addr)
         free(addr);
 }
 
+PRBool
+nsHostRecord::Blacklisted(PRNetAddr *aQuery)
+{
+    // must call locked
+    LOG(("nsHostRecord::Blacklisted() %p %s\n", this, host));
+
+    // skip the string conversion for the common case of no blacklist
+    if (!mBlacklistedItems.Length())
+        return PR_FALSE;
+    
+    char buf[64];
+    if (PR_NetAddrToString(aQuery, buf, sizeof(buf)) != PR_SUCCESS)
+        return PR_FALSE;
+
+    nsDependentCString strQuery(buf);
+    LOG(("nsHostRecord::Blacklisted() query %s\n", buf));
+    
+    for (PRUint32 i = 0; i < mBlacklistedItems.Length(); i++)
+        if (mBlacklistedItems.ElementAt(i).Equals(strQuery)) {
+            LOG(("nsHostRecord::Blacklisted() %s blacklist confirmed\n", buf));
+            return PR_TRUE;
+        }
+
+    return PR_FALSE;
+}
+
+void
+nsHostRecord::ReportUnusable(PRNetAddr *aAddress)
+{
+    // must call locked
+    LOG(("nsHostRecord::ReportUnusable() %p %s\n", this, host));
+
+    char buf[64];
+    if (PR_NetAddrToString(aAddress, buf, sizeof(buf)) == PR_SUCCESS) {
+        LOG(("nsHostrecord::ReportUnusable addr %s\n",buf));
+        mBlacklistedItems.AppendElement(nsCString(buf));
+    }
+}
+
+void
+nsHostRecord::ResetBlacklist()
+{
+    // must call locked
+    LOG(("nsHostRecord::ResetBlacklist() %p %s\n", this, host));
+    mBlacklistedItems.Clear();
+}
+
 //----------------------------------------------------------------------------
 
 struct nsHostDBEnt : PLDHashEntryHdr
@@ -787,7 +838,7 @@ nsHostResolver::OnLookupComplete(nsHostRecord *rec, nsresult status, PRAddrInfo
         // previous lookup result expired and we're reresolving it..
         PRAddrInfo  *old_addr_info;
         {
-            MutexAutoLock lock(*rec->addr_info_lock);
+            MutexAutoLock lock(rec->addr_info_lock);
             old_addr_info = rec->addr_info;
             rec->addr_info = result;
             rec->addr_info_gencnt++;

netwerk/dns/nsHostResolver.h

@@ -46,6 +46,8 @@
 #include "mozilla/CondVar.h"
 #include "mozilla/Mutex.h"
 #include "nsISupportsImpl.h"
+#include "nsString.h"
+#include "nsTArray.h"
 
 class nsHostResolver;
 class nsHostRecord;
@@ -111,7 +113,7 @@ public:
      * the other threads just read it.  therefore the resolver worker
      * thread doesn't need to lock when reading |addr_info|.
      */
-    Mutex       *addr_info_lock;
+    Mutex        addr_info_lock;
     int          addr_info_gencnt; /* generation count of |addr_info| */
     PRAddrInfo  *addr_info;
     PRNetAddr   *addr;
@@ -124,6 +126,11 @@ public:
 
     PRBool HasResult() const { return addr_info || addr || negative; }
 
+    // hold addr_info_lock when calling the blacklist functions
+    PRBool Blacklisted(PRNetAddr *query);
+    void   ResetBlacklist();
+    void   ReportUnusable(PRNetAddr *addr);
+
 private:
     friend class nsHostResolver;
 
@@ -135,8 +142,13 @@ private:
     
     PRBool  onQueue;  /* true if pending and on the queue (not yet given to getaddrinfo())*/
     PRBool  usingAnyThread; /* true if off queue and contributing to mActiveAnyThreadCount */
-    
 
+    // a list of addresses associated with this record that have been reported
+    // as unusable. the list is kept as a set of strings to make it independent
+    // of gencnt.
+    nsTArray<nsCString> mBlacklistedItems;
+
+    nsHostRecord(const nsHostKey *key);           /* use Create() instead */
    ~nsHostRecord();
 };
 

netwerk/dns/nsIDNSRecord.idl

@@ -46,7 +46,7 @@ native PRNetAddr(union PRNetAddr);
  * like an enumerator, allowing the caller to easily step through the
  * list of IP addresses.
  */
-[scriptable, uuid(31c9c52e-1100-457d-abac-d2729e43f506)]
+[scriptable, uuid(ead9e9d8-7eef-4dae-a7f0-a1edcfb20478)]
 interface nsIDNSRecord : nsISupports
 {
     /**
@@ -88,4 +88,14 @@ interface nsIDNSRecord : nsISupports
      * address in the record.
      */
     void rewind();
+
+    /**
+     * This function indicates that the last address obtained via getNextAddr*()
+     * was not usuable and should be skipped in future uses of this
+     * record if other addresses are available.
+     *
+     * @param aPort is the port number associated with the failure, if any.
+     *        It may be zero if not applicable.
+     */
+    void reportUnusable(in PRUint16 aPort);
 };

netwerk/socket/nsSOCKSIOLayer.cpp

@@ -276,7 +276,11 @@ nsSOCKSSocketInfo::ConnectToProxy(PRFileDesc *fd)
         }
     }
 
+    PRInt32 addresses = 0;
     do {
+        if (addresses++)
+            mDnsRec->ReportUnusable(mProxyPort);
+        
         rv = mDnsRec->GetNextAddr(mProxyPort, &mInternalProxyAddr);
         // No more addresses to try? If so, we'll need to bail
         if (NS_FAILED(rv)) {