Bug 1380649 - Part 1. Ensure SurfaceCache::CollectSizeOfSurfaces removes purged volatile buffer-backed surfaces. r=tnikkel

When we lookup a surface in the cache, we are careful to remove any
surfaces which were backed by volatile memory and got purged before we
could reacquire the buffer. We were not so careful in doing that when
generating memory reports. ISurfaceProvider::AddSizeOfExcludingThis will
cause us to acquire the buffer, and if it was purged, forget about its
purged status. Later when we performed a lookup, we would forget the
purged status, and assume we have the right data. This would appear as
completely transparent for BGRA surfaces, and completely black for BGRX
surfaces.

With this patch, we now properly remove purged surfaces instead of
including them in the report. This ensures that the cache state is
consistent. This also resolves memory reports of surfaces which reported
using no data -- they were purged when the report was generated.

Additionally, there was a bug in SurfaceCache::PruneImage where we did
not discard surfaces outside the module lock. Both PruneImage and
CollectSizeOfSurfaces now free any discarded surfaces outside the lock.

Author: aosmond

image/SurfaceCache.cpp

@@ -614,12 +614,29 @@ class ImageSurfaceCache
     return false;
   }
 
+  template<typename Function>
   void CollectSizeOfSurfaces(nsTArray<SurfaceMemoryCounter>& aCounters,
-                             MallocSizeOf                    aMallocSizeOf)
+                             MallocSizeOf                    aMallocSizeOf,
+                             Function&&                      aRemoveCallback)
   {
     CachedSurface::SurfaceMemoryReport report(aCounters, aMallocSizeOf);
-    for (auto iter = ConstIter(); !iter.Done(); iter.Next()) {
+    for (auto iter = mSurfaces.Iter(); !iter.Done(); iter.Next()) {
       NotNull<CachedSurface*> surface = WrapNotNull(iter.UserData());
+
+      // We don't need the drawable surface for ourselves, but adding a surface
+      // to the report will trigger this indirectly. If the surface was
+      // discarded by the OS because it was in volatile memory, we should remove
+      // it from the cache immediately rather than include it in the report.
+      DrawableSurface drawableSurface;
+      if (!surface->IsPlaceholder()) {
+        drawableSurface = surface->GetDrawableSurface();
+        if (!drawableSurface) {
+          aRemoveCallback(surface);
+          iter.Remove();
+          continue;
+        }
+      }
+
       const IntSize& size = surface->GetSurfaceKey().Size();
       bool factor2Size = false;
       if (mFactor2Mode) {
@@ -1073,6 +1090,8 @@ class SurfaceCacheImpl final : public nsIMemoryReporter
 
     cache->Prune([this, &aAutoLock](NotNull<CachedSurface*> aSurface) -> void {
       StopTracking(aSurface, /* aIsTracked */ true, aAutoLock);
+      // Individual surfaces must be freed outside the lock.
+      mCachedSurfacesDiscard.AppendElement(aSurface);
     });
   }
 
@@ -1165,15 +1184,21 @@ class SurfaceCacheImpl final : public nsIMemoryReporter
 
   void CollectSizeOfSurfaces(const ImageKey                  aImageKey,
                              nsTArray<SurfaceMemoryCounter>& aCounters,
-                             MallocSizeOf                    aMallocSizeOf)
+                             MallocSizeOf                    aMallocSizeOf,
+                             const StaticMutexAutoLock&      aAutoLock)
   {
     RefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey);
     if (!cache) {
       return;  // No surfaces for this image.
     }
 
     // Report all surfaces in the per-image cache.
-    cache->CollectSizeOfSurfaces(aCounters, aMallocSizeOf);
+    cache->CollectSizeOfSurfaces(aCounters, aMallocSizeOf,
+      [this, &aAutoLock](NotNull<CachedSurface*> aSurface) -> void {
+      StopTracking(aSurface, /* aIsTracked */ true, aAutoLock);
+      // Individual surfaces must be freed outside the lock.
+      mCachedSurfacesDiscard.AppendElement(aSurface);
+    });
   }
 
 private:
@@ -1544,9 +1569,13 @@ SurfaceCache::RemoveImage(const ImageKey aImageKey)
 /* static */ void
 SurfaceCache::PruneImage(const ImageKey aImageKey)
 {
-  StaticMutexAutoLock lock(sInstanceMutex);
-  if (sInstance) {
-    sInstance->PruneImage(aImageKey, lock);
+  nsTArray<RefPtr<CachedSurface>> discard;
+  {
+    StaticMutexAutoLock lock(sInstanceMutex);
+    if (sInstance) {
+      sInstance->PruneImage(aImageKey, lock);
+      sInstance->TakeDiscard(discard, lock);
+    }
   }
 }
 
@@ -1568,12 +1597,16 @@ SurfaceCache::CollectSizeOfSurfaces(const ImageKey                  aImageKey,
                                     nsTArray<SurfaceMemoryCounter>& aCounters,
                                     MallocSizeOf                    aMallocSizeOf)
 {
-  StaticMutexAutoLock lock(sInstanceMutex);
-  if (!sInstance) {
-    return;
-  }
+  nsTArray<RefPtr<CachedSurface>> discard;
+  {
+    StaticMutexAutoLock lock(sInstanceMutex);
+    if (!sInstance) {
+      return;
+    }
 
-  return sInstance->CollectSizeOfSurfaces(aImageKey, aCounters, aMallocSizeOf);
+    sInstance->CollectSizeOfSurfaces(aImageKey, aCounters, aMallocSizeOf, lock);
+    sInstance->TakeDiscard(discard, lock);
+  }
 }
 
 /* static */ size_t