Bug 940714 - Add a RAII class to make synchronous decoding safer. r=tn
authorSeth Fowler <seth@mozilla.com>
Wed, 20 Nov 2013 17:21:51 -0800
changeset 156659 b0433773e27c3fe5849f0aca1de17e6132b62336
parent 156658 0df2441272fb5cbf9878f38273d3b9f76cdc547d
child 156660 bada32435a8e6552ec728b28e95bb621e628bf1d
push id36500
push usermfowler@mozilla.com
push dateThu, 21 Nov 2013 01:22:21 +0000
treeherdermozilla-inbound@b0433773e27c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstn
bugs940714
milestone28.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 940714 - Add a RAII class to make synchronous decoding safer. r=tn
image/src/Decoder.h
image/src/RasterImage.cpp
image/src/RasterImage.h
--- a/image/src/Decoder.h
+++ b/image/src/Decoder.h
@@ -89,21 +89,16 @@ public:
   // must be enabled by SetSizeDecode() _before_calling Init().
   bool IsSizeDecode() { return mSizeDecode; }
   void SetSizeDecode(bool aSizeDecode)
   {
     NS_ABORT_IF_FALSE(!mInitialized, "Can't set size decode after Init()!");
     mSizeDecode = aSizeDecode;
   }
 
-  void SetSynchronous(bool aSynchronous)
-  {
-    mSynchronous = aSynchronous;
-  }
-
   bool IsSynchronous() const
   {
     return mSynchronous;
   }
 
   void SetObserver(imgDecoderObserver* aObserver)
   {
     MOZ_ASSERT(aObserver);
@@ -241,16 +236,27 @@ protected:
   uint32_t* mColormap;       // Current colormap to be used in Cairo format
   uint32_t mColormapSize;
 
   uint32_t mDecodeFlags;
   bool mDecodeDone;
   bool mDataError;
 
 private:
+  // Decode in synchronous mode. This is unsafe off-main-thread since it may
+  // attempt to allocate frames. To ensure that we never accidentally leave the
+  // decoder in synchronous mode, this should only be called by
+  // AutoSetSyncDecode.
+  void SetSynchronous(bool aSynchronous)
+  {
+    mSynchronous = aSynchronous;
+  }
+
+  friend class AutoSetSyncDecode;
+
   uint32_t mFrameCount; // Number of frames, including anything in-progress
 
   nsIntRect mInvalidRect; // Tracks an invalidation region in the current frame.
 
   nsresult mFailCode;
 
   struct NewFrameData
   {
@@ -280,12 +286,40 @@ private:
   bool mNeedsNewFrame;
   bool mInitialized;
   bool mSizeDecode;
   bool mInFrame;
   bool mIsAnimated;
   bool mSynchronous;
 };
 
+// A RAII helper class to automatically pair a call to SetSynchronous(true)
+// with a call to SetSynchronous(false), since failing to do so can lead us
+// to try to allocate frames off-main-thread, which is unsafe. Synchronous
+// decoding may only happen within the scope of an AutoSetSyncDecode. Nested
+// AutoSetSyncDecode's are OK.
+class AutoSetSyncDecode
+{
+public:
+  AutoSetSyncDecode(Decoder* aDecoder)
+    : mDecoder(aDecoder)
+  {
+    MOZ_ASSERT(NS_IsMainThread());
+    MOZ_ASSERT(mDecoder);
+
+    mOriginalValue = mDecoder->IsSynchronous();
+    mDecoder->SetSynchronous(true);
+  }
+
+  ~AutoSetSyncDecode()
+  {
+    mDecoder->SetSynchronous(mOriginalValue);
+  }
+
+private:
+  nsRefPtr<Decoder> mDecoder;
+  bool              mOriginalValue;
+};
+
 } // namespace image
 } // namespace mozilla
 
 #endif // MOZILLA_IMAGELIB_DECODER_H_
