Bug 817256 - DOMSVGTransform should hold strong ref to matrix r=longsonr
authorDavid Zbarsky <dzbarsky@gmail.com>
Fri, 11 Jan 2013 02:15:06 -0500
changeset 118634 6aa0940b31993cd8af6791b5354a2420f9c0e54d
parent 118633 2bcb93c79fff7da0349ccbdd6deb0b6b99228c41
child 118635 c12c5e31d72fefca30d83ed5bd14d577fefa1b67
push idunknown
push userunknown
push dateunknown
reviewerslongsonr
bugs817256
milestone21.0a1
Bug 817256 - DOMSVGTransform should hold strong ref to matrix r=longsonr
content/svg/content/src/DOMSVGMatrix.cpp
content/svg/content/src/DOMSVGMatrix.h
content/svg/content/src/DOMSVGTransform.cpp
content/svg/content/src/DOMSVGTransform.h
--- a/content/svg/content/src/DOMSVGMatrix.cpp
+++ b/content/svg/content/src/DOMSVGMatrix.cpp
@@ -16,19 +16,16 @@ namespace mozilla {
 
 //----------------------------------------------------------------------
 // nsISupports methods:
 
 // Make sure we clear the weak ref in the owning transform (if there is one)
 // upon unlink.
 NS_IMPL_CYCLE_COLLECTION_CLASS(DOMSVGMatrix)
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DOMSVGMatrix)
-  if (tmp->mTransform) {
-    tmp->mTransform->ClearMatrixTearoff(tmp);
-  }
 NS_IMPL_CYCLE_COLLECTION_UNLINK(mTransform)
 NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(DOMSVGMatrix)
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mTransform)
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
--- a/content/svg/content/src/DOMSVGMatrix.h
+++ b/content/svg/content/src/DOMSVGMatrix.h
@@ -80,22 +80,16 @@ public:
   DOMSVGMatrix() {
     SetIsDOMBinding();
   }
 
   DOMSVGMatrix(const gfxMatrix &aMatrix) : mMatrix(aMatrix) {
     SetIsDOMBinding();
   }
 
-  ~DOMSVGMatrix() {
-    if (mTransform) {
-      mTransform->ClearMatrixTearoff(this);
-    }
-  }
-
   const gfxMatrix& Matrix() const {
     return mTransform ? mTransform->Matrixgfx() : mMatrix;
   }
 
   // WebIDL
   DOMSVGTransform* GetParentObject() const;
   virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope, bool* aTriedToWrap);
 
--- a/content/svg/content/src/DOMSVGTransform.cpp
+++ b/content/svg/content/src/DOMSVGTransform.cpp
@@ -6,20 +6,23 @@
 
 #include "DOMSVGTransform.h"
 #include "DOMSVGMatrix.h"
 #include "SVGAnimatedTransformList.h"
 #include "nsError.h"
 #include <math.h>
 #include "nsContentUtils.h"
 #include "nsAttrValueInlines.h"
