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
authorJoe Drew <joe@drew.ca>
Tue, 17 Mar 2009 17:07:16 -0400
changeset 26259 d3c2bf145fb8c11bdfc266027e50276190c7acb0
parent 26258 e743e00363e3eb8d2ef271f78423005ddafd84c0
child 26260 717dc653253c605d1d60764c4cfd04c6326e37c7
push idunknown
push userunknown
push dateunknown
reviewersvlad, bzbarsky
bugs479328, 481753
milestone1.9.2a1pre
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
modules/libpr0n/src/imgLoader.h
modules/libpr0n/src/imgRequest.cpp
modules/libpr0n/src/imgRequest.h
testing/mochitest/runtests.py.in
--- a/modules/libpr0n/src/imgLoader.cpp
+++ b/modules/libpr0n/src/imgLoader.cpp
@@ -123,18 +123,16 @@ static PRBool NewRequestAndEntry(nsIURI 
   if (!*entry) {
     delete *request;
     return PR_FALSE;
   }
 
   NS_ADDREF(*request);
   NS_ADDREF(*entry);
 
-  imgLoader::SetHasNoProxies(uri, *entry);
-
   return PR_TRUE;
 }
 
 static PRBool ShouldRevalidateEntry(imgCacheEntry *aEntry,
                               nsLoadFlags aFlags,
                               PRBool aHasExpired)
 {
   PRBool bValidateEntry = PR_FALSE;
@@ -262,36 +260,32 @@ void imgCacheEntry::TouchWithSize(PRInt3
 {
   LOG_SCOPE(gImgLog, "imgCacheEntry::TouchWithSize");
 
   mTouchedTime = SecondsFromPRTime(PR_Now());
 
   // 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);
   }
 }
 
 void imgCacheEntry::Touch(PRBool updateTime /* = PR_TRUE */)
 {
   LOG_SCOPE(gImgLog, "imgCacheEntry::Touch");
 
   if (updateTime)
     mTouchedTime = SecondsFromPRTime(PR_Now());
 
   // 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);
   }
 }
 
 void imgCacheEntry::SetHasNoProxies(PRBool hasNoProxies)
 {
@@ -706,16 +700,37 @@ PRBool imgLoader::PutIntoCache(nsIURI *k
   } else {
     PR_LOG(gImgLog, PR_LOG_DEBUG,
            ("[this=%p] imgLoader::PutIntoCache -- Element NOT already in the cache", nsnull));
   }
 
   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;
 }
 
 PRBool imgLoader::SetHasNoProxies(nsIURI *key, imgCacheEntry *entry)
 {
 #if defined(PR_LOGGING)
   nsCAutoString spec;
   key->GetSpec(spec);
@@ -1034,16 +1049,19 @@ PRBool imgLoader::RemoveFromCache(nsIURI
     if (entry->HasNoProxies()) {
       if (gCacheTracker)
         gCacheTracker->RemoveObject(entry);
       queue.Remove(entry);
     }
 
     entry->SetEvicted(PR_TRUE);
 
+    nsRefPtr<imgRequest> request(getter_AddRefs(entry->GetRequest()));
+    request->SetIsInCache(PR_FALSE);
+
     return PR_TRUE;
   }
   else
     return PR_FALSE;
 }
 
 PRBool imgLoader::RemoveFromCache(imgCacheEntry *entry)
 {
@@ -1065,16 +1083,17 @@ PRBool imgLoader::RemoveFromCache(imgCac
       if (entry->HasNoProxies()) {
         LOG_STATIC_FUNC(gImgLog, "imgLoader::RemoveFromCache removing from tracker");
         if (gCacheTracker)
           gCacheTracker->RemoveObject(entry);
         queue.Remove(entry);
       }
 
       entry->SetEvicted(PR_TRUE);
+      request->SetIsInCache(PR_FALSE);
 
       return PR_TRUE;
     }
   }
 
   return PR_FALSE;
 }
 
