Bug 1015466 - Part 2, break reference cycle among HttpChannelParent, HttpChannelParentListener, and nsHttpChannel while async open is failed. r=mayhemer
authorShih-Chiang Chien <schien@mozilla.com>
Wed, 03 May 2017 19:20:12 +0800
changeset 361766 272dbc46892e0b6bfc602792bc6747927d93a333
parent 361765 dc7ed794a3ea448ee1d51cbb4a0e1acf848282df
child 361767 3814e17ae85efaad6d9740bc2e979179c2a4e99e
push id31942
push userryanvm@gmail.com
push dateThu, 01 Jun 2017 15:54:15 +0000
treeherdermozilla-central@cac2fd43de81 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs1015466
milestone55.0a1
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 1015466 - Part 2, break reference cycle among HttpChannelParent, HttpChannelParentListener, and nsHttpChannel while async open is failed. r=mayhemer AsyncOpen procedure can failed and trigger FailedAsyncOpen IPC to do the clean up. However FailedAsyncOpen might not complete if content process is destroyed at the meantime. We can delay the timing of holding the strong reference to parent listener and channel object to make sure no reference cycle is created by HttpChannelParent. In addition, clean up the strong reference as soon as FailedAsyncOpen IPC is triggered. MozReview-Commit-ID: LDOt0BpBgFe
netwerk/protocol/http/HttpChannelParent.cpp
netwerk/protocol/http/HttpChannelParent.h
--- a/netwerk/protocol/http/HttpChannelParent.cpp
+++ b/netwerk/protocol/http/HttpChannelParent.cpp
@@ -231,37 +231,53 @@ HttpChannelParent::GetInterface(const ns
   return QueryInterface(aIID, result);
 }
 
 //-----------------------------------------------------------------------------
 // HttpChannelParent::PHttpChannelParent
 //-----------------------------------------------------------------------------
 
 void
