Bug 1271987 - After on-***-request observers and loadGroup::AddRequest are called, on a failure AsyncOpen() must return async.r=mcmanus, r=mayhemer
authorDragana Damjanovic dd.mozilla@gmail.com
Tue, 31 May 2016 22:13:23 -0700
changeset 324342 38f8d1092552919b3797d2975337e56602614b9b
parent 324341 253aa0a9809b9c3f7a4e02a3beb5baca3fc5fe13
child 324343 62903b5a04a7cabecdb76e59e65462d5bd594a8d
push id9671
push userraliiev@mozilla.com
push dateMon, 06 Jun 2016 20:27:52 +0000
treeherdermozilla-aurora@cea65ca3d0bd [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmcmanus, mayhemer
bugs1271987
milestone49.0a1
Bug 1271987 - After on-***-request observers and loadGroup::AddRequest are called, on a failure AsyncOpen() must return async.r=mcmanus, r=mayhemer MozReview-Commit-ID: AAi6R0pb6It
netwerk/protocol/http/nsHttpChannel.cpp
--- a/netwerk/protocol/http/nsHttpChannel.cpp
+++ b/netwerk/protocol/http/nsHttpChannel.cpp
@@ -5308,71 +5308,77 @@ nsHttpChannel::AsyncOpen(nsIStreamListen
     // Remember the cookie header that was set, if any
     nsAutoCString cookieHeader;
     if (NS_SUCCEEDED(mRequestHead.GetHeader(nsHttp::Cookie, cookieHeader))) {
         mUserSetCookieHeader = cookieHeader;
     }
 
     AddCookiesToRequest();
 
+    // After we notify any observers (on-opening-request, loadGroup, etc) we
+    // must return NS_OK and return any errors asynchronously via
+    // OnStart/OnStopRequest.  Observers may add a reference to the channel
+    // and expect to get OnStopRequest so they know when to drop the reference,
+    // etc.
+
     // notify "http-on-opening-request" observers, but not if this is a redirect
     if (!(mLoadFlags & LOAD_REPLACE)) {
         gHttpHandler->OnOpeningRequest(this);
     }
 
     // Set user agent override
     HttpBaseChannel::SetDocshellUserAgentOverride();
 
     mIsPending = true;
     mWasOpened = true;
 
     mListener = listener;
     mListenerContext = context;
 
-    // add ourselves to the load group.  from this point forward, we'll report
-    // all failures asynchronously.
     if (mLoadGroup)
         mLoadGroup->AddRequest(this, nullptr);
 
     // record asyncopen time unconditionally and clear it if we
     // don't want it after OnModifyRequest() weighs in. But waiting for
     // that to complete would mean we don't include proxy resolution in the
     // timing.
     mAsyncOpenTime = TimeStamp::Now();
 
     // Remember we have Authorization header set here.  We need to check on it
     // just once and early, AsyncOpen is the best place.
     mCustomAuthHeader = mRequestHead.HasHeader(nsHttp::Authorization);
 
-    // the only time we would already know the proxy information at this
-    // point would be if we were proxying a non-http protocol like ftp
-    if (!mProxyInfo && NS_SUCCEEDED(ResolveProxy()))
+    // The common case for HTTP channels is to begin proxy resolution and return
+    // at this point. The only time we know mProxyInfo already is if we're
+    // proxying a non-http protocol like ftp.
+    if (!mProxyInfo && NS_SUCCEEDED(ResolveProxy())) {
         return NS_OK;
+    }
 
     rv = BeginConnect();
-    if (NS_FAILED(rv))
-        ReleaseListeners();
-
-    return rv;
+    if (NS_FAILED(rv)) {
+        CloseCacheEntry(false);
+        AsyncAbort(rv);
+    }
+
+    return NS_OK;
 }
 
 NS_IMETHODIMP
 nsHttpChannel::AsyncOpen2(nsIStreamListener *aListener)
 {
   nsCOMPtr<nsIStreamListener> listener = aListener;
   nsresult rv = nsContentSecurityManager::doContentSecurityCheck(this, listener);
   NS_ENSURE_SUCCESS(rv, rv);
   return AsyncOpen(listener, nullptr);
 }
 
-// BeginConnect() will not call AsyncAbort() on an error and if AsyncAbort needs
-// to be called the function calling BeginConnect will need to call AsyncAbort.
-// If BeginConnect is called from AsyncOpen, AsyncnAbort doesn't need to be
-// called. If it is called form another function (e.g. the function is called
-// from OnProxyAvailable) that function should call AsyncOpen.
+// BeginConnect() SHOULD NOT call AsyncAbort(). AsyncAbort will be called by
+// functions that called BeginConnect if needed. Only AsyncOpen and
+// OnProxyAvailable ever call BeginConnect.
 nsresult
 nsHttpChannel::BeginConnect()
 {
     LOG(("nsHttpChannel::BeginConnect [this=%p]\n", this));
     nsresult rv;
 
     // Construct connection info object
     nsAutoCString host;
@@ -5530,23 +5536,22 @@ nsHttpChannel::BeginConnect()
     if (mIsPackagedAppResource) {
         // If this is a packaged app resource, the content will be fetched
         // by the packaged app service into the cache, and the cache entry will
         // be passed to OnCacheEntryAvailable.
 
         nsCOMPtr<nsIPackagedAppService> pas =
             do_GetService("@mozilla.org/network/packaged-app-service;1", &rv);
         if (NS_WARN_IF(NS_FAILED(rv))) {
-            AsyncAbort(rv);
             return rv;
         }
 
         rv = pas->GetResource(this, this);
         if (NS_FAILED(rv)) {
-            AsyncAbort(rv);
+            return rv;
         }
 
         // We need to alter the flags so the cache entry returned by the
         // packaged app service is always accepted. Revalidation is handled
         // by the service.
         mLoadFlags |= LOAD_ONLY_FROM_CACHE;
         mLoadFlags |= LOAD_FROM_CACHE;
         mLoadFlags &= ~VALIDATE_ALWAYS;
@@ -5616,18 +5621,17 @@ nsHttpChannel::BeginConnect()
     // We may have been cancelled already, either by on-modify-request
     // listeners or load group observers; in that case, we should not send the
     // request to the server
     if (mCanceled) {
         return mStatus;
     }
 
     if (!(mLoadFlags & LOAD_CLASSIFY_URI)) {
-        ContinueBeginConnect();
-        return NS_OK;
+        return ContinueBeginConnectWithResult();
     }
 
     // mLocalBlocklist is true only if tracking protection is enabled and the
     // URI is a tracking domain, it makes no guarantees about phishing or
     // malware, so if LOAD_CLASSIFY_URI is true we must call
     // nsChannelClassifier to catch phishing and malware URIs.
     bool callContinueBeginConnect = true;
     if (!mLocalBlocklist) {
@@ -5643,17 +5647,17 @@ nsHttpChannel::BeginConnect()
     // nsChannelClassifier calls ContinueBeginConnect if it has not already
     // been called, after optionally cancelling the channel once we have a
     // remote verdict. We call a concrete class instead of an nsI* that might
     // be overridden.
     LOG(("nsHttpChannel::Starting nsChannelClassifier %p [this=%p]",
          channelClassifier.get(), this));
     channelClassifier->Start(this);
     if (callContinueBeginConnect) {
-        ContinueBeginConnect();
+        return ContinueBeginConnectWithResult();
     }
     return NS_OK;
 }
 
 NS_IMETHODIMP
 nsHttpChannel::GetEncodedBodySize(uint64_t *aEncodedBodySize)
 {
     if (mCacheEntry && !mCacheEntryIsWriteOnly) {
@@ -5739,17 +5743,17 @@ nsHttpChannel::ContinueBeginConnectWithR
     return rv;
 }
 
 void
 nsHttpChannel::ContinueBeginConnect()
 {
     nsresult rv = ContinueBeginConnectWithResult();
     if (NS_FAILED(rv)) {
-        CloseCacheEntry(true);
+        CloseCacheEntry(false);
         AsyncAbort(rv);
     }
 }
 
 //-----------------------------------------------------------------------------
 // HttpChannel::nsIClassOfService
 //-----------------------------------------------------------------------------
 NS_IMETHODIMP
@@ -5800,18 +5804,18 @@ nsHttpChannel::OnProxyAvailable(nsICance
              "Handler no longer active.\n", this));
         rv = NS_ERROR_NOT_AVAILABLE;
     }
     else {
         rv = BeginConnect();
     }
 
     if (NS_FAILED(rv)) {
+        CloseCacheEntry(false);
         AsyncAbort(rv);
-        Cancel(rv);
     }
     return rv;
 }
 
 //-----------------------------------------------------------------------------
 // nsHttpChannel::nsIProxiedChannel
 //-----------------------------------------------------------------------------