Bug 481553 - Make imgRequests not try to update their status in the image cache when they aren't stored in it. r=vlad

modules/libpr0n/src/imgLoader.cpp

@@ -692,6 +692,8 @@ PRBool imgLoader::PutIntoCache(nsIURI *key, imgCacheEntry *entry)
     nsRefPtr<imgRequest> tmpRequest = getter_AddRefs(tmpCacheEntry->GetRequest());
     void *cacheId = NS_GetCurrentThread();
 
+    // If the existing request is currently loading, or loading on a different
+    // thread, we'll leave it be, and not put this new entry into the cache.
     if (!tmpRequest->IsReusable(cacheId))
       return PR_FALSE;
 
@@ -1267,7 +1269,8 @@ NS_IMETHODIMP imgLoader::LoadImage(nsIURI *aURI,
     }
 
     // Try to add the new request into the cache.
-    PutIntoCache(aURI, entry);
+    if (!PutIntoCache(aURI, entry))
+      request->SetCacheable(PR_FALSE);
 
   // If we did get a cache hit, use it.
   } else {
@@ -1403,7 +1406,8 @@ NS_IMETHODIMP imgLoader::LoadImageWithChannel(nsIChannel *channel, imgIDecoderOb
     NS_RELEASE(pl);
 
     // Try to add the new request into the cache.
-    PutIntoCache(uri, entry);
+    if (!PutIntoCache(uri, entry))
+      request->SetCacheable(PR_FALSE);
   }
 
   // XXX: It looks like the wrong load flags are being passed in...
@@ -1682,7 +1686,8 @@ NS_IMETHODIMP imgCacheValidator::OnStartRequest(nsIRequest *aRequest, nsISupport
   // 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.
-  sImgLoader.PutIntoCache(uri, entry);
+  if (!sImgLoader.PutIntoCache(uri, entry))
+    request->SetCacheable(PR_FALSE);
 
   PRUint32 count = mProxies.Count();
   for (PRInt32 i = count-1; i>=0; i--) {

modules/libpr0n/src/imgRequest.cpp

@@ -82,10 +82,10 @@ NS_IMPL_ISUPPORTS8(imgRequest, imgILoad,
                    nsIInterfaceRequestor)
 
 imgRequest::imgRequest() : 
-  mLoading(PR_FALSE), mProcessing(PR_FALSE), mHadLastPart(PR_FALSE),
-  mGotData(PR_FALSE), mImageStatus(imgIRequest::STATUS_NONE),
-  mState(0), mCacheId(0), mValidator(nsnull), mIsMultiPartChannel(PR_FALSE),
-  mImageSniffers("image-sniffing-services") 
+  mImageStatus(imgIRequest::STATUS_NONE), mState(0), mCacheId(0), 
+  mValidator(nsnull), mImageSniffers("image-sniffing-services"), 
+  mIsMultiPartChannel(PR_FALSE), mLoading(PR_FALSE), mProcessing(PR_FALSE),
+  mHadLastPart(PR_FALSE), mGotData(PR_FALSE), mIsCacheable(PR_TRUE)
 {
   /* member initializers and constructor code */
 }
@@ -303,7 +303,7 @@ nsresult imgRequest::NotifyProxyListener(imgRequestProxy *proxy)
     proxy->OnStopDecode(GetResultFromImageStatus(mImageStatus), nsnull);
 
   if (mImage && !HaveProxyWithObserver(proxy) && proxy->HasObserver()) {
-    LOG_MSG(gImgLog, "imgRequest::AddProxy", "resetting animation");
+    LOG_MSG(gImgLog, "imgRequest::NotifyProxyListener", "resetting animation");
 
     mImage->ResetAnimation();
   }
@@ -419,12 +419,14 @@ void imgRequest::RemoveFromCache()
 {
   LOG_SCOPE(gImgLog, "imgRequest::RemoveFromCache");
 
-  if (mCacheEntry)
-    imgLoader::RemoveFromCache(mCacheEntry);
-  else
-    imgLoader::RemoveFromCache(mKeyURI);
+  if (mIsCacheable) {
+    if (mCacheEntry)
+      imgLoader::RemoveFromCache(mCacheEntry);
+    else
+      imgLoader::RemoveFromCache(mKeyURI);
 
-  mCacheEntry = nsnull;
+    mCacheEntry = nsnull;
+  }
 }
 
 PRBool imgRequest::HaveProxyWithObserver(imgRequestProxy* aProxyToIgnore) const
@@ -471,6 +473,15 @@ void imgRequest::AdjustPriority(imgRequestProxy *proxy, PRInt32 delta)
     p->AdjustPriority(delta);
 }
 
+void imgRequest::SetCacheable(PRBool cacheable)
+{
+  LOG_FUNC_WITH_PARAM(gImgLog, "imgRequest::SetIsCacheable", "cacheable", cacheable);
+  mIsCacheable = cacheable;
+
+  if (!mIsCacheable)
+    mCacheEntry = nsnull;
+}
+
 /** imgILoad methods **/
 
 NS_IMETHODIMP imgRequest::SetImage(imgIContainer *aImage)
@@ -1087,9 +1098,11 @@ imgRequest::OnChannelRedirect(nsIChannel *oldChannel, nsIChannel *newChannel, PR
   LOG_MSG_WITH_PARAM(gImgLog, "imgRequest::OnChannelRedirect", "new", spec.get());
 #endif
 
-  // If we don't still have a cache entry, we don't want to refresh the cache.
-  if (mKeyURI && mCacheEntry)
-    imgLoader::PutIntoCache(mKeyURI, mCacheEntry);
+  if (mIsCacheable) {
+    // If we don't still have a cache entry, we don't want to refresh the cache.
+    if (mKeyURI && mCacheEntry)
+      imgLoader::PutIntoCache(mKeyURI, mCacheEntry);
+  }
 
   return rv;
 }

modules/libpr0n/src/imgRequest.h

@@ -164,6 +164,11 @@ private:
   // Return whether we've seen some data at this point
   PRBool HasTransferredData() const { return mGotData; }
 
+  // Set whether this request is cacheable. By default, all requests are
+  // cacheable, but they might not be if there is already a request with this
+  // key URI in the cache.
+  void SetCacheable(PRBool cacheable);
+
 public:
   NS_DECL_IMGILOAD
   NS_DECL_IMGIDECODEROBSERVER
@@ -189,10 +194,6 @@ private:
 
   nsTObserverArray<imgRequestProxy*> mObservers;
 
-  PRPackedBool mLoading;
-  PRPackedBool mProcessing;
-  PRPackedBool mHadLastPart;
-  PRPackedBool mGotData;
   PRUint32 mImageStatus;
   PRUint32 mState;
   nsCString mContentType;
@@ -205,9 +206,14 @@ private:
   PRTime mLoadTime;
 
   imgCacheValidator *mValidator;
-  PRBool   mIsMultiPartChannel;
-
   nsCategoryCache<nsIContentSniffer> mImageSniffers;
+
+  PRPackedBool mIsMultiPartChannel : 1;
+  PRPackedBool mLoading : 1;
+  PRPackedBool mProcessing : 1;
+  PRPackedBool mHadLastPart : 1;
+  PRPackedBool mGotData : 1;
+  PRPackedBool mIsCacheable : 1;
 };
 
 #endif