Bug 1342567. r=aosmond, a=gchang
☠☠ backed out by ad4fa51c539f ☠ ☠
authorTimothy Nikkel <tnikkel@gmail.com>
Tue, 11 Apr 2017 03:14:11 -0500
changeset 396267 0f4b47c9fab1a82abab0a1e5418dd0125f6c181d
parent 396266 b891a2a9af3d824ae302a35300398be7b5554b73
child 396268 87a47b9dff16ef7fbd9bdcbd9e8d9f571a89c6fc
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaosmond, gchang
bugs1342567
milestone54.0
Bug 1342567. r=aosmond, a=gchang
image/imgLoader.cpp
image/imgLoader.h
--- a/image/imgLoader.cpp
+++ b/image/imgLoader.cpp
@@ -883,17 +883,18 @@ imgCacheEntry::imgCacheEntry(imgLoader* 
    mTouchedTime(SecondsFromPRTime(PR_Now())),
    mLoadTime(SecondsFromPRTime(PR_Now())),
    mExpiryTime(0),
    mMustValidate(false),
    // We start off as evicted so we don't try to update the cache. PutIntoCache
    // will set this to false.
    mEvicted(true),
    mHasNoProxies(true),
-   mForcePrincipalCheck(forcePrincipalCheck)
+   mForcePrincipalCheck(forcePrincipalCheck),
+   mInUse(false)
 { }
 
 imgCacheEntry::~imgCacheEntry()
 {
   LOG_FUNC(gImgLog, "imgCacheEntry::~imgCacheEntry()");
 }
 
 void
@@ -1871,16 +1872,20 @@ imgLoader::RemoveFromCache(const ImageCa
   LOG_STATIC_FUNC_WITH_PARAM(gImgLog,
                              "imgLoader::RemoveFromCache", "uri", aKey.Spec());
 
   imgCacheTable& cache = GetCache(aKey);
   imgCacheQueue& queue = GetCacheQueue(aKey);
 
   RefPtr<imgCacheEntry> entry;
   if (cache.Get(aKey, getter_AddRefs(entry)) && entry) {
+    if (MOZ_UNLIKELY(entry->GetInUse())) {
+      gfxCriticalNoteOnce << "RemoveFromCache(key) removing inuse cache entry";
+    }
+
     cache.Remove(aKey);
 
     MOZ_ASSERT(!entry->Evicted(), "Evicting an already-evicted cache entry!");
 
     // Entries with no proxies are in the tracker.
     if (entry->HasNoProxies()) {
       if (mCacheTracker) {
         mCacheTracker->RemoveObject(entry);
@@ -1909,16 +1914,21 @@ imgLoader::RemoveFromCache(imgCacheEntry
     const ImageCacheKey& key = request->CacheKey();
     imgCacheTable& cache = GetCache(key);
     imgCacheQueue& queue = GetCacheQueue(key);
 
     LOG_STATIC_FUNC_WITH_PARAM(gImgLog,
                                "imgLoader::RemoveFromCache", "entry's uri",
                                key.Spec());
 
+    if (MOZ_UNLIKELY(entry->GetInUse())) {
+      gfxCriticalNoteOnce << "RemoveFromCache(entry) removing inuse cache entry";
+      MOZ_RELEASE_ASSERT(false);
+    }
+
     cache.Remove(key);
 
     if (entry->HasNoProxies()) {
       LOG_STATIC_FUNC(gImgLog,
                       "imgLoader::RemoveFromCache removing from tracker");
       if (mCacheTracker) {
         mCacheTracker->RemoveObject(entry);
       }
@@ -2136,39 +2146,59 @@ imgLoader::LoadImage(nsIURI* aURI,
   if (aLoadingPrincipal) {
     attrs = aLoadingPrincipal->OriginAttributesRef();
   }
   ImageCacheKey key(aURI, attrs, aLoadingDocument, rv);
   NS_ENSURE_SUCCESS(rv, rv);
   imgCacheTable& cache = GetCache(key);
 
   if (cache.Get(key, getter_AddRefs(entry)) && entry) {
+    entry->SetInUse(true);
     if (ValidateEntry(entry, aURI, aInitialDocumentURI, aReferrerURI,
                       aReferrerPolicy, aLoadGroup, aObserver, aLoadingDocument,
                       requestFlags, aContentPolicyType, true, _retval,
                       aLoadingPrincipal, corsmode)) {
       request = 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", key.Spec());
         MOZ_ASSERT(!request->HasCacheEntry(),
           "Proxyless entry's request has cache entry!");
         request->SetCacheEntry(entry);
 
         if (mCacheTracker) {
+          if (MOZ_UNLIKELY(!entry->GetExpirationState()->IsTracked())) {
+            bool inCache = false;
+            bool cacheHasEntry = false;
+            RefPtr<imgCacheEntry> e;
+            if (cache.Get(key, getter_AddRefs(e)) && e) {
+              cacheHasEntry = true;
+              inCache = (e == entry);
+            }
+            gfxCriticalNoteOnce << "entry with no proxies is no in tracker "
+                                << "request->HasConsumers() "
+                                << (request->HasConsumers() ? "true" : "false")
+                                << " inCache " << (inCache ? "true" : "false")
+                                << " cacheHasEntry " << (cacheHasEntry ? "true" : "false");
+          }
+
           mCacheTracker->MarkUsed(entry);
         }
       }
 
       entry->Touch();
 
+      entry->SetInUse(false);
+
     } else {
+      entry->SetInUse(false);
+
       // We can't use this entry. We'll try to load it off the network, and if
       // successful, overwrite the old entry in the cache with a new one.
       entry = nullptr;
     }
   }
 
   // Keep the channel in this scope, so we can adjust its notificationCallbacks
   // later when we create the proxy.
@@ -2362,16 +2392,18 @@ imgLoader::LoadImageWithChannel(nsIChann
     RemoveFromCache(key);
   } else {
     // Look in the cache for our URI, and then validate it.
     // XXX For now ignore aCacheKey. We will need it in the future
     // for correctly dealing with image load requests that are a result
     // of post data.
     imgCacheTable& cache = GetCache(key);
     if (cache.Get(key, getter_AddRefs(entry)) && entry) {
+      entry->SetInUse(true);
+
       // 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.
       //
@@ -2396,16 +2428,17 @@ imgLoader::LoadImageWithChannel(nsIChann
 
         if (cacheChan) {
           cacheChan->IsFromCache(&bUseCacheCopy);
         } else {
           bUseCacheCopy = false;
         }
 
         if (!bUseCacheCopy) {
+          entry->SetInUse(false);
           entry = nullptr;
         } else {
           request = entry->GetRequest();
         }
       }
 
       if (request && entry) {
         // If this entry has no proxies, its request has no reference to
@@ -2418,16 +2451,19 @@ imgLoader::LoadImageWithChannel(nsIChann
             "Proxyless entry's request has cache entry!");
           request->SetCacheEntry(entry);
 
           if (mCacheTracker) {
             mCacheTracker->MarkUsed(entry);
           }
         }
       }
+      if (entry) {
+        entry->SetInUse(false);
+      }
     }
   }
 
   nsCOMPtr<nsILoadGroup> loadGroup;
   channel->GetLoadGroup(getter_AddRefs(loadGroup));
 
   // Filter out any load flags not from nsIRequest
   requestFlags &= nsIRequest::LOAD_REQUESTMASK;
--- a/image/imgLoader.h
+++ b/image/imgLoader.h
@@ -139,16 +139,24 @@ public:
     return mHasNoProxies;
   }
 
   bool ForcePrincipalCheck() const
   {
     return mForcePrincipalCheck;
   }
 
+  bool GetInUse() const {
+    return mInUse;
+  }
+
+  void SetInUse(bool aInUse) {
+    mInUse = aInUse;
+  }
+
   imgLoader* Loader() const
   {
     return mLoader;
   }
 
 private: // methods
   friend class imgLoader;
   friend class imgCacheQueue;
@@ -173,16 +181,17 @@ private: // data
   int32_t mTouchedTime;
   uint32_t mLoadTime;
   int32_t mExpiryTime;
   nsExpirationState mExpirationState;
   bool mMustValidate : 1;
   bool mEvicted : 1;
   bool mHasNoProxies : 1;
   bool mForcePrincipalCheck : 1;
+  bool mInUse : 1;
 };
 
 #include <vector>
 
 #define NS_IMGLOADER_CID \
 { /* c1354898-e3fe-4602-88a7-c4520c21cb4e */         \
      0xc1354898,                                     \
      0xe3fe,                                         \