Bug 1160703 (Part 3) - Associate an imgRequest with its ImageCacheKey. r=baku

image/imgLoader.cpp

@@ -549,9 +549,10 @@ nsProgressNotificationProxy::GetInterface(const nsIID& iid,
 
 static void
 NewRequestAndEntry(bool aForcePrincipalCheckForCacheEntry, imgLoader* aLoader,
+                   const ImageCacheKey& aKey,
                    imgRequest** aRequest, imgCacheEntry** aEntry)
 {
-  nsRefPtr<imgRequest> request = new imgRequest(aLoader);
+  nsRefPtr<imgRequest> request = new imgRequest(aLoader, aKey);
   nsRefPtr<imgCacheEntry> entry =
     new imgCacheEntry(aLoader, request, aForcePrincipalCheckForCacheEntry);
   aLoader->AddToUncachedImages(request);
@@ -894,18 +895,12 @@ void
 imgCacheEntry::SetHasNoProxies(bool hasNoProxies)
 {
   if (PR_LOG_TEST(GetImgLog(), PR_LOG_DEBUG)) {
-    nsRefPtr<ImageURL> uri;
-    mRequest->GetURI(getter_AddRefs(uri));
-    nsAutoCString spec;
-    if (uri) {
-      uri->GetSpec(spec);
-    }
     if (hasNoProxies) {
       LOG_FUNC_WITH_PARAM(GetImgLog(), "imgCacheEntry::SetHasNoProxies true",
-                          "uri", spec.get());
+                          "uri", mRequest->CacheKey().Spec());
     } else {
       LOG_FUNC_WITH_PARAM(GetImgLog(), "imgCacheEntry::SetHasNoProxies false",
-                          "uri", spec.get());
+                          "uri", mRequest->CacheKey().Spec());
     }
   }
 
@@ -1078,15 +1073,11 @@ imgCacheExpirationTracker::NotifyExpired(imgCacheEntry* entry)
   nsRefPtr<imgCacheEntry> kungFuDeathGrip(entry);
 
   if (PR_LOG_TEST(GetImgLog(), PR_LOG_DEBUG)) {
-    nsRefPtr<imgRequest> req(entry->GetRequest());
+    nsRefPtr<imgRequest> req = entry->GetRequest();
     if (req) {
-      nsRefPtr<ImageURL> uri;
-      req->GetURI(getter_AddRefs(uri));
-      nsAutoCString spec;
-      uri->GetSpec(spec);
       LOG_FUNC_WITH_PARAM(GetImgLog(),
                          "imgCacheExpirationTracker::NotifyExpired",
-                         "entry", spec.get());
+                         "entry", req->CacheKey().Spec());
     }
   }
 
@@ -1459,15 +1450,9 @@ imgLoader::PutIntoCache(const ImageCacheKey& aKey, imgCacheEntry* entry)
 bool
 imgLoader::SetHasNoProxies(imgRequest* aRequest, imgCacheEntry* aEntry)
 {
-  if (PR_LOG_TEST(GetImgLog(), PR_LOG_DEBUG)) {
-    nsRefPtr<ImageURL> uri;
-    aRequest->GetURI(getter_AddRefs(uri));
-    nsAutoCString spec;
-    uri->GetSpec(spec);
-
   LOG_STATIC_FUNC_WITH_PARAM(GetImgLog(),
-                               "imgLoader::SetHasNoProxies", "uri", spec.get());
+                             "imgLoader::SetHasNoProxies", "uri",
-  }
+                             aRequest->CacheKey().Spec());
 
   aEntry->SetHasNoProxies(true);
 
