Bug 1377457. Only apply locks to the image if SetHasImage has been called. r=aosmond
authorTimothy Nikkel <tnikkel@gmail.com>
Wed, 01 May 2019 23:05:47 +0000
changeset 531013 5acce9aaceb96cd5b8c0c9fde26286752e8cae29
parent 531012 34dd0113415f1aa2b4fd8e457006e28b047a185d
child 531014 bcb48454bcae0d274f5b238dfb254d64897b5344
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersaosmond
bugs1377457
milestone68.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 1377457. Only apply locks to the image if SetHasImage has been called. r=aosmond This was observed in an intermittent failure of image/test/mochitest/test_discardAnimatedImage.html. What happened was: 1) Document::MaybePreLoadImage was called for the images in the test. 2) imgRequest::OnDataAvailable is called on at least one of the images. This creates the RasterImage, so any proxy for this imgRequest will now return the image via GetImage(). imgRequest::OnDataAvailable also queues the FinishPreparingForNewPartRunnable back to the main thread to call OnImageAvailable on the progress tracker on the main thread. 3) We get the actual LoadImage calls for the images of the document. We create new proxies for the existing imgRequests. imgRequestProxy::Init calls mBehaviour->SetOwner(aOwner), which sets mOwnerHasImage to true because the progress tracker has an mImage (the one we created above). 4) We get a call to LockImage, this gets forwarded to the RasterImage because mOwnerHasImage is true and we can access the image. 5) The FinishPreparingForNewPartRunnable finally runs on the main thread. The OnImageAvailable notification from the progress tracker ends up in imgRequestProxy::SetHasImage. imgRequestProxy::SetHasImage applies our local count mLockCount to the RasterImage, even though we've already forwarded one of those LockImage calls to the image. LockImage calls are now unbalanced and the image will always remain locked. The fix is simple. Only apply the Lock/Unlock calls if the FinishPreparingForNewPartRunnable has hit the main thread (ie ignore an image we can access until this happens). Differential Revision: https://phabricator.services.mozilla.com/D29326
image/imgRequest.cpp
image/imgRequest.h
image/imgRequestProxy.cpp
--- a/image/imgRequest.cpp
+++ b/image/imgRequest.cpp
@@ -56,16 +56,17 @@ imgRequest::imgRequest(imgLoader* aLoade
       mCacheKey(aCacheKey),
       mLoadId(nullptr),
       mFirstProxy(nullptr),
       mValidator(nullptr),
       mInnerWindowId(0),
       mCORSMode(imgIRequest::CORS_NONE),
       mReferrerPolicy(mozilla::net::RP_Unset),
       mImageErrorCode(NS_OK),
+      mImageAvailable(false),
       mMutex("imgRequest"),
       mProgressTracker(new ProgressTracker()),
       mIsMultiPartChannel(false),
       mIsInCache(false),
       mDecodeRequested(false),
       mNewPartPending(false),
       mHadInsecureRedirect(false) {
   LOG_FUNC(gImgLog, "imgRequest::imgRequest()");
@@ -973,30 +974,33 @@ void imgRequest::FinishPreparingForNewPa
   MOZ_ASSERT(NS_IsMainThread());
 
   mContentType = aResult.mContentType;
 
   SetProperties(aResult.mContentType, aResult.mContentDisposition);
 
   if (aResult.mIsFirstPart) {
     // Notify listeners that we have an image.
+    mImageAvailable = true;
     RefPtr<ProgressTracker> progressTracker = GetProgressTracker();
     progressTracker->OnImageAvailable();
     MOZ_ASSERT(progressTracker->HasImage());
   }
 
   if (aResult.mShouldResetCacheEntry) {
     ResetCacheEntry();
   }
 
   if (IsDecodeRequested()) {
     aResult.mImage->StartDecoding(imgIContainer::FLAG_NONE);
   }
 }
 
+bool imgRequest::ImageAvailable() const { return mImageAvailable; }
+
 NS_IMETHODIMP
 imgRequest::OnDataAvailable(nsIRequest* aRequest, nsIInputStream* aInStr,
                             uint64_t aOffset, uint32_t aCount) {
   LOG_SCOPE_WITH_PARAM(gImgLog, "imgRequest::OnDataAvailable", "count", aCount);
 
   NS_ASSERTION(aRequest, "imgRequest::OnDataAvailable -- no request!");
 
   RefPtr<Image> image;
--- a/image/imgRequest.h
+++ b/image/imgRequest.h
@@ -192,16 +192,18 @@ class imgRequest final : public nsIStrea
   // Sets properties for this image; will dispatch to main thread if needed.
   void SetProperties(const nsACString& aContentType,
                      const nsACString& aContentDisposition);
 
   nsIProperties* Properties() const { return mProperties; }
 
   bool HasConsumers() const;
 
+  bool ImageAvailable() const;
+
  private:
   friend class FinishPreparingForNewPartRunnable;
 
   virtual ~imgRequest();
 
   void FinishPreparingForNewPart(const NewPartResult& aResult);
 
   void Cancel(nsresult aStatus);
@@ -265,16 +267,19 @@ class imgRequest final : public nsIStrea
   // The Referrer Policy (defined in ReferrerPolicy.h) used for this image.
   ReferrerPolicy mReferrerPolicy;
 
   nsresult mImageErrorCode;
 
   // The categories of prioritization strategy that have been requested.
   uint32_t mBoostCategoriesRequested = 0;
 
+  // If we've called OnImageAvailable.
+  bool mImageAvailable;
+
   mutable mozilla::Mutex mMutex;
 
   // Member variables protected by mMutex. Note that *all* flags in our bitfield
   // are protected by mMutex; if you're adding a new flag that isn'protected, it
   // must not be a part of this bitfield.
   RefPtr<ProgressTracker> mProgressTracker;
   RefPtr<Image> mImage;
   bool mIsMultiPartChannel : 1;
--- a/image/imgRequestProxy.cpp
+++ b/image/imgRequestProxy.cpp
@@ -559,29 +559,31 @@ bool imgRequestProxy::RequestDecodeWithR
   }
 
   return false;
 }
 
 NS_IMETHODIMP
 imgRequestProxy::LockImage() {
   mLockCount++;
-  RefPtr<Image> image = GetImage();
+  RefPtr<Image> image =
+      GetOwner() && GetOwner()->ImageAvailable() ? GetImage() : nullptr;
   if (image) {
     return image->LockImage();
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 imgRequestProxy::UnlockImage() {
   MOZ_ASSERT(mLockCount > 0, "calling unlock but no locks!");
 
   mLockCount--;
-  RefPtr<Image> image = GetImage();
+  RefPtr<Image> image =
+      GetOwner() && GetOwner()->ImageAvailable() ? GetImage() : nullptr;
   if (image) {
     return image->UnlockImage();
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 imgRequestProxy::RequestDiscard() {
@@ -590,34 +592,36 @@ imgRequestProxy::RequestDiscard() {
     return image->RequestDiscard();
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 imgRequestProxy::IncrementAnimationConsumers() {
   mAnimationConsumers++;
-  RefPtr<Image> image = GetImage();
+  RefPtr<Image> image =
+      GetOwner() && GetOwner()->ImageAvailable() ? GetImage() : nullptr;
   if (image) {
     image->IncrementAnimationConsumers();
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 imgRequestProxy::DecrementAnimationConsumers() {
   // We may get here if some responsible code called Increment,
   // then called us, but we have meanwhile called ClearAnimationConsumers
   // because we needed to get rid of them earlier (see
   // imgRequest::RemoveProxy), and hence have nothing left to
   // decrement. (In such a case we got rid of the animation consumers
   // early, but not the observer.)
   if (mAnimationConsumers > 0) {
     mAnimationConsumers--;
-    RefPtr<Image> image = GetImage();
+    RefPtr<Image> image =
+        GetOwner() && GetOwner()->ImageAvailable() ? GetImage() : nullptr;
     if (image) {
       image->DecrementAnimationConsumers();
     }
   }
   return NS_OK;
 }
 
 void imgRequestProxy::ClearAnimationConsumers() {