From ca716f70836f0eafbff6d6c4243ef72dd420da79 Mon Sep 17 00:00:00 2001
From: Lee Salzman <lsalzman@mozilla.com>
Date: Wed, 27 Apr 2022 18:48:59 +0000
Subject: [PATCH] Bug 1765911 - Support cloning Patterns with internal weak
 references. r=aosmond

Currently calling Pattern::Clone() produces a heap-allocated Pattern with RefPtrs
to any internal resources such as SourceSurfaces or GradientStops. While this is
okay if the Pattern is transient, this causes problems if the clone is long-lived
and causes via the RefPtr the stored resource to stay alive, even when there are
no other external references to the resource.

In the case of DrawTargetWebgl's PathCache, we need to be able to create Pattern
clones that have internal weak references, while also still supporting the original
use-case of strong references. To this end we template-ize some of the Pattern
derivatives so that we can make either strong or weak versions according to use-case.
This case way we store a weak clone of a Pattern in the PathCache key, the reference
will automatically invalidate itself when all other external strong references to it
go away, without having to do external crawling of the PathCache to constantly prune
the last reference manually.

Differential Revision: https://phabricator.services.mozilla.com/D144381
---
 dom/canvas/DrawTargetWebgl.cpp       |   2 +-
 dom/canvas/DrawTargetWebglInternal.h |   9 +-
 gfx/2d/2D.h                          | 259 ++++++++++++++++++---------
 mfbt/ThreadSafeWeakPtr.h             |   6 +
 4 files changed, 187 insertions(+), 89 deletions(-)

