Bug 1294719 - Make sure HttpChannelParent stops sending IPC messages before calling Send__delete__() r=honzab a=ritu
authorValentin Gosu <valentin.gosu@gmail.com>
Fri, 28 Oct 2016 04:30:58 +0200
changeset 356391 058c4a1cd78b7682061d5f5104b9bd3177644305
parent 356390 559084cb05017e8d3abb4dc6191e42fa1a21fe41
child 356392 b3ed92294f434e05ea178c6ae3fb3e71d8da10ec
push id6570
push userraliiev@mozilla.com
push dateMon, 14 Nov 2016 12:26:13 +0000
treeherdermozilla-beta@f455459b2ae5 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewershonzab, ritu
bugs1294719
milestone51.0a2
Bug 1294719 - Make sure HttpChannelParent stops sending IPC messages before calling Send__delete__() r=honzab a=ritu MozReview-Commit-ID: 4lb0gs32tOq
netwerk/protocol/http/HttpChannelChild.cpp
netwerk/protocol/http/HttpChannelChild.h
netwerk/protocol/http/HttpChannelParent.cpp
netwerk/protocol/http/HttpChannelParent.h
netwerk/protocol/http/PHttpChannel.ipdl
--- a/netwerk/protocol/http/HttpChannelChild.cpp
+++ b/netwerk/protocol/http/HttpChannelChild.cpp
@@ -199,20 +199,20 @@ NS_IMETHODIMP_(MozExternalRefCountType) 
   NS_LOG_RELEASE(this, mRefCnt, "HttpChannelChild");
 
   // Normally we Send_delete in OnStopRequest, but when we need to retain the
   // remote channel for security info IPDL itself holds 1 reference, so we
   // Send_delete when refCnt==1.  But if !mIPCOpen, then there's nobody to send
   // to, so we fall through.
   if (mKeptAlive && mRefCnt == 1 && mIPCOpen) {
     mKeptAlive = false;
-    // Send_delete calls NeckoChild::DeallocPHttpChannel, which will release
-    // again to refcount==0
-    PHttpChannelChild::Send__delete__(this);
-    return 0;
+    // We send a message to the parent, which calls SendDelete, and then the
+    // child calling Send__delete__() to finally drop the refcount to 0.
+    SendDeletingChannel();
+    return 1;
   }
 
   if (mRefCnt == 0) {
     mRefCnt = 1; /* stabilize */
     delete this;
     return 0;
   }
   return mRefCnt;
