Bug 1311921 - Store base and relative URIs explicitly in nsStyleImageRequests for comparison purposes, rather than use css::ImageValues. r=bholley
authorCameron McCormack <cam@mcc.id.au>
Fri, 28 Oct 2016 14:34:33 +0800
changeset 320072 288d92c34790593a91aadb83e0e578bdd8ec4b39
parent 320071 9f0e99ac3fca2edfe9ba5964b20cabff6a0c3705
child 320102 fb72036436f98ac04e6290c0c481e01c30257e5d
push id20749
push userryanvm@gmail.com
push dateSat, 29 Oct 2016 13:21:21 +0000
treeherderfx-team@1b170b39ed6b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbholley
bugs1311921
milestone52.0a1
Bug 1311921 - Store base and relative URIs explicitly in nsStyleImageRequests for comparison purposes, rather than use css::ImageValues. r=bholley MozReview-Commit-ID: 5aArKCI7Rhx
layout/style/nsStyleStruct.cpp
layout/style/nsStyleStruct.h
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@@ -1908,126 +1908,135 @@ nsStyleGradient::HasCalc()
   return mBgPosX.IsCalcUnit() || mBgPosY.IsCalcUnit() || mAngle.IsCalcUnit() ||
          mRadiusX.IsCalcUnit() || mRadiusY.IsCalcUnit();
 }
 
 
 // --------------------
 // nsStyleImageRequest
 