@@ -1264,18 +1283,17 @@ NS_IMETHODIMP imgLoader::LoadImage(nsIUR
       PR_LOG(gImgLog, PR_LOG_DEBUG,
              ("[this=%p] imgLoader::LoadImage -- AsyncOpen() failed: 0x%x\n",
               this, openRes));
       request->CancelAndAbort(openRes);
       return openRes;
     }
 
     // Try to add the new request into the cache.
-    if (!PutIntoCache(aURI, entry))
-      request->SetCacheable(PR_FALSE);
+    PutIntoCache(aURI, entry);
 
   // If we did get a cache hit, use it.
   } else {
     // XXX: Should this be executed if an expired cache entry does not have a caching channel??
     LOG_MSG_WITH_PARAM(gImgLog, 
                        "imgLoader::LoadImage |cache hit|", "request", request);
 
     // Update the request's LoadId
@@ -1401,18 +1419,17 @@ NS_IMETHODIMP imgLoader::LoadImageWithCh
     NS_ADDREF(pl);
 
     *listener = static_cast<nsIStreamListener*>(pl);
     NS_ADDREF(*listener);
 
     NS_RELEASE(pl);
 
     // Try to add the new request into the cache.
-    if (!PutIntoCache(uri, entry))
-      request->SetCacheable(PR_FALSE);
+    PutIntoCache(uri, entry);
   }
 
   // XXX: It looks like the wrong load flags are being passed in...
   requestFlags &= 0xFFFF;
 
   rv = CreateNewProxyForRequest(request, loadGroup, aObserver,
                                 requestFlags, nsnull, _retval);
   request->NotifyProxyListener(static_cast<imgRequestProxy*>(*_retval));
@@ -1681,18 +1698,17 @@ NS_IMETHODIMP imgCacheValidator::OnStart
     return NS_ERROR_OUT_OF_MEMORY;
   }
 
   mDestListener = static_cast<nsIStreamListener*>(pl);
 
   // 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))
-    request->SetCacheable(PR_FALSE);
+  sImgLoader.PutIntoCache(uri, entry);
 
   PRUint32 count = mProxies.Count();
   for (PRInt32 i = count-1; i>=0; i--) {
     imgRequestProxy *proxy = static_cast<imgRequestProxy *>(mProxies[i]);
     proxy->ChangeOwner(request);
     request->NotifyProxyListener(proxy);
   }
 
--- a/modules/libpr0n/src/imgLoader.h
+++ b/modules/libpr0n/src/imgLoader.h
@@ -163,17 +163,16 @@ private: // methods
   // Private, unimplemented copy constructor.
   imgCacheEntry(const imgCacheEntry &);
 
 private: // data
   nsAutoRefCnt mRefCnt;
   NS_DECL_OWNINGTHREAD
 
   nsRefPtr<imgRequest> mRequest;
-  nsCOMPtr<nsIURI> mKeyURI;
   PRUint32 mDataSize;
   PRInt32 mTouchedTime;
   PRInt32 mExpiryTime;
   nsExpirationState mExpirationState;
   PRPackedBool mMustValidateIfExpired : 1;
   PRPackedBool mEvicted : 1;
   PRPackedBool mHasNoProxies : 1;
 };