@@ -1498,10 +1483,7 @@ imgLoader::SetHasProxies(imgRequest* aRequest)
 {
   VerifyCacheSizes();
 
-  nsRefPtr<ImageURL> uri;
+  const ImageCacheKey& key = aRequest->CacheKey();
-  aRequest->GetURI(getter_AddRefs(uri));
-  ImageCacheKey key(uri);
-
   imgCacheTable& cache = GetCache(key);
 
   LOG_STATIC_FUNC_WITH_PARAM(GetImgLog(),
@@ -1552,15 +1534,11 @@ imgLoader::CheckCacheLimits(imgCacheTable& cache, imgCacheQueue& queue)
     NS_ASSERTION(entry, "imgLoader::CheckCacheLimits -- NULL entry pointer");
 
     if (PR_LOG_TEST(GetImgLog(), PR_LOG_DEBUG)) {
-      nsRefPtr<imgRequest> req(entry->GetRequest());
+      nsRefPtr<imgRequest> req = entry->GetRequest();
       if (req) {
-        nsRefPtr<ImageURL> uri;
-        req->GetURI(getter_AddRefs(uri));
-        nsAutoCString spec;
-        uri->GetSpec(spec);
         LOG_STATIC_FUNC_WITH_PARAM(GetImgLog(),
                                    "imgLoader::CheckCacheLimits",
-                                   "entry", spec.get());
+                                   "entry", req->CacheKey().Spec());
       }
     }
 
@@ -1878,18 +1856,13 @@ imgLoader::RemoveFromCache(imgCacheEntry* entry)
 
   nsRefPtr<imgRequest> request = entry->GetRequest();
   if (request) {
-    nsRefPtr<ImageURL> uri;
+    const ImageCacheKey& key = request->CacheKey();
-    if (NS_SUCCEEDED(request->GetURI(getter_AddRefs(uri))) && uri) {
-      ImageCacheKey key(uri);
-
     imgCacheTable& cache = GetCache(key);
     imgCacheQueue& queue = GetCacheQueue(key);
 
-#ifdef DEBUG
     LOG_STATIC_FUNC_WITH_PARAM(GetImgLog(),
                                "imgLoader::RemoveFromCache", "entry's uri",
                                key.Spec());
-#endif
 
     cache.Remove(key);
 
@@ -1908,7 +1881,6 @@ imgLoader::RemoveFromCache(imgCacheEntry* entry)
 
     return true;
   }
-  }
 
   return false;
 }
