Bug 1474900: Assert there are no pending locks when destroying the image proxy. r=tnikkel
authorEmilio Cobos Álvarez <emilio@crisal.io>
Wed, 11 Jul 2018 17:06:40 +0200
changeset 430339 1efeecd0ac69a8f5852ee48e3a5539e3b3e367e4
parent 430338 ef7f43b981ef872b3faa3a8d62d8115d59df1cd2
child 430340 3f30b1a694972d96332c9165a6131cf400c68b44
push id34401
push useraiakab@mozilla.com
push dateTue, 07 Aug 2018 15:42:55 +0000
treeherdermozilla-central@936c5d6bd40b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1474900
milestone63.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 1474900: Assert there are no pending locks when destroying the image proxy. r=tnikkel
image/imgRequestProxy.cpp
image/test/gtest/TestDecoders.cpp
image/test/unit/async_load_tests.js
layout/xul/nsImageBoxFrame.cpp
layout/xul/tree/nsTreeBodyFrame.cpp
layout/xul/tree/nsTreeBodyFrame.h
--- a/image/imgRequestProxy.cpp
+++ b/image/imgRequestProxy.cpp
@@ -145,21 +145,17 @@ imgRequestProxy::~imgRequestProxy()
   // when we issue state notifications, some or all need to be dispatched to the
   // appropriate scheduler group for each request. This should be rare, so we
   // want to monitor the frequency of dispatching in the wild.
   if (mHadListener) {
     mozilla::Telemetry::Accumulate(mozilla::Telemetry::IMAGE_REQUEST_DISPATCHED,
                                    mHadDispatch);
   }
 
-  // Unlock the image the proper number of times if we're holding locks on
-  // it. Note that UnlockImage() decrements mLockCount each time it's called.
-  while (mLockCount) {
-    UnlockImage();
-  }
+  MOZ_RELEASE_ASSERT(!mLockCount, "Someone forgot to unlock on time?");
 
   ClearAnimationConsumers();
 
   // Explicitly set mListener to null to ensure that the RemoveProxy
   // call below can't send |this| to an arbitrary listener while |this|
   // is being destroyed.  This is all belt-and-suspenders in view of the
   // above assert.
   NullOutListener();
--- a/image/test/gtest/TestDecoders.cpp
+++ b/image/test/gtest/TestDecoders.cpp
@@ -402,16 +402,20 @@ TEST_F(ImageDecoders, AnimatedGIFWithFRA
   ASSERT_TRUE(NS_SUCCEEDED(rv));
 
   RefPtr<ProgressTracker> tracker = image->GetProgressTracker();
   tracker->SyncNotifyProgress(FLAG_LOAD_COMPLETE);
 
   // 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->GetFrame(imgIContainer::FRAME_FIRST,
                     imgIContainer::FLAG_SYNC_DECODE);
 
   // Ensure that the image's metadata meets our expectations.
   IntSize imageSize(0, 0);
