From 2b45f66a5b0659ee5d5fac777a2c06c76be140d3 Mon Sep 17 00:00:00 2001 From: Patrick McManus Date: Thu, 21 Jul 2011 09:18:01 -0400 Subject: [PATCH 01/43] 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 | 2 + netwerk/dns/nsDNSService2.cpp | 62 ++++++++++++++-- netwerk/dns/nsHostResolver.cpp | 99 +++++++++++++++++++------ netwerk/dns/nsHostResolver.h | 16 +++- netwerk/dns/nsIDNSRecord.idl | 12 ++- netwerk/socket/nsSOCKSIOLayer.cpp | 4 + 6 files changed, 162 insertions(+), 33 deletions(-) diff --git a/netwerk/base/src/nsSocketTransport2.cpp b/netwerk/base/src/nsSocketTransport2.cpp index 50291cedc73..99927f93e06 100644 --- a/netwerk/base/src/nsSocketTransport2.cpp +++ b/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")); diff --git a/netwerk/dns/nsDNSService2.cpp b/netwerk/dns/nsDNSService2.cpp index 72a2c7f7f73..d14e9db44b7 100644 --- a/netwerk/dns/nsDNSService2.cpp +++ b/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 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 diff --git a/netwerk/dns/nsHostResolver.cpp b/netwerk/dns/nsHostResolver.cpp index 51ad82322f9..2e6c88970df 100644 --- a/netwerk/dns/nsHostResolver.cpp +++ b/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++; diff --git a/netwerk/dns/nsHostResolver.h b/netwerk/dns/nsHostResolver.h index 7c74f89e89d..0e0eead002c 100644 --- a/netwerk/dns/nsHostResolver.h +++ b/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 mBlacklistedItems; + + nsHostRecord(const nsHostKey *key); /* use Create() instead */ ~nsHostRecord(); }; diff --git a/netwerk/dns/nsIDNSRecord.idl b/netwerk/dns/nsIDNSRecord.idl index 4e6d68fae80..3a2b769f5b6 100644 --- a/netwerk/dns/nsIDNSRecord.idl +++ b/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); }; diff --git a/netwerk/socket/nsSOCKSIOLayer.cpp b/netwerk/socket/nsSOCKSIOLayer.cpp index 4d3a4e87785..be72d73bb9d 100644 --- a/netwerk/socket/nsSOCKSIOLayer.cpp +++ b/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)) { From 35aaeb0cf4dddaa9536d16eeaeed614df9eff3bf Mon Sep 17 00:00:00 2001 From: Mounir Lamouri Date: Tue, 19 Jul 2011 11:36:51 -0700 Subject: [PATCH 02/43] Bug 669580 - Add a reflectBool method to reflect.js. r=Ms2ger --- .../forms/test_formnovalidate_attribute.html | 1 - .../test_input_attributes_reflection.html | 55 ++++++++-- .../test/forms/test_required_attribute.html | 3 - content/html/content/test/reflect.js | 102 ++++++++++++++++++ content/html/content/test/test_bug546995.html | 2 - 5 files changed, 148 insertions(+), 15 deletions(-) diff --git a/content/html/content/test/forms/test_formnovalidate_attribute.html b/content/html/content/test/forms/test_formnovalidate_attribute.html index 81138afaeb8..180eb81cdcf 100644 --- a/content/html/content/test/forms/test_formnovalidate_attribute.html +++ b/content/html/content/test/forms/test_formnovalidate_attribute.html @@ -75,7 +75,6 @@ function checkFormNoValidateAttribute(aElementName) "formnovalidate attribute should be disabled"); } -checkFormNoValidateAttribute('input'); checkFormNoValidateAttribute('button'); netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect'); diff --git a/content/html/content/test/forms/test_input_attributes_reflection.html b/content/html/content/test/forms/test_input_attributes_reflection.html index 9a0552b7c1d..81e29efcd7a 100644 --- a/content/html/content/test/forms/test_input_attributes_reflection.html +++ b/content/html/content/test/forms/test_input_attributes_reflection.html @@ -40,11 +40,28 @@ reflectLimitedEnumerated({ invalidValues: [ "", "default", "foo", "tulip" ], }); -// TODO: autofocus (boolean) -// TODO: defaultChecked (boolean) -// TODO: checked (boolean) +// .autofocus +reflectBoolean({ + element: document.createElement("input"), + attribute: "autofocus", +}); + +// .defaultChecked +reflectBoolean({ + element: document.createElement("input"), + attribute: { idl: "defaultChecked", content: "checked" }, +}); + +// .checked doesn't reflect a content attribute. + // TODO: dirName (not implemented) -// TODO: disabled (boolean) + +// .disabled +reflectBoolean({ + element: document.createElement("input"), + attribute: "disabled", +}); + // TODO: form (HTMLFormElement) // TODO: files (FileList) // TODO: formAction (URL) @@ -68,7 +85,11 @@ reflectLimitedEnumerated({ defaultValue: "get" }); -// TODO: formNoValidate (boolean) +// .formNoValidate +reflectBoolean({ + element: document.createElement("input"), + attribute: "formNoValidate", +}); // .formTarget reflectString({ @@ -78,12 +99,19 @@ reflectString({ }); // TODO: height (non-negative integer) -// TODO: indeterminate (boolean) + +// .indeterminate doesn't reflect a content attribute. + // TODO: list (HTMLElement) // TODO: max (not implemented) // TODO: maxLength (long) // TODO: min (not implemented) -// TODO: multiple (boolean) + +// .multiple +reflectBoolean({ + element: document.createElement("input"), + attribute: "multiple", +}); // .name reflectString({ @@ -106,8 +134,17 @@ reflectString({ otherValues: [ "foo\nbar", "foo\rbar", "foo\r\nbar" ], }); -// TODO: readOnly (boolean) -// TODO: required (boolean) +// .readOnly +reflectBoolean({ + element: document.createElement("input"), + attribute: "readOnly", +}); + +// .required +reflectBoolean({ + element: document.createElement("input"), + attribute: "required", +}); // .size reflectUnsignedInt({ diff --git a/content/html/content/test/forms/test_required_attribute.html b/content/html/content/test/forms/test_required_attribute.html index 5e1391ea720..f9621424efc 100644 --- a/content/html/content/test/forms/test_required_attribute.html +++ b/content/html/content/test/forms/test_required_attribute.html @@ -327,9 +327,6 @@ function checkInputRequiredValidityForFile() checkRequiredAttribute(document.createElement('textarea')); checkTextareaRequiredValidity(); -// Every input element should have the required attribute. -checkRequiredAttribute(document.createElement('input')); - // The require attribute behavior depend of the input type. // First of all, checks for types that make the element barred from // constraint validation. diff --git a/content/html/content/test/reflect.js b/content/html/content/test/reflect.js index 0578a49de50..2ca84f8bf3e 100644 --- a/content/html/content/test/reflect.js +++ b/content/html/content/test/reflect.js @@ -334,3 +334,105 @@ function reflectLimitedEnumerated(aParameters) }); } +/** + * Checks that a given attribute is correctly reflected as a boolean. + * + * @param aParameters Object object containing the parameters, which are: + * - element Element node to test on + * - attribute String name of the attribute + * OR + * attribute Object object containing two attributes, 'content' and 'idl' + */ +function reflectBoolean(aParameters) +{ + var element = aParameters.element; + var contentAttr = typeof aParameters.attribute === "string" + ? aParameters.attribute : aParameters.attribute.content; + var idlAttr = typeof aParameters.attribute === "string" + ? aParameters.attribute : aParameters.attribute.idl; + + ok(idlAttr in element, + idlAttr + " should be an IDL attribute of this element"); + is(typeof element[idlAttr], "boolean", + idlAttr + " IDL attribute should be a boolean"); + + // Tests when the attribute isn't set. + is(element.getAttribute(contentAttr), null, + "When not set, the content attribute should be null."); + is(element[idlAttr], false, + "When not set, the IDL attribute should return false"); + + /** + * Test various values. + * Each value to test is actually an object containing a 'value' property + * containing the value to actually test, a 'stringified' property containing + * the stringified value and a 'result' property containing the expected + * result when the value is set to the IDL attribute. + */ + var valuesToTest = [ + { value: true, stringified: "true", result: true }, + { value: false, stringified: "false", result: false }, + { value: "true", stringified: "true", result: true }, + { value: "false", stringified: "false", result: true }, + { value: "foo", stringified: "foo", result: true }, + { value: idlAttr, stringified: idlAttr, result: true }, + { value: contentAttr, stringified: contentAttr, result: true }, + { value: "null", stringified: "null", result: true }, + { value: "undefined", stringified: "undefined", result: true }, + { value: "", stringified: "", result: false }, + { value: undefined, stringified: "undefined", result: false }, + { value: null, stringified: "null", result: false }, + { value: +0, stringified: "0", result: false }, + { value: -0, stringified: "0", result: false }, + { value: NaN, stringified: "NaN", result: false }, + { value: 42, stringified: "42", result: true }, + { value: Infinity, stringified: "Infinity", result: true }, + { value: -Infinity, stringified: "-Infinity", result: true }, + // ES5, verse 9.2. + { value: { toString: function() { return "foo" } }, stringified: "foo", + result: true }, + { value: { valueOf: function() { return "foo" } }, + stringified: "[object Object]", result: true }, + { value: { valueOf: function() { return "quux" }, toString: undefined }, + stringified: "quux", result: true }, + { value: { valueOf: function() { return "foo" }, + toString: function() { return "bar" } }, stringified: "bar", + result: true }, + { value: { valueOf: function() { return false } }, + stringified: "[object Object]", result: true }, + { value: { foo: false, bar: false }, stringified: "[object Object]", + result: true }, + { value: { }, stringified: "[object Object]", result: true }, + ]; + + valuesToTest.forEach(function(v) { + element.setAttribute(contentAttr, v.value); + is(element[idlAttr], true, + "IDL attribute should return always return 'true' if the content attribute has been set"); + if (v.value === null) { + // bug 667856 + todo(element.getAttribute(contentAttr), v.stringified, + "Content attribute should return the stringified value it has been set to."); + } else { + is(element.getAttribute(contentAttr), v.stringified, + "Content attribute should return the stringified value it has been set to."); + } + element.removeAttribute(contentAttr); + + element[idlAttr] = v.value; + is(element[idlAttr], v.result, "IDL attribute should return " + v.result); + is(element.getAttribute(contentAttr), v.result ? "" : null, + v.result ? "Content attribute should return the empty string." + : "Content attribute should return null."); + is(element.hasAttribute(contentAttr), v.result, + v.result ? contentAttr + " should not be present" + : contentAttr + " should be present"); + element.removeAttribute(contentAttr); + }); + + // Tests after removeAttribute() is called. Should be equivalent with not set. + is(element.getAttribute(contentAttr), null, + "When not set, the content attribute should be null."); + is(element[contentAttr], false, + "When not set, the IDL attribute should return false"); +} diff --git a/content/html/content/test/test_bug546995.html b/content/html/content/test/test_bug546995.html index dfc66b22f11..399c5b52186 100644 --- a/content/html/content/test/test_bug546995.html +++ b/content/html/content/test/test_bug546995.html @@ -13,7 +13,6 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=546995 Mozilla Bug 546995