+static void
+MaybeUntrackAndUnlock(nsStyleImageRequest::Mode aModeFlags,
+                      imgRequestProxy* aRequestProxy,
+                      ImageTracker* aImageTracker)
+{
+  MOZ_ASSERT(NS_IsMainThread());
+  MOZ_ASSERT(aRequestProxy);
+  MOZ_ASSERT(aImageTracker);
+
+  if (aModeFlags & nsStyleImageRequest::Mode::Lock) {
+    aRequestProxy->UnlockImage();
+  }
+
+  if (aModeFlags & nsStyleImageRequest::Mode::Discard) {
+    aRequestProxy->RequestDiscard();
+  }
+
+  if (aModeFlags & nsStyleImageRequest::Mode::Track) {
+    aImageTracker->Remove(aRequestProxy);
+  }
+}
+
 /**
  * Runnable to release the nsStyleImageRequest's mRequestProxy,
- * mImageValue and mImageValue on the main thread, and to perform
+ * mImageValue and mImageTracker on the main thread, and to perform
  * any necessary unlocking and untracking of the image.
  */
 class StyleImageRequestCleanupTask : public mozilla::Runnable
 {
 public:
   typedef nsStyleImageRequest::Mode Mode;
 
   StyleImageRequestCleanupTask(Mode aModeFlags,
                                already_AddRefed<imgRequestProxy> aRequestProxy,
                                already_AddRefed<css::ImageValue> aImageValue,
                                already_AddRefed<ImageTracker> aImageTracker)
     : mModeFlags(aModeFlags)
     , mRequestProxy(aRequestProxy)
     , mImageValue(aImageValue)
     , mImageTracker(aImageTracker)
   {
+    MOZ_ASSERT(!!mRequestProxy == !!mImageTracker);
   }
 
   NS_IMETHOD Run() final
   {
-    MOZ_ASSERT(NS_IsMainThread());
-
-    if (!mRequestProxy) {
-      return NS_OK;
+    if (mRequestProxy) {
+      MaybeUntrackAndUnlock(mModeFlags, mRequestProxy, mImageTracker);
     }
-
-    MOZ_ASSERT(mImageTracker);
-
-    if (mModeFlags & Mode::Lock) {
-      mRequestProxy->UnlockImage();
-    }
-
-    if (mModeFlags & Mode::Discard) {
-      mRequestProxy->RequestDiscard();
-    }
-
-    if (mModeFlags & Mode::Track) {
-      mImageTracker->Remove(mRequestProxy);
-    }
-
     return NS_OK;
   }
 
 protected:
   virtual ~StyleImageRequestCleanupTask() { MOZ_ASSERT(NS_IsMainThread()); }
 
 private:
   Mode mModeFlags;
-  // Since we always dispatch this runnable to the main thread, these will be
-  // released on the main thread when the runnable itself is released.
+  // These will be released on the main thread when the
+  // StyleImageRequestCleanupTask is deleted.
   RefPtr<imgRequestProxy> mRequestProxy;
   RefPtr<css::ImageValue> mImageValue;
   RefPtr<ImageTracker> mImageTracker;
 };
 
 nsStyleImageRequest::nsStyleImageRequest(Mode aModeFlags,
                                          imgRequestProxy* aRequestProxy,
                                          css::ImageValue* aImageValue,
                                          ImageTracker* aImageTracker)
   : mRequestProxy(aRequestProxy)
-  , mImageValue(aImageValue)
   , mImageTracker(aImageTracker)
+  , mBaseURI(aImageValue->mBaseURI)
+  , mURIString(aImageValue->mString)
   , mModeFlags(aModeFlags)
   , mResolved(true)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(aRequestProxy);
-  MOZ_ASSERT(aImageValue);
   MOZ_ASSERT(aImageTracker);
 
   MaybeTrackAndLock();
 }
 
 nsStyleImageRequest::nsStyleImageRequest(
     Mode aModeFlags,
     nsStringBuffer* aURLBuffer,
     already_AddRefed<PtrHolder<nsIURI>> aBaseURI,
     already_AddRefed<PtrHolder<nsIURI>> aReferrer,
     already_AddRefed<PtrHolder<nsIPrincipal>> aPrincipal)
   : mModeFlags(aModeFlags)
   , mResolved(false)
 {
   mImageValue = new css::ImageValue(aURLBuffer, Move(aBaseURI),
                                     Move(aReferrer), Move(aPrincipal));
+  mBaseURI = mImageValue->mBaseURI;
+  mURIString = mImageValue->mString;
 }
 
 nsStyleImageRequest::~nsStyleImageRequest()
 {
-  // We may or may not be being destroyed on the main thread.  To clean
-  // up, we must untrack and unlock the image (depending on mModeFlags),
-  // and release mRequestProxy and mImageValue, all on the main thread.
-  {
-    RefPtr<StyleImageRequestCleanupTask> task =
-        new StyleImageRequestCleanupTask(mModeFlags,
-                                         mRequestProxy.forget(),
-                                         mImageValue.forget(),
-                                         mImageTracker.forget());
-    if (NS_IsMainThread()) {
-      task->Run();
-    } else {
+  if (NS_IsMainThread()) {
+    // In the main thread case, mRequestProxy, mImageValue and
+    // mImageTracker will be released as normal after the nsStyleImageRequest
+    // has run.
+    if (mRequestProxy) {
+      MaybeUntrackAndUnlock(mModeFlags, mRequestProxy, mImageTracker);
+    }
+  } else {
+    // In the OMT case, we transfer ownership of these objects to the
+    // cleanup task runnable, which will call MaybeUntrackAndUnlock and
+    // then release the objects.
+    if (mRequestProxy || mImageValue) {
+      RefPtr<StyleImageRequestCleanupTask> task =
+          new StyleImageRequestCleanupTask(mModeFlags,
+                                           mRequestProxy.forget(),
+                                           mImageValue.forget(),
+                                           mImageTracker.forget());
       NS_DispatchToMainThread(task.forget());
     }
   }
-
-  MOZ_ASSERT(!mRequestProxy);
-  MOZ_ASSERT(!mImageValue);
-  MOZ_ASSERT(!mImageTracker);
 }
 
 bool
 nsStyleImageRequest::Resolve(nsPresContext* aPresContext)
 {
   MOZ_ASSERT(NS_IsMainThread());
   MOZ_ASSERT(!IsResolved(), "already resolved");
 
@@ -2039,16 +2048,19 @@ nsStyleImageRequest::Resolve(nsPresConte
   // document.
   mImageValue->Initialize(aPresContext->Document());
 
   nsCSSValue value;
   value.SetImageValue(mImageValue);
   mRequestProxy = value.GetPossiblyStaticImageValue(aPresContext->Document(),
                                                     aPresContext);
 
+  // We no longer need the ImageValue.
+  mImageValue = nullptr;
+
   if (!mRequestProxy) {
     // The URL resolution or image load failed.
     return false;
   }
 
   mImageTracker = aPresContext->Document()->ImageTracker();
   MaybeTrackAndLock();
   return true;
@@ -2066,20 +2078,32 @@ nsStyleImageRequest::MaybeTrackAndLock()
     mImageTracker->Add(mRequestProxy);
   }
 
   if (mModeFlags & Mode::Lock) {
     mRequestProxy->LockImage();
   }
 }
 
+static const char16_t*
+GetBufferValue(nsStringBuffer* aBuffer)
+{
+  // Since the nsStringBuffers we work with come from a css::ImageValue, we
+  // can assume (just like nsCSSValue::GetBufferValue does) that we have
+  // a 16 bit, null terminated string.
+  return static_cast<char16_t*>(aBuffer->Data());
+}
+
 bool
 nsStyleImageRequest::DefinitelyEquals(const nsStyleImageRequest& aOther) const
 {
-  return DefinitelyEqualURIs(mImageValue, aOther.mImageValue);
+  return mBaseURI == aOther.mBaseURI &&
+         (mURIString == aOther.mURIString ||
+          NS_strcmp(GetBufferValue(mURIString),
+                    GetBufferValue(aOther.mURIString)) == 0);
 }
 
 // --------------------
 // CachedBorderImageData
 //
 void
 CachedBorderImageData::SetCachedSVGViewportSize(
   const mozilla::Maybe<nsSize>& aSVGViewportSize)
--- a/layout/style/nsStyleStruct.h
+++ b/layout/style/nsStyleStruct.h
@@ -300,22 +300,28 @@ private:
  *    any thread.  The nsStyleImageRequest is not considered "resolved"
  *    at this point, and the Resolve() method must be called later
  *    to initiate the image load and make calls to get() valid.
  *
  * Calls to TrackImage(), UntrackImage(), LockImage(), UnlockImage() and
  * RequestDiscard() are made to the imgRequestProxy and ImageTracker as
  * appropriate, according to the mode flags passed in to the constructor.
  *
- * The main thread constructor takes a pointer to the css::ImageValue that
- * is the specified url() value, while the off-main-thread constructor
- * creates a new css::ImageValue to represent the url() information passed
- * to the constructor.  This ImageValue is held on to for the comparisons done
- * in DefinitelyEquals(), so that we don't need to call into the non-OMT-safe
- * Equals() on the nsIURI objects returned from imgRequestProxy::GetURI().
+ * The main thread constructor takes a pointer to the already-created
+ * imgRequestProxy, and the css::ImageValue that was used while creating it.
+ * The ImageValue object is only used to grab the URL details to store
+ * into mBaseURI and mURIString.
+ *
+ * The off-main-thread constructor creates a new css::ImageValue to
+ * hold all the data required to resolve the imgRequestProxy later.  This
+ * constructor also stores the URL details into mbaseURI and mURIString.
+ * The ImageValue is held on to in mImageTracker until the Resolve call.
+ *
+ * We use mBaseURI and mURIString so that we can perform nsStyleImageRequest
+ * equality comparisons without needing an imgRequestProxy.
  */
 class nsStyleImageRequest
 {
 public:
   // Flags describing whether the imgRequestProxy must be tracked in the
   // ImageTracker, whether LockImage/UnlockImage calls will be made
   // when obtaining and releasing the imgRequestProxy, and whether
   // RequestDiscard will be called on release.
@@ -347,32 +353,39 @@ public:
     MOZ_ASSERT(IsResolved(), "Resolve() must be called first");
     MOZ_ASSERT(NS_IsMainThread());
     return mRequestProxy.get();
   }
   const imgRequestProxy* get() const {
     return const_cast<nsStyleImageRequest*>(this)->get();
   }
 
-  // Returns whether the ImageValue objects in the two nsStyleImageRequests
-  // return true from URLValueData::DefinitelyEqualURIs.
+  // Returns whether the mBaseURI and mURIString values in the two
+  // nsStyleImageRequests are equal.
   bool DefinitelyEquals(const nsStyleImageRequest& aOther) const;
 
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(nsStyleImageRequest);
 
 private:
   ~nsStyleImageRequest();
   nsStyleImageRequest& operator=(const nsStyleImageRequest& aOther) = delete;
 
   void MaybeTrackAndLock();
 
   RefPtr<imgRequestProxy> mRequestProxy;
   RefPtr<mozilla::css::ImageValue> mImageValue;
   RefPtr<mozilla::dom::ImageTracker> mImageTracker;
 
+  // Components that are (or were) used to produce the nsIURI for resolving
+  // the imgRequestProxy.  We use these in DefinitelyEquals, rather than
+  // comparing the URI information in the imgRequestProxy objects, since
+  // they may not yet be resolved (or might have failed to resolve).
+  mozilla::PtrHandle<nsIURI> mBaseURI;
+  RefPtr<nsStringBuffer> mURIString;
+
   Mode mModeFlags;
   bool mResolved;
 };
 
 MOZ_MAKE_ENUM_CLASS_BITWISE_OPERATORS(nsStyleImageRequest::Mode)
 
 enum nsStyleImageType {
   eStyleImageType_Null,