@@ -2189,7 +2161,8 @@ imgLoader::LoadImage(nsIURI* aURI,
 
     MOZ_ASSERT(NS_UsePrivateBrowsing(newChannel) == mRespectPrivacy);
 
-    NewRequestAndEntry(forcePrincipalCheck, this, getter_AddRefs(request),
+    NewRequestAndEntry(forcePrincipalCheck, this, key,
+                       getter_AddRefs(request),
                        getter_AddRefs(entry));
 
     PR_LOG(GetImgLog(), PR_LOG_DEBUG,
@@ -2440,16 +2413,24 @@ imgLoader::LoadImageWithChannel(nsIChannel* channel,
                                   requestFlags, _retval);
     static_cast<imgRequestProxy*>(*_retval)->NotifyListener();
   } else {
-    // Default to doing a principal check because we don't know who
-    // started that load and whether their principal ended up being
-    // inherited on the channel.
-    NewRequestAndEntry(true, this, getter_AddRefs(request),
-                       getter_AddRefs(entry));
-
     // We use originalURI here to fulfil the imgIRequest contract on GetURI.
     nsCOMPtr<nsIURI> originalURI;
     channel->GetOriginalURI(getter_AddRefs(originalURI));
 
+    // XXX(seth): We should be able to just use |key| here, except that |key| is
+    // constructed above with the *current URI* and not the *original URI*. I'm
+    // pretty sure this is a bug, and it's preventing us from ever getting a
+    // cache hit in LoadImageWithChannel when redirects are involved.
+    ImageCacheKey originalURIKey(originalURI);
+
+    // Default to doing a principal check because we don't know who
+    // started that load and whether their principal ended up being
+    // inherited on the channel.
+    NewRequestAndEntry(/* aForcePrincipalCheckForCacheEntry = */ true,
+                       this, originalURIKey,
+                       getter_AddRefs(request),
+                       getter_AddRefs(entry));
+
     // No principal specified here, because we're not passed one.
     // In LoadImageWithChannel, the redirects that may have been
     // assoicated with this load would have gone through necko.
@@ -2467,7 +2448,7 @@ imgLoader::LoadImageWithChannel(nsIChannel* channel,
     pl.forget(listener);
 
     // Try to add the new request into the cache.
-    PutIntoCache(ImageCacheKey(originalURI), entry);
+    PutIntoCache(originalURIKey, entry);
 
     rv = CreateNewProxyForRequest(request, loadGroup, aObserver,
                                   requestFlags, _retval);
@@ -2719,6 +2700,7 @@ imgCacheValidator::imgCacheValidator(nsProgressNotificationProxy* progress,
    mHadInsecureRedirect(false)
 {
   NewRequestAndEntry(forcePrincipalCheckForCacheEntry, loader,
+                     mRequest->CacheKey(),
                      getter_AddRefs(mNewRequest),
                      getter_AddRefs(mNewEntry));
 }
@@ -2847,7 +2829,7 @@ imgCacheValidator::OnStartRequest(nsIRequest* aRequest, nsISupports* ctxt)
   // 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(ImageCacheKey(originalURI), mNewEntry);
+  mImgLoader->PutIntoCache(mNewRequest->CacheKey(), mNewEntry);
 
   uint32_t count = mProxies.Count();
   for (int32_t i = count-1; i>=0; i--) {

image/imgRequest.cpp

@@ -60,8 +60,10 @@ NS_IMPL_ISUPPORTS(imgRequest,
                   nsIInterfaceRequestor,
                   nsIAsyncVerifyRedirectCallback)
 
-imgRequest::imgRequest(imgLoader* aLoader)
+imgRequest::imgRequest(imgLoader* aLoader, const ImageCacheKey& aCacheKey)
  : mLoader(aLoader)
+ , mCacheKey(aCacheKey)
+ , mLoadId(nullptr)
  , mValidator(nullptr)
  , mInnerWindowId(0)
  , mCORSMode(imgIRequest::CORS_NONE)

image/imgRequest.h

@@ -21,6 +21,7 @@
 #include "nsIAsyncVerifyRedirectCallback.h"
 #include "mozilla/Mutex.h"
 #include "mozilla/net/ReferrerPolicy.h"
+#include "ImageCacheKey.h"
 
 class imgCacheValidator;
 class imgLoader;
@@ -49,12 +50,13 @@ class imgRequest final : public nsIStreamListener,
                          public nsIAsyncVerifyRedirectCallback
 {
   typedef mozilla::image::Image Image;
+  typedef mozilla::image::ImageCacheKey ImageCacheKey;
   typedef mozilla::image::ImageURL ImageURL;
   typedef mozilla::image::ProgressTracker ProgressTracker;
   typedef mozilla::net::ReferrerPolicy ReferrerPolicy;
 
 public:
-  explicit imgRequest(imgLoader* aLoader);
+  imgRequest(imgLoader* aLoader, const ImageCacheKey& aCacheKey);
 
   NS_DECL_THREADSAFE_ISUPPORTS
   NS_DECL_NSISTREAMLISTENER
@@ -143,6 +145,9 @@ public:
   // Get the current principal of the image. No AddRefing.
   inline nsIPrincipal* GetPrincipal() const { return mPrincipal.get(); }
 
+  /// Get the ImageCacheKey associated with this request.
+  const ImageCacheKey& CacheKey() const { return mCacheKey; }
+
   // Resize the cache entry to 0 if it exists
   void ResetCacheEntry();
 
@@ -246,6 +251,9 @@ private:
   /* we hold on to this to this so long as we have observers */
   nsRefPtr<imgCacheEntry> mCacheEntry;
 
+  /// The key under which this imgRequest is stored in the image cache.
+  ImageCacheKey mCacheKey;
+
   void* mLoadId;
 
   imgCacheValidator* mValidator;