--- a/image/test/unit/async_load_tests.js
+++ b/image/test/unit/async_load_tests.js
@@ -40,17 +40,17 @@ function checkClone(other_listener, aReq
   do_test_pending();
 
   // For as long as clone notification is synchronous, we can't test the clone state reliably.
   var listener = new ImageListener(null, function(foo, bar) { do_test_finished(); } /*getCloneStopCallback(other_listener)*/);
   listener.synchronous = false;
   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.
 function checkSizeAndLoad(listener, aRequest)
 {
   Assert.notEqual(listener.state & SIZE_AVAILABLE, 0);
   Assert.notEqual(listener.state & LOAD_COMPLETE, 0);
 
@@ -66,17 +66,17 @@ function secondLoadDone(oldlistener, aRe
 
     // For as long as clone notification is synchronous, we can't test the
     // clone state reliably.
     var listener = new ImageListener(null, checkSizeAndLoad);
     listener.synchronous = false;
     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.
     do_test_finished();
   }
 
   run_loadImageWithChannel_tests();
 
@@ -87,17 +87,20 @@ function secondLoadDone(oldlistener, aRe
 // therefore would be at most risk of being served synchronously.
 function checkSecondLoad()
 {
   do_test_pending();
 
   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;
 }
 
 function firstLoadDone(oldlistener, aRequest)
 {
   checkSecondLoad(uri);
 
   do_test_finished();
@@ -125,17 +128,20 @@ function checkSecondChannelLoad()
   channel.asyncOpen2(channellistener);
 
   var listener = new ImageListener(null,
                                    getChannelLoadImageStopCallback(channellistener,
                                                                    all_done_callback));
   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;
 }
 
 function run_loadImageWithChannel_tests()
 {
   // To ensure we're testing what we expect to, create a new loader and cache.
@@ -147,17 +153,20 @@ function run_loadImageWithChannel_tests(
   channel.asyncOpen2(channellistener);
 
   var listener = new ImageListener(null,
                                    getChannelLoadImageStopCallback(channellistener,
                                                                    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;
 }
 
 function all_done_callback()
 {
   server.stop(function() { do_test_finished(); });
@@ -167,43 +176,50 @@ function startImageCallback(otherCb)
 {
   return function(listener, request)
   {
     // Make sure we can load the same image immediately out of the cache.
     do_test_pending();
     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.
     otherCb(listener, request);
   }
 }
 
 var gCurrentLoader;
 
 function cleanup()
 {
-  for (var i = 0; i < requests.length; ++i) {
-    requests[i].cancelAndForgetObserver(0);
+  for (let {request, locked} of requests) {
+    if (locked) {
+      try { request.unlockImage() } catch (e) {}
+    }
+    request.cancelAndForgetObserver(0);
   }
 }
 
 function run_test()
 {
   registerCleanupFunction(cleanup);
 
   gCurrentLoader = Cc["@mozilla.org/image/loader;1"].createInstance(Ci.imgILoader);
 
   do_test_pending();
   var listener = new ImageListener(startImageCallback(checkClone), firstLoadDone);
   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;
 }
--- a/layout/xul/nsImageBoxFrame.cpp
+++ b/layout/xul/nsImageBoxFrame.cpp
@@ -177,16 +177,18 @@ nsImageBoxFrame::MarkIntrinsicISizesDirt
 
 void
 nsImageBoxFrame::DestroyFrom(nsIFrame* aDestructRoot, PostDestroyData& aPostDestroyData)
 {
   if (mImageRequest) {
     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);
   }
 
   if (mListener)
     reinterpret_cast<nsImageBoxListener*>(mListener.get())->ClearFrame(); // set the frame to null so we don't send messages to a dead object.
 
   nsLeafBoxFrame::DestroyFrom(aDestructRoot, aPostDestroyData);
--- a/layout/xul/tree/nsTreeBodyFrame.cpp
+++ b/layout/xul/tree/nsTreeBodyFrame.cpp
@@ -84,16 +84,17 @@ void
 nsTreeBodyFrame::CancelImageRequests()
 {
   for (auto iter = mImageCache.Iter(); !iter.Done(); iter.Next()) {
     // If our imgIRequest object was registered with the refresh driver
     // then we need to deregister it.
     nsTreeImageCacheEntry entry = iter.UserData();
     nsLayoutUtils::DeregisterImageRequest(PresContext(), entry.request,
                                           nullptr);
+    entry.request->UnlockImage();
     entry.request->CancelAndForgetObserver(NS_BINDING_ABORTED);
   }
 }
 
 //
 // NS_NewTreeFrame
 //
 // Creates a new tree frame
@@ -4238,16 +4239,17 @@ void
 nsTreeBodyFrame::RemoveImageCacheEntry(int32_t aRowIndex, nsTreeColumn* aCol)
 {
   nsAutoString imageSrc;
   if (NS_SUCCEEDED(mView->GetImageSrc(aRowIndex, aCol, imageSrc))) {
     nsTreeImageCacheEntry entry;
     if (mImageCache.Get(imageSrc, &entry)) {
       nsLayoutUtils::DeregisterImageRequest(PresContext(), entry.request,
                                             nullptr);
+      entry.request->UnlockImage();
       entry.request->CancelAndForgetObserver(NS_BINDING_ABORTED);
       mImageCache.Remove(imageSrc);
     }
   }
 }
 
 /* virtual */ void
 nsTreeBodyFrame::DidSetComputedStyle(ComputedStyle* aOldComputedStyle)
--- a/layout/xul/tree/nsTreeBodyFrame.h
+++ b/layout/xul/tree/nsTreeBodyFrame.h
@@ -33,19 +33,22 @@ namespace mozilla {
 namespace layout {
 class ScrollbarActivity;
 } // namespace layout
 } // namespace mozilla
 
 // An entry in the tree's image cache
 struct nsTreeImageCacheEntry
 {
-  nsTreeImageCacheEntry() {}
-  nsTreeImageCacheEntry(imgIRequest *aRequest, imgINotificationObserver *aListener)
-    : request(aRequest), listener(aListener) {}
+  nsTreeImageCacheEntry() = default;
+  nsTreeImageCacheEntry(imgIRequest* aRequest,
+                        imgINotificationObserver* aListener)
+    : request(aRequest)
+    , listener(aListener)
+  { }
 
   nsCOMPtr<imgIRequest> request;
   nsCOMPtr<imgINotificationObserver> listener;
 };
 
 // The actual frame that paints the cells and rows.
 class nsTreeBodyFrame final
   : public nsLeafBoxFrame