+#include "nsSVGAttrTearoffTable.h"
 #include "mozilla/dom/SVGTransformBinding.h"
 
 namespace mozilla {
 
+static nsSVGAttrTearoffTable<DOMSVGTransform, DOMSVGMatrix> sSVGMatrixTearoffTable;
+
 //----------------------------------------------------------------------
 // nsISupports methods:
 
 // We could use NS_IMPL_CYCLE_COLLECTION_1, except that in Unlink() we need to
 // clear our list's weak ref to us to be safe. (The other option would be to
 // not unlink and rely on the breaking of the other edges in the cycle, as
 // NS_SVG_VAL_IMPL_CYCLE_COLLECTION does.)
 NS_IMPL_CYCLE_COLLECTION_CLASS(DOMSVGTransform)
@@ -62,73 +65,81 @@ DOMSVGTransform::WrapObject(JSContext* a
 
 DOMSVGTransform::DOMSVGTransform(DOMSVGTransformList *aList,
                                  uint32_t aListIndex,
                                  bool aIsAnimValItem)
   : mList(aList)
   , mListIndex(aListIndex)
   , mIsAnimValItem(aIsAnimValItem)
   , mTransform(nullptr)
-  , mMatrixTearoff(nullptr)
 {
   SetIsDOMBinding();
   // These shifts are in sync with the members in the header.
   NS_ABORT_IF_FALSE(aList &&
                     aListIndex <= MaxListIndex(), "bad arg");
 
   NS_ABORT_IF_FALSE(IndexIsValid(), "Bad index for DOMSVGNumber!");
 }
 
 DOMSVGTransform::DOMSVGTransform()
   : mList(nullptr)
   , mListIndex(0)
   , mIsAnimValItem(false)
   , mTransform(new SVGTransform()) // Default ctor for objects not in a list
                                    // initialises to matrix type with identity
                                    // matrix
-  , mMatrixTearoff(nullptr)
 {
   SetIsDOMBinding();
 }
 
 DOMSVGTransform::DOMSVGTransform(const gfxMatrix &aMatrix)
   : mList(nullptr)
   , mListIndex(0)
   , mIsAnimValItem(false)
   , mTransform(new SVGTransform(aMatrix))
-  , mMatrixTearoff(nullptr)
 {
   SetIsDOMBinding();
 }
 
 DOMSVGTransform::DOMSVGTransform(const SVGTransform &aTransform)
   : mList(nullptr)
   , mListIndex(0)
   , mIsAnimValItem(false)
   , mTransform(new SVGTransform(aTransform))
-  , mMatrixTearoff(nullptr)
 {
   SetIsDOMBinding();
 }
 
+DOMSVGTransform::~DOMSVGTransform()
+{
+  sSVGMatrixTearoffTable.RemoveTearoff(this);
+  // Our mList's weak ref to us must be nulled out when we die. If GC has
+  // unlinked us using the cycle collector code, then that has already
+  // happened, and mList is null.
+  if (mList) {
+    mList->mItems[mListIndex] = nullptr;
+  }
+}
 
 uint16_t
 DOMSVGTransform::Type() const
 {
   return Transform().Type();
 }
 
 already_AddRefed<DOMSVGMatrix>
 DOMSVGTransform::Matrix()
 {
-  if (!mMatrixTearoff) {
-    mMatrixTearoff = new DOMSVGMatrix(*this);
+  nsRefPtr<DOMSVGMatrix> wrapper =
+    sSVGMatrixTearoffTable.GetTearoff(this);
+  if (!wrapper) {
+    wrapper = new DOMSVGMatrix(*this);
+    sSVGMatrixTearoffTable.AddTearoff(this, wrapper);
   }
-  nsRefPtr<DOMSVGMatrix> matrix = mMatrixTearoff;
-  return matrix.forget();
+  return wrapper.forget();
 }
 
 float
 DOMSVGTransform::Angle() const
 {
   return Transform().Angle();
 }
 
@@ -313,25 +324,16 @@ DOMSVGTransform::SetMatrix(const gfxMatr
     return;
   }
 
   nsAttrValue emptyOrOldValue = NotifyElementWillChange();
   Transform().SetMatrix(aMatrix);
   NotifyElementDidChange(emptyOrOldValue);
 }
 
-void
-DOMSVGTransform::ClearMatrixTearoff(DOMSVGMatrix* aMatrix)
-{
-  NS_ABORT_IF_FALSE(mMatrixTearoff == aMatrix,
-      "Unexpected matrix pointer to be cleared");
-  mMatrixTearoff = nullptr;
-}
-
-
 //----------------------------------------------------------------------
 // Implementation helpers
 
 void
 DOMSVGTransform::NotifyElementDidChange(const nsAttrValue& aEmptyOrOldValue)
 {
   if (HasOwner()) {
     Element()->DidChangeTransformList(aEmptyOrOldValue);
--- a/content/svg/content/src/DOMSVGTransform.h
+++ b/content/svg/content/src/DOMSVGTransform.h
@@ -64,29 +64,17 @@ public:
   explicit DOMSVGTransform();
   explicit DOMSVGTransform(const gfxMatrix &aMatrix);
 
   /**
    * Ctor for creating an unowned copy. Used with Clone().
    */
   explicit DOMSVGTransform(const SVGTransform &aMatrix);
 
-  ~DOMSVGTransform() {
-    // Our matrix tear-off pointer should be cleared before we are destroyed
-    // (since matrix tear-offs keep an owning reference to their transform, and
-    // clear the tear-off pointer themselves if unlinked).
-    NS_ABORT_IF_FALSE(!mMatrixTearoff, "Matrix tear-off pointer not cleared."
-        " Transform being destroyed before matrix?");
-    // Our mList's weak ref to us must be nulled out when we die. If GC has
-    // unlinked us using the cycle collector code, then that has already
-    // happened, and mList is null.
-    if (mList) {
-      mList->mItems[mListIndex] = nullptr;
-    }
-  }
+  ~DOMSVGTransform();
 
   /**
    * Create an unowned copy of an owned transform. The caller is responsible for
    * the first AddRef().
    */
   DOMSVGTransform* Clone() {
     NS_ASSERTION(mList, "unexpected caller");
     return new DOMSVGTransform(InternalItem());
@@ -140,34 +128,33 @@ public:
   }
 
   // WebIDL
   DOMSVGTransformList* GetParentObject() const { return mList; }
   virtual JSObject* WrapObject(JSContext* aCx, JSObject* aScope, bool* aTriedToWrap);
   uint16_t Type() const;
   already_AddRefed<DOMSVGMatrix> Matrix();
   float Angle() const;
-  void SetMatrix(mozilla::DOMSVGMatrix& matrix, ErrorResult& rv);
+  void SetMatrix(DOMSVGMatrix& matrix, ErrorResult& rv);
   void SetTranslate(float tx, float ty, ErrorResult& rv);
   void SetScale(float sx, float sy, ErrorResult& rv);
   void SetRotate(float angle, float cx, float cy, ErrorResult& rv);
   void SetSkewX(float angle, ErrorResult& rv);
   void SetSkewY(float angle, ErrorResult& rv);
 
 protected:
   // Interface for DOMSVGMatrix's use
   friend class DOMSVGMatrix;
   const bool IsAnimVal() const {
     return mIsAnimValItem;
   }
   const gfxMatrix& Matrixgfx() const {
     return Transform().Matrix();
   }
   void SetMatrix(const gfxMatrix& aMatrix);
-  void ClearMatrixTearoff(DOMSVGMatrix* aMatrix);
 
 private:
   nsSVGElement* Element() {
     return mList->Element();
   }
 
   /**
    * Get a reference to the internal SVGTransform list item that this DOM
@@ -199,24 +186,16 @@ private:
 
   // Usually this class acts as a wrapper for an SVGTransform object which is
   // part of a list and is accessed by going via the owning Element.
   //
   // However, in some circumstances, objects of this class may not be associated
   // with any particular list and thus, no internal SVGTransform object. In
   // that case we allocate an SVGTransform object on the heap to store the data.
   nsAutoPtr<SVGTransform> mTransform;
-
-  // Weak ref to DOMSVGMatrix tearoff. The DOMSVGMatrix object will take of
-  // clearing this pointer when it is destroyed (by calling ClearMatrixTearoff).
-  //
-  // If this extra pointer member proves undesirable, it can be replaced with
-  // a hashmap (nsSVGAttrTearoffTable) to map from DOMSVGTransform to
-  // DOMSVGMatrix.
-  DOMSVGMatrix* mMatrixTearoff;
 };
 
 NS_DEFINE_STATIC_IID_ACCESSOR(DOMSVGTransform, MOZILLA_DOMSVGTRANSFORM_IID)
 
 nsAttrValue
 DOMSVGTransform::NotifyElementWillChange()
 {
   nsAttrValue result;