--- a/modules/libpr0n/src/imgRequest.cpp
+++ b/modules/libpr0n/src/imgRequest.cpp
@@ -80,17 +80,17 @@ NS_IMPL_ISUPPORTS8(imgRequest, imgILoad,
                    nsISupportsWeakReference,
                    nsIChannelEventSink,
                    nsIInterfaceRequestor)
 
 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 */
 }
 
 imgRequest::~imgRequest()
 {
   if (mKeyURI) {
     nsCAutoString spec;
@@ -414,24 +414,25 @@ nsresult imgRequest::GetSecurityInfo(nsI
   NS_IF_ADDREF(*aSecurityInfo = mSecurityInfo);
   return NS_OK;
 }
 
 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
 {
   nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mObservers);
   imgRequestProxy* proxy;
   while (iter.HasMore()) {
     proxy = iter.GetNext();
@@ -468,23 +469,20 @@ void imgRequest::AdjustPriority(imgReque
   if (mObservers.SafeElementAt(0, nsnull) != proxy)
     return;
 
   nsCOMPtr<nsISupportsPriority> p = do_QueryInterface(mRequest);
   if (p)
     p->AdjustPriority(delta);
 }
 
-void imgRequest::SetCacheable(PRBool cacheable)
+void imgRequest::SetIsInCache(PRBool incache)
 {
-  LOG_FUNC_WITH_PARAM(gImgLog, "imgRequest::SetIsCacheable", "cacheable", cacheable);
-  mIsCacheable = cacheable;
-
-  if (!mIsCacheable)
-    mCacheEntry = nsnull;
+  LOG_FUNC_WITH_PARAM(gImgLog, "imgRequest::SetIsCacheable", "incache", incache);
+  mIsInCache = incache;
 }
 
 /** imgILoad methods **/
 
 NS_IMETHODIMP imgRequest::SetImage(imgIContainer *aImage)
 {
   LOG_FUNC(gImgLog, "imgRequest::SetImage");
 
@@ -1074,35 +1072,48 @@ imgRequest::OnChannelRedirect(nsIChannel
   nsresult rv = NS_OK;
   nsCOMPtr<nsIChannelEventSink> sink(do_GetInterface(mPrevChannelSink));
   if (sink) {
     rv = sink->OnChannelRedirect(oldChannel, newChannel, flags);
     if (NS_FAILED(rv))
       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)
-  mKeyURI->GetSpec(spec);
+  nsCOMPtr<nsIURI> newURI;
+  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());
-#endif
+  if (oldspec != newspec) {
+    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) {
-    // If we don't still have a cache entry, we don't want to refresh the cache.
-    if (mKeyURI && mCacheEntry)
-      imgLoader::PutIntoCache(mKeyURI, mCacheEntry);
+    mKeyURI = newURI;
+ 
+    if (mIsInCache) {
+      // 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;
 }
--- a/modules/libpr0n/src/imgRequest.h
+++ b/modules/libpr0n/src/imgRequest.h
@@ -159,20 +159,20 @@ private:
 
   // Adjust the priority of the underlying network request by the given delta
   // on behalf of the given proxy.
   void AdjustPriority(imgRequestProxy *aProxy, PRInt32 aDelta);
 
   // 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);
+  // Set whether this request is stored in the cache. If it isn't, regardless
+  // of whether this request has a non-null mCacheEntry, this imgRequest won't
+  // try to update or modify the image cache.
+  void SetIsInCache(PRBool cacheable);
 
 public:
   NS_DECL_IMGILOAD
   NS_DECL_IMGIDECODEROBSERVER
   NS_DECL_IMGICONTAINEROBSERVER
   NS_DECL_NSISTREAMLISTENER
   NS_DECL_NSIREQUESTOBSERVER
   NS_DECL_NSICHANNELEVENTSINK
@@ -208,12 +208,12 @@ private:
   imgCacheValidator *mValidator;
   nsCategoryCache<nsIContentSniffer> mImageSniffers;
 
   PRPackedBool mIsMultiPartChannel : 1;
   PRPackedBool mLoading : 1;
   PRPackedBool mProcessing : 1;
   PRPackedBool mHadLastPart : 1;
   PRPackedBool mGotData : 1;
-  PRPackedBool mIsCacheable : 1;
+  PRPackedBool mIsInCache : 1;
 };
 
 #endif
--- a/testing/mochitest/runtests.py.in
+++ b/testing/mochitest/runtests.py.in
@@ -462,16 +462,19 @@ Are you executing $objdir/_tests/testing
       urlOpts.append("logFile=" + encodeURIComponent(options.logFile))
       urlOpts.append("fileLevel=" + encodeURIComponent(options.fileLevel))
     if options.consoleLevel:
       urlOpts.append("consoleLevel=" + encodeURIComponent(options.consoleLevel))
     if len(urlOpts) > 0:
       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"
 
   status = automation.runApp(testURL, browserEnv, options.app,
                              PROFILE_DIRECTORY, options.browserArgs,
                              runSSLTunnel = True,
                              utilityPath = options.utilityPath,