Bug 1321946. Adjust asserts about the progress of an image to deal with how multipart images are handled. r=aosmond
authorTimothy Nikkel <tnikkel@gmail.com>
Sat, 03 Dec 2016 16:07:10 -0600
changeset 325247 6520346b0a23bda448e25df6b50ab309e940cdbd
parent 325246 84818f50b4f0b9afad86824b14924b44dc7e96e3
child 325248 522ef8286421b04ade4f963542ac35aa51e72e6a
push id24
push usermaklebus@msu.edu
push dateTue, 20 Dec 2016 03:11:33 +0000
reviewersaosmond
bugs1321946
milestone53.0a1
Bug 1321946. Adjust asserts about the progress of an image to deal with how multipart images are handled. r=aosmond For multipart images we create a MultipartImage which contains each part. Each part in turn is a VectorImage or RasterImage. The MultipartImage and each part image all have their own ProgressTracker. The ProgressTracker for the MultipartImage observes the notifications of each part image via the IProgressObserver interface. This interfaces notably has no way to notify about an image error. So when a part image has an error it never gets propagated to the MultipartImage's ProgressTracker. This confuses our assertions about consistency of progress notifications. In this case we expect that when we get the load complete notification then we either have the size of the image or we encountered an error. So if the first part of a multipart image is broken and we are unable to get a size from it we will trigger this assertion. There are two ways to fix this. One would be to propagate errors to the MultipartImage's ProgressTracker. This would put the ProgressTracker for the MultipartImage permanently into error state and prevent showing the images from the remaining parts if one part image had an error. So in this patch I create a way to tell a ProgressTracker that is is for a multipart image, and use that to relax the assertions. As far as I can tell our code should be able to handle "ignoring" an error in a bad part image. Addtionaly there is a way that an error flag can get propagated to the MultipartImage's tracker: in MultipartImage::FinishTransition we get the progress directly from the part image and notify for it. This seems like an oversight as the comment at https://dxr.mozilla.org/mozilla-central/rev/bfa85d23df57c8a1db17c99b267667becc1c4afd/image/imgRequest.cpp#989 indicates that we don't want one bad part to prevent later parts from displaying. So we add the error flag to the ones we filter out when we propagate progress.
image/MultipartImage.cpp
image/ProgressTracker.cpp
image/ProgressTracker.h
image/imgRequest.cpp
--- a/image/MultipartImage.cpp
+++ b/image/MultipartImage.cpp
@@ -170,17 +170,19 @@ MultipartImage::BeginTransitionToPart(Im
   mNextPart->IncrementAnimationConsumers();
 }
 
 static Progress
 FilterProgress(Progress aProgress)
 {
   // Filter out onload blocking notifications, since we don't want to block
   // onload for multipart images.
-  return aProgress & ~(FLAG_ONLOAD_BLOCKED | FLAG_ONLOAD_UNBLOCKED);
+  // Filter out errors, since we don't want errors in one part to error out
+  // the whole stream.
+  return aProgress & ~(FLAG_ONLOAD_BLOCKED | FLAG_ONLOAD_UNBLOCKED | FLAG_HAS_ERROR);
 }
 
 void
 MultipartImage::FinishTransition()
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(mNextPart, "Should have a next part here");
 
