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 165764 b0433773e27c3fe5849f0aca1de17e6132b62336
parent 165763 0df2441272fb5cbf9878f38273d3b9f76cdc547d
child 165765 bada32435a8e6552ec728b28e95bb621e628bf1d
push idunknown
push userunknown
push dateunknown
reviewerstn
bugs940714
milestone28.0a1
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);