Bug 479328 and bug 481753 - Ensure imgRequest always knows when it's in the cache, and doesn't try to do cache manipulation when it isn't. Also, fix redirect handling to not simply invalidate the cache entry. r=vlad, sr=bzbarsky

modules/libpr0n/src/imgLoader.cpp

@@ -128,8 +128,6 @@ static PRBool NewRequestAndEntry(nsIURI *uri, imgRequest **request, imgCacheEntr
   NS_ADDREF(*request);
   NS_ADDREF(*entry);
 
-  imgLoader::SetHasNoProxies(uri, *entry);
-
   return PR_TRUE;
 }
 
@@ -267,8 +265,6 @@ void imgCacheEntry::TouchWithSize(PRInt32 diff)
   // Don't update the cache if we've been removed from it or it doesn't care
   // about our size or usage.
   if (!Evicted() && HasNoProxies()) {
-    // We can't use mKeyURI here, because we're not guaranteed to be updated if
-    // the request has no observers and has thus dropped its reference to us.
     nsCOMPtr<nsIURI> uri;
     mRequest->GetKeyURI(getter_AddRefs(uri));
     imgLoader::CacheEntriesChanged(uri, diff);
@@ -285,8 +281,6 @@ void imgCacheEntry::Touch(PRBool updateTime /* = PR_TRUE */)
   // Don't update the cache if we've been removed from it or it doesn't care
   // about our size or usage.
   if (!Evicted() && HasNoProxies()) {
-    // We can't use mKeyURI here, because we're not guaranteed to be updated if
-    // the request has no observers and has thus dropped its reference to us.
     nsCOMPtr<nsIURI> uri;
     mRequest->GetKeyURI(getter_AddRefs(uri));
     imgLoader::CacheEntriesChanged(uri);
@@ -711,6 +705,27 @@ PRBool imgLoader::PutIntoCache(nsIURI *key, imgCacheEntry *entry)
   if (!cache.Put(spec, entry))
     return PR_FALSE;
 
+  // We can be called to resurrect an evicted entry.
+  if (entry->Evicted())
+    entry->SetEvicted(PR_FALSE);
+
+  // If we're resurrecting an entry with no proxies, put it back in the
+  // tracker and queue.
+  if (entry->HasNoProxies()) {
+    nsresult addrv = NS_OK;
+
+    if (gCacheTracker)
+      addrv = gCacheTracker->AddObject(entry);
+
+    if (NS_SUCCEEDED(addrv)) {
+      imgCacheQueue &queue = GetCacheQueue(key);
+      queue.Push(entry);
+    }
+  }
+
+  nsRefPtr<imgRequest> request(getter_AddRefs(entry->GetRequest()));
+  request->SetIsInCache(PR_TRUE);
+
   return PR_TRUE;
 }
 
@@ -1039,6 +1054,9 @@ PRBool imgLoader::RemoveFromCache(nsIURI *aKey)
 
     entry->SetEvicted(PR_TRUE);
 
+    nsRefPtr<imgRequest> request(getter_AddRefs(entry->GetRequest()));
+    request->SetIsInCache(PR_FALSE);
+
     return PR_TRUE;
   }
   else
@@ -1070,6 +1088,7 @@ PRBool imgLoader::RemoveFromCache(imgCacheEntry *entry)
       }
 
       entry->SetEvicted(PR_TRUE);
+      request->SetIsInCache(PR_FALSE);
 
       return PR_TRUE;
     }
@@ -1269,8 +1288,7 @@ NS_IMETHODIMP imgLoader::LoadImage(nsIURI *aURI,
     }
 
     // Try to add the new request into the cache.
