Bug 1346510. Don't allow the surface cache to unlock the animated frames of an animated image (when discarding of animated images is disabled). r=aosmond
authorTimothy Nikkel <tnikkel@gmail.com>
Tue, 14 Mar 2017 01:11:44 -0500
changeset 347505 778683798a9aad9b0801ebd2bf6d0eda4ce240f6
parent 347504 62dd2dbd728be2f3b4c0932aafce8a810d3a284f
child 347506 1697080a4726def0547486a6e7ef057cfccdc5a8
push id31497
push usercbook@mozilla.com
push dateTue, 14 Mar 2017 13:23:16 +0000
treeherdermozilla-central@08f709c14bf7 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaosmond
bugs1346510
milestone55.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1346510. Don't allow the surface cache to unlock the animated frames of an animated image (when discarding of animated images is disabled). r=aosmond The pref has never been enabled, so this is quite surprising! It is currently possible (and has been for quite a while) to discard animated images. All we need is the follow sequence of events. 1. Decode an animated image. 2. Move the animated image out of view (so it is not painted). 3. Call canvas.drawImage on the animated image (or anything else that asks for a first frame only decode). This creates a static entry in the surface cache for this first frame in addition to the animated entry. Because it is a static request we will also start a first frame decode. RasterImage::Decode calls SurfaceCache::UnlockEntries https://dxr.mozilla.org/mozilla-central/rev/4ceb9062ea8f4113bfd1b3536ace4a840a72faa7/image/RasterImage.cpp#1166 and bam, the animated frames are now unlocked (even though the RasterImage, and it's entry in the surface cache is still locked). 4. Switch tabs, open about:memory and minimize memory to actual throw away the animated frames. 5. Switch back to the image tab, scroll the image back into view, it will not animate, it will just show the last composited frame forever.
image/SurfaceCache.cpp
--- a/image/SurfaceCache.cpp
+++ b/image/SurfaceCache.cpp
@@ -693,29 +693,30 @@ public:
   void UnlockImage(const ImageKey aImageKey)
   {
     RefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey);
     if (!cache || !cache->IsLocked()) {
       return;  // Already unlocked.
     }
 
     cache->SetLocked(false);
-    DoUnlockSurfaces(WrapNotNull(cache));
+    DoUnlockSurfaces(WrapNotNull(cache), /* aStaticOnly = */ false);
   }
 
   void UnlockEntries(const ImageKey aImageKey)
   {
     RefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey);
     if (!cache || !cache->IsLocked()) {
       return;  // Already unlocked.
     }
 
     // (Note that we *don't* unlock the per-image cache here; that's the
     // difference between this and UnlockImage.)
-    DoUnlockSurfaces(WrapNotNull(cache));
+    DoUnlockSurfaces(WrapNotNull(cache),
+      /* aStaticOnly = */ !gfxPrefs::ImageMemAnimatedDiscardable());
   }
 
   void RemoveImage(const ImageKey aImageKey)
   {
     RefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey);
     if (!cache) {
       return;  // No cached surfaces for this image, so nothing to do.
     }
@@ -851,24 +852,27 @@ private:
   {
     if (aCache->IsLocked()) {
       LockSurface(aSurface);
     } else {
       mExpirationTracker.MarkUsed(aSurface);
     }
   }
 
-  void DoUnlockSurfaces(NotNull<ImageSurfaceCache*> aCache)
+  void DoUnlockSurfaces(NotNull<ImageSurfaceCache*> aCache, bool aStaticOnly)
   {
     // Unlock all the surfaces the per-image cache is holding.
     for (auto iter = aCache->ConstIter(); !iter.Done(); iter.Next()) {
       NotNull<CachedSurface*> surface = WrapNotNull(iter.UserData());
       if (surface->IsPlaceholder() || !surface->IsLocked()) {
         continue;
       }
+      if (aStaticOnly && surface->GetSurfaceKey().Playback() != PlaybackType::eStatic) {
+        continue;
+      }
       StopTracking(surface);
       surface->SetLocked(false);
       StartTracking(surface);
     }
   }
 
   void RemoveEntry(const ImageKey    aImageKey,
                    const SurfaceKey& aSurfaceKey)