Bug 1805599. Fix handling of invalidations in non-animated vector images that use the webrender imageprovider path. r=aosmond

When not using blob recordings for vector images we use simple surface providers. When we get an invalidation we will call SurfaceCache::InvalidateImage which will set a dirty bit on all blob recordings and remove all other surfaces from the surface cache. And if it finds a blob recording we will call ResumeHonoringInvalidations. This is not enough to invalidate in the case of a non-animated vector image using the webrender GetImageProvider path. Even though the surface isn't in the surface cache anymore it is still cached on the frame and when we tell it about the invalidation it will just has the image provider to update its key. That will call this code

https://searchfox.org/mozilla-central/rev/cdddec7fd690700efa4d6b48532cf70155e0386b/image/DecodedSurfaceProvider.cpp#222

which will just share the same surface as before the invalidation happened again. That will let us handle one invalidation but it still won't fix the bug, we need to call ResumeHonoringInvalidations so that we handle further invalidations.

When not using the image provider path, we don't save an image provider on the frame, that alone would avoid this problem. But we also call ResumeHonoringInvalidations for every successful call of VectorImage::Draw.

* * *
imported patch asvfref

Differential Revision: https://phabricator.services.mozilla.com/D174450

image/BlobSurfaceProvider.cpp

@@ -123,7 +123,7 @@ nsresult BlobSurfaceProvider::UpdateKey(
   return NS_ERROR_FAILURE;
 }
 
