Bug 508496. SVGAnimatedLength's animVal and baseVal attributes always return different objects. r=roc

content/svg/content/src/nsSVGAttrTearoffTable.h

@@ -0,0 +1,119 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is the Mozilla SVG project.
+ *
+ * Contributor(s):
+ *   Brian Birtles <birtles@gmail.com>
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL.
+ *
+ * ***** END LICENSE BLOCK ***** */
+
+#ifndef NS_SVGATTRTEAROFFTABLE_H_
+#define NS_SVGATTRTEAROFFTABLE_H_
+
+#include "nsDataHashtable.h"
+
+/**
+ * Global hashmap to associate internal SVG data types (e.g. nsSVGLength2) with
+ * DOM tear-off objects (e.g. nsIDOMSVGLength). This allows us to always return
+ * the same object for subsequent requests for DOM objects.
+ *
+ * We don't keep an owning reference to the tear-off objects so they are
+ * responsible for removing themselves from this table when they die.
+ */
+template<class SimpleType, class TearoffType>
+class nsSVGAttrTearoffTable
+{
+public:
+  ~nsSVGAttrTearoffTable()
+  {
+    NS_ABORT_IF_FALSE(mTable.Count() == 0,
+        "Tear-off objects remain in hashtable at shutdown.");
+  }
+
+  TearoffType* GetTearoff(SimpleType* aSimple);
+
+  void AddTearoff(SimpleType* aSimple, TearoffType* aTearoff);
+
+  void RemoveTearoff(SimpleType* aSimple);
+
+private:
+  typedef nsPtrHashKey<SimpleType> SimpleTypePtrKey;
+  typedef nsDataHashtable<SimpleTypePtrKey, TearoffType* > TearoffTable;
+
+  TearoffTable mTable;
+};
+
+template<class SimpleType, class TearoffType>
+TearoffType*
+nsSVGAttrTearoffTable<SimpleType, TearoffType>::GetTearoff(SimpleType* aSimple)
+{
+  if (!mTable.IsInitialized())
+    return nsnull;
+
+  TearoffType *tearoff = nsnull;
+  PRBool found = mTable.Get(aSimple, &tearoff);
+  NS_ABORT_IF_FALSE(!found || tearoff,
+      "NULL pointer stored in attribute tear-off map");
+
+  return tearoff;
+}
+
+template<class SimpleType, class TearoffType>
+void
+nsSVGAttrTearoffTable<SimpleType, TearoffType>::AddTearoff(SimpleType* aSimple,
+                                                          TearoffType* aTearoff)
+{
+  if (!mTable.IsInitialized()) {
+    mTable.Init();
+  }
+
+  // We shouldn't be adding a tear-off if there already is one. If that happens,
+  // something is wrong.
+  if (mTable.Get(aSimple, nsnull)) {
+    NS_ABORT_IF_FALSE(PR_FALSE, "There is already a tear-off for this object.");
+    return;
+  }
+
+  PRBool result = mTable.Put(aSimple, aTearoff);
+  NS_ABORT_IF_FALSE(result, "Out of memory.");
+}
+
+template<class SimpleType, class TearoffType>
+void
+nsSVGAttrTearoffTable<SimpleType, TearoffType>::RemoveTearoff(
+    SimpleType* aSimple)
+{
+  if (!mTable.IsInitialized()) {
+    // Perhaps something happened in between creating the SimpleType object and
+    // registering it
+    return;
+  }
+
+  mTable.Remove(aSimple);
+}
+
+#endif // NS_SVGATTRTEAROFFTABLE_H_

content/svg/content/src/nsSVGLength2.cpp

@@ -43,6 +43,7 @@
 #include "nsSVGSVGElement.h"
 #include "nsIFrame.h"
 #include "nsSVGIntegrationUtils.h"
+#include "nsSVGAttrTearoffTable.h"
 #ifdef MOZ_SMIL
 #include "nsSMILValue.h"
 #include "nsSMILFloatType.h"
@@ -96,6 +97,13 @@ static nsIAtom** const unitMap[] =
   &nsGkAtoms::pc
 };
 
+static nsSVGAttrTearoffTable<nsSVGLength2, nsIDOMSVGAnimatedLength>
+  sSVGAnimatedLengthTearoffTable;
+static nsSVGAttrTearoffTable<nsSVGLength2, nsIDOMSVGLength>
+  sBaseSVGLengthTearoffTable;
+static nsSVGAttrTearoffTable<nsSVGLength2, nsIDOMSVGLength>
+  sAnimSVGLengthTearoffTable;
+
 /* Helper functions */
 
 static PRBool
