Bug 880544 - Fix leak in DOMSVGPathSegList. r=dzbarksy

content/svg/content/src/DOMSVGPathSegList.cpp

@@ -287,7 +287,18 @@ DOMSVGPathSegList::Initialize(DOMSVGPathSeg& aNewItem, ErrorResult& aError)
   return InsertItemBefore(*domItem, 0, aError);
 }
 
-DOMSVGPathSeg*
+already_AddRefed<DOMSVGPathSeg>
+DOMSVGPathSegList::GetItem(uint32_t index, ErrorResult& error)
+{
+  bool found;
+  nsRefPtr<DOMSVGPathSeg> item = IndexedGetter(index, found, error);
+  if (!found) {
+    error.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
+  }
+  return item.forget();
+}
+
+already_AddRefed<DOMSVGPathSeg>
 DOMSVGPathSegList::IndexedGetter(uint32_t aIndex, bool& aFound,
                                  ErrorResult& aError)
 {
@@ -296,8 +307,7 @@ DOMSVGPathSegList::IndexedGetter(uint32_t aIndex, bool& aFound,
   }
   aFound = aIndex < LengthNoFlush();
   if (aFound) {
-    EnsureItemAt(aIndex);
-    return ItemAt(aIndex);
+    return GetItemAt(aIndex);
   }
   return nullptr;
 }
@@ -443,14 +453,13 @@ DOMSVGPathSegList::RemoveItem(uint32_t aIndex,
     aError.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
     return nullptr;
   }
-  // We have to return the removed item, so make sure it exists:
-  EnsureItemAt(aIndex);
+  // We have to return the removed item, so get it, creating it if necessary:
+  nsRefPtr<DOMSVGPathSeg> result = GetItemAt(aIndex);
 
   nsAttrValue emptyOrOldValue = Element()->WillChangePathSegList();
   // Notify the DOM item of removal *before* modifying the lists so that the
   // DOM item can copy its *old* value:
   ItemAt(aIndex)->RemovingFromList();
-  nsRefPtr<DOMSVGPathSeg> result = ItemAt(aIndex);
 
   uint32_t internalIndex = mItems[aIndex].mInternalDataIndex;
   uint32_t segType = SVGPathSegUtils::DecodeType(InternalList().mData[internalIndex]);
@@ -476,12 +485,16 @@ DOMSVGPathSegList::RemoveItem(uint32_t aIndex,
   return result.forget();
 }
 
-void
-DOMSVGPathSegList::EnsureItemAt(uint32_t aIndex)
+already_AddRefed<DOMSVGPathSeg>
+DOMSVGPathSegList::GetItemAt(uint32_t aIndex)
 {
+  MOZ_ASSERT(aIndex < mItems.Length());
+
   if (!ItemAt(aIndex)) {
     ItemAt(aIndex) = DOMSVGPathSeg::CreateFor(this, aIndex, IsAnimValList());
   }
+  nsRefPtr<DOMSVGPathSeg> result = ItemAt(aIndex);
+  return result.forget();
 }
 
 void

content/svg/content/src/DOMSVGPathSegList.h

@@ -138,17 +138,10 @@ public:
   void Clear(ErrorResult& aError);
   already_AddRefed<DOMSVGPathSeg> Initialize(DOMSVGPathSeg& aNewItem,
                                              ErrorResult& aError);
-  DOMSVGPathSeg* GetItem(uint32_t aIndex, ErrorResult& aError)
-  {
-    bool found;
-    DOMSVGPathSeg* item = IndexedGetter(aIndex, found, aError);
-    if (!found) {
-      aError.Throw(NS_ERROR_DOM_INDEX_SIZE_ERR);
-    }
-    return item;
-  }
-  DOMSVGPathSeg* IndexedGetter(uint32_t aIndex, bool& found,
-                               ErrorResult& aError);
+  already_AddRefed<DOMSVGPathSeg> GetItem(uint32_t index,
+                                          ErrorResult& error);
+  already_AddRefed<DOMSVGPathSeg> IndexedGetter(uint32_t index, bool& found,
+                                                ErrorResult& error);
   already_AddRefed<DOMSVGPathSeg> InsertItemBefore(DOMSVGPathSeg& aNewItem,
                                                    uint32_t aIndex,
                                                    ErrorResult& aError);
@@ -206,8 +199,8 @@ private:
   SVGAnimatedPathSegList& InternalAList() const;
 
   /// Creates an instance of the appropriate DOMSVGPathSeg sub-class for
-  // aIndex, if it doesn't already exist.
-  void EnsureItemAt(uint32_t aIndex);
+  // aIndex, if it doesn't already exist, and then returs it.
+  already_AddRefed<DOMSVGPathSeg> GetItemAt(uint32_t aIndex);
 
   void MaybeInsertNullInAnimValListAt(uint32_t aIndex,
                                       uint32_t aInternalIndex,

content/svg/content/src/crashtests/880544-4.svg

@@ -0,0 +1,15 @@
+<svg xmlns="http://www.w3.org/2000/svg">
+  <script>//<![CDATA[
+
+function add_watch()
+{
+  document.getElementById("e").pathSegList.watch("0", function(){});
+}
+
+window.addEventListener("load", add_watch, false);
+
+  //]]></script>
+
+  <path id="e" d="M0,0"/>
+
+</svg>

content/svg/content/src/crashtests/crashtests.list

@@ -69,3 +69,4 @@ load 864509.svg
 load 880544-1.svg
 load 880544-2.svg
 load 880544-3.svg
+load 880544-4.svg

dom/bindings/Bindings.conf

@@ -1052,8 +1052,7 @@ DOMInterfaces = {
 
 'SVGPathSegList': {
     'nativeType': 'mozilla::DOMSVGPathSegList',
-    'headerFile': 'DOMSVGPathSegList.h',
-    'resultNotAddRefed': [ 'getItem' ]
+    'headerFile': 'DOMSVGPathSegList.h'
 },
 
 'SVGPoint': {