Bug 1142849 - Upliftable fix for imgRequest TSan violations. r=tn, a=lmandel
authorSeth Fowler <seth@mozilla.com>
Fri, 13 Mar 2015 11:43:32 -0700
changeset 250453 9b7aa96d0e11
parent 250452 02b9c74353ad
child 250454 5bd29483f85e
push id4591
push userryanvm@gmail.com
push date2015-03-19 18:13 +0000
treeherdermozilla-beta@9b7aa96d0e11 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstn, lmandel
bugs1142849
milestone37.0
Bug 1142849 - Upliftable fix for imgRequest TSan violations. r=tn, a=lmandel
image/src/imgLoader.cpp
image/src/imgRequest.cpp
image/src/imgRequest.h
image/src/imgRequestProxy.cpp
--- a/image/src/imgLoader.cpp
+++ b/image/src/imgLoader.cpp
@@ -398,17 +398,17 @@ private:
                             req->HasConsumers());
     return PL_DHASH_NEXT;
   }
 
   static void RecordCounterForRequest(imgRequest* aRequest,
                                       nsTArray<ImageMemoryCounter>* aArray,
                                       bool aIsUsed)
   {
-    auto image = static_cast<Image*>(aRequest->mImage.get());
+    nsRefPtr<Image> image = aRequest->GetImage();
     if (!image) {
       return;
     }
 
     nsRefPtr<ImageURL> imageURL(image->GetURI());
     nsAutoCString spec;
     imageURL->GetSpec(spec);
 
@@ -428,17 +428,17 @@ private:
                                                     imgCacheEntry* aEntry,
                                                     void* aUserArg)
   {
     if (aEntry->HasNoProxies()) {
       return PL_DHASH_NEXT;
     }
 
     nsRefPtr<imgRequest> req = aEntry->GetRequest();
-    auto image = static_cast<Image*>(req->mImage.get());
+    nsRefPtr<Image> image = req->GetImage();
     if (!image) {
       return PL_DHASH_NEXT;
     }
 
     // Both this and EntryImageSizes measure images/content/raster/used/decoded
     // memory.  This function's measurement is secondary -- the result doesn't
     // go in the "explicit" tree -- so we use moz_malloc_size_of instead of
     // ImagesMallocSizeOf to prevent DMD from seeing it reported twice.
--- a/image/src/imgRequest.cpp
+++ b/image/src/imgRequest.cpp
@@ -61,16 +61,17 @@ NS_IMPL_ISUPPORTS(imgRequest,
                   nsIChannelEventSink,
                   nsIInterfaceRequestor,
                   nsIAsyncVerifyRedirectCallback)
 
 imgRequest::imgRequest(imgLoader* aLoader)
  : mLoader(aLoader)
  , mProgressTracker(new ProgressTracker())
  , mValidator(nullptr)
+ , mMutex("imgRequest")
  , mInnerWindowId(0)
  , mCORSMode(imgIRequest::CORS_NONE)
  , mReferrerPolicy(mozilla::net::RP_Default)
  , mImageErrorCode(NS_OK)
  , mDecodeRequested(false)
  , mIsMultiPartChannel(false)
  , mGotData(false)
  , mIsInCache(false)
@@ -136,32 +137,57 @@ nsresult imgRequest::Init(nsIURI *aURI,
 
   return NS_OK;
 }
 
 void imgRequest::ClearLoader() {
   mLoader = nullptr;
 }
 
+already_AddRefed<Image>
+imgRequest::GetImage()
+{
+  MutexAutoLock lock(mMutex);
+
+  nsRefPtr<Image> image = mImage;
+  return image.forget();
+}
+
 already_AddRefed<ProgressTracker>
 imgRequest::GetProgressTracker()
 {
+  MutexAutoLock lock(mMutex);
+
   if (mImage) {
     NS_ABORT_IF_FALSE(!mProgressTracker,
                       "Should have given mProgressTracker to mImage");
     return mImage->GetProgressTracker();
   } else {
     NS_ABORT_IF_FALSE(mProgressTracker,
                       "Should have mProgressTracker until we create mImage");
     nsRefPtr<ProgressTracker> progressTracker = mProgressTracker;
     MOZ_ASSERT(progressTracker);
     return progressTracker.forget();
   }
 }
 
+void
+imgRequest::SetImage(Image* aImage)
+{
+  MutexAutoLock lock(mMutex);
+  mImage = aImage;
+}
+
+void
+imgRequest::SetProgressTracker(ProgressTracker* aProgressTracker)
+{
+  MutexAutoLock lock(mMutex);
+  mProgressTracker = aProgressTracker;
+}
+
 void imgRequest::SetCacheEntry(imgCacheEntry *entry)
 {
   mCacheEntry = entry;
 }
 
 bool imgRequest::HasCacheEntry() const
 {
   return mCacheEntry != nullptr;
@@ -454,17 +480,18 @@ void imgRequest::SetIsInCache(bool incac
 {
   LOG_FUNC_WITH_PARAM(GetImgLog(), "imgRequest::SetIsCacheable", "incache", incache);
   mIsInCache = incache;
 }
 
 void imgRequest::UpdateCacheEntrySize()
 {
   if (mCacheEntry) {
-    size_t size = mImage->SizeOfSourceWithComputedFallback(moz_malloc_size_of);
+    nsRefPtr<Image> image = GetImage();
+    size_t size = image->SizeOfSourceWithComputedFallback(moz_malloc_size_of);
     mCacheEntry->SetDataSize(size);
   }
 }
 
 void imgRequest::SetCacheValidation(imgCacheEntry* aCacheEntry, nsIRequest* aRequest)
 {
   /* get the expires info */
   if (aCacheEntry) {
@@ -587,45 +614,49 @@ imgRequest::CacheChanged(nsIRequest* aNe
   // just in one of the loads what we also consider as a change
   // in a loading cache.
   return true;
 }
 
 nsresult
 imgRequest::LockImage()
 {
-  return mImage->LockImage();
+  nsRefPtr<Image> image = GetImage();
+  return image->LockImage();
 }
 
 nsresult
 imgRequest::UnlockImage()
 {
-  return mImage->UnlockImage();
+  nsRefPtr<Image> image = GetImage();
+  return image->UnlockImage();
 }
 
 nsresult
 imgRequest::RequestDecode()
 {
   // If we've initialized our image, we can request a decode.
-  if (mImage) {
-    return mImage->RequestDecode();
+  nsRefPtr<Image> image = GetImage();
+  if (image) {
+    return image->RequestDecode();
   }
 
   // Otherwise, flag to do it when we get the image
   mDecodeRequested = true;
 
   return NS_OK;
 }
 
 nsresult
 imgRequest::StartDecoding()
 {
   // If we've initialized our image, we can request a decode.
-  if (mImage) {
-    return mImage->StartDecoding();
+  nsRefPtr<Image> image = GetImage();
+  if (image) {
+    return image->StartDecoding();
   }
 
   // Otherwise, flag to do it when we get the image
   mDecodeRequested = true;
 
   return NS_OK;
 }
 
@@ -643,18 +674,22 @@ NS_IMETHODIMP imgRequest::OnStartRequest
   nsRefPtr<ProgressTracker> progressTracker = GetProgressTracker();
   if (mpchan) {
     mIsMultiPartChannel = true;
   } else {
     NS_ABORT_IF_FALSE(!mIsMultiPartChannel, "Something went wrong");
   }
 
   // If we're not multipart, we shouldn't have an image yet
-  NS_ABORT_IF_FALSE(mIsMultiPartChannel || !mImage,
-                    "Already have an image for non-multipart request");
+  nsRefPtr<Image> image = GetImage();
+  if (image && !mIsMultiPartChannel) {
+    MOZ_ASSERT_UNREACHABLE("Already have an image for a non-multipart request");
+    Cancel(NS_IMAGELIB_ERROR_FAILURE);
+    return NS_ERROR_FAILURE;
+  }
 
   /*
    * If mRequest is null here, then we need to set it so that we'll be able to
    * cancel it if our Cancel() method is called.  Note that this can only
    * happen for multipart channels.  We could simply not null out mRequest for
    * non-last parts, if GetIsLastPart() were reliable, but it's not.  See
    * https://bugzilla.mozilla.org/show_bug.cgi?id=339610
    */
@@ -736,55 +771,56 @@ NS_IMETHODIMP imgRequest::OnStopRequest(
   }
 
   bool lastPart = true;
   nsCOMPtr<nsIMultiPartChannel> mpchan(do_QueryInterface(aRequest));
   if (mpchan)
     mpchan->GetIsLastPart(&lastPart);
 
   bool isPartial = false;
-  if (mImage && (status == NS_ERROR_NET_PARTIAL_TRANSFER)) {
+  nsRefPtr<Image> image = GetImage();
+  if (image && (status == NS_ERROR_NET_PARTIAL_TRANSFER)) {
     isPartial = true;
     status = NS_OK; // fake happy face
   }
 
   // Tell the image that it has all of the source data. Note that this can
   // trigger a failure, since the image might be waiting for more non-optional
   // data and this is the point where we break the news that it's not coming.
-  if (mImage) {
-    nsresult rv = mImage->OnImageDataComplete(aRequest, ctxt, status, lastPart);
+  if (image) {
+    nsresult rv = image->OnImageDataComplete(aRequest, ctxt, status, lastPart);
 
     // If we got an error in the OnImageDataComplete() call, we don't want to
     // proceed as if nothing bad happened. However, we also want to give
     // precedence to failure status codes from necko, since presumably they're
     // more meaningful.
     if (NS_FAILED(rv) && NS_SUCCEEDED(status))
       status = rv;
   }
 
   // If the request went through, update the cache entry size. Otherwise,
   // cancel the request, which removes us from the cache.
-  if (mImage && NS_SUCCEEDED(status) && !isPartial) {
+  if (image && NS_SUCCEEDED(status) && !isPartial) {
     // We update the cache entry size here because this is where we finish
     // loading compressed source data, which is part of our size calculus.
     UpdateCacheEntrySize();
   }
   else if (isPartial) {
     // Remove the partial image from the cache.
     this->EvictFromCache();
   }
   else {
     mImageErrorCode = status;
 
     // if the error isn't "just" a partial transfer
     // stops animations, removes from cache
     this->Cancel(status);
   }
 
-  if (!mImage) {
+  if (!image) {
     // We have to fire the OnStopRequest notifications ourselves because there's
     // no image capable of doing so.
     Progress progress =
       LoadCompleteProgress(lastPart, /* aError = */ false, status);
 
     nsRefPtr<ProgressTracker> progressTracker = GetProgressTracker();
     progressTracker->SyncNotifyProgress(progress);
   }
@@ -820,16 +856,17 @@ imgRequest::OnDataAvailable(nsIRequest *
                             uint32_t count)
 {
   LOG_SCOPE_WITH_PARAM(GetImgLog(), "imgRequest::OnDataAvailable", "count", count);
 
   NS_ASSERTION(aRequest, "imgRequest::OnDataAvailable -- no request!");
 
   nsresult rv;
   mGotData = true;
+  nsRefPtr<Image> image = GetImage();
 
   if (mNewPartPending) {
     LOG_SCOPE(GetImgLog(), "imgRequest::OnDataAvailable |New part; finding MIME type|");
 
     mNewPartPending = false;
 
     mimetype_closure closure;
     nsAutoCString newType;
@@ -860,79 +897,89 @@ imgRequest::OnDataAvailable(nsIRequest *
         return NS_BINDING_ABORTED;
       }
 
       LOG_MSG(GetImgLog(), "imgRequest::OnDataAvailable", "Got content type from the channel");
     }
 
     mContentType = newType;
     SetProperties(chan);
-    bool firstPart = !mImage;
+    bool firstPart = !image;
 
     LOG_MSG_WITH_PARAM(GetImgLog(), "imgRequest::OnDataAvailable", "content type", mContentType.get());
 
     // XXX If server lied about mimetype and it's SVG, we may need to copy
     // the data and dispatch back to the main thread, AND tell the channel to
     // dispatch there in the future.
 
+    nsRefPtr<ProgressTracker> progressTracker;
+    {
+      MutexAutoLock lock(mMutex);
+      progressTracker = mProgressTracker;
+    }
+
     // Create the new image and give it ownership of our ProgressTracker.
     if (mIsMultiPartChannel) {
       // Create the ProgressTracker and image for this part.
-      nsRefPtr<ProgressTracker> progressTracker = new ProgressTracker();
-      nsRefPtr<Image> image =
-        ImageFactory::CreateImage(aRequest, progressTracker, mContentType,
+      nsRefPtr<ProgressTracker> partProgressTracker = new ProgressTracker();
+      nsRefPtr<Image> partImage =
+        ImageFactory::CreateImage(aRequest, partProgressTracker, mContentType,
                                   mURI, /* aIsMultipart = */ true,
                                   static_cast<uint32_t>(mInnerWindowId));
 
-      if (!mImage) {
+      if (!image) {
         // First part for a multipart channel. Create the MultipartImage wrapper.
-        MOZ_ASSERT(mProgressTracker, "Shouldn't have given away tracker yet");
-        mImage = new MultipartImage(image, mProgressTracker);
-        mProgressTracker = nullptr;
+        MOZ_ASSERT(progressTracker, "Shouldn't have given away tracker yet");
+        image = new MultipartImage(partImage, progressTracker);
+        SetImage(image);
+        progressTracker = nullptr;
+        SetProgressTracker(nullptr);
       } else {
         // Transition to the new part.
-        static_cast<MultipartImage*>(mImage.get())->BeginTransitionToPart(image);
+        static_cast<MultipartImage*>(image.get())->BeginTransitionToPart(partImage);
       }
     } else {
-      MOZ_ASSERT(!mImage, "New part for non-multipart channel?");
-      MOZ_ASSERT(mProgressTracker, "Shouldn't have given away tracker yet");
+      MOZ_ASSERT(!image, "New part for non-multipart channel?");
+      MOZ_ASSERT(progressTracker, "Shouldn't have given away tracker yet");
 
       // Create an image using our progress tracker.
-      mImage =
-        ImageFactory::CreateImage(aRequest, mProgressTracker, mContentType,
+      image =
+        ImageFactory::CreateImage(aRequest, progressTracker, mContentType,
                                   mURI, /* aIsMultipart = */ false,
                                   static_cast<uint32_t>(mInnerWindowId));
-      mProgressTracker = nullptr;
+      SetImage(image);
+      progressTracker = nullptr;
+      SetProgressTracker(nullptr);
     }
 
     if (firstPart) {
       // Notify listeners that we have an image.
       nsRefPtr<ProgressTracker> progressTracker = GetProgressTracker();
       progressTracker->OnImageAvailable();
       MOZ_ASSERT(progressTracker->HasImage());
     }
 
-    if (mImage->HasError() && !mIsMultiPartChannel) { // Probably bad mimetype
+    if (image->HasError() && !mIsMultiPartChannel) { // Probably bad mimetype
       // We allow multipart images to fail to initialize without cancelling the
       // load because subsequent images might be fine; thus only single part
       // images end up here.
       this->Cancel(NS_IMAGELIB_ERROR_FAILURE);
       return NS_BINDING_ABORTED;
     }
 
-    MOZ_ASSERT(!mProgressTracker, "Should've given tracker to image");
-    MOZ_ASSERT(mImage, "Should have image");
+    MOZ_ASSERT(!progressTracker, "Should've given tracker to image");
+    MOZ_ASSERT(image, "Should have image");
 
     if (mDecodeRequested) {
-      mImage->StartDecoding();
+      image->StartDecoding();
     }
   }
 
   // Notify the image that it has new data.
-  rv = mImage->OnImageDataAvailable(aRequest, ctxt, inStr, sourceOffset, count);
+  rv = image->OnImageDataAvailable(aRequest, ctxt, inStr, sourceOffset, count);
 
   if (NS_FAILED(rv)) {
     PR_LOG(GetImgLog(), PR_LOG_WARNING,
            ("[this=%p] imgRequest::OnDataAvailable -- "
             "copy to RasterImage failed\n", this));
     this->Cancel(NS_IMAGELIB_ERROR_FAILURE);
     return NS_BINDING_ABORTED;
   }
--- a/image/src/imgRequest.h
+++ b/image/src/imgRequest.h
@@ -14,16 +14,17 @@
 #include "nsIPrincipal.h"
 
 #include "nsAutoPtr.h"
 #include "nsCOMPtr.h"
 #include "nsProxyRelease.h"
 #include "nsStringGlue.h"
 #include "nsError.h"
 #include "nsIAsyncVerifyRedirectCallback.h"
+#include "mozilla/Mutex.h"
 #include "mozilla/net/ReferrerPolicy.h"
 
 class imgCacheValidator;
 class imgLoader;
 class imgRequestProxy;
 class imgCacheEntry;
 class imgMemoryReporter;
 class imgRequestNotifyRunnable;
@@ -125,16 +126,18 @@ public:
   // The principal for the document that loaded this image. Used when trying to
   // validate a CORS image load.
   already_AddRefed<nsIPrincipal> GetLoadingPrincipal() const
   {
     nsCOMPtr<nsIPrincipal> principal = mLoadingPrincipal;
     return principal.forget();
   }
 
+  already_AddRefed<Image> GetImage();
+
   // Return the ProgressTracker associated with this imgRequest. It may live
   // in |mProgressTracker| or in |mImage.mProgressTracker|, depending on whether
   // mImage has been instantiated yet.
   already_AddRefed<ProgressTracker> GetProgressTracker();
 
   // Get the current principal of the image. No AddRefing.
   inline nsIPrincipal* GetPrincipal() const { return mPrincipal.get(); }
 
@@ -151,16 +154,19 @@ private:
   friend class imgCacheEntry;
   friend class imgRequestProxy;
   friend class imgLoader;
   friend class imgCacheValidator;
   friend class imgCacheExpirationTracker;
   friend class imgRequestNotifyRunnable;
   friend class mozilla::image::ProgressTracker;
 
+  void SetImage(Image* aImage);
+  void SetProgressTracker(ProgressTracker* aProgressTracker);
+
   inline void SetLoadId(void *aLoadId) {
     mLoadId = aLoadId;
   }
   void Cancel(nsresult aStatus);
   void EvictFromCache();
   void RemoveFromCache();
 
   nsresult GetSecurityInfo(nsISupports **aSecurityInfo);
@@ -247,16 +253,18 @@ private:
   nsRefPtr<imgCacheEntry> mCacheEntry; /* we hold on to this to this so long as we have observers */
 
   void *mLoadId;
 
   imgCacheValidator *mValidator;
   nsCOMPtr<nsIAsyncVerifyRedirectCallback> mRedirectCallback;
   nsCOMPtr<nsIChannel> mNewRedirectChannel;
 
+  mozilla::Mutex mMutex;
+
   // The ID of the inner window origin, used for error reporting.
   uint64_t mInnerWindowId;
 
   // The CORS mode (defined in imgIRequest) this image was loaded with. By
   // default, imgIRequest::CORS_NONE.
   int32_t mCORSMode;
 
   // The Referrer Policy (defined in ReferrerPolicy.h) used for this image.
--- a/image/src/imgRequestProxy.cpp
+++ b/image/src/imgRequestProxy.cpp
@@ -491,18 +491,19 @@ NS_IMETHODIMP imgRequestProxy::GetImage(
   // It's possible that our owner has an image but hasn't notified us of it -
   // that'll happen if we get Canceled before the owner instantiates its image
   // (because Canceling unregisters us as a listener on mOwner). If we're
   // in that situation, just grab the image off of mOwner.
   nsRefPtr<Image> image = GetImage();
   nsCOMPtr<imgIContainer> imageToReturn;
   if (image)
     imageToReturn = do_QueryInterface(image);
-  if (!imageToReturn && GetOwner())
-    imageToReturn = GetOwner()->mImage;
+  if (!imageToReturn && GetOwner()) {
+    imageToReturn = GetOwner()->GetImage();
+  }
 
   if (!imageToReturn)
     return NS_ERROR_FAILURE;
 
   imageToReturn.swap(*aImage);
 
   return NS_OK;
 }