Bug 1474900: Assert there are no pending locks when destroying the image proxy. r=tnikkel

image/imgRequestProxy.cpp

@@ -150,11 +150,7 @@ imgRequestProxy::~imgRequestProxy()
                                    mHadDispatch);
   }
 
-  // Unlock the image the proper number of times if we're holding locks on
+  MOZ_RELEASE_ASSERT(!mLockCount, "Someone forgot to unlock on time?");
-  // it. Note that UnlockImage() decrements mLockCount each time it's called.
-  while (mLockCount) {
-    UnlockImage();
-  }
 
   ClearAnimationConsumers();
 

image/test/gtest/TestDecoders.cpp

@@ -407,6 +407,10 @@ TEST_F(ImageDecoders, AnimatedGIFWithFRAME_FIRST)
   // Lock the image so its surfaces don't disappear during the test.
   image->LockImage();
 
+  auto unlock = mozilla::MakeScopeExit([&] {
+    image->UnlockImage();
+  });
+
   // Use GetFrame() to force a sync decode of the image, specifying FRAME_FIRST
   // to ensure that we don't get an animated decode.
   RefPtr<SourceSurface> surface =

image/test/unit/async_load_tests.js

@@ -45,7 +45,7 @@ function checkClone(other_listener, aRequest)
   var outer = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
                 .createScriptedObserver(listener);
   var clone = aRequest.clone(outer);
-  requests.push(clone);
+  requests.push({ request: clone, locked: false });
 }
 
 // Ensure that all the callbacks were called on aRequest.
@@ -71,7 +71,7 @@ function secondLoadDone(oldlistener, aRequest)
     var outer = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
                   .createScriptedObserver(listener);
     var staticrequestclone = staticrequest.clone(outer);
-    requests.push(staticrequestclone);
+    requests.push({ request: staticrequestclone, locked: false });
   } catch(e) {
     // We can't create a static request. Most likely the request we started
     // with didn't load successfully.
@@ -92,7 +92,10 @@ function checkSecondLoad()
   var listener = new ImageListener(checkClone, secondLoadDone);
   var outer = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
                 .createScriptedObserver(listener);
-  requests.push(gCurrentLoader.loadImageXPCOM(uri, null, null, "default", null, null, outer, null, 0, null));
+  requests.push({
+    request: gCurrentLoader.loadImageXPCOM(uri, null, null, "default", null, null, outer, null, 0, null),
+    locked: false,
+  });
   listener.synchronous = false;
 }
 
@@ -130,7 +133,10 @@ function checkSecondChannelLoad()
   var outer = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
                 .createScriptedObserver(listener);
   var outlistener = {};
-  requests.push(gCurrentLoader.loadImageWithChannelXPCOM(channel, outer, null, outlistener));
+  requests.push({
+    request: gCurrentLoader.loadImageWithChannelXPCOM(channel, outer, null, outlistener),
+    locked: false,
+  });
   channellistener.outputListener = outlistener.value;
 
   listener.synchronous = false;
@@ -152,7 +158,10 @@ function run_loadImageWithChannel_tests()
   var outer = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
                 .createScriptedObserver(listener);
   var outlistener = {};
-  requests.push(gCurrentLoader.loadImageWithChannelXPCOM(channel, outer, null, outlistener));
+  requests.push({
+    request: gCurrentLoader.loadImageWithChannelXPCOM(channel, outer, null, outlistener),
+    locked: false,
+  });
   channellistener.outputListener = outlistener.value;
 
   listener.synchronous = false;
@@ -172,7 +181,10 @@ function startImageCallback(otherCb)
     var listener2 = new ImageListener(null, function(foo, bar) { do_test_finished(); });
     var outer = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
                   .createScriptedObserver(listener2);
-    requests.push(gCurrentLoader.loadImageXPCOM(uri, null, null, "default", null, null, outer, null, 0, null));
+    requests.push({
+      request: gCurrentLoader.loadImageXPCOM(uri, null, null, "default", null, null, outer, null, 0, null),
+      locked: false,
+    });
     listener2.synchronous = false;
 
     // Now that we've started another load, chain to the callback.
@@ -184,8 +196,11 @@ var gCurrentLoader;
 
 function cleanup()
 {
-  for (var i = 0; i < requests.length; ++i) {
+  for (let {request, locked} of requests) {
-    requests[i].cancelAndForgetObserver(0);
+    if (locked) {
+      try { request.unlockImage() } catch (e) {}
+    }
+    request.cancelAndForgetObserver(0);
   }
 }
 
@@ -200,10 +215,11 @@ function run_test()
   var outer = Cc["@mozilla.org/image/tools;1"].getService(Ci.imgITools)
                 .createScriptedObserver(listener);
   var req = gCurrentLoader.loadImageXPCOM(uri, null, null, "default", null, null, outer, null, 0, null);
-  requests.push(req);
 
   // Ensure that we don't cause any mayhem when we lock an image.
   req.lockImage();
 
+  requests.push({ request: req, locked: true });
+
   listener.synchronous = false;
 }

layout/xul/nsImageBoxFrame.cpp

@@ -182,6 +182,8 @@ nsImageBoxFrame::DestroyFrom(nsIFrame* aDestructRoot, PostDestroyData& aPostDest
     nsLayoutUtils::DeregisterImageRequest(PresContext(), mImageRequest,
                                           &mRequestRegistered);
 
+    mImageRequest->UnlockImage();
+
     // Release image loader first so that it's refcnt can go to zero
     mImageRequest->CancelAndForgetObserver(NS_ERROR_FAILURE);
   }

layout/xul/tree/nsTreeBodyFrame.cpp

@@ -89,6 +89,7 @@ nsTreeBodyFrame::CancelImageRequests()
     nsTreeImageCacheEntry entry = iter.UserData();
     nsLayoutUtils::DeregisterImageRequest(PresContext(), entry.request,
                                           nullptr);
+    entry.request->UnlockImage();
     entry.request->CancelAndForgetObserver(NS_BINDING_ABORTED);
   }
 }
@@ -4243,6 +4244,7 @@ nsTreeBodyFrame::RemoveImageCacheEntry(int32_t aRowIndex, nsTreeColumn* aCol)
     if (mImageCache.Get(imageSrc, &entry)) {
       nsLayoutUtils::DeregisterImageRequest(PresContext(), entry.request,
                                             nullptr);
+      entry.request->UnlockImage();
       entry.request->CancelAndForgetObserver(NS_BINDING_ABORTED);
       mImageCache.Remove(imageSrc);
     }

layout/xul/tree/nsTreeBodyFrame.h

@@ -38,9 +38,12 @@ class ScrollbarActivity;
 // An entry in the tree's image cache
 struct nsTreeImageCacheEntry
 {
-  nsTreeImageCacheEntry() {}
+  nsTreeImageCacheEntry() = default;
-  nsTreeImageCacheEntry(imgIRequest *aRequest, imgINotificationObserver *aListener)
+  nsTreeImageCacheEntry(imgIRequest* aRequest,
-    : request(aRequest), listener(aListener) {}
+                        imgINotificationObserver* aListener)
+    : request(aRequest)
+    , listener(aListener)
+  { }
 
   nsCOMPtr<imgIRequest> request;
   nsCOMPtr<imgINotificationObserver> listener;