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

@@ -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),
+      mFlags(aFlags) {}
-      mIPCOpen(false) {}
 
 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<ChildDNSService> 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<nsIRunnable> runnable = new CancelDNSRequestEvent(this, reason);
     SystemGroup::Dispatch(TaskCategory::Other, runnable.forget());

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

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) {
+  if (NS_FAILED(rv) && CanSend()) {
-    mIPCClosed = true;
     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;
 }
 

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

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;
 

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<DNSRequestChild*>(aChild);
-  p->ReleaseIPDLReference();
-  return true;
-}
-
 PChannelDiverterChild* NeckoChild::AllocPChannelDiverterChild(
     const ChannelDiverterArgs& channel) {
   return new ChannelDiverterChild();

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(

netwerk/ipc/NeckoParent.cpp

@@ -598,12 +598,11 @@ bool NeckoParent::DeallocPUDPSocketParent(PUDPSocketParent* actor) {
   return true;
 }
 
-PDNSRequestParent* NeckoParent::AllocPDNSRequestParent(
+already_AddRefed<PDNSRequestParent> NeckoParent::AllocPDNSRequestParent(
     const nsCString& aHost, const OriginAttributes& aOriginAttributes,
     const uint32_t& aFlags) {
-  DNSRequestParent* p = new DNSRequestParent();
+  RefPtr<DNSRequestParent> actor = new DNSRequestParent();
-  p->AddRef();
+  return actor.forget();
-  return p;
 }
 
 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<DNSRequestParent*>(aParent);
-  p->Release();
-  return true;
-}
-
 mozilla::ipc::IPCResult NeckoParent::RecvSpeculativeConnect(
     const URIParams& aURI, nsIPrincipal* aPrincipal, const bool& aAnonymous) {
   nsCOMPtr<nsISpeculativeConnect> speculator(gIOService);

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<PDNSRequestParent> 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);

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<DNSRequestChild*>(aChild);
-  p->ReleaseIPDLReference();
-  return true;
-}
-
 }  // namespace net
 }  // namespace mozilla

netwerk/ipc/SocketProcessChild.h

@@ -47,10 +47,6 @@ class SocketProcessChild final : public PSocketProcessChild {
 
   PWebrtcTCPSocketChild* AllocPWebrtcTCPSocketChild(const Maybe<TabId>& 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);

netwerk/ipc/SocketProcessParent.cpp

@@ -141,12 +141,11 @@ bool SocketProcessParent::DeallocPWebrtcTCPSocketParent(
   return true;
 }
 
-PDNSRequestParent* SocketProcessParent::AllocPDNSRequestParent(
+already_AddRefed<PDNSRequestParent> SocketProcessParent::AllocPDNSRequestParent(
     const nsCString& aHost, const OriginAttributes& aOriginAttributes,
     const uint32_t& aFlags) {
-  DNSRequestParent* p = new DNSRequestParent();
+  RefPtr<DNSRequestParent> actor = new DNSRequestParent();
-  p->AddRef();
+  return actor.forget();
-  return p;
 }
 
 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<DNSRequestParent*>(aParent);
-  p->Release();
-  return true;
-}
-
 // To ensure that IPDL is finished before SocketParent gets deleted.
 class DeferredDeleteSocketProcessParent : public Runnable {
  public:

netwerk/ipc/SocketProcessParent.h

@@ -56,14 +56,13 @@ class SocketProcessParent final
   PWebrtcTCPSocketParent* AllocPWebrtcTCPSocketParent(
       const Maybe<TabId>& aTabId);
   bool DeallocPWebrtcTCPSocketParent(PWebrtcTCPSocketParent* aActor);
-  PDNSRequestParent* AllocPDNSRequestParent(
+  already_AddRefed<PDNSRequestParent> 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,