Bug 1513603 - Remove SVG tearoff objects from the table when they are unlinked. r=longsonr

The only three types that can hit the PreservedWrapper() assertion
currently, and which this patch avoids, are DOMSVGPathSegList,
DOMSVGPointList, and DOMSVGStringList.

This is because these are the only types that are implemented as proxies
(due to having e.g. indexed getters on them, and thus have the offending
DOMProxyHandler::GetExpandoObject() call in their bindings) and which we
store in tearoff tables.

Differential Revision: https://phabricator.services.mozilla.com/D70314

--HG--
extra : moz-landing-system : lando

dom/svg/DOMSVGPathSegList.cpp

@@ -33,6 +33,7 @@ NS_IMPL_CYCLE_COLLECTION_CLASS(DOMSVGPathSegList)
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DOMSVGPathSegList)
   // No unlinking of mElement, we'd need to null out the value pointer (the
   // object it points to is held by the element) and null-check it everywhere.
+  tmp->RemoveFromTearoffTable();
   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(DOMSVGPathSegList)
@@ -95,7 +96,7 @@ DOMSVGPathSegList* DOMSVGPathSegList::GetDOMWrapperIfExists(void* aList) {
   return SVGPathSegListTearoffTable().GetTearoff(aList);
 }
 
-DOMSVGPathSegList::~DOMSVGPathSegList() {
+void DOMSVGPathSegList::RemoveFromTearoffTable() {
   // There are now no longer any references to us held by script or list items.
   // Note we must use GetAnimValKey/GetBaseValKey here, NOT InternalList()!
   void* key = mIsAnimValList ? InternalAList().GetAnimValKey()
@@ -103,6 +104,8 @@ DOMSVGPathSegList::~DOMSVGPathSegList() {
   SVGPathSegListTearoffTable().RemoveTearoff(key);
 }
 
+DOMSVGPathSegList::~DOMSVGPathSegList() { RemoveFromTearoffTable(); }
+
 JSObject* DOMSVGPathSegList::WrapObject(JSContext* cx,
                                         JS::Handle<JSObject*> aGivenProto) {
   return mozilla::dom::SVGPathSegList_Binding::Wrap(cx, this, aGivenProto);

dom/svg/DOMSVGPathSegList.h

@@ -200,6 +200,8 @@ class DOMSVGPathSegList final : public nsISupports, public nsWrapperCache {
 
   DOMSVGPathSeg*& ItemAt(uint32_t aIndex) { return mItems[aIndex].mItem; }
 
+  void RemoveFromTearoffTable();
+
   /**
    * This struct is used in our array of mItems to provide us with somewhere to
    * store the indexes into the internal SVGPathData of the internal seg data

dom/svg/DOMSVGPointList.cpp

@@ -50,6 +50,7 @@ NS_IMPL_CYCLE_COLLECTION_CLASS(DOMSVGPointList)
 NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DOMSVGPointList)
   // No unlinking of mElement, we'd need to null out the value pointer (the
   // object it points to is held by the element) and null-check it everywhere.
+  tmp->RemoveFromTearoffTable();
   NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
 NS_IMPL_CYCLE_COLLECTION_UNLINK_END
 NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(DOMSVGPointList)
@@ -112,7 +113,9 @@ DOMSVGPointList* DOMSVGPointList::GetDOMWrapperIfExists(void* aList) {
   return SVGPointListTearoffTable().GetTearoff(aList);
 }
 
-DOMSVGPointList::~DOMSVGPointList() {
+void DOMSVGPointList::RemoveFromTearoffTable() {
+  // Called from Unlink and the destructor.
+  //
   // There are now no longer any references to us held by script or list items.
   // Note we must use GetAnimValKey/GetBaseValKey here, NOT InternalList()!
   void* key = mIsAnimValList ? InternalAList().GetAnimValKey()
@@ -120,6 +123,8 @@ DOMSVGPointList::~DOMSVGPointList() {
   SVGPointListTearoffTable().RemoveTearoff(key);
 }
 
+DOMSVGPointList::~DOMSVGPointList() { RemoveFromTearoffTable(); }
+
 JSObject* DOMSVGPointList::WrapObject(JSContext* cx,
                                       JS::Handle<JSObject*> aGivenProto) {
   return mozilla::dom::SVGPointList_Binding::Wrap(cx, this, aGivenProto);

dom/svg/DOMSVGPointList.h

@@ -191,6 +191,8 @@ class DOMSVGPointList final : public nsISupports, public nsWrapperCache {
   void MaybeInsertNullInAnimValListAt(uint32_t aIndex);
   void MaybeRemoveItemFromAnimValListAt(uint32_t aIndex);
 
+  void RemoveFromTearoffTable();
+
   // Weak refs to our nsISVGPoint items. The items are friends and take care
   // of clearing our pointer to them when they die.
   FallibleTArray<nsISVGPoint*> mItems;

dom/svg/DOMSVGStringList.cpp

@@ -27,7 +27,20 @@ SVGStringListTearoffTable() {
   return sSVGStringListTearoffTable;
 }
 
-NS_SVG_VAL_IMPL_CYCLE_COLLECTION_WRAPPERCACHED(DOMSVGStringList, mElement)
+NS_IMPL_CYCLE_COLLECTION_CLASS(DOMSVGStringList)
+
+NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN(DOMSVGStringList)
+  // No unlinking of mElement, we'd need to null out the value pointer (the
+  // object it points to is held by the element) and null-check it everywhere.
+  tmp->RemoveFromTearoffTable();
+  NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER
+NS_IMPL_CYCLE_COLLECTION_UNLINK_END
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(DOMSVGStringList)
+  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mElement)
+NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
+NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(DOMSVGStringList)
+  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
+NS_IMPL_CYCLE_COLLECTION_TRACE_END
 
 NS_IMPL_CYCLE_COLLECTING_ADDREF(DOMSVGStringList)
 NS_IMPL_CYCLE_COLLECTING_RELEASE(DOMSVGStringList)
@@ -80,11 +93,13 @@ already_AddRefed<DOMSVGStringList> DOMSVGStringList::GetDOMWrapper(
   return wrapper.forget();
 }
 
-DOMSVGStringList::~DOMSVGStringList() {
+void DOMSVGStringList::RemoveFromTearoffTable() {
   // Script no longer has any references to us.
   SVGStringListTearoffTable().RemoveTearoff(&InternalList());
 }
 
+DOMSVGStringList::~DOMSVGStringList() { RemoveFromTearoffTable(); }
+
 /* virtual */
 JSObject* DOMSVGStringList::WrapObject(JSContext* aCx,
                                        JS::Handle<JSObject*> aGivenProto) {

dom/svg/DOMSVGStringList.h

@@ -100,6 +100,8 @@ class DOMSVGStringList final : public nsISupports, public nsWrapperCache {
 
   SVGStringList& InternalList() const;
 
+  void RemoveFromTearoffTable();
+
   // Strong ref to our element to keep it alive.
   RefPtr<dom::SVGElement> mElement;
 

dom/svg/crashtests/1513603.html

@@ -0,0 +1,20 @@
+<script>
+function go() {
+  b.addEventListener("DOMSubtreeModified", eh)
+  b.max = 0.44
+  b.value = 0.94
+}
+function eh() {
+  SpecialPowers.gc()
+  SpecialPowers.forceCC()
+  polygon.animatedPoints.z = 1;
+  path.animatedPathSegList.z = 1;
+  svg.requiredExtensions.z = 1;
+}
+</script>
+<body onload=go()>
+<svg id="svg">
+<polygon id="polygon"/>
+<path id="path"/>
+<dl>
+<meter id="b">

dom/svg/crashtests/crashtests.list

@@ -91,6 +91,7 @@ load 1477853.html
 load 1486488.html
 load 1493447.html
 skip-if(Android) load 1507961-1.html  # times out on Android due to the test size
+load 1513603.html
 load 1531578-1.html
 load test_nested_svg.html
 load 1555795.html