Bug 466586 - Only expire cache elements that don't have any proxies/observers. r=stuart sr=vlad a=blocking1.9.1+
authorJoe Drew <joe@drew.ca>
Fri, 30 Jan 2009 21:17:47 -0500
changeset 25078 a6b38a1f66241a06b91b0d7d974c02adcc4344d7
parent 25077 c48c9cc923204e79109706784069271e46acd896
child 25079 f5cf71cfe64a13fa0228ae578acc7ae3238bef8e
push idunknown
push userunknown
push dateunknown
reviewersstuart, vlad, blocking1.9.1
bugs466586
milestone1.9.2a1pre
Bug 466586 - Only expire cache elements that don't have any proxies/observers. r=stuart sr=vlad a=blocking1.9.1+ The image cache is now composed of two things: a heap of tracked items that can be expired and evicted, and a superset of that which hashes from URI to cache element. Cache elements start out in the second set, and are moved to the first (and start to be tracked for expiry) once they have no observers.
modules/libpr0n/src/imgLoader.cpp
modules/libpr0n/src/imgLoader.h
modules/libpr0n/src/imgRequest.cpp
modules/libpr0n/src/imgRequest.h
--- a/modules/libpr0n/src/imgLoader.cpp
+++ b/modules/libpr0n/src/imgLoader.cpp
@@ -123,16 +123,18 @@ 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;
@@ -242,46 +244,77 @@ static PRUint32 SecondsFromPRTime(PRTime
 }
 
 imgCacheEntry::imgCacheEntry(imgRequest *request, PRBool mustValidateIfExpired /* = PR_FALSE */)
  : mRequest(request),
    mDataSize(0),
    mTouchedTime(SecondsFromPRTime(PR_Now())),
    mExpiryTime(0),
    mMustValidateIfExpired(mustValidateIfExpired),
-   mEvicted(PR_FALSE)
+   mEvicted(PR_FALSE),
+   mHasNoProxies(PR_TRUE)
 {}
 
