Bug 1089046 (Part 2) - Remove guards against recursive notifications. r=tn
authorSeth Fowler <seth@mozilla.com>
Fri, 14 Nov 2014 20:06:21 -0800
changeset 240200 02d3440cd34b569b21842a27350d45612297fe3d
parent 240199 b892bd18e0d923cafb676a2dc1dc33e992992a04
child 240201 9a3e7bca8051f7ad8af18b9ac8461bf76ea36caa
push id4311
push userraliiev@mozilla.com
push dateMon, 12 Jan 2015 19:37:41 +0000
treeherdermozilla-beta@150c9fed433b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstn
bugs1089046
milestone36.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 1089046 (Part 2) - Remove guards against recursive notifications. r=tn
image/src/RasterImage.cpp
image/src/RasterImage.h
--- a/image/src/RasterImage.cpp
+++ b/image/src/RasterImage.cpp
@@ -315,29 +315,26 @@ RasterImage::RasterImage(imgStatusTracke
   mLockCount(0),
   mDecodeCount(0),
   mRequestedSampleSize(0),
 #ifdef DEBUG
   mFramesNotified(0),
 #endif
   mDecodingMonitor("RasterImage Decoding Monitor"),
   mDecoder(nullptr),
-  mInDecoder(false),
   mStatusDiff(ImageStatusDiff::NoChange()),
   mNotifying(false),
   mHasSize(false),
   mDecodeOnDraw(false),
   mMultipart(false),
   mDiscardable(false),
   mHasSourceData(false),
   mDecoded(false),
   mHasBeenDecoded(false),
   mAnimationFinished(false),
-  mFinishing(false),
-  mInUpdateImageContainer(false),
   mWantFullDecode(false),
   mPendingError(false)
 {
   mStatusTrackerInit = new imgStatusTrackerInit(this, aStatusTracker);
 
   // Set up the discard tracker node.
   mDiscardTrackerNode.img = this;
   Telemetry::GetHistogramById(Telemetry::IMAGE_DECODE_COUNT)->Add(0);
@@ -773,20 +770,16 @@ RasterImage::CopyFrame(uint32_t aWhichFr
                        bool aShouldSyncNotify /* = true */)
 {
   if (aWhichFrame > FRAME_MAX_VALUE)
     return nullptr;
 
   if (mError)
     return nullptr;
 
-  // Disallowed in the API
-  if (mInDecoder && (aFlags & imgIContainer::FLAG_SYNC_DECODE))
-    return nullptr;
-
   if (!ApplyDecodeFlags(aFlags, aWhichFrame))
     return nullptr;
 
   // Get the frame. If it's not there, it's probably the caller's fault for
   // not waiting for the data to be loaded from the network or not passing
   // FLAG_SYNC_DECODE
   DrawableFrameRef frameRef = LookupFrame(GetRequestedFrameIndex(aWhichFrame),
                                           aFlags, aShouldSyncNotify);
@@ -854,20 +847,16 @@ RasterImage::GetFrameInternal(uint32_t a
   MOZ_ASSERT(aWhichFrame <= FRAME_MAX_VALUE);
 
   if (aWhichFrame > FRAME_MAX_VALUE)
     return nullptr;
 
   if (mError)
     return nullptr;
 
-  // Disallowed in the API
-  if (mInDecoder && (aFlags & imgIContainer::FLAG_SYNC_DECODE))
-    return nullptr;
-
   if (!ApplyDecodeFlags(aFlags, aWhichFrame))
     return nullptr;
 
   // Get the frame. If it's not there, it's probably the caller's fault for
   // not waiting for the data to be loaded from the network or not passing
   // FLAG_SYNC_DECODE
   DrawableFrameRef frameRef = LookupFrame(GetRequestedFrameIndex(aWhichFrame),
                                           aFlags, aShouldSyncNotify);
@@ -963,28 +952,26 @@ RasterImage::GetImageContainer(LayerMana
   }
 
   return NS_OK;
 }
 
 void
 RasterImage::UpdateImageContainer()
 {
-  if (!mImageContainer || IsInUpdateImageContainer()) {
+  if (!mImageContainer) {
     return;
   }
 
-  SetInUpdateImageContainer(true);
-
   nsRefPtr<layers::Image> image = GetCurrentImage();
   if (!image) {
     return;
   }
+
   mImageContainer->SetCurrentImage(image);
-  SetInUpdateImageContainer(false);
 }
 
 size_t
 RasterImage::HeapSizeOfSourceWithComputedFallback(MallocSizeOf aMallocSizeOf) const
 {
   // n == 0 is possible for two reasons.
   // - This is a zero-length image.
   // - We're on a platform where moz_malloc_size_of always returns 0.
@@ -1525,19 +1512,16 @@ RasterImage::AddSourceData(const char *a
   // We should not call this if we're not initialized
   NS_ABORT_IF_FALSE(mInitialized, "Calling AddSourceData() on uninitialized "
                                   "RasterImage!");
 
   // We should not call this if we're already finished adding source data
   NS_ABORT_IF_FALSE(!mHasSourceData, "Calling AddSourceData() after calling "
                                      "sourceDataComplete()!");
 
-  // This call should come straight from necko - no reentrancy allowed
-  NS_ABORT_IF_FALSE(!mInDecoder, "Re-entrant call to AddSourceData!");
-
   // Image is already decoded, we shouldn't be getting data, but it could
   // be extra garbage data at the end of a file.
   if (mDecoded) {
     return NS_OK;
   }
 
   // Starting a new part's frames, let's clean up before we add any
   // This needs to happen just before we start getting EnsureFrame() call(s),
@@ -2044,21 +2028,17 @@ RasterImage::ShutdownDecoder(eShutdownIn
   bool wasSizeDecode = mDecoder->IsSizeDecode();
 
   // Finalize the decoder
   // null out mDecoder, _then_ check for errors on the close (otherwise the
   // error routine might re-invoke ShutdownDecoder)
   nsRefPtr<Decoder> decoder = mDecoder;
   mDecoder = nullptr;
 
-  mFinishing = true;
-  mInDecoder = true;
   decoder->Finish(aIntent);
-  mInDecoder = false;
-  mFinishing = false;
 
   // Unlock the last frame (if we have any). Our invariant is that, while we
   // have a decoder open, the last frame is always locked.
   if (GetNumFrames() > 0) {
     nsRefPtr<imgFrame> curframe = mFrameBlender.RawGetFrame(GetNumFrames() - 1);
     curframe->UnlockImageData();
   }
 
@@ -2099,19 +2079,17 @@ RasterImage::WriteToDecoder(const char *
 {
   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, aStrategy);
-  mInDecoder = false;
 
   CONTAINER_ENSURE_SUCCESS(mDecoder->GetDecoderError());
 
   return NS_OK;
 }
 
 // This function is called in situations where it's clear that we want the
 // frames in decoded form (Draw, LookupFrame, etc).  If we're completely decoded,
@@ -2181,38 +2159,21 @@ RasterImage::RequestDecodeCore(RequestDe
 
   if (mError)
     return NS_ERROR_FAILURE;
 
   // If we're already decoded, there's nothing to do.
   if (mDecoded)
     return NS_OK;
 
-  // mFinishing protects against the case when we enter RequestDecode from
-  // ShutdownDecoder -- in that case, we're done with the decode, we're just
-  // not quite ready to admit it.  See bug 744309.
-  if (mFinishing)
-    return NS_OK;
-
   // If we're currently waiting for a new frame, we can't do anything until
   // that frame is allocated.
   if (mDecoder && mDecoder->NeedsNewFrame())
     return NS_OK;
 
-  // If our callstack goes through a size decoder, we have a problem.
-  // We need to shutdown the size decode and replace it with  a full
-  // decoder, but can't do that from within the decoder itself. Thus, we post
-  // an asynchronous event to the event loop to do it later. Since
-  // RequestDecode() is an asynchronous function this works fine (though it's
-  // a little slower).
-  if (mInDecoder) {
-    nsRefPtr<imgDecodeRequestor> requestor = new imgDecodeRequestor(*this);
-    return NS_DispatchToCurrentThread(requestor);
-  }
-
   // If we have a size decoder open, make sure we get the size
   if (mDecoder && mDecoder->IsSizeDecode()) {
     nsresult rv = DecodePool::Singleton()->DecodeUntilSizeAvailable(this);
     CONTAINER_ENSURE_SUCCESS(rv);
 
     // 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) {
@@ -2301,17 +2262,17 @@ RasterImage::RequestDecodeCore(RequestDe
   // If we've read all the data we have, we're done
   if (mHasSourceData && mDecoder->BytesDecoded() == 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) {
+  if (!mDecoded && mHasSourceData && aDecodeType == SYNCHRONOUS_NOTIFY_AND_SOME_DECODE) {
     PROFILER_LABEL_PRINTF("RasterImage", "DecodeABitOf",
       js::ProfileEntry::Category::GRAPHICS, "%s", GetURIString().get());
 
     DecodePool::Singleton()->DecodeABitOf(this, DECODE_SYNC);
     return NS_OK;
   }
 
   if (!mDecoded) {
@@ -2341,22 +2302,16 @@ RasterImage::SyncDecode()
     if (!mHasSize) {
       mWantFullDecode = true;
       return NS_ERROR_NOT_AVAILABLE;
     }
   }
 
   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) {
     // If the image is waiting for decode work to be notified, go ahead and do that.
     if (mDecodeRequest->mRequestStatus == DecodeRequest::REQUEST_WORK_DONE) {
       nsresult rv = FinishedSomeDecoding();
       CONTAINER_ENSURE_SUCCESS(rv);
     }
   }
 
@@ -2597,20 +2552,16 @@ RasterImage::Draw(gfxContext* aContext,
                   uint32_t aFlags)
 {
   if (aWhichFrame > FRAME_MAX_VALUE)
     return NS_ERROR_INVALID_ARG;
 
   if (mError)
     return NS_ERROR_FAILURE;
 
-  // Disallowed in the API
-  if (mInDecoder && (aFlags & imgIContainer::FLAG_SYNC_DECODE))
-    return NS_ERROR_FAILURE;
-
   // Illegal -- you can't draw with non-default decode flags.
   // (Disabling colorspace conversion might make sense to allow, but
   // we don't currently.)
   if ((aFlags & DECODE_FLAGS_MASK) != DECODE_FLAGS_DEFAULT)
     return NS_ERROR_FAILURE;
 
   NS_ENSURE_ARG_POINTER(aContext);
 
--- a/image/src/RasterImage.h
+++ b/image/src/RasterImage.h
@@ -553,18 +553,16 @@ private:
 
   nsresult DoImageDataComplete();
 
   bool ApplyDecodeFlags(uint32_t aNewFlags, uint32_t aWhichFrame);
 
   already_AddRefed<layers::Image> GetCurrentImage();
   void UpdateImageContainer();
 
-  void SetInUpdateImageContainer(bool aInUpdate) { mInUpdateImageContainer = aInUpdate; }
-  bool IsInUpdateImageContainer() { return mInUpdateImageContainer; }
   enum RequestDecodeType {
       ASYNCHRONOUS,
       SYNCHRONOUS_NOTIFY,
       SYNCHRONOUS_NOTIFY_AND_SOME_DECODE
   };
   NS_IMETHOD RequestDecodeCore(RequestDecodeType aDecodeType);
 
   // We would like to just check if we have a zero lock count, but we can't do
@@ -635,18 +633,16 @@ private: // data
   // BEGIN LOCKED MEMBER VARIABLES
   ReentrantMonitor           mDecodingMonitor;
 
   FallibleTArray<char>       mSourceData;
 
   // Decoder and friends
   nsRefPtr<Decoder>          mDecoder;
   nsRefPtr<DecodeRequest>    mDecodeRequest;
-
-  bool                       mInDecoder;
   // END LOCKED MEMBER VARIABLES
 
   // Notification state. Used to avoid recursive notifications.
   ImageStatusDiff            mStatusDiff;
   nsIntRect                  mInvalidRect;
   bool                       mNotifying:1;
 
   // Boolean flags (clustered together to conserve space):
@@ -660,21 +656,16 @@ private: // data
   bool                       mDecoded:1;
   bool                       mHasBeenDecoded:1;
 
 
   // Whether the animation can stop, due to running out
   // of frames, or no more owning request
   bool                       mAnimationFinished:1;
 
-  // Whether we're calling Decoder::Finish() from ShutdownDecoder.
-  bool                       mFinishing:1;
-
-  bool                       mInUpdateImageContainer:1;
-
   // Whether, once we are done doing a size decode, we should immediately kick
   // off a full decode.
   bool                       mWantFullDecode:1;
 
   // 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;
@@ -750,34 +741,12 @@ protected:
 
   friend class ImageFactory;
 };
 
 inline NS_IMETHODIMP RasterImage::GetAnimationMode(uint16_t *aAnimationMode) {
   return GetAnimationModeInternal(aAnimationMode);
 }
 
-// Asynchronous Decode Requestor
-//
-// We use this class when someone calls requestDecode() from within a decode
-// notification. Since requestDecode() involves modifying the decoder's state
-// (for example, possibly shutting down a header-only decode and starting a
-// full decode), we don't want to do this from inside a decoder.
-class imgDecodeRequestor : public nsRunnable
-{
-  public:
-    explicit imgDecodeRequestor(RasterImage &aContainer) {
-      mContainer = &aContainer;
-    }
-    NS_IMETHOD Run() {
-      if (mContainer)
-        mContainer->StartDecoding();
-      return NS_OK;
-    }
-
-  private:
-    WeakPtr<RasterImage> mContainer;
-};
-
 } // namespace image
 } // namespace mozilla
 
 #endif /* mozilla_imagelib_RasterImage_h_ */