-void BlobSurfaceProvider::InvalidateRecording() {
+void BlobSurfaceProvider::InvalidateSurface() {
   MOZ_ASSERT(NS_IsMainThread());
 
   auto i = mKeys.Length();

image/BlobSurfaceProvider.h

@@ -97,7 +97,7 @@ class BlobSurfaceProvider final : public ISurfaceProvider {
                      wr::IpcResourceUpdateQueue& aResources,
                      wr::ImageKey& aKey) override;
 
-  void InvalidateRecording() override;
+  void InvalidateSurface() override;
 
   void AddSizeOfExcludingThis(MallocSizeOf aMallocSizeOf,
                               const AddSizeOfCb& aCallback) override {

image/DecodedSurfaceProvider.cpp

@@ -222,6 +222,10 @@ nsresult DecodedSurfaceProvider::UpdateKey(
 nsresult SimpleSurfaceProvider::UpdateKey(
     layers::RenderRootStateManager* aManager,
     wr::IpcResourceUpdateQueue& aResources, wr::ImageKey& aKey) {
+  if (mDirty) {
+    return NS_ERROR_FAILURE;
+  }
+
   RefPtr<SourceSurface> surface = mSurface->GetSourceSurface();
   if (!surface) {
     return NS_ERROR_FAILURE;
@@ -230,5 +234,7 @@ nsresult SimpleSurfaceProvider::UpdateKey(
   return SharedSurfacesChild::Share(surface, aManager, aResources, aKey);
 }
 
+void SimpleSurfaceProvider::InvalidateSurface() { mDirty = true; }
+
 }  // namespace image
 }  // namespace mozilla

image/ISurfaceProvider.h

@@ -314,6 +314,8 @@ class SimpleSurfaceProvider final : public ISurfaceProvider {
                      wr::IpcResourceUpdateQueue& aResources,
                      wr::ImageKey& aKey) override;
 
+  void InvalidateSurface() override;
+
  protected:
   DrawableFrameRef DrawableRef(size_t aFrame) override {
     MOZ_ASSERT(aFrame == 0,
@@ -338,6 +340,7 @@ class SimpleSurfaceProvider final : public ISurfaceProvider {
 
   NotNull<RefPtr<imgFrame>> mSurface;
   DrawableFrameRef mLockRef;
+  bool mDirty = false;
 };
 
 }  // namespace image

image/SurfaceCache.cpp

@@ -181,7 +181,7 @@ class CachedSurface {
     return aMallocSizeOf(this) + aMallocSizeOf(mProvider.get());
   }
 
-  void InvalidateRecording() { mProvider->InvalidateRecording(); }
+  void InvalidateSurface() { mProvider->InvalidateSurface(); }
 
   // A helper type used by SurfaceCacheImpl::CollectSizeOfSurfaces.
   struct MOZ_STACK_CLASS SurfaceMemoryReport {
@@ -543,13 +543,14 @@ class ImageSurfaceCache {
   bool Invalidate(Function&& aRemoveCallback) {
     // Remove all non-blob recordings from the cache. Invalidate any blob
     // recordings.
-    bool foundRecording = false;
+    bool found = false;
     for (auto iter = mSurfaces.Iter(); !iter.Done(); iter.Next()) {
       NotNull<CachedSurface*> current = WrapNotNull(iter.UserData());
 
+      found = true;
+      current->InvalidateSurface();
+
       if (current->GetSurfaceKey().Flags() & SurfaceFlags::RECORD_BLOB) {
-        foundRecording = true;
-        current->InvalidateRecording();
         continue;
       }
 
@@ -558,7 +559,7 @@ class ImageSurfaceCache {
     }
 
     AfterMaybeRemove();
-    return foundRecording;
+    return found;
   }
 
   IntSize SuggestedSize(const IntSize& aSize) const {

image/SurfaceCache.h

@@ -424,12 +424,14 @@ struct SurfaceCache {
 
   /**
    * Removes all rasterized cache entries (including placeholders) associated
-   * with the given image from the cache. Any blob recordings are marked as
-   * dirty and must be regenerated.
+   * with the given image from the cache and marks their surface providers as
+   * dirty and should not be drawn again. Any blob recordings are left in th
+   * cache but marked as dirty and must be regenerated.
    *
    * @param aImageKey  The image whose cache which should be regenerated.
    *
-   * @returns true if any recordings were invalidated, else false.
+   * @returns true if any surface providers were present in the cache, else
+   * false.
    */
   static bool InvalidateImage(const ImageKey aImageKey);
 

image/VectorImage.cpp

@@ -513,7 +513,7 @@ void VectorImage::SendInvalidationNotifications() {
   mHasPendingInvalidation = false;
 
   if (SurfaceCache::InvalidateImage(ImageKey(this))) {
-    // If we still have recordings in the cache, make sure we handle future
+    // If we had any surface providers in the cache, make sure we handle future
     // invalidations.
     MOZ_ASSERT(mRenderingObserver, "Should have a rendering observer by now");
     mRenderingObserver->ResumeHonoringInvalidations();

image/WebRenderImageProvider.h

@@ -48,9 +48,10 @@ class WebRenderImageProvider {
   }
 
   /**
-   * Invalidate if a blob recording, requiring it to be regenerated.
+   * Invalidate if a blob recording or simple surface provider (both are only
+   * used by vector images), requiring it to be regenerated.
    */
-  virtual void InvalidateRecording() {}
+  virtual void InvalidateSurface() {}
 
  protected:
   WebRenderImageProvider(const ImageResource* aImage);

image/test/reftest/generic/1805599-1-ref.html

@@ -0,0 +1,2 @@
+<!-- this is the same png as in the data uri in 1805599-1.svg. svg's used as images cannot contain external images so it has to be a data uri in the svg, here is doesn't matter, we use a png to save space. to trigger the bug the data uri necessarily has to be large, if its too small then we load the data quickly enough that we don't require to invalidate the svg after painting it once before the data uri is loaded -->
+<img src="1805599-1.png">

image/test/reftest/generic/1805599-1.html

@@ -0,0 +1,34 @@
+<!DOCTYPE html>
+<html class="reftest-no-flush reftest-wait" reftest-no-sync-layers>
+<!-- reftest-no-sync-layers so that the updateLayerTree call doesn't a sync decode paint which would hide the bug -->
+<script>
+async function loadimg() {
+  let theimg = document.createElement("img");
+  document.body.appendChild(theimg);
+  theimg.src = "1805599-1.svg";
+  let paintpromise = new Promise(resolve => {
+    window.addEventListener("MozAfterPaint", resolve, { once: true });
+  });
+  await theimg.decode();
+  await paintpromise;
+  setTimeout(finish, 0);
+}
+async function waitForPendingPaints() {
+  while (SpecialPowers.wrap(window).windowUtils.isMozAfterPaintPending) {
+    await new Promise(resolve => {
+      window.addEventListener("MozAfterPaint", resolve, { once: true });
+    });
+  }
+}
+// we have to do this when we use reftest-no-sync-layers to make sure the rendering is up to date
+async function finish() {
+  await waitForPendingPaints();
+  await new Promise(resolve => requestAnimationFrame(resolve));
+  await new Promise(resolve => requestAnimationFrame(resolve));
+  await waitForPendingPaints();
+  document.documentElement.className = "";
+}
+window.addEventListener("MozReftestInvalidate", loadimg);
+</script>
+
+</html>

image/test/reftest/generic/reftest.list

@@ -4,3 +4,4 @@ skip-if(Android) != moz-icon-1.html about:blank
 skip-if(Android) != moz-icon-blank-1-ref.html moz-icon-blank-1-antiref.html
 skip-if(Android) != moz-icon-blank-1-ref.html moz-icon-blank-1-antiref2.html
 fuzzy-if(cocoaWidget,44-49,335-348) fuzzy-if(winWidget,64-140,45-191) == moz-icon-blank-1-almostref.html moz-icon-blank-1-ref.html
+pref(image.testing.decode-sync.enabled,false) == 1805599-1.html 1805599-1-ref.html