Bug 867183 - Don't recursively notify in FinishedSomeDecoding. r=jlebar
authorSeth Fowler <seth@mozilla.com>
Wed, 28 Aug 2013 15:39:05 -0700
changeset 152794 7a22d9a84b429f66c5284683232b28343626beb8
parent 152793 995407bc149a629a6bb39d845e335299a78146af
child 152795 25da4a357fb97aa544d9bc29ff9b3f822075226e
push idunknown
push userunknown
push dateunknown
reviewersjlebar
bugs867183
milestone26.0a1
Bug 867183 - Don't recursively notify in FinishedSomeDecoding. r=jlebar
image/src/RasterImage.cpp
image/src/RasterImage.h
--- a/image/src/RasterImage.cpp
+++ b/image/src/RasterImage.cpp
@@ -391,16 +391,18 @@ RasterImage::RasterImage(imgStatusTracke
   mDecodeCount(0),
 #ifdef DEBUG
   mFramesNotified(0),
 #endif
   mDecodingMutex("RasterImage"),
   mDecoder(nullptr),
   mBytesDecoded(0),
   mInDecoder(false),
+  mStatusDiff(ImageStatusDiff::NoChange()),
+  mNotifying(false),
   mHasSize(false),
   mDecodeOnDraw(false),
   mMultipart(false),
   mDiscardable(false),
   mHasSourceData(false),
   mDecoded(false),
   mHasBeenDecoded(false),
   mAnimationFinished(false),
@@ -2891,16 +2893,43 @@ RasterImage::GetFramesNotified(uint32_t 
 
   *aFramesNotified = mFramesNotified;
 
   return NS_OK;
 }
 #endif
 
 nsresult
+RasterImage::RequestDecodeIfNeeded(nsresult aStatus,
+                                   eShutdownIntent aIntent,
+                                   bool aDone,
+                                   bool aWasSize)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+
+  // If we were a size decode and a full decode was requested, now's the time.
+  if (NS_SUCCEEDED(aStatus) &&
+      aIntent != eShutdownIntent_Error &&
+      aDone &&
+      aWasSize &&
+      mWantFullDecode) {
+    mWantFullDecode = false;
+
+    // If we're not meant to be storing source data and we just got the size,
+    // we need to synchronously flush all the data we got to a full decoder.
+    // When that decoder is shut down, we'll also clear our source data.
+    return StoringSourceData() ? RequestDecode()
+                               : SyncDecode();
+  }
+
+  // We don't need a full decode right now, so just return the existing status.
+  return aStatus;
+}
+
+nsresult
 RasterImage::FinishedSomeDecoding(eShutdownIntent aIntent /* = eShutdownIntent_Done */,
                                   DecodeRequest* aRequest /* = nullptr */)
 {
   MOZ_ASSERT(NS_IsMainThread());
 
   mDecodingMutex.AssertCurrentThreadOwns();
 
   nsRefPtr<DecodeRequest> request;
@@ -2958,49 +2987,49 @@ RasterImage::FinishedSomeDecoding(eShutd
       // decoding routines have been finished.
       rv = image->ShutdownDecoder(aIntent);
       if (NS_FAILED(rv)) {
         image->DoError();
       }
     }
   }
 
-  ImageStatusDiff diff;
-  if (request) {
-    diff = image->mStatusTracker->Difference(request->mStatusTracker);
-    image->mStatusTracker->ApplyDifference(diff);
+  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.
+    mStatusDiff.Combine(diff);
   } else {
-    diff = image->mStatusTracker->DecodeStateAsDifference();
+    MOZ_ASSERT(mStatusDiff.IsNoChange(), "Shouldn't have an accumulated change at this point");
+
+    while (!diff.IsNoChange()) {
+      // Tell the observers what happened.
+      mNotifying = true;
+      image->mStatusTracker->SyncNotifyDifference(diff);
+      mNotifying = false;
+
+      // Gather any status changes that may have occurred as a result of sending
+      // out the previous notifications. If there were any, we'll send out
+      // notifications for them next.
+      diff = mStatusDiff;
+      mStatusDiff = ImageStatusDiff::NoChange();
+    }
   }
 
-  {
-    // Notifications can't go out with the decoding lock held.
-    MutexAutoUnlock unlock(mDecodingMutex);
-
-    // Then, tell the observers what has happened.
-    image->mStatusTracker->SyncNotifyDifference(diff);
-
-    // If we were a size decode and a full decode was requested, now's the time.
-    if (NS_SUCCEEDED(rv) && aIntent != eShutdownIntent_Error && done &&
-        wasSize && image->mWantFullDecode) {
-      image->mWantFullDecode = false;
-
-      // If we're not meant to be storing source data and we just got the size,
-      // we need to synchronously flush all the data we got to a full decoder.
-      // When that decoder is shut down, we'll also clear our source data.
-      if (!image->StoringSourceData()) {
-        rv = image->SyncDecode();
-      } else {
-        rv = image->RequestDecode();
-      }
-    }
-
-  }
-
-  return rv;
+  return RequestDecodeIfNeeded(rv, aIntent, done, wasSize);
 }
 
 NS_IMPL_ISUPPORTS1(RasterImage::DecodePool,
                               nsIObserver)
 
 /* static */ RasterImage::DecodePool*
 RasterImage::DecodePool::Singleton()
 {
--- a/image/src/RasterImage.h
+++ b/image/src/RasterImage.h
@@ -641,16 +641,20 @@ private: // data
   // Decoder and friends
   nsRefPtr<Decoder>          mDecoder;
   nsRefPtr<DecodeRequest>    mDecodeRequest;
   uint32_t                   mBytesDecoded;
 
   bool                       mInDecoder;
   // END LOCKED MEMBER VARIABLES
 
+  // Notification state. Used to avoid recursive notifications.
+  ImageStatusDiff            mStatusDiff;
+  bool                       mNotifying:1;
+
   // Boolean flags (clustered together to conserve space):
   bool                       mHasSize:1;       // Has SetSize() been called?
   bool                       mDecodeOnDraw:1;  // Decoding on draw?
   bool                       mMultipart:1;     // Multipart?
   bool                       mDiscardable:1;   // Is container discardable?
   bool                       mHasSourceData:1; // Do we have source data?
 
   // Do we have the frames in decoded form?
@@ -667,16 +671,20 @@ private: // data
 
   bool                       mInUpdateImageContainer:1;
 
   // Whether, once we are done doing a size decode, we should immediately kick
   // off a full decode.
   bool                       mWantFullDecode:1;
 
   // Decoding
+  nsresult RequestDecodeIfNeeded(nsresult aStatus,
+                                 eShutdownIntent aIntent,
+                                 bool aDone,
+                                 bool aWasSize);
   nsresult WantDecodedFrames();
   nsresult SyncDecode();
   nsresult InitDecoder(bool aDoSizeDecode, bool aIsSynchronous = false);
   nsresult WriteToDecoder(const char *aBuffer, uint32_t aCount);
   nsresult DecodeSomeData(uint32_t aMaxBytes);
   bool     IsDecodeFinished();
   TimeStamp mDrawStartTime;