--- a/image/src/RasterImage.cpp
+++ b/image/src/RasterImage.cpp
@@ -1584,20 +1584,21 @@ RasterImage::AddSourceData(const char *a
       mFrameBlender.ClearFrames();
     }
   }
 
   // If we're not storing source data and we've previously gotten the size,
   // write the data directly to the decoder. (If we haven't gotten the size,
   // we'll queue up the data and write it out when we do.)
   if (!StoringSourceData() && mHasSize) {
-    mDecoder->SetSynchronous(true);
-    rv = WriteToDecoder(aBuffer, aCount);
-    mDecoder->SetSynchronous(false);
-    CONTAINER_ENSURE_SUCCESS(rv);
+    {
+      AutoSetSyncDecode syncDecode(mDecoder);
+      rv = WriteToDecoder(aBuffer, aCount);
+      CONTAINER_ENSURE_SUCCESS(rv);
+    }
 
     // We're not storing source data, so this data is probably coming straight
     // from the network. In this case, we want to display data as soon as we
     // get it, so we want to flush invalidations after every write.
     nsRefPtr<Decoder> kungFuDeathGrip = mDecoder;
     mInDecoder = true;
     mDecoder->FlushInvalidations();
     mInDecoder = false;
@@ -1956,17 +1957,17 @@ bool
 RasterImage::StoringSourceData() const {
   return (mDecodeOnDraw || mDiscardable);
 }
 
 
 // Sets up a decoder for this image. It is an error to call this function
 // when decoding is already in process (ie - when mDecoder is non-null).
 nsresult
-RasterImage::InitDecoder(bool aDoSizeDecode, bool aIsSynchronous /* = false */)
+RasterImage::InitDecoder(bool aDoSizeDecode)
 {
   // Ensure that the decoder is not already initialized
   NS_ABORT_IF_FALSE(!mDecoder, "Calling InitDecoder() while already decoding!");
 
   // We shouldn't be firing up a decoder if we already have the frames decoded
   NS_ABORT_IF_FALSE(!mDecoded, "Calling InitDecoder() but already decoded!");
 
   // Since we're not decoded, we should not have a discard timer active
@@ -2021,17 +2022,16 @@ RasterImage::InitDecoder(bool aDoSizeDec
   if (!mDecodeRequest) {
     mDecodeRequest = new DecodeRequest(this);
   }
   MOZ_ASSERT(mDecodeRequest->mStatusTracker);
   MOZ_ASSERT(mDecodeRequest->mStatusTracker->GetDecoderObserver());
   mDecoder->SetObserver(mDecodeRequest->mStatusTracker->GetDecoderObserver());
   mDecoder->SetSizeDecode(aDoSizeDecode);
   mDecoder->SetDecodeFlags(mFrameDecodeFlags);
-  mDecoder->SetSynchronous(aIsSynchronous);
   if (!aDoSizeDecode) {
     // We already have the size; tell the decoder so it can preallocate a
     // frame.  By default, we create an ARGB frame with no offset. If decoders
     // need a different type, they need to ask for it themselves.
     mDecoder->NeedNewFrame(0, 0, 0, mSize.width, mSize.height,
                            gfxImageFormatARGB32);
     mDecoder->AllocateFrame();
   }
@@ -2308,24 +2308,18 @@ RasterImage::RequestDecodeCore(RequestDe
   if (mHasSourceData && mBytesDecoded == mSourceData.Length())
     return NS_OK;
 
   // If we can do decoding now, do so.  Small images will decode completely,
   // large images will decode a bit and post themselves to the event loop
   // to finish decoding.
   if (!mDecoded && !mInDecoder && mHasSourceData && aDecodeType == SYNCHRONOUS_NOTIFY_AND_SOME_DECODE) {
     PROFILER_LABEL_PRINTF("RasterImage", "DecodeABitOf", "%s", GetURIString().get());
-    mDecoder->SetSynchronous(true);
-
+    AutoSetSyncDecode syncDecode(mDecoder);
     DecodePool::Singleton()->DecodeABitOf(this);
-
-    // DecodeABitOf can destroy mDecoder.
-    if (mDecoder) {
-      mDecoder->SetSynchronous(false);
-    }
     return NS_OK;
   }
 
   if (!mDecoded) {
     // If we get this far, dispatch the worker. We do this instead of starting
     // any immediate decoding to guarantee that all our decode notifications are
     // dispatched asynchronously, and to ensure we stay responsive.
     DecodePool::Singleton()->RequestDecode(this);
@@ -2393,42 +2387,42 @@ RasterImage::SyncDecode()
   // it now.
   if (mDecoder && mDecoder->NeedsNewFrame()) {
     mDecoder->AllocateFrame();
     mDecodeRequest->mAllocatedNewFrame = true;
   }
 
   // If we don't have a decoder, create one
   if (!mDecoder) {
-    rv = InitDecoder(/* aDoSizeDecode = */ false, /* aIsSynchronous = */ true);
+    rv = InitDecoder(/* aDoSizeDecode = */ false);
     CONTAINER_ENSURE_SUCCESS(rv);
-  } else {
-    mDecoder->SetSynchronous(true);
   }
 
-  // Write everything we have
-  rv = DecodeSomeData(mSourceData.Length() - mBytesDecoded);
-  CONTAINER_ENSURE_SUCCESS(rv);
+  {
+    AutoSetSyncDecode syncDecode(mDecoder);
+
+    // Write everything we have
+    rv = DecodeSomeData(mSourceData.Length() - mBytesDecoded);
+    CONTAINER_ENSURE_SUCCESS(rv);
+  }
 
   // When we're doing a sync decode, we want to get as much information from the
   // image as possible. We've send the decoder all of our data, so now's a good
   // time  to flush any invalidations (in case we don't have all the data and what
   // we got left us mid-frame).
   nsRefPtr<Decoder> kungFuDeathGrip = mDecoder;
   mInDecoder = true;
   mDecoder->FlushInvalidations();
   mInDecoder = false;
 
   rv = FinishedSomeDecoding();
   CONTAINER_ENSURE_SUCCESS(rv);
-
+  
+  // If our decoder's still open, there's still work to be done.
   if (mDecoder) {
-    mDecoder->SetSynchronous(false);
-
-    // If our decoder's still open, there's still work to be done.
     DecodePool::Singleton()->RequestDecode(this);
   }
 
   // All good if no errors!
   return mError ? NS_ERROR_FAILURE : NS_OK;
 }
 
 bool
--- a/image/src/RasterImage.h
+++ b/image/src/RasterImage.h
@@ -685,17 +685,17 @@ private: // data
   // Set when a decode worker detects an error off-main-thread. Once the error
   // is handled on the main thread, mError is set, but mPendingError is used to
   // stop decode work immediately.
   bool                       mPendingError:1;
 
   // Decoding
   nsresult WantDecodedFrames();
   nsresult SyncDecode();
-  nsresult InitDecoder(bool aDoSizeDecode, bool aIsSynchronous = false);
+  nsresult InitDecoder(bool aDoSizeDecode);
   nsresult WriteToDecoder(const char *aBuffer, uint32_t aCount);
   nsresult DecodeSomeData(uint32_t aMaxBytes);
   bool     IsDecodeFinished();
   TimeStamp mDrawStartTime;
 
   inline bool CanQualityScale(const gfxSize& scale);
   inline bool CanScale(GraphicsFilter aFilter, gfxSize aScale, uint32_t aFlags);