+imgCacheEntry::~imgCacheEntry()
+{
+  LOG_FUNC(gImgLog, "imgCacheEntry::~imgCacheEntry()");
+}
+
 void imgCacheEntry::TouchWithSize(PRInt32 diff)
 {
   LOG_SCOPE(gImgLog, "imgCacheEntry::TouchWithSize");
 
   mTouchedTime = SecondsFromPRTime(PR_Now());
 
-  if (!Evicted()) {
+  // 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());
 
-  if (!Evicted()) {
+  // 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)
+{
+#if defined(PR_LOGGING)
+  nsCOMPtr<nsIURI> uri;
+  mRequest->GetKeyURI(getter_AddRefs(uri));
+  nsCAutoString spec;
+  if (uri)
+    uri->GetSpec(spec);
+  if (hasNoProxies)
+    LOG_FUNC_WITH_PARAM(gImgLog, "imgCacheEntry::SetHasNoProxies true", "uri", spec.get());
+  else
+    LOG_FUNC_WITH_PARAM(gImgLog, "imgCacheEntry::SetHasNoProxies false", "uri", spec.get());
+#endif
+
+  mHasNoProxies = hasNoProxies;
+}
+
 imgCacheQueue::imgCacheQueue()
  : mDirty(PR_FALSE),
    mSize(0)
 {}
 
 void imgCacheQueue::UpdateSize(PRInt32 diff)
 {
   mSize += diff;
@@ -444,16 +477,27 @@ protected:
 };
 
 imgCacheExpirationTracker::imgCacheExpirationTracker()
  : nsExpirationTracker<imgCacheEntry, 3>(TIMEOUT_SECONDS * 1000)
 {}
 
 void imgCacheExpirationTracker::NotifyExpired(imgCacheEntry *entry)
 {
+#if defined(PR_LOGGING)
+  nsRefPtr<imgRequest> req(entry->GetRequest());
+  if (req) {
+    nsCOMPtr<nsIURI> uri;
+    req->GetKeyURI(getter_AddRefs(uri));
+    nsCAutoString spec;
+    uri->GetSpec(spec);
+    LOG_FUNC_WITH_PARAM(gImgLog, "imgCacheExpirationTracker::NotifyExpired", "entry", spec.get());
+  }
+#endif
+
   // We can be called multiple times on the same entry. Don't do work multiple
   // times.
   if (!entry->Evicted())
     imgLoader::RemoveFromCache(entry);
 
   imgLoader::VerifyCacheSizes();
 }
 
@@ -484,23 +528,23 @@ imgLoader::~imgLoader()
   /* destructor code */
 }
 
 void imgLoader::VerifyCacheSizes()
 {
   if (!gCacheTracker)
     return;
 
+  PRUint32 cachesize = sCache.Count() + sChromeCache.Count();
   PRUint32 queuesize = sCacheQueue.GetNumElements() + sChromeCacheQueue.GetNumElements();
-  PRUint32 cachesize = sCache.Count() + sChromeCache.Count();
   PRUint32 trackersize = 0;
   for (nsExpirationTracker<imgCacheEntry, 3>::Iterator it(gCacheTracker); it.Next(); )
     trackersize++;
-  NS_ASSERTION(queuesize == cachesize, "Queue and cache sizes out of sync!");
-  NS_ASSERTION(queuesize == trackersize, "Queue and tracker sizes out of sync!");
+  NS_ABORT_IF_FALSE(queuesize == trackersize, "Queue and tracker sizes out of sync!");
+  NS_ABORT_IF_FALSE(queuesize <= cachesize, "Queue has more elements than cache!");
 }
 
 imgLoader::imgCacheTable & imgLoader::GetCache(nsIURI *aURI)
 {
   PRBool chrome = PR_FALSE;
   aURI->SchemeIs("chrome", &chrome);
   if (chrome)
     return sChromeCache;
@@ -588,18 +632,19 @@ NS_IMETHODIMP imgLoader::FindEntryProper
   nsRefPtr<imgCacheEntry> entry;
   nsCAutoString spec;
   imgCacheTable &cache = GetCache(uri);
 
   uri->GetSpec(spec);
   *_retval = nsnull;
 
   if (cache.Get(spec, getter_AddRefs(entry)) && entry) {
-    if (gCacheTracker)
+    if (gCacheTracker && entry->HasNoProxies())
       gCacheTracker->MarkUsed(entry);
+
     nsRefPtr<imgRequest> request = getter_AddRefs(entry->GetRequest());
     if (request) {
       *_retval = request->Properties();
       NS_ADDREF(*_retval);
     }
   }
 
   return NS_OK;
@@ -621,23 +666,23 @@ nsresult imgLoader::ClearChromeImageCach
 
 nsresult imgLoader::ClearImageCache()
 {
   return EvictEntries(sCache, sCacheQueue);
 }
 
 PRBool imgLoader::PutIntoCache(nsIURI *key, imgCacheEntry *entry)
 {
-  LOG_STATIC_FUNC(gImgLog, "imgLoader::PutIntoCache");
-
   imgCacheTable &cache = GetCache(key);
 
   nsCAutoString spec;
   key->GetSpec(spec);
 
+  LOG_STATIC_FUNC_WITH_PARAM(gImgLog, "imgLoader::PutIntoCache", "uri", spec.get());
+
   // Check to see if this request already exists in the cache and is being
   // loaded on a different thread. If so, don't allow this entry to be added to
   // the cache.
   nsRefPtr<imgCacheEntry> tmpCacheEntry;
   if (cache.Get(spec, getter_AddRefs(tmpCacheEntry)) && tmpCacheEntry) {
     PR_LOG(gImgLog, PR_LOG_DEBUG,
            ("[this=%p] imgLoader::PutIntoCache -- Element already in the cache", nsnull));
     nsRefPtr<imgRequest> tmpRequest = getter_AddRefs(tmpCacheEntry->GetRequest());
@@ -655,27 +700,76 @@ 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;
 
+  return PR_TRUE;
+}
+
+PRBool imgLoader::SetHasNoProxies(nsIURI *key, imgCacheEntry *entry)
+{
+#if defined(PR_LOGGING)
+  nsCAutoString spec;
+  key->GetSpec(spec);
+
+  LOG_STATIC_FUNC_WITH_PARAM(gImgLog, "imgLoader::SetHasNoProxies", "uri", spec.get());
+#endif
+
+  if (entry->Evicted())
+    return PR_FALSE;
+
   imgCacheQueue &queue = GetCacheQueue(key);
-  queue.Push(entry);
+
+  nsresult addrv = NS_OK;
 
   if (gCacheTracker)
-    gCacheTracker->AddObject(entry);
+    addrv = gCacheTracker->AddObject(entry);
 
+  if (NS_SUCCEEDED(addrv)) {
+    queue.Push(entry);
+    entry->SetHasNoProxies(PR_TRUE);
+  }
+
+  imgCacheTable &cache = GetCache(key);
   CheckCacheLimits(cache, queue);
 
   return PR_TRUE;
 }
 
+PRBool imgLoader::SetHasProxies(nsIURI *key)
+{
+  VerifyCacheSizes();
+
+  imgCacheTable &cache = GetCache(key);
+
+  nsCAutoString spec;
+  key->GetSpec(spec);
+
+  LOG_STATIC_FUNC_WITH_PARAM(gImgLog, "imgLoader::SetHasProxies", "uri", spec.get());
+
+  nsRefPtr<imgCacheEntry> entry;
+  if (cache.Get(spec, getter_AddRefs(entry)) && entry && entry->HasNoProxies()) {
+    imgCacheQueue &queue = GetCacheQueue(key);
+    queue.Remove(entry);
+
+    if (gCacheTracker)
+      gCacheTracker->RemoveObject(entry);
+
+    entry->SetHasNoProxies(PR_FALSE);
+
+    return PR_TRUE;
+  }
+
+  return PR_FALSE;
+}
+
 void imgLoader::CacheEntriesChanged(nsIURI *uri, PRInt32 sizediff /* = 0 */)
 {
   imgCacheQueue &queue = GetCacheQueue(uri);
   queue.MarkDirty();
   queue.UpdateSize(sizediff);
 }
 
 void imgLoader::CheckCacheLimits(imgCacheTable &cache, imgCacheQueue &queue)
@@ -686,16 +780,27 @@ void imgLoader::CheckCacheLimits(imgCach
 
   // Remove entries from the cache until we're back under our desired size.
   while (queue.GetSize() >= sCacheMaxSize) {
     // Remove the first entry in the queue.
     nsRefPtr<imgCacheEntry> entry(queue.Pop());
 
     NS_ASSERTION(entry, "imgLoader::CheckCacheLimits -- NULL entry pointer");
 
+#if defined(PR_LOGGING)
+    nsRefPtr<imgRequest> req(entry->GetRequest());
+    if (req) {
+      nsCOMPtr<nsIURI> uri;
+      req->GetKeyURI(getter_AddRefs(uri));
+      nsCAutoString spec;
+      uri->GetSpec(spec);
+      LOG_STATIC_FUNC_WITH_PARAM(gImgLog, "imgLoader::CheckCacheLimits", "entry", spec.get());
+    }
+#endif
+
     if (entry)
       RemoveFromCache(entry);
   }
 }
 
 PRBool imgLoader::ValidateRequestWithNewChannel(imgRequest *request,
                                                 nsIURI *aURI,
                                                 nsIURI *aInitialDocumentURI,
@@ -708,16 +813,18 @@ PRBool imgLoader::ValidateRequestWithNew
                                                 imgIRequest **aProxyRequest)
 {
   // now we need to insert a new channel request object inbetween the real
   // request and the proxy that basically delays loading the image until it
   // gets a 304 or figures out that this needs to be a new request
 
   nsresult rv;
 
+  // If we're currently in the middle of validating this request, just hand
+  // back a proxy to it; the required work will be done for us.
   if (request->mValidator) {
     rv = CreateNewProxyForRequest(request, aLoadGroup, aObserver,
                                   aLoadFlags, aExistingRequest, 
                                   reinterpret_cast<imgIRequest **>(aProxyRequest));
 
     if (*aProxyRequest)
       request->mValidator->AddProxy(static_cast<imgRequestProxy*>(*aProxyRequest));
 
@@ -896,63 +1003,102 @@ PRBool imgLoader::ValidateEntry(imgCache
   } 
 
   return !validateRequest;
 }
 
 
 PRBool imgLoader::RemoveFromCache(nsIURI *aKey)
 {
-  LOG_STATIC_FUNC(gImgLog, "imgLoader::RemoveFromCache uri");
   if (!aKey) return PR_FALSE;
 
   imgCacheTable &cache = GetCache(aKey);
   imgCacheQueue &queue = GetCacheQueue(aKey);
 
   nsCAutoString spec;
   aKey->GetSpec(spec);
 
+  LOG_STATIC_FUNC_WITH_PARAM(gImgLog, "imgLoader::RemoveFromCache", "uri", spec.get());
+
   nsRefPtr<imgCacheEntry> entry;
   if (cache.Get(spec, getter_AddRefs(entry)) && entry) {
-    if (gCacheTracker)
-      gCacheTracker->RemoveObject(entry);
     cache.Remove(spec);
-    queue.Remove(entry);
+
+    NS_ABORT_IF_FALSE(!entry->Evicted(), "Evicting an already-evicted cache entry!");
+
+    // Entries with no proxies are in the tracker.
+    if (entry->HasNoProxies()) {
+      if (gCacheTracker)
+        gCacheTracker->RemoveObject(entry);
+      queue.Remove(entry);
+    }
+
     entry->SetEvicted(PR_TRUE);
+
     return PR_TRUE;
   }
   else
     return PR_FALSE;
 }
 
 PRBool imgLoader::RemoveFromCache(imgCacheEntry *entry)
 {
   LOG_STATIC_FUNC(gImgLog, "imgLoader::RemoveFromCache entry");
-  PRBool ret = PR_FALSE;
+
   nsRefPtr<imgRequest> request(getter_AddRefs(entry->GetRequest()));
   if (request) {
     nsCOMPtr<nsIURI> key;
-    if (NS_SUCCEEDED(request->GetKeyURI(getter_AddRefs(key))) && key)
-      ret = RemoveFromCache(key);
+    if (NS_SUCCEEDED(request->GetKeyURI(getter_AddRefs(key))) && key) {
+      imgCacheTable &cache = GetCache(key);
+      imgCacheQueue &queue = GetCacheQueue(key);
+      nsCAutoString spec;
+      key->GetSpec(spec);
+
+      LOG_STATIC_FUNC_WITH_PARAM(gImgLog, "imgLoader::RemoveFromCache", "entry's uri", spec.get());
+
+      cache.Remove(spec);
+
+      if (entry->HasNoProxies()) {
+        LOG_STATIC_FUNC(gImgLog, "imgLoader::RemoveFromCache removing from tracker");
+        if (gCacheTracker)
+          gCacheTracker->RemoveObject(entry);
+        queue.Remove(entry);
+      }
+
+      entry->SetEvicted(PR_TRUE);
+
+      return PR_TRUE;
+    }
   }
 
-  return ret;
+  return PR_FALSE;
+}
+
+static PLDHashOperator EnumEvictEntries(const nsACString&, 
+                                        nsRefPtr<imgCacheEntry> &aData,
+                                        void *data)
+{
+  nsTArray<nsRefPtr<imgCacheEntry> > *entries = 
+    reinterpret_cast<nsTArray<nsRefPtr<imgCacheEntry> > *>(data);
+
+  entries->AppendElement(aData);
+
+  return PL_DHASH_NEXT;
 }
 
 nsresult imgLoader::EvictEntries(imgCacheTable &aCacheToClear, imgCacheQueue &aQueueToClear)
 {
   LOG_STATIC_FUNC(gImgLog, "imgLoader::EvictEntries");
 
   // We have to make a temporary, since RemoveFromCache removes the element
   // from the queue, invalidating iterators.
   nsTArray<nsRefPtr<imgCacheEntry> > entries;
-  for (imgCacheQueue::iterator it = aQueueToClear.begin(); it != aQueueToClear.end(); ++it)
-    entries.AppendElement(*it);
-  
-  for (PRUint32  i = 0; i < entries.Length(); ++i)
+  aCacheToClear.Enumerate(EnumEvictEntries, &entries);
+
+  for (PRUint32 i = 0; i < entries.Length(); ++i)
     if (!RemoveFromCache(entries[i]))
       return NS_ERROR_FAILURE;
 
   return NS_OK;
 }
 
 #define LOAD_FLAGS_CACHE_MASK    (nsIRequest::LOAD_BYPASS_CACHE | \
                                   nsIRequest::LOAD_FROM_CACHE)
@@ -1033,24 +1179,32 @@ NS_IMETHODIMP imgLoader::LoadImage(nsIUR
     // for correctly dealing with image load requests that are a result
     // of post data.
     imgCacheTable &cache = GetCache(aURI);
 
     nsCAutoString spec;
     aURI->GetSpec(spec);
 
     if (cache.Get(spec, getter_AddRefs(entry)) && entry) {
-      if (gCacheTracker)
-        gCacheTracker->MarkUsed(entry);
-
       if (ValidateEntry(entry, aURI, aInitialDocumentURI, aReferrerURI, aLoadGroup, aObserver, aCX,
                         requestFlags, PR_TRUE, aRequest, _retval)) {
         request = getter_AddRefs(entry->GetRequest());
 
+        // If this entry has no proxies, its request has no reference to the entry.
+        if (entry->HasNoProxies()) {
+          LOG_FUNC_WITH_PARAM(gImgLog, "imgLoader::LoadImage() adding proxyless entry", "uri", spec.get());
+          NS_ABORT_IF_FALSE(!request->HasCacheEntry(), "Proxyless entry's request has cache entry!");
+          request->SetCacheEntry(entry);
+
+          if (gCacheTracker)
+            gCacheTracker->MarkUsed(entry);
+        } 
+
         entry->Touch();
+
 #ifdef DEBUG_joe
         printf("CACHEGET: %d %s %d\n", time(NULL), spec.get(), entry->GetDataSize());
 #endif
       }
       else
         entry = nsnull;
     }
   }
@@ -1168,19 +1322,16 @@ NS_IMETHODIMP imgLoader::LoadImageWithCh
     // for correctly dealing with image load requests that are a result
     // of post data.
     imgCacheTable &cache = GetCache(uri);
     nsCAutoString spec;
 
     uri->GetSpec(spec);
 
     if (cache.Get(spec, getter_AddRefs(entry)) && entry) {
-      if (gCacheTracker)
-        gCacheTracker->MarkUsed(entry);
-
       // We don't want to kick off another network load. So we ask
       // ValidateEntry to only do validation without creating a new proxy. If
       // it says that the entry isn't valid any more, we'll only use the entry
       // we're getting if the channel is loading from the cache anyways.
       //
       // XXX -- should this be changed? it's pretty much verbatim from the old
       // code, but seems nonsensical.
       if (ValidateEntry(entry, uri, nsnull, nsnull, nsnull, aObserver, aCX,
@@ -1196,39 +1347,54 @@ NS_IMETHODIMP imgLoader::LoadImageWithCh
           bUseCacheCopy = PR_FALSE;
 
         if (!bUseCacheCopy)
           entry = nsnull;
         else {
           request = getter_AddRefs(entry->GetRequest());
         }
       }
+
+      if (request && entry) {
+        // If this entry has no proxies, its request has no reference to the entry.
+        if (entry->HasNoProxies()) {
+          LOG_FUNC_WITH_PARAM(gImgLog, "imgLoader::LoadImageWithChannel() adding proxyless entry", "uri", spec.get());
+          NS_ABORT_IF_FALSE(!request->HasCacheEntry(), "Proxyless entry's request has cache entry!");
+          request->SetCacheEntry(entry);
+
+          if (gCacheTracker)
+            gCacheTracker->MarkUsed(entry);
+        } 
+      }
     }
   }
 
   nsCOMPtr<nsILoadGroup> loadGroup;
   channel->GetLoadGroup(getter_AddRefs(loadGroup));
 
   if (request) {
     // we have this in our cache already.. cancel the current (document) load
 
     channel->Cancel(NS_ERROR_PARSED_DATA_CACHED); // this should fire an OnStopRequest
 
     *listener = nsnull; // give them back a null nsIStreamListener
   } else {
-    NewRequestAndEntry(uri, getter_AddRefs(request), getter_AddRefs(entry));
+    if (!NewRequestAndEntry(uri, getter_AddRefs(request), getter_AddRefs(entry)))
+      return NS_ERROR_OUT_OF_MEMORY;
 
     // We use originalURI here to fulfil the imgIRequest contract on GetURI.
     nsCOMPtr<nsIURI> originalURI;
     channel->GetOriginalURI(getter_AddRefs(originalURI));
     request->Init(originalURI, uri, channel, channel, entry, NS_GetCurrentThread(), aCX);
 
     ProxyListener *pl = new ProxyListener(static_cast<nsIStreamListener *>(request.get()));
-    if (!pl)
+    if (!pl) {
+      request->CancelAndAbort(NS_ERROR_OUT_OF_MEMORY);
       return NS_ERROR_OUT_OF_MEMORY;
+    }
 
     NS_ADDREF(pl);
 
     *listener = static_cast<nsIStreamListener*>(pl);
     NS_ADDREF(*listener);
 
     NS_RELEASE(pl);
 
@@ -1241,17 +1407,16 @@ NS_IMETHODIMP imgLoader::LoadImageWithCh
 
   rv = CreateNewProxyForRequest(request, loadGroup, aObserver,
                                 requestFlags, nsnull, _retval);
   request->NotifyProxyListener(static_cast<imgRequestProxy*>(*_retval));
 
   return rv;
 }
 
-
 NS_IMETHODIMP imgLoader::SupportImageWithMimeType(const char* aMimeType, PRBool *_retval)
 {
   *_retval = PR_FALSE;
   nsCOMPtr<nsIComponentRegistrar> reg;
   nsresult rv = NS_GetComponentRegistrar(getter_AddRefs(reg));
   if (NS_FAILED(rv))
     return rv;
   nsCAutoString mimeType(aMimeType);
@@ -1466,57 +1631,67 @@ NS_IMETHODIMP imgCacheValidator::OnStart
       mRequest->mValidator = nsnull;
 
       mRequest = nsnull;
 
       return NS_OK;
     }
   }
 
-  // fun stuff.
+  // We can't load out of cache. We have to create a whole new request for the
+  // data that's coming in off the channel.
   nsCOMPtr<nsIChannel> channel(do_QueryInterface(aRequest));
   nsRefPtr<imgCacheEntry> entry;
   nsCOMPtr<nsIURI> uri;
 
+  mRequest->GetURI(getter_AddRefs(uri));
+
+#if defined(PR_LOGGING)
+  nsCAutoString spec;
+  uri->GetSpec(spec);
+  LOG_MSG_WITH_PARAM(gImgLog, "imgCacheValidator::OnStartRequest creating new request", "uri", spec.get());
+#endif
+
   // Doom the old request's cache entry
   mRequest->RemoveFromCache();
 
-  mRequest->GetURI(getter_AddRefs(uri));
-
   mRequest->mValidator = nsnull;
   mRequest = nsnull;
 
   imgRequest *request;
 
   if (!NewRequestAndEntry(uri, &request, getter_AddRefs(entry)))
       return NS_ERROR_OUT_OF_MEMORY;
 
   // We use originalURI here to fulfil the imgIRequest contract on GetURI.
   nsCOMPtr<nsIURI> originalURI;
   channel->GetOriginalURI(getter_AddRefs(originalURI));
   request->Init(originalURI, uri, channel, channel, entry, NS_GetCurrentThread(), mContext);
 
   ProxyListener *pl = new ProxyListener(static_cast<nsIStreamListener *>(request));
   if (!pl) {
+    request->CancelAndAbort(NS_ERROR_OUT_OF_MEMORY);
     NS_RELEASE(request);
     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.
+  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);
   }
 
-  // Try to add the new request into the cache.
-  sImgLoader.PutIntoCache(uri, entry);
-
   NS_RELEASE(request);
 
   if (!mDestListener)
     return NS_OK;
 
   return mDestListener->OnStartRequest(aRequest, ctxt);
 }
 
--- a/modules/libpr0n/src/imgLoader.h
+++ b/modules/libpr0n/src/imgLoader.h
@@ -56,30 +56,31 @@ class imgRequestProxy;
 class imgIRequest;
 class imgIDecoderObserver;
 class nsILoadGroup;
 
 class imgCacheEntry
 {
 public:
   imgCacheEntry(imgRequest *request, PRBool mustValidateIfExpired = PR_FALSE);
+  ~imgCacheEntry();
 
   nsrefcnt AddRef()
   {
     NS_PRECONDITION(PRInt32(mRefCnt) >= 0, "illegal refcnt");
-    NS_ASSERT_OWNINGTHREAD(imgCacheEntry);
+    NS_ABORT_IF_FALSE(_mOwningThread.GetThread() == PR_GetCurrentThread(), "imgCacheEntry addref isn't thread-safe!");
     ++mRefCnt;
     NS_LOG_ADDREF(this, mRefCnt, "imgCacheEntry", sizeof(*this));
     return mRefCnt;
   }
  
   nsrefcnt Release()
   {
     NS_PRECONDITION(0 != mRefCnt, "dup release");
-    NS_ASSERT_OWNINGTHREAD(imgCacheEntry);
+    NS_ABORT_IF_FALSE(_mOwningThread.GetThread() == PR_GetCurrentThread(), "imgCacheEntry release isn't thread-safe!");
     --mRefCnt;
     NS_LOG_RELEASE(this, mRefCnt, "imgCacheEntry");
     if (mRefCnt == 0) {
       mRefCnt = 1; /* stabilize */
       delete this;
       return 0;
     }
     return mRefCnt;                              
@@ -138,37 +139,48 @@ public:
     return mEvicted;
   }
 
   nsExpirationState *GetExpirationState()
   {
     return &mExpirationState;
   }
 
+  PRBool HasNoProxies() const
+  {
+    return mHasNoProxies;
+  }
+
 private: // methods
   friend class imgLoader;
   friend class imgCacheQueue;
   void Touch(PRBool updateTime = PR_TRUE);
   void TouchWithSize(PRInt32 diff);
   void SetEvicted(PRBool evict)
   {
     mEvicted = evict;
   }
+  void SetHasNoProxies(PRBool hasNoProxies);
+
+  // 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;
-  PRBool mMustValidateIfExpired;
-  PRBool mEvicted;
+  PRPackedBool mMustValidateIfExpired : 1;
+  PRPackedBool mEvicted : 1;
+  PRPackedBool mHasNoProxies : 1;
 };
 
 #include <vector>
 
 #define NS_IMGLOADER_CID \
 { /* 9f6a0d2e-1dd1-11b2-a5b8-951f13c846f7 */         \
      0x9f6a0d2e,                                     \
      0x1dd1,                                         \
@@ -245,16 +257,30 @@ public:
     const PRFloat64 sizeweight = 1.0 - sCacheTimeWeight;
     PRInt32 diffsize = PRInt32(two->GetDataSize()) - PRInt32(one->GetDataSize());
     PRInt32 difftime = one->GetTouchedTime() - two->GetTouchedTime();
     return difftime * sCacheTimeWeight + diffsize * sizeweight < 0;
   }
 
   static void VerifyCacheSizes();
 
+  // The image loader maintains a hash table of all imgCacheEntries. However,
+  // only some of them will be evicted from the cache: those who have no
+  // imgRequestProxies watching their imgRequests. 
+  //
+  // Once an imgRequest has no imgRequestProxies, it should notify us by
+  // calling HasNoObservers(), and null out its cache entry pointer.
+  // 
+  // Upon having a proxy start observing again, it should notify us by calling
+  // HasObservers(). The request's cache entry will be re-set before this
+  // happens, by calling imgRequest::SetCacheEntry() when an entry with no
+  // observers is re-requested.
+  static PRBool SetHasNoProxies(nsIURI *key, imgCacheEntry *entry);
+  static PRBool SetHasProxies(nsIURI *key);
+
 private: // methods
 
 
   PRBool ValidateEntry(imgCacheEntry *aEntry, nsIURI *aKey,
                        nsIURI *aInitialDocumentURI, nsIURI *aReferrerURI, 
                        nsILoadGroup *aLoadGroup,
                        imgIDecoderObserver *aObserver, nsISupports *aCX,
                        nsLoadFlags aLoadFlags, PRBool aCanMakeNewChannel,
--- a/modules/libpr0n/src/imgRequest.cpp
+++ b/modules/libpr0n/src/imgRequest.cpp
@@ -87,38 +87,45 @@ imgRequest::imgRequest() :
   mState(0), mCacheId(0), mValidator(nsnull), mIsMultiPartChannel(PR_FALSE),
   mImageSniffers("image-sniffing-services") 
 {
   /* member initializers and constructor code */
 }
 
 imgRequest::~imgRequest()
 {
-  /* destructor code */
+  if (mKeyURI) {
+    nsCAutoString spec;
+    mKeyURI->GetSpec(spec);
+    LOG_FUNC_WITH_PARAM(gImgLog, "imgRequest::~imgRequest()", "keyuri", spec.get());
+  } else
+    LOG_FUNC(gImgLog, "imgRequest::~imgRequest()");
 }
 
 nsresult imgRequest::Init(nsIURI *aURI,
                           nsIURI *aKeyURI,
                           nsIRequest *aRequest,
                           nsIChannel *aChannel,
                           imgCacheEntry *aCacheEntry,
                           void *aCacheId,
                           void *aLoadId)
 {
   LOG_FUNC(gImgLog, "imgRequest::Init");
 
-  NS_ASSERTION(!mImage, "Multiple calls to init");
-  NS_ASSERTION(aURI, "No uri");
-  NS_ASSERTION(aRequest, "No request");
-  NS_ASSERTION(aChannel, "No channel");
+  NS_ABORT_IF_FALSE(!mImage, "Multiple calls to init");
+  NS_ABORT_IF_FALSE(aURI, "No uri");
+  NS_ABORT_IF_FALSE(aKeyURI, "No key uri");
+  NS_ABORT_IF_FALSE(aRequest, "No request");
+  NS_ABORT_IF_FALSE(aChannel, "No channel");
 
   mProperties = do_CreateInstance("@mozilla.org/properties;1");
   if (!mProperties)
     return NS_ERROR_OUT_OF_MEMORY;
 
+
   mURI = aURI;
   mKeyURI = aKeyURI;
   mRequest = aRequest;
   mChannel = aChannel;
   mChannel->GetNotificationCallbacks(getter_AddRefs(mPrevChannelSink));
 
   NS_ASSERTION(mPrevChannelSink != this,
                "Initializing with a channel that already calls back to us!");
@@ -136,21 +143,38 @@ nsresult imgRequest::Init(nsIURI *aURI,
 
   mCacheId = aCacheId;
 
   SetLoadId(aLoadId);
 
   return NS_OK;
 }
 
+void imgRequest::SetCacheEntry(imgCacheEntry *entry)
+{
+  mCacheEntry = entry;
+}
+
+PRBool imgRequest::HasCacheEntry() const
+{
+  return mCacheEntry != nsnull;
+}
+
 nsresult imgRequest::AddProxy(imgRequestProxy *proxy)
 {
   NS_PRECONDITION(proxy, "null imgRequestProxy passed in");
   LOG_SCOPE_WITH_PARAM(gImgLog, "imgRequest::AddProxy", "proxy", proxy);
 
+  // If we're empty before adding, we have to tell the loader we now have
+  // proxies.
+  if (mObservers.IsEmpty()) {
+    NS_ABORT_IF_FALSE(mKeyURI, "Trying to SetHasProxies without key uri.");
+    imgLoader::SetHasProxies(mKeyURI);
+  }
+
   return mObservers.AppendElementUnlessExists(proxy) ?
     NS_OK : NS_ERROR_OUT_OF_MEMORY;
 }
 
 nsresult imgRequest::RemoveProxy(imgRequestProxy *proxy, nsresult aStatus, PRBool aNotify)
 {
   LOG_SCOPE_WITH_PARAM(gImgLog, "imgRequest::RemoveProxy", "proxy", proxy);
 
@@ -177,16 +201,32 @@ nsresult imgRequest::RemoveProxy(imgRequ
 
   if (mImage && !HaveProxyWithObserver(nsnull)) {
     LOG_MSG(gImgLog, "imgRequest::RemoveProxy", "stopping animation");
 
     mImage->StopAnimation();
   }
 
   if (mObservers.IsEmpty()) {
+    // If we have no observers, there's nothing holding us alive. If we haven't
+    // been cancelled and thus removed from the cache, tell the image loader so
+    // we can be evicted from the cache.
+    if (mCacheEntry) {
+      NS_ABORT_IF_FALSE(mKeyURI, "Removing last observer without key uri.");
+
+      imgLoader::SetHasNoProxies(mKeyURI, mCacheEntry);
+    } 
+#if defined(PR_LOGGING)
+    else {
+      nsCAutoString spec;
+      mKeyURI->GetSpec(spec);
+      LOG_MSG_WITH_PARAM(gImgLog, "imgRequest::RemoveProxy no cache entry", "uri", spec.get());
+    }
+#endif
+
     /* If |aStatus| is a failure code, then cancel the load if it is still in progress.
        Otherwise, let the load continue, keeping 'this' in the cache with no observers.
        This way, if a proxy is destroyed without calling cancel on it, it won't leak
        and won't leave a bad pointer in mObservers.
      */
     if (mRequest && mLoading && NS_FAILED(aStatus)) {
       LOG_MSG(gImgLog, "imgRequest::RemoveProxy", "load in progress.  canceling");
 
@@ -309,16 +349,18 @@ void imgRequest::Cancel(nsresult aStatus
   }
 
   if (mRequest && mLoading)
     mRequest->Cancel(aStatus);
 }
 
 void imgRequest::CancelAndAbort(nsresult aStatus)
 {
+  LOG_SCOPE(gImgLog, "imgRequest::CancelAndAbort");
+
   Cancel(aStatus);
 
   // It's possible for the channel to fail to open after we've set our
   // notification callbacks. In that case, make sure to break the cycle between
   // the channel and us, because it won't.
   if (mChannel) {
     mChannel->SetNotificationCallbacks(mPrevChannelSink);
     mPrevChannelSink = nsnull;
@@ -372,20 +414,22 @@ nsresult imgRequest::GetSecurityInfo(nsI
   NS_IF_ADDREF(*aSecurityInfo = mSecurityInfo);
   return NS_OK;
 }
 
 void imgRequest::RemoveFromCache()
 {
   LOG_SCOPE(gImgLog, "imgRequest::RemoveFromCache");
 
-  if (mCacheEntry) {
-    imgLoader::RemoveFromCache(mURI);
-    mCacheEntry = nsnull;
-  }
+  if (mCacheEntry)
+    imgLoader::RemoveFromCache(mCacheEntry);
+  else
+    imgLoader::RemoveFromCache(mKeyURI);
+
+  mCacheEntry = nsnull;
 }
 
 PRBool imgRequest::HaveProxyWithObserver(imgRequestProxy* aProxyToIgnore) const
 {
   nsTObserverArray<imgRequestProxy*>::ForwardIterator iter(mObservers);
   imgRequestProxy* proxy;
   while (iter.HasMore()) {
     proxy = iter.GetNext();
@@ -1019,20 +1063,33 @@ 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));
 
+#if defined(PR_LOGGING)
+  mKeyURI->GetSpec(spec);
+
+  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);
 
   return rv;
 }
--- a/modules/libpr0n/src/imgRequest.h
+++ b/modules/libpr0n/src/imgRequest.h
@@ -115,16 +115,17 @@ public:
   // won't be sufficient.
   void CancelAndAbort(nsresult aStatus);
 
 private:
   friend class imgCacheEntry;
   friend class imgRequestProxy;
   friend class imgLoader;
   friend class imgCacheValidator;
+  friend class imgCacheExpirationTracker;
 
   inline void SetLoadId(void *aLoadId) {
     mLoadId = aLoadId;
     mLoadTime = PR_Now();
   }
   inline PRUint32 GetImageStatus() const { return mImageStatus; }
   inline nsresult GetResultFromImageStatus(PRUint32 aStatus) const;
   void Cancel(nsresult aStatus);
@@ -135,16 +136,24 @@ private:
   void RemoveFromCache();
   inline const char *GetMimeType() const {
     return mContentType.get();
   }
   inline nsIProperties *Properties() {
     return mProperties;
   }
 
+  // Reset the cache entry after we've dropped our reference to it. Used by the
+  // imgLoader when our cache entry is re-requested after we've dropped our
+  // reference to it.
+  void SetCacheEntry(imgCacheEntry *entry);
+
+  // Returns whether we've got a reference to the cache entry.
+  PRBool HasCacheEntry() const;
+
   // Return true if at least one of our proxies, excluding
   // aProxyToIgnore, has an observer.  aProxyToIgnore may be null.
   PRBool HaveProxyWithObserver(imgRequestProxy* aProxyToIgnore) const;
 
   // Return the priority of the underlying network request, or return
   // PRIORITY_NORMAL if it doesn't support nsISupportsPriority.
   PRInt32 Priority() const;