@@ -913,19 +913,19 @@ HttpChannelChild::OnStopRequest(const ns
 
   ReleaseListeners();
 
   if (mLoadFlags & LOAD_DOCUMENT_URI) {
     // Keep IPDL channel open, but only for updating security info.
     mKeptAlive = true;
     SendDocumentChannelCleanup();
   } else {
-    // This calls NeckoChild::DeallocPHttpChannelChild(), which deletes |this| if IPDL
-    // holds the last reference.  Don't rely on |this| existing after here.
-    PHttpChannelChild::Send__delete__(this);
+    // The parent process will respond by sending a DeleteSelf message and
+    // making sure not to send any more messages after that.
+    SendDeletingChannel();
   }
 }
 
 void
 HttpChannelChild::DoPreOnStopRequest(nsresult aStatus)
 {
   LOG(("HttpChannelChild::DoPreOnStopRequest [this=%p status=%x]\n",
        this, aStatus));
@@ -1099,22 +1099,18 @@ HttpChannelChild::FailedAsyncOpen(const 
   LOG(("HttpChannelChild::FailedAsyncOpen [this=%p status=%x]\n", this, status));
 
   mStatus = status;
 
   // We're already being called from IPDL, therefore already "async"
   HandleAsyncAbort();
 
   if (mIPCOpen) {
-    PHttpChannelChild::Send__delete__(this);
+    SendDeletingChannel();
   }
-  // WARNING:  DO NOT RELY ON |THIS| EXISTING ANY MORE! 
-  // 
-  // NeckoChild::DeallocPHttpChannelChild() may have been called, which deletes 
-  // |this| if IPDL holds the last reference.
 }
 
 void
 HttpChannelChild::DoNotifyListenerCleanup()
 {
   LOG(("HttpChannelChild::DoNotifyListenerCleanup [this=%p]\n", this));
 
   if (mInterceptListener) {
@@ -1135,22 +1131,95 @@ class DeleteSelfEvent : public ChannelEv
 bool
 HttpChannelChild::RecvDeleteSelf()
 {
   LOG(("HttpChannelChild::RecvDeleteSelf [this=%p]\n", this));
   mEventQ->RunOrEnqueue(new DeleteSelfEvent(this));
   return true;
 }
 
+HttpChannelChild::OverrideRunnable::OverrideRunnable(HttpChannelChild* aChannel,
+                                                     HttpChannelChild* aNewChannel,
+                                                     InterceptStreamListener* aListener,
+                                                     nsIInputStream* aInput,
+                                                     nsAutoPtr<nsHttpResponseHead>& aHead)
+{
+  mChannel = aChannel;
+  mNewChannel = aNewChannel;
+  mListener = aListener;
+  mInput = aInput;
+  mHead = aHead;
+}
+
+void
+HttpChannelChild::OverrideRunnable::OverrideWithSynthesizedResponse()
+{
+  mNewChannel->OverrideWithSynthesizedResponse(mHead, mInput, mListener);
+}
+
+NS_IMETHODIMP
+HttpChannelChild::OverrideRunnable::Run()
+{
+  bool ret = mChannel->Redirect3Complete(this);
+
+  // If the method returns false, it means the IPDL connection is being
+  // asyncly torn down and reopened, and OverrideWithSynthesizedResponse
+  // will be called later from FinishInterceptedRedirect. This object will
+  // be assigned to HttpChannelChild::mOverrideRunnable in order to do so.
+  // If it is true, we can call the method right now.
+  if (ret) {
+    OverrideWithSynthesizedResponse();
+  }
+
+  return NS_OK;
+}
+
+bool
+HttpChannelChild::RecvFinishInterceptedRedirect()
+{
+  // Hold a ref to this to keep it from being deleted by Send__delete__()
+  RefPtr<HttpChannelChild> self(this);
+  Send__delete__(this);
+
+  // The IPDL connection was torn down by a interception logic in
+  // CompleteRedirectSetup, and we need to call FinishInterceptedRedirect.
+  NS_DispatchToMainThread(NewRunnableMethod(this, &HttpChannelChild::FinishInterceptedRedirect));
+
+  return true;
+}
+
 void
 HttpChannelChild::DeleteSelf()
 {
   Send__delete__(this);
 }
 
+void HttpChannelChild::FinishInterceptedRedirect()
+{
+  nsresult rv;
+  if (mLoadInfo && mLoadInfo->GetEnforceSecurity()) {
+    MOZ_ASSERT(!mInterceptedRedirectContext, "the context should be null!");
+    rv = AsyncOpen2(mInterceptedRedirectListener);
+  } else {
+    rv = AsyncOpen(mInterceptedRedirectListener, mInterceptedRedirectContext);
+  }
+  mInterceptedRedirectListener = nullptr;
+  mInterceptedRedirectContext = nullptr;
+
+  if (mInterceptingChannel) {
+    mInterceptingChannel->CleanupRedirectingChannel(rv);
+    mInterceptingChannel = nullptr;
+  }
+
+  if (mOverrideRunnable) {
+    mOverrideRunnable->OverrideWithSynthesizedResponse();
+    mOverrideRunnable = nullptr;
+  }
+}
+
 bool
 HttpChannelChild::RecvReportSecurityMessage(const nsString& messageTag,
                                             const nsString& messageCategory)
 {
   AddSecurityMessage(messageTag, messageCategory);
   return true;
 }
 
@@ -1342,17 +1411,17 @@ HttpChannelChild::OverrideSecurityInfoFo
   mResponseCouldBeSynthesized = true;
   OverrideSecurityInfo(securityInfo);
 }
 
 class Redirect3Event : public ChannelEvent
 {
  public:
   explicit Redirect3Event(HttpChannelChild* child) : mChild(child) {}
-  void Run() { mChild->Redirect3Complete(); }
+  void Run() { mChild->Redirect3Complete(nullptr); }
  private:
   HttpChannelChild* mChild;
 };
 
 bool
 HttpChannelChild::RecvRedirect3Complete()
 {
   LOG(("HttpChannelChild::RecvRedirect3Complete [this=%p]\n", this));
@@ -1418,27 +1487,55 @@ HttpChannelChild::RecvDivertMessages()
 
   // DivertTo() has been called on parent, so we can now start sending queued
   // IPDL messages back to parent listener.
   MOZ_RELEASE_ASSERT(NS_SUCCEEDED(Resume()));
 
   return true;
 }
 
-void
-HttpChannelChild::Redirect3Complete()
+// Returns true if has actually completed the redirect and cleaned up the
+// channel, or false the interception logic kicked in and we need to asyncly
+// call FinishInterceptedRedirect and CleanupRedirectingChannel.
+// The argument is an optional OverrideRunnable that we pass to the redirected
+// channel.
+bool
+HttpChannelChild::Redirect3Complete(OverrideRunnable* aRunnable)
 {
   LOG(("HttpChannelChild::Redirect3Complete [this=%p]\n", this));
   nsresult rv = NS_OK;
 
+  nsCOMPtr<nsIHttpChannelChild> chan = do_QueryInterface(mRedirectChannelChild);
+  RefPtr<HttpChannelChild> httpChannelChild = static_cast<HttpChannelChild*>(chan.get());
   // Chrome channel has been AsyncOpen'd.  Reflect this in child.
-  if (mRedirectChannelChild)
+  if (mRedirectChannelChild) {
+    if (httpChannelChild) {
+      httpChannelChild->mOverrideRunnable = aRunnable;
+      httpChannelChild->mInterceptingChannel = this;
+    }
     rv = mRedirectChannelChild->CompleteRedirectSetup(mListener,
                                                       mListenerContext);
-
+  }
+
+  if (!httpChannelChild || !httpChannelChild->mShouldParentIntercept) {
+    // The redirect channel either isn't a HttpChannelChild, or the interception
+    // logic wasn't triggered, so we can clean it up right here.
+    CleanupRedirectingChannel(rv);
+    if (httpChannelChild) {
+      httpChannelChild->mOverrideRunnable = nullptr;
+      httpChannelChild->mInterceptingChannel = nullptr;
+    }
+    return true;
+  }
+  return false;
+}
+
+void
+HttpChannelChild::CleanupRedirectingChannel(nsresult rv)
+{
   // Redirecting to new channel: shut this down and init new channel
   if (mLoadGroup)
     mLoadGroup->RemoveRequest(this, nullptr, NS_BINDING_ABORTED);
 
   if (NS_SUCCEEDED(rv)) {
     if (mLoadInfo) {
       mLoadInfo->AppendRedirectedPrincipal(GetURIPrincipal(), false);
     }
@@ -1506,22 +1603,36 @@ HttpChannelChild::CompleteRedirectSetup(
   NS_ENSURE_TRUE(!mIsPending, NS_ERROR_IN_PROGRESS);
   NS_ENSURE_TRUE(!mWasOpened, NS_ERROR_ALREADY_OPENED);
 
   if (mShouldParentIntercept) {
     // This is a redirected channel, and the corresponding parent channel has started
     // AsyncOpen but was intercepted and suspended. We must tear it down and start
     // fresh - we will intercept the child channel this time, before creating a new
     // parent channel unnecessarily.
-    PHttpChannelChild::Send__delete__(this);
-    if (mLoadInfo && mLoadInfo->GetEnforceSecurity()) {
-        MOZ_ASSERT(!aContext, "aContext should be null!");
-        return AsyncOpen2(listener);
-    }
-    return AsyncOpen(listener, aContext);
+
+    // Since this method is called from RecvRedirect3Complete which itself is
+    // called from either OnRedirectVerifyCallback via OverrideRunnable, or from
+    // RecvRedirect3Complete. The order of events must always be:
+    //  1. Teardown the IPDL connection
+    //  2. AsyncOpen the connection again
+    //  3. Cleanup the redirecting channel (the one calling Redirect3Complete)
+    //  4. [optional] Call OverrideWithSynthesizedResponse on the redirected
+    //  channel if the call came from OverrideRunnable.
+    mInterceptedRedirectListener = listener;
+    mInterceptedRedirectContext = aContext;
+
+    // This will send a message to the parent notifying it that we are closing
+    // down. After closing the IPC channel, we will proceed to execute
+    // FinishInterceptedRedirect() which AsyncOpen's the channel again.
+    SendFinishInterceptedRedirect();
+
+    // XXX valentin: The interception logic should be rewritten to avoid
+    // calling AsyncOpen on the channel _after_ we call Send__delete__()
+    return NS_OK;
   }
 
   /*
    * No need to check for cancel: we don't get here if nsHttpChannel canceled
    * before AsyncOpen(); if it's canceled after that, OnStart/Stop will just
    * get called with error code as usual.  So just setup mListener and make the
    * channel reflect AsyncOpen'ed state.
    */
@@ -1540,44 +1651,16 @@ HttpChannelChild::CompleteRedirectSetup(
   // and send it back to us naturally.
   return NS_OK;
 }
 
 //-----------------------------------------------------------------------------
 // HttpChannelChild::nsIAsyncVerifyRedirectCallback
 //-----------------------------------------------------------------------------
 
-class OverrideRunnable : public Runnable {
-  RefPtr<HttpChannelChild> mChannel;
-  RefPtr<HttpChannelChild> mNewChannel;
-  RefPtr<InterceptStreamListener> mListener;
-  nsCOMPtr<nsIInputStream> mInput;
-  nsAutoPtr<nsHttpResponseHead> mHead;
-
-public:
-  OverrideRunnable(HttpChannelChild* aChannel,
-                   HttpChannelChild* aNewChannel,
-                   InterceptStreamListener* aListener,
-                   nsIInputStream* aInput,
-                   nsAutoPtr<nsHttpResponseHead>& aHead)
-  : mChannel(aChannel)
-  , mNewChannel(aNewChannel)
-  , mListener(aListener)
-  , mInput(aInput)
-  , mHead(aHead)
-  {
-  }
-
-  NS_IMETHOD Run() override {
-    mChannel->Redirect3Complete();
-    mNewChannel->OverrideWithSynthesizedResponse(mHead, mInput, mListener);
-    return NS_OK;
-  }
-};
-
 NS_IMETHODIMP
 HttpChannelChild::OnRedirectVerifyCallback(nsresult result)
 {
   LOG(("HttpChannelChild::OnRedirectVerifyCallback [this=%p]\n", this));
   nsresult rv;
   OptionalURIParams redirectURI;
   nsCOMPtr<nsIHttpChannel> newHttpChannel =
       do_QueryInterface(mRedirectChannelChild);
--- a/netwerk/protocol/http/HttpChannelChild.h
+++ b/netwerk/protocol/http/HttpChannelChild.h
@@ -33,17 +33,16 @@
 
 class nsInputStreamPump;
 
 namespace mozilla {
 namespace net {
 
 class InterceptedChannelContent;
 class InterceptStreamListener;
-class OverrideRunnable;
 
 class HttpChannelChild final : public PHttpChannelChild
                              , public HttpBaseChannel
                              , public HttpAsyncAborter<HttpChannelChild>
                              , public nsICacheInfoChannel
                              , public nsIProxiedChannel
                              , public nsIApplicationCacheChannel
                              , public nsIAsyncVerifyRedirectCallback
@@ -140,29 +139,49 @@ protected:
                           const nsCString& securityInfoSerialization,
                           const nsCString& channelId) override;
   bool RecvRedirect3Complete() override;
   bool RecvAssociateApplicationCache(const nsCString& groupID,
                                      const nsCString& clientID) override;
   bool RecvFlushedForDiversion() override;
   bool RecvDivertMessages() override;
   bool RecvDeleteSelf() override;
+  bool RecvFinishInterceptedRedirect() override;
 
   bool RecvReportSecurityMessage(const nsString& messageTag,
                                  const nsString& messageCategory) override;
 
   bool RecvIssueDeprecationWarning(const uint32_t& warning,
                                    const bool& asError) override;
 
   bool GetAssociatedContentSecurity(nsIAssociatedContentSecurity** res = nullptr);
   virtual void DoNotifyListenerCleanup() override;
 
   NS_IMETHOD GetResponseSynthesized(bool* aSynthesized) override;
 
 private:
+
+  class OverrideRunnable : public Runnable {
+  public:
+    OverrideRunnable(HttpChannelChild* aChannel,
+                     HttpChannelChild* aNewChannel,
+                     InterceptStreamListener* aListener,
+                     nsIInputStream* aInput,
+                     nsAutoPtr<nsHttpResponseHead>& aHead);
+
+    NS_IMETHOD Run() override;
+    void OverrideWithSynthesizedResponse();
+  private:
+    RefPtr<HttpChannelChild> mChannel;
+    RefPtr<HttpChannelChild> mNewChannel;
+    RefPtr<InterceptStreamListener> mListener;
+    nsCOMPtr<nsIInputStream> mInput;
+    nsAutoPtr<nsHttpResponseHead> mHead;
+  };
+
   nsresult ContinueAsyncOpen();
 
   void DoOnStartRequest(nsIRequest* aRequest, nsISupports* aContext);
   void DoOnStatus(nsIRequest* aRequest, nsresult status);
   void DoOnProgress(nsIRequest* aRequest, int64_t progress, int64_t progressMax);
   void DoOnDataAvailable(nsIRequest* aRequest, nsISupports* aContext, nsIInputStream* aStream,
                          uint64_t offset, uint32_t count);
   void DoPreOnStopRequest(nsresult aStatus);
@@ -240,16 +259,27 @@ private:
   // Set if the corresponding parent channel should force an interception to occur
   // before the network transaction is initiated.
   bool mShouldParentIntercept;
 
   // Set if the corresponding parent channel should suspend after a response
   // is synthesized.
   bool mSuspendParentAfterSynthesizeResponse;
 
+  // Needed to call AsyncOpen in FinishInterceptedRedirect
+  nsCOMPtr<nsIStreamListener> mInterceptedRedirectListener;
+  nsCOMPtr<nsISupports> mInterceptedRedirectContext;
+  // Needed to call CleanupRedirectingChannel in FinishInterceptedRedirect
+  RefPtr<HttpChannelChild> mInterceptingChannel;
+  // Used to call OverrideWithSynthesizedResponse in FinishInterceptedRedirect
+  RefPtr<OverrideRunnable> mOverrideRunnable;
+
+  void FinishInterceptedRedirect();
+  void CleanupRedirectingChannel(nsresult rv);
+
   // true after successful AsyncOpen until OnStopRequest completes.
   bool RemoteChannelExists() { return mIPCOpen && !mKeptAlive; }
 
   void AssociateApplicationCache(const nsCString &groupID,
                                  const nsCString &clientID);
   void OnStartRequest(const nsresult& channelStatus,
                       const nsHttpResponseHead& responseHead,
                       const bool& useResponseHead,
@@ -279,17 +309,17 @@ private:
   void FailedAsyncOpen(const nsresult& status);
   void HandleAsyncAbort();
   void Redirect1Begin(const uint32_t& registrarId,
                       const URIParams& newUri,
                       const uint32_t& redirectFlags,
                       const nsHttpResponseHead& responseHead,
                       const nsACString& securityInfoSerialization,
                       const nsACString& channelId);
-  void Redirect3Complete();
+  bool Redirect3Complete(OverrideRunnable* aRunnable);
   void DeleteSelf();
 
   // Create a a new channel to be used in a redirection, based on the provided
   // response headers.
   nsresult SetupRedirect(nsIURI* uri,
                          const nsHttpResponseHead* responseHead,
                          const uint32_t& redirectFlags,
                          nsIChannel** outChannel);
@@ -311,17 +341,16 @@ private:
   friend class StatusEvent;
   friend class FailedAsyncOpenEvent;
   friend class Redirect1Event;
   friend class Redirect3Event;
   friend class DeleteSelfEvent;
   friend class HttpAsyncAborter<HttpChannelChild>;
   friend class InterceptStreamListener;
   friend class InterceptedChannelContent;
-  friend class OverrideRunnable;
 };
 
 // A stream listener interposed between the nsInputStreamPump used for intercepted channels
 // and this channel's original listener. This is only used to ensure the original listener
 // sees the channel as the request object, and to synthesize OnStatus and OnProgress notifications.
 class InterceptStreamListener : public nsIStreamListener
                               , public nsIProgressEventSink
 {
--- a/netwerk/protocol/http/HttpChannelParent.cpp
+++ b/netwerk/protocol/http/HttpChannelParent.cpp
@@ -1652,16 +1652,34 @@ HttpChannelParent::UpdateAndSerializeSec
 bool
 HttpChannelParent::DoSendDeleteSelf()
 {
   bool rv = SendDeleteSelf();
   mIPCClosed = true;
   return rv;
 }
 
+bool
+HttpChannelParent::RecvDeletingChannel()
+{
+  // We need to ensure that the parent channel will not be sending any more IPC
+  // messages after this, as the child is going away. DoSendDeleteSelf will
+  // set mIPCClosed = true;
+  return DoSendDeleteSelf();
+}
+
+bool
+HttpChannelParent::RecvFinishInterceptedRedirect()
+{
+  // We make sure not to send any more messages until the IPC channel is torn
+  // down by the child.
+  mIPCClosed = true;
+  return SendFinishInterceptedRedirect();
+}
+
 //-----------------------------------------------------------------------------
 // HttpChannelSecurityWarningReporter
 //-----------------------------------------------------------------------------
 
 nsresult
 HttpChannelParent::ReportSecurityMessage(const nsAString& aMessageTag,
                                          const nsAString& aMessageCategory)
 {
--- a/netwerk/protocol/http/HttpChannelParent.h
+++ b/netwerk/protocol/http/HttpChannelParent.h
@@ -181,16 +181,19 @@ protected:
   uint32_t GetAppId() override;
 
   nsresult ReportSecurityMessage(const nsAString& aMessageTag,
                                  const nsAString& aMessageCategory) override;
 
   // Calls SendDeleteSelf and sets mIPCClosed to true because we should not
   // send any more messages after that. Bug 1274886
   bool DoSendDeleteSelf();
+  // Called to notify the parent channel to not send any more IPC messages.
+  virtual bool RecvDeletingChannel() override;
+  virtual bool RecvFinishInterceptedRedirect() override;
 
 private:
   void UpdateAndSerializeSecurityInfo(nsACString& aSerializedSecurityInfoOut);
 
   void DivertOnDataAvailable(const nsCString& data,
                              const uint64_t& offset,
                              const uint32_t& count);
   void DivertOnStopRequest(const nsresult& statusCode);
--- a/netwerk/protocol/http/PHttpChannel.ipdl
+++ b/netwerk/protocol/http/PHttpChannel.ipdl
@@ -80,16 +80,20 @@ parent:
   // Child has no more events/messages to divert to the parent.
   async DivertComplete();
 
   // Child has detected a CORS check failure, so needs to tell the parent
   // to remove any matching entry from the CORS preflight cache.
   async RemoveCorsPreflightCacheEntry(URIParams uri,
                                       PrincipalInfo requestingPrincipal);
 
+  // After receiving this message, the parent calls SendDeleteSelf, and makes
+  // sure not to send any more messages after that.
+  async DeletingChannel();
+
   async __delete__();
 
 child:
   async OnStartRequest(nsresult            channelStatus,
                        nsHttpResponseHead  responseHead,
                        bool                useResponseHead,
                        nsHttpHeaderArray   requestHeaders,
                        bool                isFromCache,
@@ -153,13 +157,20 @@ child:
   async ReportSecurityMessage(nsString messageTag, nsString messageCategory);
 
   // Tell child to delete channel (all IPDL deletes must be done from child to
   // avoid races: see bug 591708).
   async DeleteSelf();
 
   // Tell the child to issue a deprecation warning.
   async IssueDeprecationWarning(uint32_t warning, bool asError);
+
+both:
+  // After receiving this message, the parent also calls
+  // SendFinishInterceptedRedirect, and makes sure not to send any more messages
+  // after that. When receiving this message, the child will call
+  // Send__delete__() and complete the steps required to finish the redirect.
+  async FinishInterceptedRedirect();
 };
 
 
 } // namespace net
 } // namespace mozilla