Bug 943803 - Use a reentrant monitor instead of unlocking for notifications in RasterImage. r=jdm, a=bajaj
authorSeth Fowler <seth@mozilla.com>
Tue, 17 Dec 2013 14:04:31 -0800
changeset 174902 15d30496c66d9bb2362b9a6cfe1aa88d7ae81d26
parent 174901 2e9d1f109655118ed2e9ab9a57041253f4c51530
child 174903 04e166b45ee3d7f7e0dbc938a8d50e1bfd16b9fb
push id3224
push userlsblakk@mozilla.com
push dateTue, 04 Feb 2014 01:06:49 +0000
treeherdermozilla-beta@60c04d0987f1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjdm, bajaj
bugs943803
milestone28.0a2
Bug 943803 - Use a reentrant monitor instead of unlocking for notifications in RasterImage. r=jdm, a=bajaj
image/src/RasterImage.cpp
image/src/RasterImage.h
--- a/image/src/RasterImage.cpp
+++ b/image/src/RasterImage.cpp
@@ -385,17 +385,17 @@ RasterImage::RasterImage(imgStatusTracke
   mFrameDecodeFlags(DECODE_FLAGS_DEFAULT),
   mMultipartDecodedFrame(nullptr),
   mAnim(nullptr),
   mLockCount(0),
   mDecodeCount(0),
 #ifdef DEBUG
   mFramesNotified(0),
 #endif
-  mDecodingMutex("RasterImage"),
+  mDecodingMonitor("RasterImage Decoding Monitor"),
   mDecoder(nullptr),
   mBytesDecoded(0),
   mInDecoder(false),
   mStatusDiff(ImageStatusDiff::NoChange()),
   mNotifying(false),
   mHasSize(false),
   mDecodeOnDraw(false),
   mMultipart(false),
@@ -437,17 +437,17 @@ RasterImage::~RasterImage()
              num_discardable_containers,
              total_source_bytes,
              discardable_source_bytes));
   }
 
   if (mDecoder) {
     // Kill off our decode request, if it's pending.  (If not, this call is
     // harmless.)
-    MutexAutoLock lock(mDecodingMutex);
+    ReentrantMonitorAutoEnter lock(mDecodingMonitor);
     DecodePool::StopDecoding(this);
     mDecoder = nullptr;
 
     // Unlock the last frame (if we have any). Our invariant is that, while we
     // have a decoder open, the last frame is always locked.
     // This would be done in ShutdownDecoder, but since mDecoder is non-null,
     // we didn't call ShutdownDecoder and we need to do it manually.
     if (GetNumFrames() > 0) {
@@ -1538,17 +1538,17 @@ RasterImage::SetLoopCount(int32_t aLoopC
   if (mAnim) {
     mAnim->SetLoopCount(aLoopCount);
   }
 }
 
 nsresult
 RasterImage::AddSourceData(const char *aBuffer, uint32_t aCount)
 {
-  MutexAutoLock lock(mDecodingMutex);
+  ReentrantMonitorAutoEnter lock(mDecodingMonitor);
 
   if (mError)
     return NS_ERROR_FAILURE;
 
   NS_ENSURE_ARG_POINTER(aBuffer);
   nsresult rv = NS_OK;
 
   // We should not call this if we're not initialized
@@ -1677,17 +1677,17 @@ RasterImage::DoImageDataComplete()
   // image's size, this does nothing.)  Then kick off an async decode of the
   // rest of the image.
   if (mDecoder) {
     nsresult rv = DecodePool::Singleton()->DecodeUntilSizeAvailable(this);
     CONTAINER_ENSURE_SUCCESS(rv);
   }
 
   {
-    MutexAutoLock lock(mDecodingMutex);
+    ReentrantMonitorAutoEnter lock(mDecodingMonitor);
 
     // If we're not storing any source data, then there's nothing more we can do
     // once we've tried decoding for size.
     if (!StoringSourceData() && mDecoder) {
       nsresult rv = ShutdownDecoder(eShutdownIntent_Done);
       CONTAINER_ENSURE_SUCCESS(rv);
     }
 
@@ -1732,17 +1732,17 @@ RasterImage::OnImageDataComplete(nsIRequ
   if (NS_FAILED(aStatus))
     finalStatus = aStatus;
 
   nsRefPtr<imgStatusTracker> statusTracker = CurrentStatusTracker();
   statusTracker->GetDecoderObserver()->OnStopRequest(aLastPart, finalStatus);
 
   // We just recorded OnStopRequest; we need to inform our listeners.
   {
-    MutexAutoLock lock(mDecodingMutex);
+    ReentrantMonitorAutoEnter lock(mDecodingMonitor);
     FinishedSomeDecoding();
   }
 
   return finalStatus;
 }
 
 nsresult
 RasterImage::OnImageDataAvailable(nsIRequest*,
@@ -2066,17 +2066,17 @@ RasterImage::InitDecoder(bool aDoSizeDec
 // aIntent specifies the intent of the shutdown. If aIntent is
 // eShutdownIntent_Done, an error is flagged if we didn't get what we should
 // have out of the decode. If aIntent is eShutdownIntent_NotNeeded, we don't
 // check this. If aIntent is eShutdownIntent_Error, we shut down in error mode.
 nsresult
 RasterImage::ShutdownDecoder(eShutdownIntent aIntent)
 {
   MOZ_ASSERT(NS_IsMainThread());
-  mDecodingMutex.AssertCurrentThreadOwns();
+  mDecodingMonitor.AssertCurrentThreadIn();
 
   // Ensure that our intent is valid
   NS_ABORT_IF_FALSE((aIntent >= 0) && (aIntent < eShutdownIntent_AllCount),
                     "Invalid shutdown intent");
 
   // Ensure that the decoder is initialized
   NS_ABORT_IF_FALSE(mDecoder, "Calling ShutdownDecoder() with no active decoder!");
 
@@ -2134,17 +2134,17 @@ RasterImage::ShutdownDecoder(eShutdownIn
 
   return NS_OK;
 }
 
 // Writes the data to the decoder, updating the total number of bytes written.
 nsresult
 RasterImage::WriteToDecoder(const char *aBuffer, uint32_t aCount)
 {
-  mDecodingMutex.AssertCurrentThreadOwns();
+  mDecodingMonitor.AssertCurrentThreadIn();
 
   // We should have a decoder
   NS_ABORT_IF_FALSE(mDecoder, "Trying to write to null decoder!");
 
   // Write
   nsRefPtr<Decoder> kungFuDeathGrip = mDecoder;
   mInDecoder = true;
   mDecoder->Write(aBuffer, aCount);
@@ -2253,17 +2253,17 @@ RasterImage::RequestDecodeCore(RequestDe
     // If we didn't get the size out of the image, we won't until we get more
     // data, so signal that we want a full decode and give up for now.
     if (!mHasSize) {
       mWantFullDecode = true;
       return NS_OK;
     }
   }
 
-  MutexAutoLock lock(mDecodingMutex);
+  ReentrantMonitorAutoEnter lock(mDecodingMonitor);
 
   // If we don't have any bytes to flush to the decoder, we can't do anything.
   // mBytesDecoded can be bigger than mSourceData.Length() if we're not storing
   // the source data.
   if (mBytesDecoded > mSourceData.Length())
     return NS_OK;
 
   // If the image is waiting for decode work to be notified, go ahead and do that.
@@ -2344,17 +2344,17 @@ RasterImage::SyncDecode()
     // If we didn't get the size out of the image, we won't until we get more
     // data, so signal that we want a full decode and give up for now.
     if (!mHasSize) {
       mWantFullDecode = true;
       return NS_ERROR_NOT_AVAILABLE;
     }
   }
 
-  MutexAutoLock imgLock(mDecodingMutex);
+  ReentrantMonitorAutoEnter lock(mDecodingMonitor);
 
   // We really have no good way of forcing a synchronous decode if we're being
   // called in a re-entrant manner (ie, from an event listener fired by a
   // decoder), because the decoding machinery is already tied up. We thus explicitly
   // disallow this type of call in the API, and check for it in API methods.
   NS_ABORT_IF_FALSE(!mInDecoder, "Yikes, forcing sync in reentrant call!");
 
   if (mDecodeRequest) {
@@ -2712,17 +2712,17 @@ RasterImage::UnlockImage()
   // and our lock count is now zero (so nothing is forcing us to keep the
   // decoded data around), try to cancel the decode and throw away whatever
   // we've decoded.
   if (mHasBeenDecoded && mDecoder &&
       mLockCount == 0 && CanForciblyDiscard()) {
     PR_LOG(GetCompressedImageAccountingLog(), PR_LOG_DEBUG,
            ("RasterImage[0x%p] canceling decode because image "
             "is now unlocked.", this));
-    MutexAutoLock lock(mDecodingMutex);
+    ReentrantMonitorAutoEnter lock(mDecodingMonitor);
     FinishedSomeDecoding(eShutdownIntent_NotNeeded);
     ForceDiscard();
     return NS_OK;
   }
 
   // Otherwise, we might still be a candidate for discarding in the future.  If
   // we are, add ourselves to the discard tracker.
   if (CanDiscard()) {
@@ -2747,17 +2747,17 @@ RasterImage::RequestDiscard()
 
 // Flushes up to aMaxBytes to the decoder.
 nsresult
 RasterImage::DecodeSomeData(uint32_t aMaxBytes)
 {
   // We should have a decoder if we get here
   NS_ABORT_IF_FALSE(mDecoder, "trying to decode without decoder!");
 
-  mDecodingMutex.AssertCurrentThreadOwns();
+  mDecodingMonitor.AssertCurrentThreadIn();
 
   // First, if we've just been called because we allocated a frame on the main
   // thread, let the decoder deal with the data it set aside at that time by
   // passing it a null buffer.
   if (mDecodeRequest->mAllocatedNewFrame) {
     mDecodeRequest->mAllocatedNewFrame = false;
     nsresult rv = WriteToDecoder(nullptr, 0);
     if (NS_FAILED(rv) || mDecoder->NeedsNewFrame()) {
@@ -2783,17 +2783,17 @@ RasterImage::DecodeSomeData(uint32_t aMa
 // There are various indicators that tell us we're finished with the decode
 // task at hand and can shut down the decoder.
 //
 // This method may not be called if there is no decoder.
 bool
 RasterImage::IsDecodeFinished()
 {
   // Precondition
-  mDecodingMutex.AssertCurrentThreadOwns();
+  mDecodingMonitor.AssertCurrentThreadIn();
   NS_ABORT_IF_FALSE(mDecoder, "Can't call IsDecodeFinished() without decoder!");
 
   // The decode is complete if we got what we wanted.
   if (mDecoder->IsSizeDecode()) {
     if (mDecoder->HasSize()) {
       return true;
     }
   } else if (mDecoder->GetDecodeDone()) {
@@ -2833,17 +2833,17 @@ RasterImage::DoError()
   // We can't safely handle errors off-main-thread, so dispatch a worker to do it.
   if (!NS_IsMainThread()) {
     HandleErrorWorker::DispatchIfNeeded(this);
     return;
   }
 
   // If we're mid-decode, shut down the decoder.
   if (mDecoder) {
-    MutexAutoLock lock(mDecodingMutex);
+    ReentrantMonitorAutoEnter lock(mDecodingMonitor);
     FinishedSomeDecoding(eShutdownIntent_Error);
   }
 
   // Put the container in an error state.
   mError = true;
 
   nsRefPtr<imgStatusTracker> statusTracker = CurrentStatusTracker();
   statusTracker->GetDecoderObserver()->OnError();
@@ -2954,17 +2954,17 @@ RasterImage::RequestDecodeIfNeeded(nsres
 }
 
 nsresult
 RasterImage::FinishedSomeDecoding(eShutdownIntent aIntent /* = eShutdownIntent_Done */,
                                   DecodeRequest* aRequest /* = nullptr */)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
-  mDecodingMutex.AssertCurrentThreadOwns();
+  mDecodingMonitor.AssertCurrentThreadIn();
 
   nsRefPtr<DecodeRequest> request;
   if (aRequest) {
     request = aRequest;
   } else {
     request = mDecodeRequest;
   }
 
@@ -3021,20 +3021,16 @@ RasterImage::FinishedSomeDecoding(eShutd
     }
   }
 
   ImageStatusDiff diff =
     request ? image->mStatusTracker->Difference(request->mStatusTracker)
             : image->mStatusTracker->DecodeStateAsDifference();
   image->mStatusTracker->ApplyDifference(diff);
 
-  // Notifications can't go out with the decoding lock held, nor can we call
-  // RequestDecodeIfNeeded, so unlock for the rest of the function.
-  MutexAutoUnlock unlock(mDecodingMutex);
-
   if (mNotifying) {
     // Accumulate the status changes. We don't permit recursive notifications
     // because they cause subtle concurrency bugs, so we'll delay sending out
     // the notifications until we pop back to the lowest invocation of
     // FinishedSomeDecoding on the stack.
     NS_WARNING("Recursively notifying in RasterImage::FinishedSomeDecoding!");
     mStatusDiff.Combine(diff);
   } else {
@@ -3166,17 +3162,17 @@ RasterImage::DecodePool::Observe(nsISupp
 
   return NS_OK;
 }
 
 void
 RasterImage::DecodePool::RequestDecode(RasterImage* aImg)
 {
   MOZ_ASSERT(aImg->mDecoder);
-  aImg->mDecodingMutex.AssertCurrentThreadOwns();
+  aImg->mDecodingMonitor.AssertCurrentThreadIn();
 
   // If we're currently waiting on a new frame for this image, we can't do any
   // decoding.
   if (!aImg->mDecoder->NeedsNewFrame()) {
     // No matter whether this is currently being decoded, we need to update the
     // number of bytes we want it to decode.
     aImg->mDecodeRequest->mBytesToDecode = aImg->mSourceData.Length() - aImg->mBytesDecoded;
 
@@ -3198,17 +3194,17 @@ RasterImage::DecodePool::RequestDecode(R
     }
   }
 }
 
 void
 RasterImage::DecodePool::DecodeABitOf(RasterImage* aImg)
 {
   MOZ_ASSERT(NS_IsMainThread());
-  aImg->mDecodingMutex.AssertCurrentThreadOwns();
+  aImg->mDecodingMonitor.AssertCurrentThreadIn();
 
   if (aImg->mDecodeRequest) {
     // If the image is waiting for decode work to be notified, go ahead and do that.
     if (aImg->mDecodeRequest->mRequestStatus == DecodeRequest::REQUEST_WORK_DONE) {
       aImg->FinishedSomeDecoding();
     }
   }
 
@@ -3230,29 +3226,29 @@ RasterImage::DecodePool::DecodeABitOf(Ra
       RequestDecode(aImg);
     }
   }
 }
 
 /* static */ void
 RasterImage::DecodePool::StopDecoding(RasterImage* aImg)
 {
-  aImg->mDecodingMutex.AssertCurrentThreadOwns();
+  aImg->mDecodingMonitor.AssertCurrentThreadIn();
 
   // If we haven't got a decode request, we're not currently decoding. (Having
   // a decode request doesn't imply we *are* decoding, though.)
   if (aImg->mDecodeRequest) {
     aImg->mDecodeRequest->mRequestStatus = DecodeRequest::REQUEST_STOPPED;
   }
 }
 
 NS_IMETHODIMP
 RasterImage::DecodePool::DecodeJob::Run()
 {
-  MutexAutoLock imglock(mImage->mDecodingMutex);
+  ReentrantMonitorAutoEnter lock(mImage->mDecodingMonitor);
 
   // If we were interrupted, we shouldn't do any work.
   if (mRequest->mRequestStatus == DecodeRequest::REQUEST_STOPPED) {
     DecodeDoneWorker::NotifyFinishedSomeDecoding(mImage, mRequest);
     return NS_OK;
   }
 
   // If someone came along and synchronously decoded us, there's nothing for us to do.
@@ -3307,18 +3303,17 @@ RasterImage::DecodePool::DecodeJob::Run(
 
   return NS_OK;
 }
 
 nsresult
 RasterImage::DecodePool::DecodeUntilSizeAvailable(RasterImage* aImg)
 {
   MOZ_ASSERT(NS_IsMainThread());
-
-  MutexAutoLock imgLock(aImg->mDecodingMutex);
+  ReentrantMonitorAutoEnter lock(aImg->mDecodingMonitor);
 
   if (aImg->mDecodeRequest) {
     // If the image is waiting for decode work to be notified, go ahead and do that.
     if (aImg->mDecodeRequest->mRequestStatus == DecodeRequest::REQUEST_WORK_DONE) {
       nsresult rv = aImg->FinishedSomeDecoding();
       if (NS_FAILED(rv)) {
         aImg->DoError();
         return rv;
@@ -3344,17 +3339,17 @@ RasterImage::DecodePool::DecodeUntilSize
 
 nsresult
 RasterImage::DecodePool::DecodeSomeOfImage(RasterImage* aImg,
                                            DecodeType aDecodeType /* = DECODE_TYPE_UNTIL_TIME */,
                                            uint32_t bytesToDecode /* = 0 */)
 {
   NS_ABORT_IF_FALSE(aImg->mInitialized,
                     "Worker active for uninitialized container!");
-  aImg->mDecodingMutex.AssertCurrentThreadOwns();
+  aImg->mDecodingMonitor.AssertCurrentThreadIn();
 
   // If an error is flagged, it probably happened while we were waiting
   // in the event queue.
   if (aImg->mError)
     return NS_OK;
 
   // If mDecoded or we don't have a decoder, we must have finished already (for
   // example, a synchronous decode request came while the worker was pending).
@@ -3462,27 +3457,27 @@ RasterImage::DecodePool::DecodeSomeOfIma
 RasterImage::DecodeDoneWorker::DecodeDoneWorker(RasterImage* image, DecodeRequest* request)
  : mImage(image)
  , mRequest(request)
 {}
 
 void
 RasterImage::DecodeDoneWorker::NotifyFinishedSomeDecoding(RasterImage* image, DecodeRequest* request)
 {
-  image->mDecodingMutex.AssertCurrentThreadOwns();
+  image->mDecodingMonitor.AssertCurrentThreadIn();
 
   nsCOMPtr<DecodeDoneWorker> worker = new DecodeDoneWorker(image, request);
   NS_DispatchToMainThread(worker);
 }
 
 NS_IMETHODIMP
 RasterImage::DecodeDoneWorker::Run()
 {
   MOZ_ASSERT(NS_IsMainThread());
-  MutexAutoLock lock(mImage->mDecodingMutex);
+  ReentrantMonitorAutoEnter lock(mImage->mDecodingMonitor);
 
   mImage->FinishedSomeDecoding(eShutdownIntent_Done, mRequest);
 
   return NS_OK;
 }
 
 RasterImage::FrameNeededWorker::FrameNeededWorker(RasterImage* image)
  : mImage(image)
@@ -3494,17 +3489,17 @@ RasterImage::FrameNeededWorker::GetNewFr
 {
   nsCOMPtr<FrameNeededWorker> worker = new FrameNeededWorker(image);
   NS_DispatchToMainThread(worker);
 }
 
 NS_IMETHODIMP
 RasterImage::FrameNeededWorker::Run()
 {
-  MutexAutoLock lock(mImage->mDecodingMutex);
+  ReentrantMonitorAutoEnter lock(mImage->mDecodingMonitor);
   nsresult rv = NS_OK;
 
   // If we got a synchronous decode in the mean time, we don't need to do
   // anything.
   if (mImage->mDecoder && mImage->mDecoder->NeedsNewFrame()) {
     rv = mImage->mDecoder->AllocateFrame();
     mImage->mDecodeRequest->mAllocatedNewFrame = true;
   }
--- a/image/src/RasterImage.h
+++ b/image/src/RasterImage.h
@@ -24,20 +24,21 @@
 #include "nsIProperties.h"
 #include "nsTArray.h"
 #include "imgFrame.h"
 #include "nsThreadUtils.h"
 #include "DiscardTracker.h"
 #include "Orientation.h"
 #include "nsIObserver.h"
 #include "mozilla/MemoryReporting.h"
+#include "mozilla/Mutex.h"
+#include "mozilla/ReentrantMonitor.h"
 #include "mozilla/TimeStamp.h"
 #include "mozilla/StaticPtr.h"
 #include "mozilla/WeakPtr.h"
-#include "mozilla/Mutex.h"
 #ifdef DEBUG
   #include "imgIContainerDebug.h"
 #endif
 
 class nsIInputStream;
 class nsIThreadPool;
 class nsIRequest;
 
@@ -117,21 +118,23 @@ class nsIRequest;
  * @par
  * The mAnim structure has members only needed for animated images, so
  * it's not allocated until the second frame is added.
  */
 
 class ScaleRequest;
 
 namespace mozilla {
+
 namespace layers {
 class LayerManager;
 class ImageContainer;
 class Image;
 }
+
 namespace image {
 
 class Decoder;
 class FrameAnimator;
 
 class RasterImage : public ImageResource
                   , public nsIProperties
                   , public SupportsWeakPtr<RasterImage>
@@ -467,20 +470,20 @@ private:
     private:
       nsRefPtr<DecodeRequest> mRequest;
       nsRefPtr<RasterImage> mImage;
     };
 
   private: /* members */
 
     // mThreadPoolMutex protects mThreadPool. For all RasterImages R,
-    // R::mDecodingMutex must be acquired before mThreadPoolMutex if both are
-    // acquired; the other order may cause deadlock.
-    mozilla::Mutex          mThreadPoolMutex;
-    nsCOMPtr<nsIThreadPool> mThreadPool;
+    // R::mDecodingMonitor must be acquired before mThreadPoolMutex
+    // if both are acquired; the other order may cause deadlock.
+    mozilla::Mutex            mThreadPoolMutex;
+    nsCOMPtr<nsIThreadPool>   mThreadPool;
   };
 
   class DecodeDoneWorker : public nsRunnable
   {
   public:
     /**
      * Called by the DecodePool with an image when it's done some significant
      * portion of decoding that needs to be notified about.
@@ -637,20 +640,20 @@ private: // data
   // Cached value for GetImageContainer.
   nsRefPtr<mozilla::layers::ImageContainer> mImageContainer;
 
 #ifdef DEBUG
   uint32_t                       mFramesNotified;
 #endif
 
   // Below are the pieces of data that can be accessed on more than one thread
-  // at once, and hence need to be locked by mDecodingMutex.
+  // at once, and hence need to be locked by mDecodingMonitor.
 
   // BEGIN LOCKED MEMBER VARIABLES
-  mozilla::Mutex             mDecodingMutex;
+  mozilla::ReentrantMonitor  mDecodingMonitor;
 
   FallibleTArray<char>       mSourceData;
 
   // Decoder and friends
   nsRefPtr<Decoder>          mDecoder;
   nsRefPtr<DecodeRequest>    mDecodeRequest;
   uint32_t                   mBytesDecoded;