@@ -322,7 +330,7 @@ void
 nsSVGLength2::SetBaseValueInSpecifiedUnits(float aValue,
                                            nsSVGElement *aSVGElement)
 {
-  mBaseVal = aValue;
+  mBaseVal = mAnimVal = aValue;
   aSVGElement->DidChangeLength(mAttrEnum, PR_TRUE);
 
 #ifdef MOZ_SMIL
@@ -367,25 +375,43 @@ nsSVGLength2::NewValueSpecifiedUnits(PRUint16 unitType,
 nsresult
 nsSVGLength2::ToDOMBaseVal(nsIDOMSVGLength **aResult, nsSVGElement *aSVGElement)
 {
-  *aResult = new DOMBaseVal(this, aSVGElement);
-  if (!*aResult)
-    return NS_ERROR_OUT_OF_MEMORY;
+  *aResult = sBaseSVGLengthTearoffTable.GetTearoff(this);
+  if (!*aResult) {
+    *aResult = new DOMBaseVal(this, aSVGElement);
+    if (!*aResult)
+      return NS_ERROR_OUT_OF_MEMORY;
+    sBaseSVGLengthTearoffTable.AddTearoff(this, *aResult);
+  }
 
   NS_ADDREF(*aResult);
   return NS_OK;
 }
 
+nsSVGLength2::DOMBaseVal::~DOMBaseVal()
+{
+  sBaseSVGLengthTearoffTable.RemoveTearoff(mVal);
+}
+
 nsresult
 nsSVGLength2::ToDOMAnimVal(nsIDOMSVGLength **aResult, nsSVGElement *aSVGElement)
 {
-  *aResult = new DOMAnimVal(this, aSVGElement);
-  if (!*aResult)
-    return NS_ERROR_OUT_OF_MEMORY;
+  *aResult = sAnimSVGLengthTearoffTable.GetTearoff(this);
+  if (!*aResult) {
+    *aResult = new DOMAnimVal(this, aSVGElement);
+    if (!*aResult)
+      return NS_ERROR_OUT_OF_MEMORY;
+    sAnimSVGLengthTearoffTable.AddTearoff(this, *aResult);
+  }
 
   NS_ADDREF(*aResult);
   return NS_OK;
 }
 
+nsSVGLength2::DOMAnimVal::~DOMAnimVal()
+{
+  sAnimSVGLengthTearoffTable.RemoveTearoff(mVal);
+}
+
 /* Implementation */
 
 nsresult
@@ -451,14 +477,23 @@ nsresult
 nsSVGLength2::ToDOMAnimatedLength(nsIDOMSVGAnimatedLength **aResult,
                                   nsSVGElement *aSVGElement)
 {
-  *aResult = new DOMAnimatedLength(this, aSVGElement);
-  if (!*aResult)
-    return NS_ERROR_OUT_OF_MEMORY;
+  *aResult = sSVGAnimatedLengthTearoffTable.GetTearoff(this);
+  if (!*aResult) {
+    *aResult = new DOMAnimatedLength(this, aSVGElement);
+    if (!*aResult)
+      return NS_ERROR_OUT_OF_MEMORY;
+    sSVGAnimatedLengthTearoffTable.AddTearoff(this, *aResult);
+  }
 
   NS_ADDREF(*aResult);
   return NS_OK;
 }
 
+nsSVGLength2::DOMAnimatedLength::~DOMAnimatedLength()
+{
+  sSVGAnimatedLengthTearoffTable.RemoveTearoff(mVal);
+}
+
 #ifdef MOZ_SMIL
 nsISMILAttr*
 nsSVGLength2::ToSMILAttr(nsSVGElement *aSVGElement)

content/svg/content/src/nsSVGLength2.h

@@ -145,6 +145,7 @@ private:
 
     DOMBaseVal(nsSVGLength2* aVal, nsSVGElement *aSVGElement)
       : mVal(aVal), mSVGElement(aSVGElement) {}
+    virtual ~DOMBaseVal();
     
     nsSVGLength2* mVal; // kept alive because it belongs to mSVGElement
     nsRefPtr<nsSVGElement> mSVGElement;
@@ -194,6 +195,7 @@ private:
 
     DOMAnimVal(nsSVGLength2* aVal, nsSVGElement *aSVGElement)
       : mVal(aVal), mSVGElement(aSVGElement) {}
+    virtual ~DOMAnimVal();
     
     nsSVGLength2* mVal; // kept alive because it belongs to mSVGElement
     nsRefPtr<nsSVGElement> mSVGElement;
@@ -255,6 +257,7 @@ private:
 
     DOMAnimatedLength(nsSVGLength2* aVal, nsSVGElement *aSVGElement)
       : mVal(aVal), mSVGElement(aSVGElement) {}
+    virtual ~DOMAnimatedLength();
     
     nsSVGLength2* mVal; // kept alive because it belongs to content
     nsRefPtr<nsSVGElement> mSVGElement;

content/svg/content/test/Makefile.in

@@ -45,6 +45,7 @@ include $(DEPTH)/config/autoconf.mk
 include $(topsrcdir)/config/rules.mk
 
 _TEST_FILES = \
+		test_animLengthObjectIdentity.xhtml \
 		test_animLengthRelativeUnits.xhtml \
 		test_animLengthUnits.xhtml \
 		test_bbox.xhtml \

content/svg/content/test/test_animLengthObjectIdentity.xhtml

@@ -0,0 +1,87 @@
+<html xmlns="http://www.w3.org/1999/xhtml">
+<!--
+https://bugzilla.mozilla.org/show_bug.cgi?id=508496
+-->
+<head>
+  <title>Test for object identity of SVG animated lengths</title>
+  <script type="text/javascript" src="/MochiKit/packed.js"></script>
+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
+</head>
+<body>
+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=506856">Mozilla Bug 508496</a>
+<p id="display"></p>
+<div id="content" style="display: none">
+<svg id="svg" xmlns="http://www.w3.org/2000/svg" width="120px" height="120px">
+  <circle cx="-100" cy="-100" r="15" fill="blue" id="circle">
+    <animate attributeName="cx" from="0" to="100" dur="4s" begin="1s; 10s"
+      fill="freeze" id="animate"/>
+  </circle>
+</svg>
+</div>
+<pre id="test">
+<script class="testbody" type="text/javascript">
+<![CDATA[
+/** Test object identity of animated lengths **/
+
+/* Global Variables */
+const svgns="http://www.w3.org/2000/svg";
+var svg = document.getElementById("svg");
+var circle = document.getElementById('circle');
+
+SimpleTest.waitForExplicitFinish();
+
+function main() {
+  var animLength = circle.cx;
+  ok(animLength === circle.cx,
+    "Got different SVGAnimatedLength objects at startup");
+
+  var baseVal = circle.cx.baseVal;
+  ok(baseVal === circle.cx.baseVal,
+    "Got different baseVal SVGLength objects at startup");
+
+  var animVal = circle.cx.animVal;
+  ok(animVal === circle.cx.animVal,
+    "Got different animVal SVGLength objects at startup");
+
+  var animate = document.getElementById('animate');
+  if (animate && animate.targetElement) {
+    // Sample mid-way through the animation
+    svg.pauseAnimations();
+    svg.setCurrentTime(5);
+
+    ok(animLength === circle.cx,
+      "Got different SVGAnimatedLength objects during animation");
+    ok(baseVal === circle.cx.baseVal,
+      "Got different baseVal SVGLength objects during animation");
+    ok(animVal === circle.cx.animVal,
+      "Got different animVal SVGLength objects during animation");
+  }
+
+  // Drop all references to the tear off objects
+  var oldValue = circle.cx.animVal.value; // Just a float, not an object ref
+  animLength = null;
+  baseVal = null;
+  animVal = null;
+  netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
+  window.QueryInterface(Components.interfaces.nsIInterfaceRequestor)
+        .getInterface(Components.interfaces.nsIDOMWindowUtils)
+        .garbageCollect();
+
+  // The tearoff objects should no longer exist and we should create new ones.
+  // If somehow, the tearoff objects have died and yet not been removed from the
+  // hashmap we'll end up in all sorts of trouble when we try to access them.
+  // So in the following, we're not really interested in the value, just that we
+  // don't crash.
+  is(circle.cx.animVal.value, oldValue,
+    "Unexpected result accessing new(?) length object.");
+
+  SimpleTest.finish();
+}
+
+window.addEventListener("load", main, false);
+]]>
+</script>
+</pre>
+</body>
+</html>