diff --git a/dom/canvas/DrawTargetWebgl.cpp b/dom/canvas/DrawTargetWebgl.cpp
index bc33f4a6e9d2..5d026db2bfb7 100644
--- a/dom/canvas/DrawTargetWebgl.cpp
+++ b/dom/canvas/DrawTargetWebgl.cpp
@@ -1807,7 +1807,7 @@ already_AddRefed<PathCacheEntry> PathCache::FindOrInsertEntry(
   }
   Pattern* pattern = nullptr;
   if (aPattern) {
-    pattern = aPattern->Clone();
+    pattern = aPattern->CloneWeak();
     if (!pattern) {
       return nullptr;
     }
diff --git a/dom/canvas/DrawTargetWebglInternal.h b/dom/canvas/DrawTargetWebglInternal.h
index 4d8272550e42..a7cb61e6c48f 100644
--- a/dom/canvas/DrawTargetWebglInternal.h
+++ b/dom/canvas/DrawTargetWebglInternal.h
@@ -72,6 +72,8 @@ class CacheEntry : public RefCounted<CacheEntry> {
   const IntRect& GetBounds() const { return mBounds; }
   HashNumber GetHash() const { return mHash; }
 
+  virtual bool IsValid() const { return true; }
+
  protected:
   virtual void RemoveFromList() = 0;
 
@@ -173,7 +175,9 @@ class TextureHandle : public RefCounted<TextureHandle>,
   void SetCacheEntry(const RefPtr<CacheEntry>& aEntry) { mCacheEntry = aEntry; }
 
   // Note as used if there is corresponding surface or cache entry.
-  bool IsUsed() const { return mSurface || mCacheEntry; }
+  bool IsUsed() const {
+    return mSurface || (mCacheEntry && mCacheEntry->IsValid());
+  }
 
  private:
   bool mValid = true;
@@ -360,6 +364,9 @@ class PathCacheEntry : public CacheEntryImpl<PathCacheEntry> {
 
   const Point& GetOrigin() const { return mOrigin; }
 
+  // Valid if either a mask (no pattern) or there is valid pattern.
+  bool IsValid() const override { return !mPattern || mPattern->IsValid(); }
+
  private:
   // The actual path geometry supplied
   SkPath mPath;
diff --git a/gfx/2d/2D.h b/gfx/2d/2D.h
index 0565c6971be0..b637294991e8 100644
--- a/gfx/2d/2D.h
+++ b/gfx/2d/2D.h
@@ -251,7 +251,7 @@ struct ShadowOptions {
  * matching DrawTarget. Not adhering to this condition will make a draw call
  * fail.
  */
-class GradientStops : public external::AtomicRefCounted<GradientStops> {
+class GradientStops : public SupportsThreadSafeWeakPtr<GradientStops> {
  public:
   MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(GradientStops)
   virtual ~GradientStops() = default;
@@ -275,8 +275,15 @@ class Pattern {
 
   virtual PatternType GetType() const = 0;
 
-  /** Instantiate a new clone with the same pattern type and values. */
-  virtual Pattern* Clone() const { return nullptr; }
+  /** Instantiate a new clone with the same pattern type and values. Any
+   * internal strong references will be converted to weak references. */
+  virtual Pattern* CloneWeak() const { return nullptr; }
+
+  /** Whether the pattern holds an internal weak reference. */
+  virtual bool IsWeak() const { return false; }
+
+  /** Whether any internal weak references still point to a target. */
+  virtual bool IsValid() const { return true; }
 
   /** Determine if the pattern type and values exactly match. */
   virtual bool operator==(const Pattern& aOther) const = 0;
@@ -285,6 +292,19 @@ class Pattern {
 
  protected:
   Pattern() = default;
+
+  // Utility functions to check if a weak reference is still valid.
+  template <typename T>
+  static inline bool IsRefValid(const RefPtr<T>& aPtr) {
+    // RefPtrs are always valid.
+    return true;
+  }
+
+  template <typename T>
+  static inline bool IsRefValid(const ThreadSafeWeakPtr<T>& aPtr) {
+    // Weak refs are only valid if they aren't dead.
+    return !aPtr.IsDead();
+  }
 };
 
 class ColorPattern : public Pattern {
@@ -295,7 +315,7 @@ class ColorPattern : public Pattern {
 
   PatternType GetType() const override { return PatternType::COLOR; }
 
-  Pattern* Clone() const override { return new ColorPattern(mColor); }
+  Pattern* CloneWeak() const override { return new ColorPattern(mColor); }
 
   bool operator==(const Pattern& aOther) const override {
     if (aOther.GetType() != PatternType::COLOR) {
@@ -313,150 +333,201 @@ class ColorPattern : public Pattern {
  * stored in a separate object and are backend dependent. This class itself
  * may be used on the stack.
  */
-class LinearGradientPattern : public Pattern {
+template <template <typename> typename REF = RefPtr>
+class LinearGradientPatternT : public Pattern {
+  typedef LinearGradientPatternT<ThreadSafeWeakPtr> Weak;
+
  public:
   /// For constructor parameter description, see member data documentation.
-  LinearGradientPattern(const Point& aBegin, const Point& aEnd,
-                        already_AddRefed<GradientStops> aStops,
-                        const Matrix& aMatrix = Matrix())
-      : mBegin(aBegin), mEnd(aEnd), mStops(aStops), mMatrix(aMatrix) {}
+  LinearGradientPatternT(const Point& aBegin, const Point& aEnd,
+                         RefPtr<GradientStops> aStops,
+                         const Matrix& aMatrix = Matrix())
+      : mBegin(aBegin),
+        mEnd(aEnd),
+        mStops(std::move(aStops)),
+        mMatrix(aMatrix) {}
 
   PatternType GetType() const override { return PatternType::LINEAR_GRADIENT; }
 
-  Pattern* Clone() const override {
-    return new LinearGradientPattern(mBegin, mEnd, do_AddRef(mStops), mMatrix);
+  Pattern* CloneWeak() const override {
+    return new Weak(mBegin, mEnd, do_AddRef(mStops), mMatrix);
+  }
+
+  bool IsWeak() const override {
+    return std::is_same<decltype(*this), Weak>::value;
+  }
+
+  bool IsValid() const override { return IsRefValid(mStops); }
+
+  template <template <typename> typename T>
+  bool operator==(const LinearGradientPatternT<T>& aOther) const {
+    return mBegin == aOther.mBegin && mEnd == aOther.mEnd &&
+           mStops == aOther.mStops && mMatrix.ExactlyEquals(aOther.mMatrix);
   }
 
   bool operator==(const Pattern& aOther) const override {
     if (aOther.GetType() != PatternType::LINEAR_GRADIENT) {
       return false;
     }
-    const LinearGradientPattern& other =
-        static_cast<const LinearGradientPattern&>(aOther);
-    return mBegin == other.mBegin && mEnd == other.mEnd &&
-           mStops == other.mStops && mMatrix.ExactlyEquals(other.mMatrix);
+    return aOther.IsWeak()
+               ? *this == static_cast<const Weak&>(aOther)
+               : *this == static_cast<const LinearGradientPatternT<>&>(aOther);
   }
 
-  Point mBegin;  //!< Start of the linear gradient
-  Point mEnd;    /**< End of the linear gradient - NOTE: In the case
-                      of a zero length gradient it will act as the
-                      color of the last stop. */
-  RefPtr<GradientStops>
-      mStops;     /**< GradientStops object for this gradient, this
-                       should match the backend type of the draw
-                       target this pattern will be used with. */
-  Matrix mMatrix; /**< A matrix that transforms the pattern into
-                       user space */
+  Point mBegin;              //!< Start of the linear gradient
+  Point mEnd;                /**< End of the linear gradient - NOTE: In the case
+                                  of a zero length gradient it will act as the
+                                  color of the last stop. */
+  REF<GradientStops> mStops; /**< GradientStops object for this gradient, this
+                                  should match the backend type of the draw
+                                  target this pattern will be used with. */
+  Matrix mMatrix;            /**< A matrix that transforms the pattern into
+                                  user space */
 };
 
+typedef LinearGradientPatternT<> LinearGradientPattern;
+
 /**
  * This class is used for Radial Gradient Patterns, the gradient stops are
  * stored in a separate object and are backend dependent. This class itself
  * may be used on the stack.
  */
-class RadialGradientPattern : public Pattern {
+template <template <typename> typename REF = RefPtr>
+class RadialGradientPatternT : public Pattern {
+  typedef RadialGradientPatternT<ThreadSafeWeakPtr> Weak;
+
  public:
   /// For constructor parameter description, see member data documentation.
-  RadialGradientPattern(const Point& aCenter1, const Point& aCenter2,
-                        Float aRadius1, Float aRadius2,
-                        already_AddRefed<GradientStops> aStops,
-                        const Matrix& aMatrix = Matrix())
+  RadialGradientPatternT(const Point& aCenter1, const Point& aCenter2,
+                         Float aRadius1, Float aRadius2,
+                         RefPtr<GradientStops> aStops,
+                         const Matrix& aMatrix = Matrix())
       : mCenter1(aCenter1),
         mCenter2(aCenter2),
         mRadius1(aRadius1),
         mRadius2(aRadius2),
-        mStops(aStops),
+        mStops(std::move(aStops)),
         mMatrix(aMatrix) {}
 
   PatternType GetType() const override { return PatternType::RADIAL_GRADIENT; }
 
-  Pattern* Clone() const override {
-    return new RadialGradientPattern(mCenter1, mCenter2, mRadius1, mRadius2,
-                                     do_AddRef(mStops), mMatrix);
+  Pattern* CloneWeak() const override {
+    return new Weak(mCenter1, mCenter2, mRadius1, mRadius2, do_AddRef(mStops),
+                    mMatrix);
+  }
+
+  bool IsWeak() const override {
+    return std::is_same<decltype(*this), Weak>::value;
+  }
+
+  bool IsValid() const override { return IsRefValid(mStops); }
+
+  template <template <typename> typename T>
+  bool operator==(const RadialGradientPatternT<T>& aOther) const {
+    return mCenter1 == aOther.mCenter1 && mCenter2 == aOther.mCenter2 &&
+           mRadius1 == aOther.mRadius1 && mRadius2 == aOther.mRadius2 &&
+           mStops == aOther.mStops && mMatrix.ExactlyEquals(aOther.mMatrix);
   }
 
   bool operator==(const Pattern& aOther) const override {
     if (aOther.GetType() != PatternType::RADIAL_GRADIENT) {
       return false;
     }
-    const RadialGradientPattern& other =
-        static_cast<const RadialGradientPattern&>(aOther);
-    return mCenter1 == other.mCenter1 && mCenter2 == other.mCenter2 &&
-           mRadius1 == other.mRadius1 && mRadius2 == other.mRadius2 &&
-           mStops == other.mStops && mMatrix.ExactlyEquals(other.mMatrix);
+    return aOther.IsWeak()
+               ? *this == static_cast<const Weak&>(aOther)
+               : *this == static_cast<const RadialGradientPatternT<>&>(aOther);
   }
 
-  Point mCenter1;  //!< Center of the inner (focal) circle.
-  Point mCenter2;  //!< Center of the outer circle.
-  Float mRadius1;  //!< Radius of the inner (focal) circle.
-  Float mRadius2;  //!< Radius of the outer circle.
-  RefPtr<GradientStops>
-      mStops;      /**< GradientStops object for this gradient, this
-                        should match the backend type of the draw target
-                        this pattern will be used with. */
+  Point mCenter1;            //!< Center of the inner (focal) circle.
+  Point mCenter2;            //!< Center of the outer circle.
+  Float mRadius1;            //!< Radius of the inner (focal) circle.
+  Float mRadius2;            //!< Radius of the outer circle.
+  REF<GradientStops> mStops; /**< GradientStops object for this gradient, this
+                                  should match the backend type of the draw
+                                target this pattern will be used with. */
   Matrix mMatrix;  //!< A matrix that transforms the pattern into user space
 };
 
+typedef RadialGradientPatternT<> RadialGradientPattern;
+
 /**
  * This class is used for Conic Gradient Patterns, the gradient stops are
  * stored in a separate object and are backend dependent. This class itself
  * may be used on the stack.
  */
-class ConicGradientPattern : public Pattern {
+template <template <typename> typename REF = RefPtr>
+class ConicGradientPatternT : public Pattern {
+  typedef ConicGradientPatternT<ThreadSafeWeakPtr> Weak;
+
  public:
   /// For constructor parameter description, see member data documentation.
-  ConicGradientPattern(const Point& aCenter, Float aAngle, Float aStartOffset,
-                       Float aEndOffset, already_AddRefed<GradientStops> aStops,
-                       const Matrix& aMatrix = Matrix())
+  ConicGradientPatternT(const Point& aCenter, Float aAngle, Float aStartOffset,
+                        Float aEndOffset, RefPtr<GradientStops> aStops,
+                        const Matrix& aMatrix = Matrix())
       : mCenter(aCenter),
         mAngle(aAngle),
         mStartOffset(aStartOffset),
         mEndOffset(aEndOffset),
-        mStops(aStops),
+        mStops(std::move(aStops)),
         mMatrix(aMatrix) {}
 
   PatternType GetType() const override { return PatternType::CONIC_GRADIENT; }
 
-  Pattern* Clone() const override {
-    return new ConicGradientPattern(mCenter, mAngle, mStartOffset, mEndOffset,
-                                    do_AddRef(mStops), mMatrix);
+  Pattern* CloneWeak() const override {
+    return new Weak(mCenter, mAngle, mStartOffset, mEndOffset,
+                    do_AddRef(mStops), mMatrix);
+  }
+
+  bool IsWeak() const override {
+    return std::is_same<decltype(*this), Weak>::value;
+  }
+
+  bool IsValid() const override { return IsRefValid(mStops); }
+
+  template <template <typename> typename T>
+  bool operator==(const ConicGradientPatternT<T>& aOther) const {
+    return mCenter == aOther.mCenter && mAngle == aOther.mAngle &&
+           mStartOffset == aOther.mStartOffset &&
+           mEndOffset == aOther.mEndOffset && mStops == aOther.mStops &&
+           mMatrix.ExactlyEquals(aOther.mMatrix);
   }
 
   bool operator==(const Pattern& aOther) const override {
     if (aOther.GetType() != PatternType::CONIC_GRADIENT) {
       return false;
     }
-    const ConicGradientPattern& other =
-        static_cast<const ConicGradientPattern&>(aOther);
-    return mCenter == other.mCenter && mAngle == other.mAngle &&
-           mStartOffset == other.mStartOffset &&
-           mEndOffset == other.mEndOffset && mStops == other.mStops &&
-           mMatrix.ExactlyEquals(other.mMatrix);
+    return aOther.IsWeak()
+               ? *this == static_cast<const Weak&>(aOther)
+               : *this == static_cast<const ConicGradientPatternT<>&>(aOther);
   }
 
-  Point mCenter;       //!< Center of the gradient
-  Float mAngle;        //!< Start angle of gradient
-  Float mStartOffset;  // Offset of first stop
-  Float mEndOffset;    // Offset of last stop
-  RefPtr<GradientStops>
-      mStops;      /**< GradientStops object for this gradient, this
-                        should match the backend type of the draw target
-                        this pattern will be used with. */
+  Point mCenter;             //!< Center of the gradient
+  Float mAngle;              //!< Start angle of gradient
+  Float mStartOffset;        // Offset of first stop
+  Float mEndOffset;          // Offset of last stop
+  REF<GradientStops> mStops; /**< GradientStops object for this gradient, this
+                                  should match the backend type of the draw
+                                target this pattern will be used with. */
   Matrix mMatrix;  //!< A matrix that transforms the pattern into user space
 };
 
+typedef ConicGradientPatternT<> ConicGradientPattern;
+
 /**
  * This class is used for Surface Patterns, they wrap a surface and a
  * repetition mode for the surface. This may be used on the stack.
  */
-class SurfacePattern : public Pattern {
+template <template <typename> typename REF = RefPtr>
+class SurfacePatternT : public Pattern {
+  typedef SurfacePatternT<ThreadSafeWeakPtr> Weak;
+
  public:
   /// For constructor parameter description, see member data documentation.
-  SurfacePattern(SourceSurface* aSourceSurface, ExtendMode aExtendMode,
-                 const Matrix& aMatrix = Matrix(),
-                 SamplingFilter aSamplingFilter = SamplingFilter::GOOD,
-                 const IntRect& aSamplingRect = IntRect())
-      : mSurface(aSourceSurface),
+  SurfacePatternT(RefPtr<SourceSurface> aSourceSurface, ExtendMode aExtendMode,
+                  const Matrix& aMatrix = Matrix(),
+                  SamplingFilter aSamplingFilter = SamplingFilter::GOOD,
+                  const IntRect& aSamplingRect = IntRect())
+      : mSurface(std::move(aSourceSurface)),
         mExtendMode(aExtendMode),
         mSamplingFilter(aSamplingFilter),
         mMatrix(aMatrix),
@@ -464,25 +535,37 @@ class SurfacePattern : public Pattern {
 
   PatternType GetType() const override { return PatternType::SURFACE; }
 
-  Pattern* Clone() const override {
-    return new SurfacePattern(mSurface, mExtendMode, mMatrix, mSamplingFilter,
-                              mSamplingRect);
+  Pattern* CloneWeak() const override {
+    return new Weak(do_AddRef(mSurface), mExtendMode, mMatrix, mSamplingFilter,
+                    mSamplingRect);
+  }
+
+  bool IsWeak() const override {
+    return std::is_same<decltype(*this), Weak>::value;
+  }
+
+  bool IsValid() const override { return IsRefValid(mSurface); }
+
+  template <template <typename> typename T>
+  bool operator==(const SurfacePatternT<T>& aOther) const {
+    return mSurface == aOther.mSurface && mExtendMode == aOther.mExtendMode &&
+           mSamplingFilter == aOther.mSamplingFilter &&
+           mMatrix.ExactlyEquals(aOther.mMatrix) &&
+           mSamplingRect.IsEqualEdges(aOther.mSamplingRect);
   }
 
   bool operator==(const Pattern& aOther) const override {
     if (aOther.GetType() != PatternType::SURFACE) {
       return false;
     }
-    const SurfacePattern& other = static_cast<const SurfacePattern&>(aOther);
-    return mSurface == other.mSurface && mExtendMode == other.mExtendMode &&
-           mSamplingFilter == other.mSamplingFilter &&
-           mMatrix.ExactlyEquals(other.mMatrix) &&
-           mSamplingRect.IsEqualEdges(other.mSamplingRect);
+    return aOther.IsWeak()
+               ? *this == static_cast<const Weak&>(aOther)
+               : *this == static_cast<const SurfacePatternT<>&>(aOther);
   }
 
-  RefPtr<SourceSurface> mSurface;  //!< Surface to use for drawing
-  ExtendMode mExtendMode; /**< This determines how the image is extended
-                               outside the bounds of the image */
+  REF<SourceSurface> mSurface;  //!< Surface to use for drawing
+  ExtendMode mExtendMode;       /**< This determines how the image is extended
+                                     outside the bounds of the image */
   SamplingFilter
       mSamplingFilter;  //!< Resampling filter for resampling the image.
   Matrix mMatrix;       //!< Transforms the pattern into user space
@@ -492,6 +575,8 @@ class SurfacePattern : public Pattern {
                               specified. */
 };
 
+typedef SurfacePatternT<> SurfacePattern;
+
 class StoredPattern;
 
 static const int32_t kReasonableSurfaceSize = 8192;
@@ -506,7 +591,7 @@ static const int32_t kReasonableSurfaceSize = 8192;
  * used on random threads now. This will be fixed in the future. Eventually
  * all SourceSurface should be thread-safe.
  */
-class SourceSurface : public external::AtomicRefCounted<SourceSurface> {
+class SourceSurface : public SupportsThreadSafeWeakPtr<SourceSurface> {
  public:
   MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(SourceSurface)
   virtual ~SourceSurface() = default;
diff --git a/mfbt/ThreadSafeWeakPtr.h b/mfbt/ThreadSafeWeakPtr.h
index 707955daa145..30481e227530 100644
--- a/mfbt/ThreadSafeWeakPtr.h
+++ b/mfbt/ThreadSafeWeakPtr.h
@@ -182,6 +182,7 @@ class SupportsThreadSafeWeakPtr : public detail::SupportsThreadSafeWeakPtrBase {
   void ref() { AddRef(); }
   void deref() { Release(); }
   MozRefCountType refCount() const { return mWeakRef->mStrongCnt; }
+  bool hasOneRef() const { return refCount() == 1; }
 
  private:
   template <typename U>
@@ -247,6 +248,11 @@ class ThreadSafeWeakPtr {
     return *this == aOther.get();
   }
 
+  friend bool operator==(const RefPtr<T>& aStrong,
+                         const ThreadSafeWeakPtr& aWeak) {
+    return aWeak == aStrong.get();
+  }
+
   bool operator==(const T* aOther) const {
     if (!mRef) {
       return !aOther;