From 5c1e52872453a4cd84e13746c884dbc87a1b26b5 Mon Sep 17 00:00:00 2001 From: Dragana Damjanovic Date: Tue, 26 Aug 2014 05:09:00 -0400 Subject: [PATCH] Bug 947721 - Fix function CancelAsyncRequest and add the same function for the child process. r=sworkman --HG-- extra : rebase_source : b89d296732cd65e8bfc0c6ff563095a668484eae --- netwerk/dns/ChildDNSService.cpp | 80 +++++++++++++++- netwerk/dns/ChildDNSService.h | 14 +++ netwerk/dns/DNSListenerProxy.cpp | 11 ++- netwerk/dns/DNSListenerProxy.h | 5 +- netwerk/dns/DNSRequestChild.cpp | 59 +++++++++++- netwerk/dns/DNSRequestChild.h | 15 ++- netwerk/dns/DNSRequestParent.cpp | 28 +++++- netwerk/dns/DNSRequestParent.h | 7 ++ netwerk/dns/PDNSRequest.ipdl | 8 +- netwerk/dns/nsDNSService2.cpp | 6 ++ netwerk/dns/nsIDNSListener.idl | 16 ++++ netwerk/test/unit/test_dns_cancel.js | 91 +++++++++++++++++++ netwerk/test/unit/xpcshell.ini | 1 + netwerk/test/unit_ipc/test_dns_cancel_wrap.js | 7 ++ netwerk/test/unit_ipc/xpcshell.ini | 1 + 15 files changed, 325 insertions(+), 24 deletions(-) create mode 100644 netwerk/test/unit/test_dns_cancel.js create mode 100644 netwerk/test/unit_ipc/test_dns_cancel_wrap.js diff --git a/netwerk/dns/ChildDNSService.cpp b/netwerk/dns/ChildDNSService.cpp index b17e0374eb1..4de1ffdf7ad 100644 --- a/netwerk/dns/ChildDNSService.cpp +++ b/netwerk/dns/ChildDNSService.cpp @@ -11,7 +11,6 @@ #include "nsIPrefService.h" #include "nsIProtocolProxyService.h" #include "mozilla/net/NeckoChild.h" -#include "mozilla/net/DNSRequestChild.h" #include "mozilla/net/DNSListenerProxy.h" namespace mozilla { @@ -44,6 +43,7 @@ NS_IMPL_ISUPPORTS(ChildDNSService, ChildDNSService::ChildDNSService() : mFirstTime(true) , mOffline(false) + , mPendingRequestsLock("DNSPendingRequestsLock") { MOZ_ASSERT(IsNeckoChild()); } @@ -53,6 +53,17 @@ ChildDNSService::~ChildDNSService() } +void +ChildDNSService::GetDNSRecordHashKey(const nsACString &aHost, + uint32_t aFlags, + nsIDNSListener* aListener, + nsACString &aHashKey) +{ + aHashKey.Assign(aHost); + aHashKey.AppendInt(aFlags); + aHashKey.AppendPrintf("%p", aListener); +} + //----------------------------------------------------------------------------- // ChildDNSService::nsIDNSService //----------------------------------------------------------------------------- @@ -70,12 +81,18 @@ ChildDNSService::AsyncResolve(const nsACString &hostname, return NS_ERROR_DNS_LOOKUP_QUEUE_FULL; } + // We need original flags for the pending requests hash. + uint32_t originalFlags = flags; + // Support apps being 'offline' even if parent is not: avoids DNS traffic by // apps that have been told they are offline. if (mOffline) { flags |= RESOLVE_OFFLINE; } + // We need original listener for the pending requests hash. + nsIDNSListener *originalListener = listener; + // make sure JS callers get notification on the main thread nsCOMPtr target = target_; nsCOMPtr wrappedListener = do_QueryInterface(listener); @@ -93,6 +110,20 @@ ChildDNSService::AsyncResolve(const nsACString &hostname, nsRefPtr childReq = new DNSRequestChild(nsCString(hostname), flags, listener, target); + { + MutexAutoLock lock(mPendingRequestsLock); + nsCString key; + GetDNSRecordHashKey(hostname, originalFlags, originalListener, key); + nsTArray> *hashEntry; + if (mPendingRequests.Get(key, &hashEntry)) { + hashEntry->AppendElement(childReq); + } else { + hashEntry = new nsTArray>(); + hashEntry->AppendElement(childReq); + mPendingRequests.Put(key, hashEntry); + } + } + childReq->StartRequest(); childReq.forget(result); @@ -109,10 +140,16 @@ ChildDNSService::CancelAsyncResolve(const nsACString &aHostname, return NS_ERROR_DNS_LOOKUP_QUEUE_FULL; } - // TODO: keep a hashtable of pending requests, so we can obey cancel semantics - // (call OnLookupComplete with aReason). Also possible we could send IPDL to - // parent to cancel. - return NS_ERROR_NOT_AVAILABLE; + MutexAutoLock lock(mPendingRequestsLock); + nsTArray> *hashEntry; + nsCString key; + GetDNSRecordHashKey(aHostname, aFlags, aListener, key); + if (mPendingRequests.Get(key, &hashEntry)) { + // We cancel just one. + hashEntry->ElementAt(0)->Cancel(aReason); + } + + return NS_OK; } NS_IMETHODIMP @@ -140,6 +177,39 @@ ChildDNSService::GetMyHostName(nsACString &result) return NS_ERROR_NOT_AVAILABLE; } +void +ChildDNSService::NotifyRequestDone(DNSRequestChild *aDnsRequest) +{ + // We need the original flags and listener for the pending requests hash. + uint32_t originalFlags = aDnsRequest->mFlags & ~RESOLVE_OFFLINE; + nsCOMPtr originalListener = aDnsRequest->mListener; + nsCOMPtr wrapper = do_QueryInterface(originalListener); + if (wrapper) { + wrapper->GetOriginalListener(getter_AddRefs(originalListener)); + if (NS_WARN_IF(!originalListener)) { + MOZ_ASSERT(originalListener); + return; + } + } + + MutexAutoLock lock(mPendingRequestsLock); + + nsCString key; + GetDNSRecordHashKey(aDnsRequest->mHost, originalFlags, originalListener, key); + + nsTArray> *hashEntry; + + if (mPendingRequests.Get(key, &hashEntry)) { + int idx; + if ((idx = hashEntry->IndexOf(aDnsRequest))) { + hashEntry->RemoveElementAt(idx); + if (hashEntry->IsEmpty()) { + mPendingRequests.Remove(key); + } + } + } +} + //----------------------------------------------------------------------------- // ChildDNSService::nsPIDNSService //----------------------------------------------------------------------------- diff --git a/netwerk/dns/ChildDNSService.h b/netwerk/dns/ChildDNSService.h index 829c31820e8..3e04828cb74 100644 --- a/netwerk/dns/ChildDNSService.h +++ b/netwerk/dns/ChildDNSService.h @@ -11,6 +11,10 @@ #include "nsPIDNSService.h" #include "nsIObserver.h" #include "mozilla/Attributes.h" +#include "mozilla/Mutex.h" +#include "DNSRequestChild.h" +#include "nsHashKeys.h" +#include "nsClassHashtable.h" namespace mozilla { namespace net { @@ -30,12 +34,22 @@ public: static ChildDNSService* GetSingleton(); + void NotifyRequestDone(DNSRequestChild *aDnsRequest); private: virtual ~ChildDNSService(); + void MOZ_ALWAYS_INLINE GetDNSRecordHashKey(const nsACString &aHost, + uint32_t aFlags, + nsIDNSListener* aListener, + nsACString &aHashKey); + bool mFirstTime; bool mOffline; bool mDisablePrefetch; + + // We need to remember pending dns requests to be able to cancel them. + nsClassHashtable>> mPendingRequests; + Mutex mPendingRequestsLock; }; } // namespace net diff --git a/netwerk/dns/DNSListenerProxy.cpp b/netwerk/dns/DNSListenerProxy.cpp index 34aab680ada..463040593fc 100644 --- a/netwerk/dns/DNSListenerProxy.cpp +++ b/netwerk/dns/DNSListenerProxy.cpp @@ -11,7 +11,9 @@ namespace mozilla { namespace net { -NS_IMPL_ISUPPORTS(DNSListenerProxy, nsIDNSListener) +NS_IMPL_ISUPPORTS(DNSListenerProxy, + nsIDNSListener, + nsIDNSListenerProxy) NS_IMETHODIMP DNSListenerProxy::OnLookupComplete(nsICancelable* aRequest, @@ -30,7 +32,12 @@ DNSListenerProxy::OnLookupCompleteRunnable::Run() return NS_OK; } - +NS_IMETHODIMP +DNSListenerProxy::GetOriginalListener(nsIDNSListener **aOriginalListener) +{ + NS_IF_ADDREF(*aOriginalListener = mListener); + return NS_OK; +} } // namespace net } // namespace mozilla diff --git a/netwerk/dns/DNSListenerProxy.h b/netwerk/dns/DNSListenerProxy.h index 3c651ba29e9..f9f021f836f 100644 --- a/netwerk/dns/DNSListenerProxy.h +++ b/netwerk/dns/DNSListenerProxy.h @@ -18,7 +18,9 @@ class nsICancelable; namespace mozilla { namespace net { -class DNSListenerProxy MOZ_FINAL : public nsIDNSListener +class DNSListenerProxy MOZ_FINAL + : public nsIDNSListener + , public nsIDNSListenerProxy { public: DNSListenerProxy(nsIDNSListener* aListener, nsIEventTarget* aTargetThread) @@ -32,6 +34,7 @@ public: NS_DECL_THREADSAFE_ISUPPORTS NS_DECL_NSIDNSLISTENER + NS_DECL_NSIDNSLISTENERPROXY class OnLookupCompleteRunnable : public nsRunnable { diff --git a/netwerk/dns/DNSRequestChild.cpp b/netwerk/dns/DNSRequestChild.cpp index d2660cf2817..25edefa2e19 100644 --- a/netwerk/dns/DNSRequestChild.cpp +++ b/netwerk/dns/DNSRequestChild.cpp @@ -4,8 +4,10 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ +#include "mozilla/net/ChildDNSService.h" #include "mozilla/net/DNSRequestChild.h" #include "mozilla/net/NeckoChild.h" +#include "mozilla/unused.h" #include "nsIDNSRecord.h" #include "nsHostResolver.h" #include "nsTArray.h" @@ -146,6 +148,32 @@ ChildDNSRecord::ReportUnusable(uint16_t aPort) return NS_OK; } +//----------------------------------------------------------------------------- +// CancelDNSRequestEvent +//----------------------------------------------------------------------------- + +class CancelDNSRequestEvent : public nsRunnable +{ +public: + CancelDNSRequestEvent(DNSRequestChild* aDnsReq, nsresult aReason) + : mDnsRequest(aDnsReq) + , mReasonForCancel(aReason) + {} + + NS_IMETHOD Run() + { + if (mDnsRequest->mIPCOpen) { + // Send request to Parent process. + mDnsRequest->SendCancelDNSRequest(mDnsRequest->mHost, mDnsRequest->mFlags, + mReasonForCancel); + } + return NS_OK; + } +private: + nsRefPtr mDnsRequest; + nsresult mReasonForCancel; +}; + //----------------------------------------------------------------------------- // DNSRequestChild //----------------------------------------------------------------------------- @@ -159,6 +187,7 @@ DNSRequestChild::DNSRequestChild(const nsCString& aHost, , mResultStatus(NS_OK) , mHost(aHost) , mFlags(aFlags) + , mIPCOpen(false) { } @@ -174,6 +203,7 @@ DNSRequestChild::StartRequest() // Send request to Parent process. gNeckoChild->SendPDNSRequestConstructor(this, mHost, mFlags); + mIPCOpen = true; // IPDL holds a reference until IPDL channel gets destroyed AddIPDLReference(); @@ -183,13 +213,13 @@ void DNSRequestChild::CallOnLookupComplete() { MOZ_ASSERT(mListener); - mListener->OnLookupComplete(this, mResultRecord, mResultStatus); } bool -DNSRequestChild::Recv__delete__(const DNSRequestResponse& reply) +DNSRequestChild::RecvLookupCompleted(const DNSRequestResponse& reply) { + mIPCOpen = false; MOZ_ASSERT(mListener); switch (reply.type()) { @@ -223,9 +253,28 @@ DNSRequestChild::Recv__delete__(const DNSRequestResponse& reply) mTarget->Dispatch(event, NS_DISPATCH_NORMAL); } + unused << Send__delete__(this); + return true; } +void +DNSRequestChild::ReleaseIPDLReference() +{ + // Request is done or destroyed. Remove it from the hash table. + nsRefPtr dnsServiceChild = + dont_AddRef(ChildDNSService::GetSingleton()); + dnsServiceChild->NotifyRequestDone(this); + + Release(); +} + +void +DNSRequestChild::ActorDestroy(ActorDestroyReason why) +{ + mIPCOpen = false; +} + //----------------------------------------------------------------------------- // DNSRequestChild::nsISupports //----------------------------------------------------------------------------- @@ -240,7 +289,11 @@ NS_IMPL_ISUPPORTS(DNSRequestChild, NS_IMETHODIMP DNSRequestChild::Cancel(nsresult reason) { - // for now Cancel is a no-op + if(mIPCOpen) { + // We can only do IPDL on the main thread + NS_DispatchToMainThread( + new CancelDNSRequestEvent(this, reason)); + } return NS_OK; } diff --git a/netwerk/dns/DNSRequestChild.h b/netwerk/dns/DNSRequestChild.h index 178e3dd396d..579e5d0bd66 100644 --- a/netwerk/dns/DNSRequestChild.h +++ b/netwerk/dns/DNSRequestChild.h @@ -30,21 +30,19 @@ public: void AddIPDLReference() { AddRef(); } - void ReleaseIPDLReference() { - // we don't need an 'mIPCOpen' variable until/unless we add calls that might - // try to send IPDL msgs to parent after ReleaseIPDLReference is called - // (when IPDL channel torn down). - Release(); - } + void ReleaseIPDLReference(); // Sends IPDL request to parent void StartRequest(); void CallOnLookupComplete(); -private: +protected: + friend class CancelDNSRequestEvent; + friend class ChildDNSService; virtual ~DNSRequestChild() {} - virtual bool Recv__delete__(const DNSRequestResponse& reply) MOZ_OVERRIDE; + virtual bool RecvLookupCompleted(const DNSRequestResponse& reply) MOZ_OVERRIDE; + virtual void ActorDestroy(ActorDestroyReason why) MOZ_OVERRIDE; nsCOMPtr mListener; nsCOMPtr mTarget; @@ -52,6 +50,7 @@ private: nsresult mResultStatus; nsCString mHost; uint16_t mFlags; + bool mIPCOpen; }; } // namespace net diff --git a/netwerk/dns/DNSRequestParent.cpp b/netwerk/dns/DNSRequestParent.cpp index e81f3b7cb26..96b29bdd891 100644 --- a/netwerk/dns/DNSRequestParent.cpp +++ b/netwerk/dns/DNSRequestParent.cpp @@ -44,10 +44,31 @@ DNSRequestParent::DoAsyncResolve(const nsACString &hostname, uint32_t flags) } if (NS_FAILED(rv) && !mIPCClosed) { - unused << Send__delete__(this, DNSRequestResponse(rv)); + mIPCClosed = true; + unused << SendLookupCompleted(DNSRequestResponse(rv)); } } +bool +DNSRequestParent::RecvCancelDNSRequest(const nsCString& hostName, + const uint32_t& flags, + const nsresult& reason) +{ + nsresult rv; + nsCOMPtr dns = do_GetService(NS_DNSSERVICE_CONTRACTID, &rv); + if (NS_SUCCEEDED(rv)) { + rv = dns->CancelAsyncResolve(hostName, flags, this, reason); + } + return true; +} + +bool +DNSRequestParent::Recv__delete__() +{ + mIPCClosed = true; + return true; +} + void DNSRequestParent::ActorDestroy(ActorDestroyReason why) { @@ -92,11 +113,12 @@ DNSRequestParent::OnLookupComplete(nsICancelable *request, array.AppendElement(addr); } - unused << Send__delete__(this, DNSRequestResponse(DNSRecord(cname, array))); + unused << SendLookupCompleted(DNSRequestResponse(DNSRecord(cname, array))); } else { - unused << Send__delete__(this, DNSRequestResponse(status)); + unused << SendLookupCompleted(DNSRequestResponse(status)); } + mIPCClosed = true; return NS_OK; } diff --git a/netwerk/dns/DNSRequestParent.h b/netwerk/dns/DNSRequestParent.h index 9415df717b3..e476a45771e 100644 --- a/netwerk/dns/DNSRequestParent.h +++ b/netwerk/dns/DNSRequestParent.h @@ -26,6 +26,13 @@ public: void DoAsyncResolve(const nsACString &hostname, uint32_t flags); + // Pass args here rather than storing them in the parent; they are only + // needed if the request is to be canceled. + bool RecvCancelDNSRequest(const nsCString& hostName, + const uint32_t& flags, + const nsresult& reason); + bool Recv__delete__(); + protected: virtual void ActorDestroy(ActorDestroyReason why) MOZ_OVERRIDE; private: diff --git a/netwerk/dns/PDNSRequest.ipdl b/netwerk/dns/PDNSRequest.ipdl index b8a201ad674..e44ece7938b 100644 --- a/netwerk/dns/PDNSRequest.ipdl +++ b/netwerk/dns/PDNSRequest.ipdl @@ -18,12 +18,16 @@ async protocol PDNSRequest { manager PNecko; -//parent: +parent: // constructor in PNecko takes AsyncResolve args that initialize request + // Pass args here rather than storing them in the parent; they are only + // needed if the request is to be canceled. + CancelDNSRequest(nsCString hostName, uint32_t flags, nsresult reason); + __delete__(); child: - __delete__(DNSRequestResponse reply); + LookupCompleted(DNSRequestResponse reply); }; diff --git a/netwerk/dns/nsDNSService2.cpp b/netwerk/dns/nsDNSService2.cpp index 0b7452a6698..edab9f0d93e 100644 --- a/netwerk/dns/nsDNSService2.cpp +++ b/netwerk/dns/nsDNSService2.cpp @@ -314,6 +314,12 @@ nsDNSAsyncRequest::OnLookupComplete(nsHostResolver *resolver, bool nsDNSAsyncRequest::EqualsAsyncListener(nsIDNSListener *aListener) { + nsCOMPtr wrapper = do_QueryInterface(mListener); + if (wrapper) { + nsCOMPtr originalListener; + wrapper->GetOriginalListener(getter_AddRefs(originalListener)); + return aListener == originalListener; + } return (aListener == mListener); } diff --git a/netwerk/dns/nsIDNSListener.idl b/netwerk/dns/nsIDNSListener.idl index 4920b7e9ac6..46c241005e6 100644 --- a/netwerk/dns/nsIDNSListener.idl +++ b/netwerk/dns/nsIDNSListener.idl @@ -28,3 +28,19 @@ interface nsIDNSListener : nsISupports in nsIDNSRecord aRecord, in nsresult aStatus); }; + +/** + * nsIDNSListenerProxy: + * + * Must be implemented by classes that wrap the original listener passed to + * nsIDNSService.AsyncResolve, so we have access to original listener for + * comparison purposes. + */ +[uuid(60eff0e4-6f7c-493c-add9-1cbea59063ad)] +interface nsIDNSListenerProxy : nsISupports +{ + /* + * The original nsIDNSListener which requested hostname resolution. + */ + readonly attribute nsIDNSListener originalListener; +}; diff --git a/netwerk/test/unit/test_dns_cancel.js b/netwerk/test/unit/test_dns_cancel.js new file mode 100644 index 00000000000..2f725bb22c6 --- /dev/null +++ b/netwerk/test/unit/test_dns_cancel.js @@ -0,0 +1,91 @@ +var dns = Cc["@mozilla.org/network/dns-service;1"].getService(Ci.nsIDNSService); + +var hostname1 = "mozilla.org"; +var hostname2 = "mozilla.com"; + +var requestList1Canceled1; +var requestList1Canceled2; +var requestList1NotCanceled; + +var requestList2Canceled; +var requestList2NotCanceled; + +var listener1 = { + onLookupComplete: function(inRequest, inRecord, inStatus) { + // One request should be resolved and two request should be canceled. + if (inRequest == requestList1Canceled1 || + inRequest == requestList1Canceled2) { + // This request is canceled. + do_check_eq(inStatus, Cr.NS_ERROR_ABORT); + + do_test_finished(); + } else if (inRequest == requestList1NotCanceled) { + // This request should not be canceled. + do_check_neq(inStatus, Cr.NS_ERROR_ABORT); + + do_test_finished(); + } + }, + QueryInterface: function(aIID) { + if (aIID.equals(Ci.nsIDNSListener) || + aIID.equals(Ci.nsISupports)) { + return this; + } + throw Cr.NS_ERROR_NO_INTERFACE; + } +}; + +var listener2 = { + onLookupComplete: function(inRequest, inRecord, inStatus) { + // One request should be resolved and the other canceled. + if (inRequest == requestList2Canceled) { + // This request is canceled. + do_check_eq(inStatus, Cr.NS_ERROR_ABORT); + + do_test_finished(); + } else { + // The request should not be canceled. + do_check_neq(inStatus, Cr.NS_ERROR_ABORT); + + do_test_finished(); + } + }, + QueryInterface: function(aIID) { + if (aIID.equals(Ci.nsIDNSListener) || + aIID.equals(Ci.nsISupports)) { + return this; + } + throw Cr.NS_ERROR_NO_INTERFACE; + } +}; + +function run_test() { + var threadManager = Cc["@mozilla.org/thread-manager;1"].getService(Ci.nsIThreadManager); + var mainThread = threadManager.currentThread; + + var flags = Ci.nsIDNSService.RESOLVE_BYPASS_CACHE; + + // This one will be canceled with cancelAsyncResolve. + requestList1Canceled1 = dns.asyncResolve(hostname2, flags, listener1, mainThread); + dns.cancelAsyncResolve(hostname2, flags, listener1, Cr.NS_ERROR_ABORT); + + // This one will not be canceled. + requestList1NotCanceled = dns.asyncResolve(hostname1, flags, listener1, mainThread); + + // This one will be canceled with cancel(Cr.NS_ERROR_ABORT). + requestList1Canceled2 = dns.asyncResolve(hostname1, flags, listener1, mainThread); + requestList1Canceled2.cancel(Cr.NS_ERROR_ABORT); + + // This one will not be canceled. + requestList2NotCanceled = dns.asyncResolve(hostname1, flags, listener2, mainThread); + + // This one will be canceled with cancel(Cr.NS_ERROR_ABORT). + requestList2Canceled = dns.asyncResolve(hostname2, flags, listener2, mainThread); + requestList2Canceled.cancel(Cr.NS_ERROR_ABORT); + + do_test_pending(); + do_test_pending(); + do_test_pending(); + do_test_pending(); + do_test_pending(); +} diff --git a/netwerk/test/unit/xpcshell.ini b/netwerk/test/unit/xpcshell.ini index 62158262fd5..6f01b28f216 100644 --- a/netwerk/test/unit/xpcshell.ini +++ b/netwerk/test/unit/xpcshell.ini @@ -161,6 +161,7 @@ skip-if = bits != 32 [test_cookie_header.js] [test_cookiejars.js] [test_cookiejars_safebrowsing.js] +[test_dns_cancel.js] [test_data_protocol.js] [test_dns_service.js] [test_dns_localredirect.js] diff --git a/netwerk/test/unit_ipc/test_dns_cancel_wrap.js b/netwerk/test/unit_ipc/test_dns_cancel_wrap.js new file mode 100644 index 00000000000..2f38aa4ecb6 --- /dev/null +++ b/netwerk/test/unit_ipc/test_dns_cancel_wrap.js @@ -0,0 +1,7 @@ +// +// Run test script in content process instead of chrome (xpcshell's default) +// + +function run_test() { + run_test_in_child("../unit/test_dns_cancel.js"); +} diff --git a/netwerk/test/unit_ipc/xpcshell.ini b/netwerk/test/unit_ipc/xpcshell.ini index 7dc605148c2..61113a33805 100644 --- a/netwerk/test/unit_ipc/xpcshell.ini +++ b/netwerk/test/unit_ipc/xpcshell.ini @@ -10,6 +10,7 @@ support-files = disabled_test_bug528292_wrap.js [test_channel_close_wrap.js] [test_cookie_header_wrap.js] [test_cookiejars_wrap.js] +[test_dns_cancel_wrap.js] [test_dns_service_wrap.js] [test_duplicate_headers_wrap.js] [test_event_sink_wrap.js]