-    if (!PutIntoCache(aURI, entry))
+    PutIntoCache(aURI, entry);
-      request->SetCacheable(PR_FALSE);
 
   // If we did get a cache hit, use it.
   } else {
@@ -1406,8 +1424,7 @@ NS_IMETHODIMP imgLoader::LoadImageWithChannel(nsIChannel *channel, imgIDecoderOb
     NS_RELEASE(pl);
 
     // Try to add the new request into the cache.
-    if (!PutIntoCache(uri, entry))
+    PutIntoCache(uri, entry);
-      request->SetCacheable(PR_FALSE);
   }
 
   // XXX: It looks like the wrong load flags are being passed in...
@@ -1686,8 +1703,7 @@ 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.
-  if (!sImgLoader.PutIntoCache(uri, entry))
+  sImgLoader.PutIntoCache(uri, entry);
-    request->SetCacheable(PR_FALSE);
 
   PRUint32 count = mProxies.Count();
   for (PRInt32 i = count-1; i>=0; i--) {

modules/libpr0n/src/imgLoader.h

@@ -168,7 +168,6 @@ private: // data
   NS_DECL_OWNINGTHREAD
 
   nsRefPtr<imgRequest> mRequest;
-  nsCOMPtr<nsIURI> mKeyURI;
   PRUint32 mDataSize;
   PRInt32 mTouchedTime;
   PRInt32 mExpiryTime;

modules/libpr0n/src/imgRequest.cpp

@@ -85,7 +85,7 @@ imgRequest::imgRequest() :
   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)
+  mHadLastPart(PR_FALSE), mGotData(PR_FALSE), mIsInCache(PR_FALSE)
 {
   /* member initializers and constructor code */
 }
@@ -419,14 +419,15 @@ void imgRequest::RemoveFromCache()
 {
   LOG_SCOPE(gImgLog, "imgRequest::RemoveFromCache");
 
-  if (mIsCacheable) {
+  if (mIsInCache) {
+    // mCacheEntry is nulled out when we have no more observers.
     if (mCacheEntry)
       imgLoader::RemoveFromCache(mCacheEntry);
     else
       imgLoader::RemoveFromCache(mKeyURI);
-
-    mCacheEntry = nsnull;
   }
+
+  mCacheEntry = nsnull;
 }
 
 PRBool imgRequest::HaveProxyWithObserver(imgRequestProxy* aProxyToIgnore) const
@@ -473,13 +474,10 @@ void imgRequest::AdjustPriority(imgRequestProxy *proxy, PRInt32 delta)
     p->AdjustPriority(delta);
 }
 
-void imgRequest::SetCacheable(PRBool cacheable)
+void imgRequest::SetIsInCache(PRBool incache)
 {
-  LOG_FUNC_WITH_PARAM(gImgLog, "imgRequest::SetIsCacheable", "cacheable", cacheable);
+  LOG_FUNC_WITH_PARAM(gImgLog, "imgRequest::SetIsCacheable", "incache", incache);
-  mIsCacheable = cacheable;
+  mIsInCache = incache;
-
-  if (!mIsCacheable)
-    mCacheEntry = nsnull;
 }
 
 /** imgILoad methods **/
@@ -1079,29 +1077,42 @@ imgRequest::OnChannelRedirect(nsIChannel *oldChannel, nsIChannel *newChannel, PR
       return rv;
   }
 
-#if defined(PR_LOGGING)
-  nsCAutoString spec;
-  mKeyURI->GetSpec(spec);
-
-  LOG_MSG_WITH_PARAM(gImgLog, "imgRequest::OnChannelRedirect", "old", spec.get());
-#endif
-
-  RemoveFromCache();
-
   mChannel = newChannel;
 
-  newChannel->GetOriginalURI(getter_AddRefs(mKeyURI));
+  // Don't make any cache changes if we're going to point to the same thing. We
+  // compare specs and not just URIs here because URIs that compare as
+  // .Equals() might have different hashes.
+  nsCAutoString oldspec;
+  if (mKeyURI)
+    mKeyURI->GetSpec(oldspec);
+  LOG_MSG_WITH_PARAM(gImgLog, "imgRequest::OnChannelRedirect", "old", oldspec.get());
 
-#if defined(PR_LOGGING)
+  nsCOMPtr<nsIURI> newURI;
-  mKeyURI->GetSpec(spec);
+  newChannel->GetOriginalURI(getter_AddRefs(newURI));
+  nsCAutoString newspec;
+  if (newURI)
+    newURI->GetSpec(newspec);
+  LOG_MSG_WITH_PARAM(gImgLog, "imgRequest::OnChannelRedirect", "new", newspec.get());
 
-  LOG_MSG_WITH_PARAM(gImgLog, "imgRequest::OnChannelRedirect", "new", spec.get());
+  if (oldspec != newspec) {
-#endif
+    if (mIsInCache) {
+      // Remove the cache entry from the cache, but don't null out mCacheEntry
+      // (as imgRequest::RemoveFromCache() does), because we need it to put
+      // ourselves back in the cache.
+      if (mCacheEntry)
+        imgLoader::RemoveFromCache(mCacheEntry);
+      else
+        imgLoader::RemoveFromCache(mKeyURI);
+    }
 
-  if (mIsCacheable) {
+    mKeyURI = newURI;
-    // If we don't still have a cache entry, we don't want to refresh the cache.
+ 
-    if (mKeyURI && mCacheEntry)
+    if (mIsInCache) {
-      imgLoader::PutIntoCache(mKeyURI, mCacheEntry);
+      // If we don't still have a URI or cache entry, we don't want to put
+      // ourselves back into the cache.
+      if (mKeyURI && mCacheEntry)
+        imgLoader::PutIntoCache(mKeyURI, mCacheEntry);
+    }
   }
 
   return rv;

modules/libpr0n/src/imgRequest.h

@@ -164,10 +164,10 @@ 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
+  // Set whether this request is stored in the cache. If it isn't, regardless
-  // cacheable, but they might not be if there is already a request with this
+  // of whether this request has a non-null mCacheEntry, this imgRequest won't
-  // key URI in the cache.
+  // try to update or modify the image cache.
-  void SetCacheable(PRBool cacheable);
+  void SetIsInCache(PRBool cacheable);
 
 public:
   NS_DECL_IMGILOAD
@@ -213,7 +213,7 @@ private:
   PRPackedBool mProcessing : 1;
   PRPackedBool mHadLastPart : 1;
   PRPackedBool mGotData : 1;
-  PRPackedBool mIsCacheable : 1;
+  PRPackedBool mIsInCache : 1;
 };
 
 #endif

testing/mochitest/runtests.py.in

@@ -467,6 +467,9 @@ Are you executing $objdir/_tests/testing/mochitest/runtests.py?"""
       testURL += "?" + "&".join(urlOpts)
 
   browserEnv["XPCOM_MEM_BLOAT_LOG"] = LEAK_REPORT_FILE
+  if options.a11y:
+    browserEnv["XPCOM_MEM_REFCNT_LOG"] = PROFILE_DIRECTORY + "/refcnt.log"
+    browserEnv["XPCOM_MEM_LOG_CLASSES"] = "imgRequest,imgCacheEntry"
 
   if options.fatalAssertions:
     browserEnv["XPCOM_DEBUG_BREAK"] = "stack-and-abort"