diff --git a/image/src/Decoder.h b/image/src/Decoder.h index 15b937de47e1..0387a98954b9 100644 --- a/image/src/Decoder.h +++ b/image/src/Decoder.h @@ -94,11 +94,6 @@ public: mSizeDecode = aSizeDecode; } - void SetSynchronous(bool aSynchronous) - { - mSynchronous = aSynchronous; - } - bool IsSynchronous() const { return mSynchronous; @@ -246,6 +241,17 @@ protected: 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. @@ -285,6 +291,34 @@ private: 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 mDecoder; + bool mOriginalValue; +}; + } // namespace image } // namespace mozilla diff --git a/image/src/RasterImage.cpp b/image/src/RasterImage.cpp index 6f0db725dbdb..389e0716750a 100644 --- a/image/src/RasterImage.cpp +++ b/image/src/RasterImage.cpp @@ -1589,10 +1589,11 @@ RasterImage::AddSourceData(const char *aBuffer, uint32_t aCount) // 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 @@ -1961,7 +1962,7 @@ RasterImage::StoringSourceData() const { // 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!"); @@ -2026,7 +2027,6 @@ RasterImage::InitDecoder(bool aDoSizeDecode, bool aIsSynchronous /* = false */) 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 @@ -2313,14 +2313,8 @@ RasterImage::RequestDecodeCore(RequestDecodeType aDecodeType) // 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; } @@ -2398,15 +2392,17 @@ RasterImage::SyncDecode() // 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 @@ -2419,11 +2415,9 @@ RasterImage::SyncDecode() 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); } diff --git a/image/src/RasterImage.h b/image/src/RasterImage.h index 13d116fbfab6..785fec2db492 100644 --- a/image/src/RasterImage.h +++ b/image/src/RasterImage.h @@ -690,7 +690,7 @@ private: // data // 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();