Bug 1445479 - Ensure we teardown requests on image cache validation failure paths. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Tue, 17 Apr 2018 14:42:35 -0400
changeset 414077 9a1f9331799726cf875cef4d2c2fa8a0f3aaa623
parent 414076 0e0dec87cddd04f4d6a3fb05e411ed7196d9d339
child 414078 b657b7a34615a3febf6bc83ff85623ae4476233f
push id102257
push useraosmond@gmail.com
push dateTue, 17 Apr 2018 18:42:51 +0000
treeherdermozilla-inbound@9a1f93317997 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1445479
milestone61.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 1445479 - Ensure we teardown requests on image cache validation failure paths. r=tnikkel If an imgCacheValidator object is destroyed without calling imgCacheValidator::OnStartRequest, or imgRequest::Init fails in OnStartRequest, we left the bound proxies hanging on an update. Now we cancel the new request, and bind the validating proxies to said request to ensure their listeners fail gracefully.
image/imgLoader.cpp
image/imgLoader.h
--- a/image/imgLoader.cpp
+++ b/image/imgLoader.cpp
@@ -2935,17 +2935,20 @@ imgCacheValidator::imgCacheValidator(nsP
                      mRequest->CacheKey(),
                      getter_AddRefs(mNewRequest),
                      getter_AddRefs(mNewEntry));
 }
 
 imgCacheValidator::~imgCacheValidator()
 {
   if (mRequest) {
-    mRequest->SetValidator(nullptr);
+    // If something went wrong, and we never unblocked the requests waiting on
+    // validation, now is our last chance. We will cancel the new request and
+    // switch the waiting proxies to it.
+    UpdateProxies(/* aCancelRequest */ true, /* aSyncNotify */ false);
   }
 }
 
 void
 imgCacheValidator::AddProxy(imgRequestProxy* aProxy)
 {
   // aProxy needs to be in the loadgroup since we're validating from
   // the network.
@@ -2956,18 +2959,33 @@ imgCacheValidator::AddProxy(imgRequestPr
 
 void
 imgCacheValidator::RemoveProxy(imgRequestProxy* aProxy)
 {
   mProxies.RemoveElement(aProxy);
 }
 
 void
-imgCacheValidator::UpdateProxies()
+imgCacheValidator::UpdateProxies(bool aCancelRequest, bool aSyncNotify)
 {
+  MOZ_ASSERT(mRequest);
+
+  // Clear the validator before updating the proxies. The notifications may
+  // clone an existing request, and its state could be inconsistent.
+  mRequest->SetValidator(nullptr);
+  mRequest = nullptr;
+
+  // If an error occurred, we will want to cancel the new request, and make the
+  // validating proxies point to it. Any proxies still bound to the original
+  // request which are not validating should remain untouched.
+  if (aCancelRequest) {
+    MOZ_ASSERT(mNewRequest);
+    mNewRequest->CancelAndAbort(NS_BINDING_ABORTED);
+  }
+
   // We have finished validating the request, so we can safely take ownership
   // of the proxy list. imgRequestProxy::SyncNotifyListener can mutate the list
   // if imgRequestProxy::CancelAndForgetObserver is called by its owner. Note
   // that any potential notifications should still be suppressed in
   // imgRequestProxy::ChangeOwner because we haven't cleared the validating
   // flag yet, and thus they will remain deferred.
   AutoTArray<RefPtr<imgRequestProxy>, 4> proxies(Move(mProxies));
 
@@ -2980,20 +2998,29 @@ imgCacheValidator::UpdateProxies()
                "Proxies waiting on cache validation should be "
                "deferring notifications!");
     if (mNewRequest) {
       proxy->ChangeOwner(mNewRequest);
     }
     proxy->ClearValidating();
   }
 
+  mNewRequest = nullptr;
+  mNewEntry = nullptr;
+
   for (auto& proxy : proxies) {
-    // Notify synchronously, because we're already in OnStartRequest, an
-    // asynchronously-called function.
-    proxy->SyncNotifyListener();
+    if (aSyncNotify) {
+      // Notify synchronously, because the caller knows we are already in an
+      // asynchronously-called function (e.g. OnStartRequest).
+      proxy->SyncNotifyListener();
+    } else {
+      // Notify asynchronously, because the caller does not know our current
+      // call state (e.g. ~imgCacheValidator).
+      proxy->NotifyListener();
+    }
   }
 }
 
 /** nsIRequestObserver methods **/
 
 NS_IMETHODIMP
 imgCacheValidator::OnStartRequest(nsIRequest* aRequest, nsISupports* ctxt)
 {
@@ -3026,28 +3053,22 @@ imgCacheValidator::OnStartRequest(nsIReq
     bool sameURI = false;
     if (channelURI && finalURI) {
       channelURI->Equals(finalURI, &sameURI);
     }
 
     if (isFromCache && sameURI) {
       // We don't need to load this any more.
       aRequest->Cancel(NS_BINDING_ABORTED);
+      mNewRequest = nullptr;
 
       // Clear the validator before updating the proxies. The notifications may
       // clone an existing request, and its state could be inconsistent.
       mRequest->SetLoadId(context);
-      mRequest->SetValidator(nullptr);
-
-      mRequest = nullptr;
-
-      mNewRequest = nullptr;
-      mNewEntry = nullptr;
-
-      UpdateProxies();
+      UpdateProxies(/* aCancelRequest */ false, /* aSyncNotify */ true);
       return NS_OK;
     }
   }
 
   // We can't load out of cache. We have to create a whole new request for the
   // data that's coming in off the channel.
   nsCOMPtr<nsIURI> uri;
   {
@@ -3064,42 +3085,34 @@ imgCacheValidator::OnStartRequest(nsIReq
 
   int32_t corsmode = mRequest->GetCORSMode();
   ReferrerPolicy refpol = mRequest->GetReferrerPolicy();
   nsCOMPtr<nsIPrincipal> triggeringPrincipal = mRequest->GetTriggeringPrincipal();
 
   // Doom the old request's cache entry
   mRequest->RemoveFromCache();
 
-  // Clear the validator before updating the proxies. The notifications may
-  // clone an existing request, and its state could be inconsistent.
-  mRequest->SetValidator(nullptr);
-  mRequest = nullptr;
-
   // We use originalURI here to fulfil the imgIRequest contract on GetURI.
   nsCOMPtr<nsIURI> originalURI;
   channel->GetOriginalURI(getter_AddRefs(originalURI));
   nsresult rv =
     mNewRequest->Init(originalURI, uri, mHadInsecureRedirect, aRequest, channel,
                       mNewEntry, context, triggeringPrincipal, corsmode, refpol);
   if (NS_FAILED(rv)) {
+    UpdateProxies(/* aCancelRequest */ true, /* aSyncNotify */ true);
     return rv;
   }
 
   mDestListener = new ProxyListener(mNewRequest);
 
   // Try to add the new request into the cache. Note that the entry must be in
   // the cache before the proxies' ownership changes, because adding a proxy
   // changes the caching behaviour for imgRequests.
   mImgLoader->PutIntoCache(mNewRequest->CacheKey(), mNewEntry);
-
-  UpdateProxies();
-  mNewRequest = nullptr;
-  mNewEntry = nullptr;
-
+  UpdateProxies(/* aCancelRequest */ false, /* aSyncNotify */ true);
   return mDestListener->OnStartRequest(aRequest, ctxt);
 }
 
 NS_IMETHODIMP
 imgCacheValidator::OnStopRequest(nsIRequest* aRequest,
                                  nsISupports* ctxt,
                                  nsresult status)
 {
--- a/image/imgLoader.h
+++ b/image/imgLoader.h
@@ -563,17 +563,17 @@ public:
   NS_DECL_NSITHREADRETARGETABLESTREAMLISTENER
   NS_DECL_NSISTREAMLISTENER
   NS_DECL_NSIREQUESTOBSERVER
   NS_DECL_NSICHANNELEVENTSINK
   NS_DECL_NSIINTERFACEREQUESTOR
   NS_DECL_NSIASYNCVERIFYREDIRECTCALLBACK
 
 private:
-  void UpdateProxies();
+  void UpdateProxies(bool aCancelRequest, bool aSyncNotify);
   virtual ~imgCacheValidator();
 
   nsCOMPtr<nsIStreamListener> mDestListener;
   RefPtr<nsProgressNotificationProxy> mProgressProxy;
   nsCOMPtr<nsIAsyncVerifyRedirectCallback> mRedirectCallback;
   nsCOMPtr<nsIChannel> mRedirectChannel;
 
   RefPtr<imgRequest> mRequest;