+HttpChannelParent::AsyncOpenFailed(nsresult aRv)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  MOZ_ASSERT(NS_FAILED(aRv));
+
+  // Break the reference cycle among HttpChannelParent,
+  // HttpChannelParentListener, and nsHttpChannel to avoid memory leakage.
+  mChannel = nullptr;
+  mParentListener = nullptr;
+
+  if (!mIPCClosed) {
+    Unused << SendFailedAsyncOpen(aRv);
+  }
+}
+
+void
 HttpChannelParent::InvokeAsyncOpen(nsresult rv)
 {
   if (NS_FAILED(rv)) {
-    Unused << SendFailedAsyncOpen(rv);
+    AsyncOpenFailed(rv);
     return;
   }
 
   nsCOMPtr<nsILoadInfo> loadInfo;
   rv = mChannel->GetLoadInfo(getter_AddRefs(loadInfo));
   if (NS_FAILED(rv)) {
-    Unused << SendFailedAsyncOpen(rv);
+    AsyncOpenFailed(rv);
     return;
   }
   if (loadInfo && loadInfo->GetEnforceSecurity()) {
     rv = mChannel->AsyncOpen2(mParentListener);
   }
   else {
     rv = mChannel->AsyncOpen(mParentListener, nullptr);
   }
   if (NS_FAILED(rv)) {
-    Unused << SendFailedAsyncOpen(rv);
+    AsyncOpenFailed(rv);
   }
 }
 
 namespace {
 class InvokeAsyncOpen : public Runnable
 {
   nsMainThreadPtrHandle<nsIInterfaceRequestor> mChannel;
   nsresult mStatus;
@@ -382,71 +398,70 @@ HttpChannelParent::DoAsyncOpen(  const U
 
   nsCOMPtr<nsIChannel> channel;
   rv = NS_NewChannelInternal(getter_AddRefs(channel), uri, loadInfo,
                              nullptr, nullptr, aLoadFlags, ios);
   if (NS_FAILED(rv)) {
     return SendFailedAsyncOpen(rv);
   }
 
-  mChannel = do_QueryObject(channel, &rv);
+  RefPtr<nsHttpChannel> httpChannel = do_QueryObject(channel, &rv);
   if (NS_FAILED(rv)) {
     return SendFailedAsyncOpen(rv);
   }
 
   // Set the channelId allocated in child to the parent instance
-  mChannel->SetChannelId(aChannelId);
-  mChannel->SetTopLevelContentWindowId(aContentWindowId);
-  mChannel->SetTopLevelOuterContentWindowId(aTopLevelOuterContentWindowId);
+  httpChannel->SetChannelId(aChannelId);
+  httpChannel->SetTopLevelContentWindowId(aContentWindowId);
+  httpChannel->SetTopLevelOuterContentWindowId(aTopLevelOuterContentWindowId);
 
-  mChannel->SetWarningReporter(this);
-  mChannel->SetTimingEnabled(true);
+  httpChannel->SetWarningReporter(this);
+  httpChannel->SetTimingEnabled(true);
   if (mPBOverride != kPBOverride_Unset) {
-    mChannel->SetPrivate(mPBOverride == kPBOverride_Private ? true : false);
+    httpChannel->SetPrivate(mPBOverride == kPBOverride_Private ? true : false);
   }
 
   if (doResumeAt)
-    mChannel->ResumeAt(startPos, entityID);
+    httpChannel->ResumeAt(startPos, entityID);
 
   if (originalUri)
-    mChannel->SetOriginalURI(originalUri);
+    httpChannel->SetOriginalURI(originalUri);
   if (docUri)
-    mChannel->SetDocumentURI(docUri);
+    httpChannel->SetDocumentURI(docUri);
   if (referrerUri) {
-    rv = mChannel->SetReferrerWithPolicyInternal(referrerUri, aReferrerPolicy);
+    rv = httpChannel->SetReferrerWithPolicyInternal(referrerUri, aReferrerPolicy);
     MOZ_ASSERT(NS_SUCCEEDED(rv));
   }
   if (apiRedirectToUri)
-    mChannel->RedirectTo(apiRedirectToUri);
+    httpChannel->RedirectTo(apiRedirectToUri);
   if (topWindowUri) {
-    rv = mChannel->SetTopWindowURI(topWindowUri);
+    rv = httpChannel->SetTopWindowURI(topWindowUri);
     MOZ_ASSERT(NS_SUCCEEDED(rv));
   }
   if (aLoadFlags != nsIRequest::LOAD_NORMAL)
-    mChannel->SetLoadFlags(aLoadFlags);
+    httpChannel->SetLoadFlags(aLoadFlags);
 
   for (uint32_t i = 0; i < requestHeaders.Length(); i++) {
     if (requestHeaders[i].mEmpty) {
-      mChannel->SetEmptyRequestHeader(requestHeaders[i].mHeader);
+      httpChannel->SetEmptyRequestHeader(requestHeaders[i].mHeader);
     } else {
-      mChannel->SetRequestHeader(requestHeaders[i].mHeader,
+      httpChannel->SetRequestHeader(requestHeaders[i].mHeader,
                                  requestHeaders[i].mValue,
                                  requestHeaders[i].mMerge);
     }
   }
 
-  mParentListener = new HttpChannelParentListener(this);
+  RefPtr<HttpChannelParentListener> parentListener
+    = new HttpChannelParentListener(this);
 
-  mChannel->SetNotificationCallbacks(mParentListener);
-
-  mChannel->SetRequestMethod(nsDependentCString(requestMethod.get()));
+  httpChannel->SetRequestMethod(nsDependentCString(requestMethod.get()));
 
   if (aCorsPreflightArgs.type() == OptionalCorsPreflightArgs::TCorsPreflightArgs) {
     const CorsPreflightArgs& args = aCorsPreflightArgs.get_CorsPreflightArgs();
-    mChannel->SetCorsPreflightParameters(args.unsafeHeaders());
+    httpChannel->SetCorsPreflightParameters(args.unsafeHeaders());
   }
 
   bool delayAsyncOpen = false;
   nsCOMPtr<nsIInputStream> stream = DeserializeIPCStream(uploadStream);
   if (stream) {
     // FIXME: The fast path of using the existing stream currently only applies to streams
     //   that have had their entire contents serialized from the child at this point.
     //   Once bug 1294446 and bug 1294450 are fixed it is worth revisiting this heuristic.
@@ -492,83 +507,83 @@ HttpChannelParent::DoAsyncOpen(  const U
       // the AsyncOpen process once the full stream has been received.
       rv = NS_AsyncCopy(stream, sink, target, NS_ASYNCCOPY_VIA_READSEGMENTS,
                         kBufferSize, // copy segment size
                         UploadCopyComplete, closure.release());
       if (NS_WARN_IF(NS_FAILED(rv))) {
         return SendFailedAsyncOpen(rv);
       }
 
-      mChannel->InternalSetUploadStream(newUploadStream);
+      httpChannel->InternalSetUploadStream(newUploadStream);
     } else {
-      mChannel->InternalSetUploadStream(stream);
+      httpChannel->InternalSetUploadStream(stream);
     }
-    mChannel->SetUploadStreamHasHeaders(uploadStreamHasHeaders);
+    httpChannel->SetUploadStreamHasHeaders(uploadStreamHasHeaders);
   }
 
   if (aSynthesizedResponseHead.type() == OptionalHttpResponseHead::TnsHttpResponseHead) {
-    mParentListener->SetupInterception(aSynthesizedResponseHead.get_nsHttpResponseHead());
+    parentListener->SetupInterception(aSynthesizedResponseHead.get_nsHttpResponseHead());
     mWillSynthesizeResponse = true;
-    mChannel->SetCouldBeSynthesized();
+    httpChannel->SetCouldBeSynthesized();
 
     if (!aSecurityInfoSerialization.IsEmpty()) {
       nsCOMPtr<nsISupports> secInfo;
       NS_DeserializeObject(aSecurityInfoSerialization, getter_AddRefs(secInfo));
-      rv = mChannel->OverrideSecurityInfo(secInfo);
+      rv = httpChannel->OverrideSecurityInfo(secInfo);
       MOZ_ASSERT(NS_SUCCEEDED(rv));
     }
   } else {
     nsLoadFlags newLoadFlags;
-    mChannel->GetLoadFlags(&newLoadFlags);
+    httpChannel->GetLoadFlags(&newLoadFlags);
     newLoadFlags |= nsIChannel::LOAD_BYPASS_SERVICE_WORKER;
-    mChannel->SetLoadFlags(newLoadFlags);
+    httpChannel->SetLoadFlags(newLoadFlags);
   }
 
   nsCOMPtr<nsISupportsPRUint32> cacheKey =
     do_CreateInstance(NS_SUPPORTS_PRUINT32_CONTRACTID, &rv);
   if (NS_FAILED(rv)) {
     return SendFailedAsyncOpen(rv);
   }
 
   rv = cacheKey->SetData(aCacheKey);
   if (NS_FAILED(rv)) {
     return SendFailedAsyncOpen(rv);
   }
 
-  mChannel->SetCacheKey(cacheKey);
-  mChannel->PreferAlternativeDataType(aPreferredAlternativeType);
+  httpChannel->SetCacheKey(cacheKey);
+  httpChannel->PreferAlternativeDataType(aPreferredAlternativeType);
 
-  mChannel->SetAllowStaleCacheContent(aAllowStaleCacheContent);
+  httpChannel->SetAllowStaleCacheContent(aAllowStaleCacheContent);
 
-  mChannel->SetContentType(aContentTypeHint);
+  httpChannel->SetContentType(aContentTypeHint);
 
   if (priority != nsISupportsPriority::PRIORITY_NORMAL) {
-    mChannel->SetPriority(priority);
+    httpChannel->SetPriority(priority);
   }
   if (classOfService) {
-    mChannel->SetClassFlags(classOfService);
+    httpChannel->SetClassFlags(classOfService);
   }
-  mChannel->SetRedirectionLimit(redirectionLimit);
-  mChannel->SetAllowSTS(allowSTS);
-  mChannel->SetThirdPartyFlags(thirdPartyFlags);
-  mChannel->SetAllowSpdy(allowSpdy);
-  mChannel->SetAllowAltSvc(allowAltSvc);
-  mChannel->SetBeConservative(beConservative);
-  mChannel->SetInitialRwin(aInitialRwin);
-  mChannel->SetBlockAuthPrompt(aBlockAuthPrompt);
+  httpChannel->SetRedirectionLimit(redirectionLimit);
+  httpChannel->SetAllowSTS(allowSTS);
+  httpChannel->SetThirdPartyFlags(thirdPartyFlags);
+  httpChannel->SetAllowSpdy(allowSpdy);
+  httpChannel->SetAllowAltSvc(allowAltSvc);
+  httpChannel->SetBeConservative(beConservative);
+  httpChannel->SetInitialRwin(aInitialRwin);
+  httpChannel->SetBlockAuthPrompt(aBlockAuthPrompt);
 
-  mChannel->SetLaunchServiceWorkerStart(aLaunchServiceWorkerStart);
-  mChannel->SetLaunchServiceWorkerEnd(aLaunchServiceWorkerEnd);
-  mChannel->SetDispatchFetchEventStart(aDispatchFetchEventStart);
-  mChannel->SetDispatchFetchEventEnd(aDispatchFetchEventEnd);
-  mChannel->SetHandleFetchEventStart(aHandleFetchEventStart);
-  mChannel->SetHandleFetchEventEnd(aHandleFetchEventEnd);
+  httpChannel->SetLaunchServiceWorkerStart(aLaunchServiceWorkerStart);
+  httpChannel->SetLaunchServiceWorkerEnd(aLaunchServiceWorkerEnd);
+  httpChannel->SetDispatchFetchEventStart(aDispatchFetchEventStart);
+  httpChannel->SetDispatchFetchEventEnd(aDispatchFetchEventEnd);
+  httpChannel->SetHandleFetchEventStart(aHandleFetchEventStart);
+  httpChannel->SetHandleFetchEventEnd(aHandleFetchEventEnd);
 
   nsCOMPtr<nsIApplicationCacheChannel> appCacheChan =
-    do_QueryObject(mChannel);
+    do_QueryObject(httpChannel);
   nsCOMPtr<nsIApplicationCacheService> appCacheService =
     do_GetService(NS_APPLICATIONCACHESERVICE_CONTRACTID);
 
   bool setChooseApplicationCache = chooseApplicationCache;
   if (appCacheChan && appCacheService) {
     // We might potentially want to drop this flag (that is TRUE by default)
     // after we successfully associate the channel with an application cache
     // reported by the channel child.  Dropping it here may be too early.
@@ -580,31 +595,39 @@ HttpChannelParent::DoAsyncOpen(  const U
       if (NS_SUCCEEDED(rv)) {
         appCacheChan->SetApplicationCache(appCache);
         setChooseApplicationCache = false;
       }
     }
 
     if (setChooseApplicationCache) {
       OriginAttributes attrs;
-      NS_GetOriginAttributes(mChannel, attrs);
+      NS_GetOriginAttributes(httpChannel, attrs);
 
       nsCOMPtr<nsIPrincipal> principal =
         BasePrincipal::CreateCodebasePrincipal(uri, attrs);
 
       bool chooseAppCache = false;
       // This works because we've already called SetNotificationCallbacks and
       // done mPBOverride logic by this point.
       chooseAppCache = NS_ShouldCheckAppCache(principal);
 
       appCacheChan->SetChooseApplicationCache(chooseAppCache);
     }
   }
 
-  mChannel->SetRequestContextID(aRequestContextID);
+  httpChannel->SetRequestContextID(aRequestContextID);
+
+  // Store the strong reference of channel and parent listener object until
+  // all the initialization procedure is complete without failure, to remove
+  // cycle reference in fail case and to avoid memory leakage.
+  mChannel = httpChannel.forget();
+  mParentListener = parentListener.forget();
+  mChannel->SetNotificationCallbacks(mParentListener);
+
 
   mSuspendAfterSynthesizeResponse = aSuspendAfterSynthesizeResponse;
 
   if (!delayAsyncOpen) {
     InvokeAsyncOpen(NS_OK);
   }
 
   return true;
--- a/netwerk/protocol/http/HttpChannelParent.h
+++ b/netwerk/protocol/http/HttpChannelParent.h
@@ -212,16 +212,18 @@ private:
   void DivertOnDataAvailable(const nsCString& data,
                              const uint64_t& offset,
                              const uint32_t& count);
   void DivertOnStopRequest(const nsresult& statusCode);
   void DivertComplete();
   void MaybeFlushPendingDiversion();
   void ResponseSynthesized();
 
+  void AsyncOpenFailed(nsresult aRv);
+
   friend class DivertDataAvailableEvent;
   friend class DivertStopRequestEvent;
   friend class DivertCompleteEvent;
 
   RefPtr<nsHttpChannel>       mChannel;
   nsCOMPtr<nsICacheEntry>       mCacheEntry;
   nsCOMPtr<nsIAssociatedContentSecurity>  mAssociatedContentSecurity;
   bool mIPCClosed;                // PHttpChannel actor has been Closed()