--- a/image/ProgressTracker.cpp
+++ b/image/ProgressTracker.cpp
@@ -18,39 +18,44 @@
 #include "mozilla/Services.h"
 
 using mozilla::WeakPtr;
 
 namespace mozilla {
 namespace image {
 
 static void
-CheckProgressConsistency(Progress aOldProgress, Progress aNewProgress)
+CheckProgressConsistency(Progress aOldProgress, Progress aNewProgress, bool aIsMultipart)
 {
   // Check preconditions for every progress bit.
 
+  // Error's do not get propagated from the tracker for each image part to the
+  // tracker for the multipart image because we don't want one bad part to
+  // prevent the remaining parts from showing. So we need to consider whether
+  // this is a tracker for a multipart image for these assertions to work.
+
   if (aNewProgress & FLAG_SIZE_AVAILABLE) {
     // No preconditions.
   }
   if (aNewProgress & FLAG_DECODE_COMPLETE) {
     MOZ_ASSERT(aNewProgress & FLAG_SIZE_AVAILABLE);
-    MOZ_ASSERT(aNewProgress & (FLAG_FRAME_COMPLETE | FLAG_HAS_ERROR));
+    MOZ_ASSERT(aIsMultipart || aNewProgress & (FLAG_FRAME_COMPLETE | FLAG_HAS_ERROR));
   }
   if (aNewProgress & FLAG_FRAME_COMPLETE) {
     MOZ_ASSERT(aNewProgress & FLAG_SIZE_AVAILABLE);
   }
   if (aNewProgress & FLAG_LOAD_COMPLETE) {
-    MOZ_ASSERT(aNewProgress & (FLAG_SIZE_AVAILABLE | FLAG_HAS_ERROR));
+    MOZ_ASSERT(aIsMultipart || aNewProgress & (FLAG_SIZE_AVAILABLE | FLAG_HAS_ERROR));
   }
   if (aNewProgress & FLAG_ONLOAD_BLOCKED) {
     // No preconditions.
   }
   if (aNewProgress & FLAG_ONLOAD_UNBLOCKED) {
     MOZ_ASSERT(aNewProgress & FLAG_ONLOAD_BLOCKED);
-    MOZ_ASSERT(aNewProgress & (FLAG_SIZE_AVAILABLE | FLAG_HAS_ERROR));
+    MOZ_ASSERT(aIsMultipart || aNewProgress & (FLAG_SIZE_AVAILABLE | FLAG_HAS_ERROR));
   }
   if (aNewProgress & FLAG_IS_ANIMATED) {
     // No preconditions; like FLAG_HAS_TRANSPARENCY, we should normally never
     // discover this *after* FLAG_SIZE_AVAILABLE, but unfortunately some corrupt
     // GIFs may fool us.
   }
   if (aNewProgress & FLAG_HAS_TRANSPARENCY) {
     // XXX We'd like to assert that transparency is only set during metadata
@@ -370,17 +375,17 @@ ProgressTracker::SyncNotifyProgress(Prog
   MOZ_ASSERT(NS_IsMainThread(), "Use mObservers on main thread only");
 
   // Don't unblock onload if we're not blocked.
   Progress progress = Difference(aProgress);
   if (!((mProgress | progress) & FLAG_ONLOAD_BLOCKED)) {
     progress &= ~FLAG_ONLOAD_UNBLOCKED;
   }
 
-  CheckProgressConsistency(mProgress, mProgress | progress);
+  CheckProgressConsistency(mProgress, mProgress | progress, mIsMultipart);
 
   // XXX(seth): Hack to work around the fact that some observers have bugs and
   // need to get onload blocking notifications multiple times. We should fix
   // those observers and remove this.
   if ((aProgress & FLAG_DECODE_COMPLETE) &&
       (mProgress & FLAG_ONLOAD_BLOCKED) &&
       (mProgress & FLAG_ONLOAD_UNBLOCKED)) {
     progress |= FLAG_ONLOAD_BLOCKED | FLAG_ONLOAD_UNBLOCKED;
--- a/image/ProgressTracker.h
+++ b/image/ProgressTracker.h
@@ -113,16 +113,17 @@ public:
   MOZ_DECLARE_WEAKREFERENCE_TYPENAME(ProgressTracker)
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(ProgressTracker)
 
   ProgressTracker()
     : mImageMutex("ProgressTracker::mImage")
     , mImage(nullptr)
     , mObservers(new ObserverTable)
     , mProgress(NoProgress)
+    , mIsMultipart(false)
   { }
 
   bool HasImage() const { MutexAutoLock lock(mImageMutex); return mImage; }
   already_AddRefed<Image> GetImage() const
   {
     MutexAutoLock lock(mImageMutex);
     RefPtr<Image> image = mImage;
     return image.forget();
@@ -190,16 +191,19 @@ public:
   void AddObserver(IProgressObserver* aObserver);
   bool RemoveObserver(IProgressObserver* aObserver);
   uint32_t ObserverCount() const;
 
   // Resets our weak reference to our image. Image subclasses should call this
   // in their destructor.
   void ResetImage();
 
+  // Tell this progress tracker that it is for a multipart image.
+  void SetIsMultipart() { mIsMultipart = true; }
+
 private:
   friend class AsyncNotifyRunnable;
   friend class AsyncNotifyCurrentStateRunnable;
   friend class ImageFactory;
 
   ProgressTracker(const ProgressTracker& aOther) = delete;
 
   // Sets our weak reference to our image. Only ImageFactory should call this.
@@ -221,14 +225,17 @@ private:
   mutable Mutex mImageMutex;
   Image* mImage;
 
   // Hashtable of observers attached to the image. Each observer represents a
   // consumer using the image. Main thread only.
   CopyOnWrite<ObserverTable> mObservers;
 
   Progress mProgress;
+
+  // Whether this is a progress tracker for a multipart image.
+  bool mIsMultipart;
 };
 
 } // namespace image
 } // namespace mozilla
 
 #endif // mozilla_image_ProgressTracker_h
--- a/image/imgRequest.cpp
+++ b/image/imgRequest.cpp
@@ -958,16 +958,17 @@ PrepareForNewPart(nsIRequest* aRequest, 
     RefPtr<Image> partImage =
       ImageFactory::CreateImage(aRequest, progressTracker, result.mContentType,
                                 aURI, /* aIsMultipart = */ true,
                                 aInnerWindowId);
 
     if (result.mIsFirstPart) {
       // First part for a multipart channel. Create the MultipartImage wrapper.
       MOZ_ASSERT(aProgressTracker, "Shouldn't have given away tracker yet");
+      aProgressTracker->SetIsMultipart();
       result.mImage =
         ImageFactory::CreateMultipartImage(partImage, aProgressTracker);
     } else {
       // Transition to the new part.
       auto multipartImage = static_cast<MultipartImage*>(aExistingImage);
       multipartImage->BeginTransitionToPart(partImage);
 
       // Reset our cache entry size so it doesn't keep growing without bound.