From a1a7f148847dba27133ae04a93fc3a00fd4c6903 Mon Sep 17 00:00:00 2001 From: Kershaw Chang Date: Mon, 2 Dec 2019 18:28:49 +0000 Subject: [PATCH] Bug 1600678 - Use IPDL refcounted for DNSRequest r=valentin Differential Revision: https://phabricator.services.mozilla.com/D55475 --HG-- extra : moz-landing-system : lando --- netwerk/dns/DNSRequestChild.cpp | 19 ++++--------------- netwerk/dns/DNSRequestChild.h | 6 +----- netwerk/dns/DNSRequestParent.cpp | 22 ++++------------------ netwerk/dns/DNSRequestParent.h | 5 ----- netwerk/dns/PDNSRequest.ipdl | 2 +- netwerk/ipc/NeckoChild.cpp | 15 --------------- netwerk/ipc/NeckoChild.h | 4 ---- netwerk/ipc/NeckoParent.cpp | 13 +++---------- netwerk/ipc/NeckoParent.h | 3 +-- netwerk/ipc/SocketProcessChild.cpp | 15 --------------- netwerk/ipc/SocketProcessChild.h | 4 ---- netwerk/ipc/SocketProcessParent.cpp | 13 +++---------- netwerk/ipc/SocketProcessParent.h | 3 +-- 13 files changed, 18 insertions(+), 106 deletions(-) diff --git a/netwerk/dns/DNSRequestChild.cpp b/netwerk/dns/DNSRequestChild.cpp index 0134817c0b9a..b21d8da9af34 100644 --- a/netwerk/dns/DNSRequestChild.cpp +++ b/netwerk/dns/DNSRequestChild.cpp @@ -196,7 +196,7 @@ class CancelDNSRequestEvent : public Runnable { mReasonForCancel(aReason) {} NS_IMETHOD Run() override { - if (mDnsRequest->mIPCOpen) { + if (mDnsRequest->CanSend()) { // Send request to Parent process. mDnsRequest->SendCancelDNSRequest(mDnsRequest->mHost, mDnsRequest->mType, mDnsRequest->mOriginAttributes, @@ -225,8 +225,7 @@ DNSRequestChild::DNSRequestChild(const nsACString& aHost, const uint16_t& aType, mHost(aHost), mType(aType), mOriginAttributes(aOriginAttributes), - mFlags(aFlags), - mIPCOpen(false) {} + mFlags(aFlags) {} void DNSRequestChild::StartRequest() { // we can only do IPDL on the main thread @@ -263,11 +262,6 @@ void DNSRequestChild::StartRequest() { MOZ_ASSERT(false, "Wrong process"); return; } - - mIPCOpen = true; - - // IPDL holds a reference until IPDL channel gets destroyed - AddIPDLReference(); } void DNSRequestChild::CallOnLookupComplete() { @@ -283,7 +277,6 @@ void DNSRequestChild::CallOnLookupByTypeComplete() { mozilla::ipc::IPCResult DNSRequestChild::RecvLookupCompleted( const DNSRequestResponse& reply) { - mIPCOpen = false; MOZ_ASSERT(mListener); switch (reply.type()) { @@ -340,17 +333,13 @@ mozilla::ipc::IPCResult DNSRequestChild::RecvLookupCompleted( return IPC_OK(); } -void DNSRequestChild::ReleaseIPDLReference() { +void DNSRequestChild::ActorDestroy(ActorDestroyReason why) { // Request is done or destroyed. Remove it from the hash table. RefPtr dnsServiceChild = dont_AddRef(ChildDNSService::GetSingleton()); dnsServiceChild->NotifyRequestDone(this); - - Release(); } -void DNSRequestChild::ActorDestroy(ActorDestroyReason why) { mIPCOpen = false; } - //----------------------------------------------------------------------------- // DNSRequestChild::nsISupports //----------------------------------------------------------------------------- @@ -363,7 +352,7 @@ NS_IMPL_ISUPPORTS(DNSRequestChild, nsICancelable) NS_IMETHODIMP DNSRequestChild::Cancel(nsresult reason) { - if (mIPCOpen) { + if (CanSend()) { // We can only do IPDL on the main thread nsCOMPtr runnable = new CancelDNSRequestEvent(this, reason); SystemGroup::Dispatch(TaskCategory::Other, runnable.forget()); diff --git a/netwerk/dns/DNSRequestChild.h b/netwerk/dns/DNSRequestChild.h index aa6b2c22e084..c425ed5c97cf 100644 --- a/netwerk/dns/DNSRequestChild.h +++ b/netwerk/dns/DNSRequestChild.h @@ -29,9 +29,6 @@ class DNSRequestChild final : public PDNSRequestChild, public nsICancelable { const uint32_t& aFlags, nsIDNSListener* aListener, nsIEventTarget* target); - void AddIPDLReference() { AddRef(); } - void ReleaseIPDLReference(); - // Sends IPDL request to parent void StartRequest(); void CallOnLookupComplete(); @@ -40,7 +37,7 @@ class DNSRequestChild final : public PDNSRequestChild, public nsICancelable { protected: friend class CancelDNSRequestEvent; friend class ChildDNSService; - virtual ~DNSRequestChild() {} + virtual ~DNSRequestChild() = default; mozilla::ipc::IPCResult RecvLookupCompleted(const DNSRequestResponse& reply); virtual void ActorDestroy(ActorDestroyReason why) override; @@ -60,7 +57,6 @@ class DNSRequestChild final : public PDNSRequestChild, public nsICancelable { uint16_t mType; const OriginAttributes mOriginAttributes; uint16_t mFlags; - bool mIPCOpen; }; } // namespace net diff --git a/netwerk/dns/DNSRequestParent.cpp b/netwerk/dns/DNSRequestParent.cpp index 92933f029558..54fe72c979bc 100644 --- a/netwerk/dns/DNSRequestParent.cpp +++ b/netwerk/dns/DNSRequestParent.cpp @@ -19,7 +19,7 @@ using namespace mozilla::ipc; namespace mozilla { namespace net { -DNSRequestParent::DNSRequestParent() : mFlags(0), mIPCClosed(false) {} +DNSRequestParent::DNSRequestParent() : mFlags(0) {} void DNSRequestParent::DoAsyncResolve(const nsACString& hostname, const OriginAttributes& originAttributes, @@ -34,8 +34,7 @@ void DNSRequestParent::DoAsyncResolve(const nsACString& hostname, getter_AddRefs(unused)); } - if (NS_FAILED(rv) && !mIPCClosed) { - mIPCClosed = true; + if (NS_FAILED(rv) && CanSend()) { Unused << SendLookupCompleted(DNSRequestResponse(rv)); } } @@ -58,17 +57,6 @@ mozilla::ipc::IPCResult DNSRequestParent::RecvCancelDNSRequest( return IPC_OK(); } -mozilla::ipc::IPCResult DNSRequestParent::Recv__delete__() { - mIPCClosed = true; - return IPC_OK(); -} - -void DNSRequestParent::ActorDestroy(ActorDestroyReason why) { - // We may still have refcount>0 if DNS hasn't called our OnLookupComplete - // yet, but child process has crashed. We must not send any more msgs - // to child, or IPDL will kill chrome process, too. - mIPCClosed = true; -} //----------------------------------------------------------------------------- // DNSRequestParent::nsISupports //----------------------------------------------------------------------------- @@ -82,7 +70,7 @@ NS_IMPL_ISUPPORTS(DNSRequestParent, nsIDNSListener) NS_IMETHODIMP DNSRequestParent::OnLookupComplete(nsICancelable* request, nsIDNSRecord* rec, nsresult status) { - if (mIPCClosed) { + if (!CanSend()) { // nothing to do: child probably crashed return NS_OK; } @@ -107,7 +95,6 @@ DNSRequestParent::OnLookupComplete(nsICancelable* request, nsIDNSRecord* rec, Unused << SendLookupCompleted(DNSRequestResponse(status)); } - mIPCClosed = true; return NS_OK; } @@ -115,7 +102,7 @@ NS_IMETHODIMP DNSRequestParent::OnLookupByTypeComplete(nsICancelable* aRequest, nsIDNSByTypeRecord* aRes, nsresult aStatus) { - if (mIPCClosed) { + if (!CanSend()) { // nothing to do: child probably crashed return NS_OK; } @@ -127,7 +114,6 @@ DNSRequestParent::OnLookupByTypeComplete(nsICancelable* aRequest, } else { Unused << SendLookupCompleted(DNSRequestResponse(aStatus)); } - mIPCClosed = true; return NS_OK; } diff --git a/netwerk/dns/DNSRequestParent.h b/netwerk/dns/DNSRequestParent.h index 6c19dbc2ebdb..db02fbc6467a 100644 --- a/netwerk/dns/DNSRequestParent.h +++ b/netwerk/dns/DNSRequestParent.h @@ -30,16 +30,11 @@ class DNSRequestParent : public PDNSRequestParent, public nsIDNSListener { const nsCString& hostName, const uint16_t& type, const OriginAttributes& originAttributes, const uint32_t& flags, const nsresult& reason); - mozilla::ipc::IPCResult Recv__delete__() override; - - protected: - virtual void ActorDestroy(ActorDestroyReason why) override; private: virtual ~DNSRequestParent() = default; uint32_t mFlags; - bool mIPCClosed; // true if IPDL channel has been closed (child crash) }; } // namespace net diff --git a/netwerk/dns/PDNSRequest.ipdl b/netwerk/dns/PDNSRequest.ipdl index 9ad06eb876f1..0ea652e4038f 100644 --- a/netwerk/dns/PDNSRequest.ipdl +++ b/netwerk/dns/PDNSRequest.ipdl @@ -17,7 +17,7 @@ using mozilla::OriginAttributes from "mozilla/ipc/BackgroundUtils.h"; namespace mozilla { namespace net { -async protocol PDNSRequest +async refcounted protocol PDNSRequest { manager PNecko or PSocketProcess; diff --git a/netwerk/ipc/NeckoChild.cpp b/netwerk/ipc/NeckoChild.cpp index 42a5f5848f4a..98fcf970cf5d 100644 --- a/netwerk/ipc/NeckoChild.cpp +++ b/netwerk/ipc/NeckoChild.cpp @@ -254,21 +254,6 @@ bool NeckoChild::DeallocPUDPSocketChild(PUDPSocketChild* child) { return true; } -PDNSRequestChild* NeckoChild::AllocPDNSRequestChild( - const nsCString& aHost, const OriginAttributes& aOriginAttributes, - const uint32_t& aFlags) { - // We don't allocate here: instead we always use IPDL constructor that takes - // an existing object - MOZ_ASSERT_UNREACHABLE("AllocPDNSRequestChild should not be called on child"); - return nullptr; -} - -bool NeckoChild::DeallocPDNSRequestChild(PDNSRequestChild* aChild) { - DNSRequestChild* p = static_cast(aChild); - p->ReleaseIPDLReference(); - return true; -} - PChannelDiverterChild* NeckoChild::AllocPChannelDiverterChild( const ChannelDiverterArgs& channel) { return new ChannelDiverterChild(); diff --git a/netwerk/ipc/NeckoChild.h b/netwerk/ipc/NeckoChild.h index 968be1c8516c..c16fc01956db 100644 --- a/netwerk/ipc/NeckoChild.h +++ b/netwerk/ipc/NeckoChild.h @@ -60,10 +60,6 @@ class NeckoChild : public PNeckoChild { PUDPSocketChild* AllocPUDPSocketChild(nsIPrincipal* aPrincipal, const nsCString& aFilter); bool DeallocPUDPSocketChild(PUDPSocketChild*); - PDNSRequestChild* AllocPDNSRequestChild( - const nsCString& aHost, const OriginAttributes& aOriginAttributes, - const uint32_t& aFlags); - bool DeallocPDNSRequestChild(PDNSRequestChild*); PSimpleChannelChild* AllocPSimpleChannelChild(const uint32_t& channelId); bool DeallocPSimpleChannelChild(PSimpleChannelChild* child); PChannelDiverterChild* AllocPChannelDiverterChild( diff --git a/netwerk/ipc/NeckoParent.cpp b/netwerk/ipc/NeckoParent.cpp index 82e6be898a8f..1d0d9490eccb 100644 --- a/netwerk/ipc/NeckoParent.cpp +++ b/netwerk/ipc/NeckoParent.cpp @@ -598,12 +598,11 @@ bool NeckoParent::DeallocPUDPSocketParent(PUDPSocketParent* actor) { return true; } -PDNSRequestParent* NeckoParent::AllocPDNSRequestParent( +already_AddRefed NeckoParent::AllocPDNSRequestParent( const nsCString& aHost, const OriginAttributes& aOriginAttributes, const uint32_t& aFlags) { - DNSRequestParent* p = new DNSRequestParent(); - p->AddRef(); - return p; + RefPtr actor = new DNSRequestParent(); + return actor.forget(); } mozilla::ipc::IPCResult NeckoParent::RecvPDNSRequestConstructor( @@ -614,12 +613,6 @@ mozilla::ipc::IPCResult NeckoParent::RecvPDNSRequestConstructor( return IPC_OK(); } -bool NeckoParent::DeallocPDNSRequestParent(PDNSRequestParent* aParent) { - DNSRequestParent* p = static_cast(aParent); - p->Release(); - return true; -} - mozilla::ipc::IPCResult NeckoParent::RecvSpeculativeConnect( const URIParams& aURI, nsIPrincipal* aPrincipal, const bool& aAnonymous) { nsCOMPtr speculator(gIOService); diff --git a/netwerk/ipc/NeckoParent.h b/netwerk/ipc/NeckoParent.h index 497ed517431d..1b77656ef792 100644 --- a/netwerk/ipc/NeckoParent.h +++ b/netwerk/ipc/NeckoParent.h @@ -150,14 +150,13 @@ class NeckoParent : public PNeckoParent { PUDPSocketParent*, nsIPrincipal* aPrincipal, const nsCString& aFilter) override; bool DeallocPUDPSocketParent(PUDPSocketParent*); - PDNSRequestParent* AllocPDNSRequestParent( + already_AddRefed AllocPDNSRequestParent( const nsCString& aHost, const OriginAttributes& aOriginAttributes, const uint32_t& aFlags); virtual mozilla::ipc::IPCResult RecvPDNSRequestConstructor( PDNSRequestParent* actor, const nsCString& hostName, const OriginAttributes& aOriginAttributes, const uint32_t& flags) override; - bool DeallocPDNSRequestParent(PDNSRequestParent*); mozilla::ipc::IPCResult RecvSpeculativeConnect(const URIParams& aURI, nsIPrincipal* aPrincipal, const bool& aAnonymous); diff --git a/netwerk/ipc/SocketProcessChild.cpp b/netwerk/ipc/SocketProcessChild.cpp index a61a3a66469e..d827833730d0 100644 --- a/netwerk/ipc/SocketProcessChild.cpp +++ b/netwerk/ipc/SocketProcessChild.cpp @@ -212,20 +212,5 @@ bool SocketProcessChild::DeallocPWebrtcTCPSocketChild( return true; } -PDNSRequestChild* SocketProcessChild::AllocPDNSRequestChild( - const nsCString& aHost, const OriginAttributes& aOriginAttributes, - const uint32_t& aFlags) { - // We don't allocate here: instead we always use IPDL constructor that takes - // an existing object - MOZ_ASSERT_UNREACHABLE("AllocPDNSRequestChild should not be called on child"); - return nullptr; -} - -bool SocketProcessChild::DeallocPDNSRequestChild(PDNSRequestChild* aChild) { - DNSRequestChild* p = static_cast(aChild); - p->ReleaseIPDLReference(); - return true; -} - } // namespace net } // namespace mozilla diff --git a/netwerk/ipc/SocketProcessChild.h b/netwerk/ipc/SocketProcessChild.h index 511d099838a4..9417716cfc7a 100644 --- a/netwerk/ipc/SocketProcessChild.h +++ b/netwerk/ipc/SocketProcessChild.h @@ -47,10 +47,6 @@ class SocketProcessChild final : public PSocketProcessChild { PWebrtcTCPSocketChild* AllocPWebrtcTCPSocketChild(const Maybe& tabId); bool DeallocPWebrtcTCPSocketChild(PWebrtcTCPSocketChild* aActor); - PDNSRequestChild* AllocPDNSRequestChild( - const nsCString& aHost, const OriginAttributes& aOriginAttributes, - const uint32_t& aFlags); - bool DeallocPDNSRequestChild(PDNSRequestChild*); void CleanUp(); void DestroySocketProcessBridgeParent(ProcessId aId); diff --git a/netwerk/ipc/SocketProcessParent.cpp b/netwerk/ipc/SocketProcessParent.cpp index eec3d46c8c02..52d35450accb 100644 --- a/netwerk/ipc/SocketProcessParent.cpp +++ b/netwerk/ipc/SocketProcessParent.cpp @@ -141,12 +141,11 @@ bool SocketProcessParent::DeallocPWebrtcTCPSocketParent( return true; } -PDNSRequestParent* SocketProcessParent::AllocPDNSRequestParent( +already_AddRefed SocketProcessParent::AllocPDNSRequestParent( const nsCString& aHost, const OriginAttributes& aOriginAttributes, const uint32_t& aFlags) { - DNSRequestParent* p = new DNSRequestParent(); - p->AddRef(); - return p; + RefPtr actor = new DNSRequestParent(); + return actor.forget(); } mozilla::ipc::IPCResult SocketProcessParent::RecvPDNSRequestConstructor( @@ -157,12 +156,6 @@ mozilla::ipc::IPCResult SocketProcessParent::RecvPDNSRequestConstructor( return IPC_OK(); } -bool SocketProcessParent::DeallocPDNSRequestParent(PDNSRequestParent* aParent) { - DNSRequestParent* p = static_cast(aParent); - p->Release(); - return true; -} - // To ensure that IPDL is finished before SocketParent gets deleted. class DeferredDeleteSocketProcessParent : public Runnable { public: diff --git a/netwerk/ipc/SocketProcessParent.h b/netwerk/ipc/SocketProcessParent.h index 3726d68ef879..b4e6d95fdc33 100644 --- a/netwerk/ipc/SocketProcessParent.h +++ b/netwerk/ipc/SocketProcessParent.h @@ -56,14 +56,13 @@ class SocketProcessParent final PWebrtcTCPSocketParent* AllocPWebrtcTCPSocketParent( const Maybe& aTabId); bool DeallocPWebrtcTCPSocketParent(PWebrtcTCPSocketParent* aActor); - PDNSRequestParent* AllocPDNSRequestParent( + already_AddRefed AllocPDNSRequestParent( const nsCString& aHost, const OriginAttributes& aOriginAttributes, const uint32_t& aFlags); virtual mozilla::ipc::IPCResult RecvPDNSRequestConstructor( PDNSRequestParent* actor, const nsCString& hostName, const OriginAttributes& aOriginAttributes, const uint32_t& flags) override; - bool DeallocPDNSRequestParent(PDNSRequestParent*); void ActorDestroy(ActorDestroyReason aWhy) override; bool SendRequestMemoryReport(const uint32_t& aGeneration,