From 194aa88773b3d8f59672fd4c43d1a410d6fc274e Mon Sep 17 00:00:00 2001 From: Seth Fowler Date: Wed, 20 May 2015 10:21:13 -0700 Subject: [PATCH] Bug 1160703 (Part 3) - Associate an imgRequest with its ImageCacheKey. r=baku --- image/imgLoader.cpp | 118 ++++++++++++++++++------------------------- image/imgRequest.cpp | 4 +- image/imgRequest.h | 10 +++- 3 files changed, 62 insertions(+), 70 deletions(-) diff --git a/image/imgLoader.cpp b/image/imgLoader.cpp index 28ee443e933..80c625c2c6f 100644 --- a/image/imgLoader.cpp +++ b/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 request = new imgRequest(aLoader); + nsRefPtr request = new imgRequest(aLoader, aKey); nsRefPtr 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 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 kungFuDeathGrip(entry); if (PR_LOG_TEST(GetImgLog(), PR_LOG_DEBUG)) { - nsRefPtr req(entry->GetRequest()); + nsRefPtr req = entry->GetRequest(); if (req) { - nsRefPtr 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 uri; - aRequest->GetURI(getter_AddRefs(uri)); - nsAutoCString spec; - uri->GetSpec(spec); - - LOG_STATIC_FUNC_WITH_PARAM(GetImgLog(), - "imgLoader::SetHasNoProxies", "uri", spec.get()); - } + LOG_STATIC_FUNC_WITH_PARAM(GetImgLog(), + "imgLoader::SetHasNoProxies", "uri", + aRequest->CacheKey().Spec()); aEntry->SetHasNoProxies(true); @@ -1498,10 +1483,7 @@ imgLoader::SetHasProxies(imgRequest* aRequest) { VerifyCacheSizes(); - nsRefPtr uri; - aRequest->GetURI(getter_AddRefs(uri)); - ImageCacheKey key(uri); - + const ImageCacheKey& key = aRequest->CacheKey(); 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 req(entry->GetRequest()); + nsRefPtr req = entry->GetRequest(); if (req) { - nsRefPtr 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,36 +1856,30 @@ imgLoader::RemoveFromCache(imgCacheEntry* entry) nsRefPtr request = entry->GetRequest(); if (request) { - nsRefPtr uri; - if (NS_SUCCEEDED(request->GetURI(getter_AddRefs(uri))) && uri) { - ImageCacheKey key(uri); + const ImageCacheKey& key = request->CacheKey(); + imgCacheTable& cache = GetCache(key); + imgCacheQueue& queue = GetCacheQueue(key); - imgCacheTable& cache = GetCache(key); - imgCacheQueue& queue = GetCacheQueue(key); + LOG_STATIC_FUNC_WITH_PARAM(GetImgLog(), + "imgLoader::RemoveFromCache", "entry's uri", + key.Spec()); -#ifdef DEBUG - LOG_STATIC_FUNC_WITH_PARAM(GetImgLog(), - "imgLoader::RemoveFromCache", "entry's uri", - key.Spec()); -#endif + cache.Remove(key); - cache.Remove(key); - - if (entry->HasNoProxies()) { - LOG_STATIC_FUNC(GetImgLog(), - "imgLoader::RemoveFromCache removing from tracker"); - if (mCacheTracker) { - mCacheTracker->RemoveObject(entry); - } - queue.Remove(entry); + if (entry->HasNoProxies()) { + LOG_STATIC_FUNC(GetImgLog(), + "imgLoader::RemoveFromCache removing from tracker"); + if (mCacheTracker) { + mCacheTracker->RemoveObject(entry); } - - entry->SetEvicted(true); - request->SetIsInCache(false); - AddToUncachedImages(request); - - return true; + queue.Remove(entry); } + + entry->SetEvicted(true); + request->SetIsInCache(false); + AddToUncachedImages(request); + + 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(*_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 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--) { diff --git a/image/imgRequest.cpp b/image/imgRequest.cpp index 69dbc231045..704c9233c22 100644 --- a/image/imgRequest.cpp +++ b/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) diff --git a/image/imgRequest.h b/image/imgRequest.h index 4beb25fa80e..e39cee3f804 100644 --- a/image/imgRequest.h +++ b/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 mCacheEntry; + /// The key under which this imgRequest is stored in the image cache. + ImageCacheKey mCacheKey; + void* mLoadId; imgCacheValidator* mValidator;