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.

--HG--
extra : rebase_source : 47ac7a7f8b5bc0ce3eee10cdc170ae27e9fc94aa

netwerk/ipc/NeckoChild.cpp

@@ -99,8 +99,8 @@ 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;
 }
 

netwerk/protocol/http/HttpChannelChild.cpp

@@ -58,6 +58,7 @@ HttpChannelChild::HttpChannelChild()
   , mCacheEntryAvailable(PR_FALSE)
   , mCacheExpirationTime(nsICache::NO_EXPIRATION_TIME)
   , mState(HCC_NEW)
+  , mIPCOpen(false)
 {
   LOG(("Creating HttpChannelChild @%x\n", this));
 }
@@ -73,18 +74,7 @@ HttpChannelChild::~HttpChannelChild()
 
 // 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)
@@ -105,6 +95,22 @@ 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,
@@ -186,7 +192,7 @@ HttpChannelChild::RecvOnStopRequest(const nsresult& statusCode)
 
   mIsPending = PR_FALSE;
   mStatus = statusCode;
-  nsresult rv = mListener->OnStopRequest(this, mListenerContext, statusCode);
+  mListener->OnStopRequest(this, mListenerContext, statusCode);
   mListener = 0;
   mListenerContext = 0;
   mCacheEntryAvailable = PR_FALSE;
@@ -194,15 +200,9 @@ HttpChannelChild::RecvOnStopRequest(const nsresult& statusCode)
   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;
 }
 
@@ -377,6 +377,10 @@ HttpChannelChild::AsyncOpen(nsIStreamListener *listener, nsISupports *aContext)
     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),
@@ -385,10 +389,6 @@ HttpChannelChild::AsyncOpen(nsIStreamListener *listener, nsISupports *aContext)
                 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;
 }
@@ -453,7 +453,7 @@ HttpChannelChild::GetCacheTokenCachedCharset(nsACString &_retval)
 NS_IMETHODIMP
 HttpChannelChild::SetCacheTokenCachedCharset(const nsACString &aCharset)
 {
-  if (!mCacheEntryAvailable)
+  if (!mCacheEntryAvailable || !mIPCOpen)
     return NS_ERROR_NOT_AVAILABLE;
 
   mCachedCharset = aCharset;
@@ -523,7 +523,7 @@ HttpChannelChild::SetPriority(PRInt32 aPriority)
   if (mPriority == newValue)
     return NS_OK;
   mPriority = newValue;
-  if (mWasOpened) 
+  if (mIPCOpen) 
     SendSetPriority(mPriority);
   return NS_OK;
 }

netwerk/protocol/http/HttpChannelChild.h

@@ -112,8 +112,13 @@ public:
   // 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,
@@ -137,6 +142,7 @@ private:
 
   // FIXME: replace with IPDL states (bug 536319) 
   enum HttpChannelChildState mState;
+  bool mIPCOpen;
 };
 
 } // namespace net

netwerk/protocol/http/HttpChannelParent.cpp

@@ -191,13 +191,6 @@ HttpChannelParent::RecvSetCacheTokenCachedCharset(const nsCString& charset)
   return true;
 }
 
-bool
-HttpChannelParent::RecvOnStopRequestCompleted()
-{
-  mCacheDescriptor = nsnull;
-  return true;
-}
-
 //-----------------------------------------------------------------------------
 // HttpChannelParent::nsIRequestObserver
 //-----------------------------------------------------------------------------

netwerk/protocol/http/HttpChannelParent.h

@@ -90,7 +90,6 @@ protected:
 
   virtual bool RecvSetPriority(const PRUint16& priority);
   virtual bool RecvSetCacheTokenCachedCharset(const nsCString& charset);
-  virtual bool RecvOnStopRequestCompleted();
 
   virtual void ActorDestroy(ActorDestroyReason why);
 

netwerk/protocol/http/PHttpChannel.ipdl

@@ -80,8 +80,6 @@ parent:
 
   SetCacheTokenCachedCharset(nsCString charset);
 
-  OnStopRequestCompleted();
-
 child:
   OnStartRequest(nsHttpResponseHead responseHead,
                  PRBool             useResponseHead,

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);
+}

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");
+}