Bug 572980 - e10s: HttpChannelChild incorrectly refcounted. r=cjones, jduell.
authorJosh Matthews <josh@joshmatthews.net>
Sun, 27 Jun 2010 12:44:15 -0400
changeset 48132 e1ff1c480d4ff7930ee90171c2651f61879866f8
parent 48131 18127ac02bf2e246e68b86c2cc1bca5d4eeb7fd5
child 48133 8b85a12f424d3648cbc19fcd6f2a9b5848fe3701
push id14589
push usercjones@mozilla.com
push dateFri, 23 Jul 2010 17:12:29 +0000
treeherdermozilla-central@49185ce20807 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerscjones, jduell
bugs572980
milestone2.0b3pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 572980 - e10s: HttpChannelChild incorrectly refcounted. r=cjones, jduell. This version adds checks to make sure child doesn't try to send any IPDL msgs to parent after IPDL has been shut down.
netwerk/ipc/NeckoChild.cpp
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
netwerk/test/unit/test_channel_close.js
netwerk/test/unit_ipc/test_channel_close_wrap.js
--- a/netwerk/ipc/NeckoChild.cpp
+++ b/netwerk/ipc/NeckoChild.cpp
@@ -94,18 +94,18 @@ NeckoChild::AllocPHttpChannel(PBrowserCh
   return nsnull;
 }
 
 bool 
 NeckoChild::DeallocPHttpChannel(PHttpChannelChild* channel)
 {
   NS_ABORT_IF_FALSE(IsNeckoChild(), "DeallocPHttpChannel called by non-child!");
 
-  // Delete channel (HttpChannelChild's refcnt must already hit 0 to get here)
-  delete channel;
+  HttpChannelChild* child = static_cast<HttpChannelChild*>(channel);
+  child->ReleaseIPDLReference();
   return true;
 }
 
 PCookieServiceChild* 
 NeckoChild::AllocPCookieService()
 {
   // We don't allocate here: see nsCookieService::GetSingleton()
   NS_NOTREACHED("AllocPCookieService should not be called");
--- a/netwerk/protocol/http/HttpChannelChild.cpp
+++ b/netwerk/protocol/http/HttpChannelChild.cpp
@@ -53,43 +53,33 @@ namespace mozilla {
 namespace net {
 
 // C++ file contents
 HttpChannelChild::HttpChannelChild()
   : mIsFromCache(PR_FALSE)
   , mCacheEntryAvailable(PR_FALSE)
   , mCacheExpirationTime(nsICache::NO_EXPIRATION_TIME)
   , mState(HCC_NEW)
+  , mIPCOpen(false)
 {
   LOG(("Creating HttpChannelChild @%x\n", this));
 }
 
 HttpChannelChild::~HttpChannelChild()
 {
   LOG(("Destroying HttpChannelChild @%x\n", this));
 }
 
 //-----------------------------------------------------------------------------
 // HttpChannelChild::nsISupports
 //-----------------------------------------------------------------------------
 
 // Override nsHashPropertyBag's AddRef: we don't need thread-safe refcnt
 NS_IMPL_ADDREF(HttpChannelChild)
-NS_IMPL_RELEASE_WITH_DESTROY(HttpChannelChild, RefcountHitZero())
-
-void
-HttpChannelChild::RefcountHitZero()
-{
-  if (mWasOpened) {
-    // NeckoChild::DeallocPHttpChannel will delete this
-    PHttpChannelChild::Send__delete__(this);
-  } else {
-    delete this;    // we never opened IPDL channel
-  }
-}
+NS_IMPL_RELEASE(HttpChannelChild)
 
 NS_INTERFACE_MAP_BEGIN(HttpChannelChild)
   NS_INTERFACE_MAP_ENTRY(nsIRequest)
   NS_INTERFACE_MAP_ENTRY(nsIChannel)
   NS_INTERFACE_MAP_ENTRY(nsIHttpChannel)
   NS_INTERFACE_MAP_ENTRY(nsIHttpChannelInternal)
   NS_INTERFACE_MAP_ENTRY(nsICacheInfoChannel)
   NS_INTERFACE_MAP_ENTRY(nsIEncodedChannel)
@@ -100,16 +90,32 @@ NS_INTERFACE_MAP_BEGIN(HttpChannelChild)
   NS_INTERFACE_MAP_ENTRY(nsIApplicationCacheContainer)
   NS_INTERFACE_MAP_ENTRY(nsIApplicationCacheChannel)
 NS_INTERFACE_MAP_END_INHERITING(HttpBaseChannel)
 
 //-----------------------------------------------------------------------------
 // HttpChannelChild::PHttpChannelChild
 //-----------------------------------------------------------------------------
 
+void
+HttpChannelChild::AddIPDLReference()
+{
+  NS_ABORT_IF_FALSE(!mIPCOpen, "Attempt to retain more than one IPDL reference");
+  mIPCOpen = true;
+  AddRef();
+}
+
+void
+HttpChannelChild::ReleaseIPDLReference()
+{
+  NS_ABORT_IF_FALSE(mIPCOpen, "Attempt to release nonexistent IPDL reference");
+  mIPCOpen = false;
+  Release();
+}
+
 bool 
 HttpChannelChild::RecvOnStartRequest(const nsHttpResponseHead& responseHead,
                                      const PRBool& useResponseHead,
                                      const PRBool& isFromCache,
                                      const PRBool& cacheEntryAvailable,
                                      const PRUint32& cacheExpirationTime,
                                      const nsCString& cachedCharset)
 {
@@ -181,33 +187,27 @@ HttpChannelChild::RecvOnStopRequest(cons
 {
   LOG(("HttpChannelChild::RecvOnStopRequest [this=%x status=%u]\n", 
            this, statusCode));
 
   mState = HCC_ONSTOP;
 
   mIsPending = PR_FALSE;
   mStatus = statusCode;
-  nsresult rv = mListener->OnStopRequest(this, mListenerContext, statusCode);
+  mListener->OnStopRequest(this, mListenerContext, statusCode);
   mListener = 0;
   mListenerContext = 0;
   mCacheEntryAvailable = PR_FALSE;
 
   if (mLoadGroup)
     mLoadGroup->RemoveRequest(this, nsnull, statusCode);
 
-  SendOnStopRequestCompleted();
-
-  // Corresponding AddRef in AsyncOpen().
-  this->Release();
-  
-  if (NS_FAILED(rv)) {
-    // TODO: Cancel request: see OnStartRequest (bug 536317)
-    return false;  
-  }
+  // This calls NeckoChild::DeallocPHttpChannel(), which deletes |this| if IPDL
+  // holds the last reference.  Don't rely on |this| existing after here.
+  PHttpChannelChild::Send__delete__(this);
   return true;
 }
 
 bool
 HttpChannelChild::RecvOnProgress(const PRUint64& progress,
                                  const PRUint64& progressMax)
 {
   LOG(("HttpChannelChild::RecvOnProgress [this=%p progress=%llu/%llu]\n",
@@ -372,28 +372,28 @@ HttpChannelChild::AsyncOpen(nsIStreamLis
 
   mozilla::dom::TabChild* tabChild = nsnull;
   nsCOMPtr<nsITabChild> iTabChild;
   GetCallback(iTabChild);
   if (iTabChild) {
     tabChild = static_cast<mozilla::dom::TabChild*>(iTabChild.get());
   }
 
+  // The socket transport layer in the chrome process now has a logical ref to
+  // us, until either OnStopRequest or OnRedirect is called.
+  AddIPDLReference();
+
   gNeckoChild->SendPHttpChannelConstructor(this, tabChild);
 
   SendAsyncOpen(IPC::URI(mURI), IPC::URI(mOriginalURI), IPC::URI(mDocumentURI),
                 IPC::URI(mReferrer), mLoadFlags, mRequestHeaders, 
                 mRequestHead.Method(), uploadStreamData, 
                 uploadStreamInfo, mPriority, mRedirectionLimit, 
                 mAllowPipelining, mForceAllowThirdPartyCookie);
 
-  // The socket transport layer in the chrome process now has a logical ref to
-  // us, until either OnStopRequest or OnRedirect is called.
-  this->AddRef();
-
   mState = HCC_OPENED;
   return NS_OK;
 }
 
 //-----------------------------------------------------------------------------
 // HttpChannelChild::nsIHttpChannel
 //-----------------------------------------------------------------------------
 
@@ -448,17 +448,17 @@ HttpChannelChild::GetCacheTokenCachedCha
     return NS_ERROR_NOT_AVAILABLE;
 
   _retval = mCachedCharset;
   return NS_OK;
 }
 NS_IMETHODIMP
 HttpChannelChild::SetCacheTokenCachedCharset(const nsACString &aCharset)
 {
-  if (!mCacheEntryAvailable)
+  if (!mCacheEntryAvailable || !mIPCOpen)
     return NS_ERROR_NOT_AVAILABLE;
 
   mCachedCharset = aCharset;
   if (!SendSetCacheTokenCachedCharset(PromiseFlatCString(aCharset))) {
     return NS_ERROR_FAILURE;
   }
   return NS_OK;
 }
@@ -518,17 +518,17 @@ HttpChannelChild::GetEntityID(nsACString
 
 NS_IMETHODIMP
 HttpChannelChild::SetPriority(PRInt32 aPriority)
 {
   PRInt16 newValue = CLAMP(aPriority, PR_INT16_MIN, PR_INT16_MAX);
   if (mPriority == newValue)
     return NS_OK;
   mPriority = newValue;
-  if (mWasOpened) 
+  if (mIPCOpen) 
     SendSetPriority(mPriority);
   return NS_OK;
 }
 
 //-----------------------------------------------------------------------------
 // HttpChannelChild::nsIProxiedChannel
 //-----------------------------------------------------------------------------
 
--- a/netwerk/protocol/http/HttpChannelChild.h
+++ b/netwerk/protocol/http/HttpChannelChild.h
@@ -107,18 +107,23 @@ public:
   NS_IMETHOD SetRequestHeader(const nsACString& aHeader, 
                               const nsACString& aValue, 
                               PRBool aMerge);
   // nsIHttpChannelInternal
   NS_IMETHOD SetupFallbackChannel(const char *aFallbackKey);
   // nsISupportsPriority
   NS_IMETHOD SetPriority(PRInt32 value);
 
+  // IPDL holds a reference while the PHttpChannel protocol is live (starting at
+  // AsyncOpen, and ending at either OnStopRequest or any IPDL error, either of
+  // which call NeckoChild::DeallocPHttpChannel()).
+  void AddIPDLReference();
+  void ReleaseIPDLReference();
+
 protected:
-  void RefcountHitZero();
   bool RecvOnStartRequest(const nsHttpResponseHead& responseHead,
                           const PRBool& useResponseHead,
                           const PRBool& isFromCache,
                           const PRBool& cacheEntryAvailable,
                           const PRUint32& cacheExpirationTime,
                           const nsCString& cachedCharset);
   bool RecvOnDataAvailable(const nsCString& data, 
                            const PRUint32& offset,
@@ -132,14 +137,15 @@ private:
 
   PRPackedBool mIsFromCache;
   PRPackedBool mCacheEntryAvailable;
   PRUint32     mCacheExpirationTime;
   nsCString    mCachedCharset;
 
   // FIXME: replace with IPDL states (bug 536319) 
   enum HttpChannelChildState mState;
+  bool mIPCOpen;
 };
 
 } // namespace net
 } // namespace mozilla
 
 #endif // mozilla_net_HttpChannelChild_h
--- a/netwerk/protocol/http/HttpChannelParent.cpp
+++ b/netwerk/protocol/http/HttpChannelParent.cpp
@@ -186,23 +186,16 @@ bool
 HttpChannelParent::RecvSetCacheTokenCachedCharset(const nsCString& charset)
 {
   if (mCacheDescriptor)
     mCacheDescriptor->SetMetaDataElement("charset",
                                          PromiseFlatCString(charset).get());
   return true;
 }
 
-bool
-HttpChannelParent::RecvOnStopRequestCompleted()
-{
-  mCacheDescriptor = nsnull;
-  return true;
-}
-
 //-----------------------------------------------------------------------------
 // HttpChannelParent::nsIRequestObserver
 //-----------------------------------------------------------------------------
 
 NS_IMETHODIMP
 HttpChannelParent::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext)
 {
   LOG(("HttpChannelParent::OnStartRequest [this=%x]\n", this));
--- a/netwerk/protocol/http/HttpChannelParent.h
+++ b/netwerk/protocol/http/HttpChannelParent.h
@@ -85,17 +85,16 @@ protected:
                              const PRInt32&             uploadStreamInfo,
                              const PRUint16&            priority,
                              const PRUint8&             redirectionLimit,
                              const PRBool&              allowPipelining,
                              const PRBool&              forceAllowThirdPartyCookie);
 
   virtual bool RecvSetPriority(const PRUint16& priority);
   virtual bool RecvSetCacheTokenCachedCharset(const nsCString& charset);
-  virtual bool RecvOnStopRequestCompleted();
 
   virtual void ActorDestroy(ActorDestroyReason why);
 
 private:
   nsCOMPtr<nsITabParent> mTabParent;
   nsCOMPtr<nsIChannel> mChannel;
   nsCOMPtr<nsICacheEntryDescriptor> mCacheDescriptor;
   bool mIPCClosed;                // PHttpChannel actor has been Closed()
--- a/netwerk/protocol/http/PHttpChannel.ipdl
+++ b/netwerk/protocol/http/PHttpChannel.ipdl
@@ -75,18 +75,16 @@ parent:
             PRUint8             redirectionLimit,
             PRBool              allowPipelining,
             PRBool              forceAllowThirdPartyCookie);
 
   SetPriority(PRUint16 priority);
 
   SetCacheTokenCachedCharset(nsCString charset);
 
-  OnStopRequestCompleted();
-
 child:
   OnStartRequest(nsHttpResponseHead responseHead,
                  PRBool             useResponseHead,
                  PRBool             isFromCache,
                  PRBool             cacheEntryAvailable,
                  PRUint32           cacheExpirationTime,
                  nsCString          cachedCharset);
 
new file mode 100644
--- /dev/null
+++ b/netwerk/test/unit/test_channel_close.js
@@ -0,0 +1,55 @@
+do_load_httpd_js();
+
+var httpserver = new nsHttpServer();
+var testpath = "/simple";
+var httpbody = "0123456789";
+
+var live_channels = [];
+
+function run_test() {
+  httpserver.registerPathHandler(testpath, serverHandler);
+  httpserver.start(4444);
+
+  var local_channel;
+
+  // Opened channel that has no remaining references on shutdown
+  local_channel = setupChannel(testpath);
+  local_channel.asyncOpen(
+      new ChannelListener(checkRequest, local_channel), null);
+
+  // Opened channel that has no remaining references after being opened
+  setupChannel(testpath).asyncOpen(
+      new ChannelListener(function() {}, null), null);
+  
+  // Unopened channel that has remaining references on shutdown
+  live_channels.push(setupChannel(testpath));
+
+  // Opened channel that has remaining references on shutdown
+  live_channels.push(setupChannel(testpath));
+  live_channels[1].asyncOpen(
+      new ChannelListener(checkRequestFinish, live_channels[1]), null);
+
+  do_test_pending();
+}
+
+function setupChannel(path) {
+  var ios = Cc["@mozilla.org/network/io-service;1"].getService(Ci.nsIIOService);
+  var chan = ios.newChannel("http://localhost:4444" + path, "", null);
+  chan.QueryInterface(Ci.nsIHttpChannel);
+  chan.requestMethod = "GET";
+  return chan;
+}
+
+function serverHandler(metadata, response) {
+  response.setHeader("Content-Type", "text/plain", false);
+  response.bodyOutputStream.write(httpbody, httpbody.length);
+}
+
+function checkRequest(request, data, context) {
+  do_check_eq(data, httpbody);
+}
+
+function checkRequestFinish(request, data, context) {
+  checkRequest(request, data, context);
+  httpserver.stop(do_test_finished);
+}
new file mode 100644
--- /dev/null
+++ b/netwerk/test/unit_ipc/test_channel_close_wrap.js
@@ -0,0 +1,3 @@
+function run_test() {
+  run_test_in_child("../unit/test_channel_close.js");
+}