Bug 610990 - Regression: SVGPathSegList should allow manipulation of invalid paths. r=roc,sicking, a=blocking.
authorJonathan Watt <jwatt@jwatt.org>
Wed, 02 Feb 2011 08:41:50 +1300
changeset 61740 bcf40420cf32d9e94cb62a10d8d1d63105a1bc4b
parent 61739 d66a65c62e2e22398b9e4b9dbdcb24d9c0d6fde8
child 61741 7aa71efbb69cd7501611fe082f9a347056c17ad1
push idunknown
push userunknown
push dateunknown
reviewersroc, sicking, blocking
bugs610990
milestone2.0b11pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 610990 - Regression: SVGPathSegList should allow manipulation of invalid paths. r=roc,sicking, a=blocking.
content/base/src/nsGenericElement.cpp
content/base/src/nsGenericElement.h
content/svg/content/src/SVGPathData.cpp
content/svg/content/src/nsSVGElement.cpp
content/svg/content/test/test_SVGPathSegList.xhtml
layout/reftests/svg/path-04.svg
layout/reftests/svg/reftest.list
--- a/content/base/src/nsGenericElement.cpp
+++ b/content/base/src/nsGenericElement.cpp
@@ -4552,82 +4552,138 @@ nsGenericElement::CopyInnerTo(nsGenericE
     nsresult rv = aDst->SetAttr(name->NamespaceID(), name->LocalName(),
                                 name->GetPrefix(), valStr, PR_FALSE);
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   return NS_OK;
 }
 
+PRBool
+nsGenericElement::MaybeCheckSameAttrVal(PRInt32 aNamespaceID, nsIAtom* aName,
+                                        nsIAtom* aPrefix, const nsAString& aValue,
+                                        PRBool aNotify, nsAutoString* aOldValue,
+                                        PRUint8* aModType, PRBool* aHasListeners)
+{
+  PRBool modification = PR_FALSE;
+  *aHasListeners = aNotify &&
+    nsContentUtils::HasMutationListeners(this,
+                                         NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
+                                         this);
+
+  // If we have no listeners and aNotify is false, we are almost certainly
+  // coming from the content sink and will almost certainly have no previous
+  // value.  Even if we do, setting the value is cheap when we have no
+  // listeners and don't plan to notify.  The check for aNotify here is an
+  // optimization, the check for *aHasListeners is a correctness issue.
+  if (*aHasListeners || aNotify) {
+    nsAttrInfo info(GetAttrInfo(aNamespaceID, aName));
+    if (info.mValue) {
+      // Check whether the old value is the same as the new one.  Note that we
+      // only need to actually _get_ the old value if we have listeners.
+      PRBool valueMatches;
+      if (*aHasListeners) {
+        // Need to store the old value
+        info.mValue->ToString(*aOldValue);
+        valueMatches = aValue.Equals(*aOldValue);
+      } else if (aNotify) {
+        valueMatches = info.mValue->Equals(aValue, eCaseMatters);
+      }
+      if (valueMatches && aPrefix == info.mName->GetPrefix()) {
+        return PR_TRUE;
+      }
+      modification = PR_TRUE;
+    }
+  }
+  *aModType = modification ?
+    static_cast<PRUint8>(nsIDOMMutationEvent::MODIFICATION) :
+    static_cast<PRUint8>(nsIDOMMutationEvent::ADDITION);
+  return PR_FALSE;
+}
+
 nsresult
 nsGenericElement::SetAttr(PRInt32 aNamespaceID, nsIAtom* aName,
                           nsIAtom* aPrefix, const nsAString& aValue,
                           PRBool aNotify)
 {
+  // Keep this in sync with SetParsedAttr below
+
   NS_ENSURE_ARG_POINTER(aName);
   NS_ASSERTION(aNamespaceID != kNameSpaceID_Unknown,
                "Don't call SetAttr with unknown namespace");
 
   if (!mAttrsAndChildren.CanFitMoreAttrs()) {
     return NS_ERROR_FAILURE;
   }
 
+  PRUint8 modType;
+  PRBool hasListeners;
   nsAutoString oldValue;
-  PRBool modification = PR_FALSE;
-  PRBool hasListeners = aNotify &&
-    nsContentUtils::HasMutationListeners(this,
-                                         NS_EVENT_BITS_MUTATION_ATTRMODIFIED,
-                                         this);
-  
-  // If we have no listeners and aNotify is false, we are almost certainly
-  // coming from the content sink and will almost certainly have no previous
-  // value.  Even if we do, setting the value is cheap when we have no
-  // listeners and don't plan to notify.  The check for aNotify here is an
-  // optimization, the check for haveListeners is a correctness issue.
-  if (hasListeners || aNotify) {
-    nsAttrInfo info(GetAttrInfo(aNamespaceID, aName));
-    if (info.mValue) {
-      // Check whether the old value is the same as the new one.  Note that we
-      // only need to actually _get_ the old value if we have listeners.
-      PRBool valueMatches;
-      if (hasListeners) {
-        // Need to store the old value
-        info.mValue->ToString(oldValue);
-        valueMatches = aValue.Equals(oldValue);
-      } else if (aNotify) {
-        valueMatches = info.mValue->Equals(aValue, eCaseMatters);
-      }
-      if (valueMatches && aPrefix == info.mName->GetPrefix()) {
-        return NS_OK;
-      }
-      modification = PR_TRUE;
-    }
-  }
-
+
+  if (MaybeCheckSameAttrVal(aNamespaceID, aName, aPrefix, aValue, aNotify,
+                            &oldValue, &modType, &hasListeners)) {
+    return NS_OK;
+  }
 
   nsresult rv = BeforeSetAttr(aNamespaceID, aName, &aValue, aNotify);
   NS_ENSURE_SUCCESS(rv, rv);
-  
-  PRUint8 modType = modification ?
-    static_cast<PRUint8>(nsIDOMMutationEvent::MODIFICATION) :
-    static_cast<PRUint8>(nsIDOMMutationEvent::ADDITION);
+
   if (aNotify) {
     nsNodeUtils::AttributeWillChange(this, aNamespaceID, aName, modType);
   }
 
   nsAttrValue attrValue;
   if (!ParseAttribute(aNamespaceID, aName, aValue, attrValue)) {
     attrValue.SetTo(aValue);
   }
 
   return SetAttrAndNotify(aNamespaceID, aName, aPrefix, oldValue,
                           attrValue, modType, hasListeners, aNotify,
                           &aValue);
 }
-  
+
+nsresult
+nsGenericElement::SetParsedAttr(PRInt32 aNamespaceID, nsIAtom* aName,
+                                nsIAtom* aPrefix, nsAttrValue& aParsedValue,
+                                PRBool aNotify)
+{
+  // Keep this in sync with SetAttr above
+
+  NS_ENSURE_ARG_POINTER(aName);
+  NS_ASSERTION(aNamespaceID != kNameSpaceID_Unknown,
+               "Don't call SetAttr with unknown namespace");
+
+  if (!mAttrsAndChildren.CanFitMoreAttrs()) {
+    return NS_ERROR_FAILURE;
+  }
+
+  nsAutoString value;
+  aParsedValue.ToString(value);
+
+  PRUint8 modType;
+  PRBool hasListeners;
+  nsAutoString oldValue;
+
+  if (MaybeCheckSameAttrVal(aNamespaceID, aName, aPrefix, value, aNotify,
+                            &oldValue, &modType, &hasListeners)) {
+    return NS_OK;
+  }
+
+  nsresult rv = BeforeSetAttr(aNamespaceID, aName, &value, aNotify);
+  NS_ENSURE_SUCCESS(rv, rv);
+
+  if (aNotify) {
+    nsNodeUtils::AttributeWillChange(this, aNamespaceID, aName, modType);
+  }
+
+  return SetAttrAndNotify(aNamespaceID, aName, aPrefix, oldValue,
+                          aParsedValue, modType, hasListeners, aNotify,
+                          &value);
+}
+
 nsresult
 nsGenericElement::SetAttrAndNotify(PRInt32 aNamespaceID,
                                    nsIAtom* aName,
                                    nsIAtom* aPrefix,
                                    const nsAString& aOldValue,
                                    nsAttrValue& aParsedValue,
                                    PRUint8 aModType,
                                    PRBool aFireMutation,
--- a/content/base/src/nsGenericElement.h
+++ b/content/base/src/nsGenericElement.h
@@ -381,18 +381,40 @@ public:
   virtual already_AddRefed<nsINodeList> GetChildren(PRUint32 aFilter);
   virtual nsIAtom *GetClassAttributeName() const;
   virtual already_AddRefed<nsINodeInfo> GetExistingAttrNameFromQName(const nsAString& aStr) const;
   nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
                    const nsAString& aValue, PRBool aNotify)
   {
     return SetAttr(aNameSpaceID, aName, nsnull, aValue, aNotify);
   }
+  /**
+   * Helper for SetAttr/SetParsedAttr. This method will return true if aNotify
+   * is true or there are mutation listeners that must be triggered, the
+   * attribute is currently set, and the new value that is about to be set is
+   * different to the current value. As a perf optimization the new and old
+   * values will not actually be compared if we aren't notifying and we don't
+   * have mutation listeners (in which case it's cheap to just return PR_FALSE
+   * and let the caller go ahead and set the value).
+   * @param aOldValue Set to the old value of the attribute, but only if there
+   *   are event listeners
+   * @param aModType Set to nsIDOMMutationEvent::MODIFICATION or to
+   *   nsIDOMMutationEvent::ADDITION, but only if this helper returns true
+   * @param aHasListeners Set to true if there are mutation event listeners
+   *   listening for NS_EVENT_BITS_MUTATION_ATTRMODIFIED
+   */
+  PRBool MaybeCheckSameAttrVal(PRInt32 aNamespaceID, nsIAtom* aName,
+                               nsIAtom* aPrefix, const nsAString& aValue,
+                               PRBool aNotify, nsAutoString* aOldValue,
+                               PRUint8* aModType, PRBool* aHasListeners);
   virtual nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, nsIAtom* aPrefix,
                            const nsAString& aValue, PRBool aNotify);
+  virtual nsresult SetParsedAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
+                                 nsIAtom* aPrefix, nsAttrValue& aParsedValue,
+                                 PRBool aNotify);
   virtual PRBool GetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
                          nsAString& aResult) const;
   virtual PRBool HasAttr(PRInt32 aNameSpaceID, nsIAtom* aName) const;
   virtual PRBool AttrValueIs(PRInt32 aNameSpaceID, nsIAtom* aName,
                              const nsAString& aValue,
                              nsCaseTreatment aCaseSensitive) const;
   virtual PRBool AttrValueIs(PRInt32 aNameSpaceID, nsIAtom* aName,
                              nsIAtom* aValue,
--- a/content/svg/content/src/SVGPathData.cpp
+++ b/content/svg/content/src/SVGPathData.cpp
@@ -46,16 +46,22 @@
 #include "string.h"
 #include "nsSVGPathDataParser.h"
 #include "nsSVGPathGeometryElement.h" // for nsSVGMark
 #include "gfxPlatform.h"
 #include <stdarg.h>
 
 using namespace mozilla;
 
+static PRBool IsMoveto(PRUint16 aSegType)
+{
+  return aSegType == nsIDOMSVGPathSeg::PATHSEG_MOVETO_ABS ||
+         aSegType == nsIDOMSVGPathSeg::PATHSEG_MOVETO_REL;
+}
+
 nsresult
 SVGPathData::CopyFrom(const SVGPathData& rhs)
 {
   if (!mData.SetCapacity(rhs.mData.Length())) {
     // Yes, we do want fallible alloc here
     return NS_ERROR_OUT_OF_MEMORY;
   }
   mData = rhs.mData;
@@ -230,16 +236,20 @@ SVGPathData::GetPathSegAtLength(float aD
   NS_ABORT_IF_FALSE(i == mData.Length(), "Very, very bad - mData corrupt");
 
   return NS_MAX(0U, segIndex - 1); // -1 because while loop takes us 1 too far
 }
 
 void
 SVGPathData::ConstructPath(gfxContext *aCtx) const
 {
+  if (!mData.Length() || !IsMoveto(SVGPathSegUtils::DecodeType(mData[0]))) {
+    return; // paths without an initial moveto are invalid
+  }
+
   PRUint32 segType, prevSegType = nsIDOMSVGPathSeg::PATHSEG_UNKNOWN;
   gfxPoint pathStart(0.0, 0.0); // start point of [sub]path
   gfxPoint segEnd(0.0, 0.0);    // end point of previous/current segment
   gfxPoint cp1, cp2;            // previous bezier's control points
   gfxPoint tcp1, tcp2;          // temporaries
 
   // Regarding cp1 and cp2: If the previous segment was a cubic bezier curve,
   // then cp2 is its second control point. If the previous segment was a
@@ -415,22 +425,16 @@ SVGPathData::ToFlattenedPath(const gfxMa
 
   ctx->SetMatrix(aMatrix);
   ConstructPath(ctx);
   ctx->IdentityMatrix();
 
   return ctx->GetFlattenedPath();
 }
 
-static PRBool IsMoveto(PRUint16 aSegType)
-{
-  return aSegType == nsIDOMSVGPathSeg::PATHSEG_MOVETO_ABS ||
-         aSegType == nsIDOMSVGPathSeg::PATHSEG_MOVETO_REL;
-}
-
 static float AngleOfVector(gfxPoint v)
 {
   // C99 says about atan2 "A domain error may occur if both arguments are
   // zero" and "On a domain error, the function returns an implementation-
   // defined value". In the case of atan2 the implementation-defined value
   // seems to commonly be zero, but it could just as easily be a NaN value.
   // We specifically want zero in this case, hence the check:
 
--- a/content/svg/content/src/nsSVGElement.cpp
+++ b/content/svg/content/src/nsSVGElement.cpp
@@ -1743,25 +1743,24 @@ nsSVGElement::DidAnimatePointList()
                             GetPointListAttrName(),
                             nsIDOMMutationEvent::MODIFICATION);
   }
 }
 
 void
 nsSVGElement::DidChangePathSegList(PRBool aDoSetAttr)
 {
-  NS_ABORT_IF_FALSE(GetPathDataAttrName(), "Changing non-existent path data?");
-
   if (!aDoSetAttr)
     return;
 
-  nsAutoString newStr;
-  GetAnimPathSegList()->GetBaseValue().GetValueAsString(newStr);
+  nsAutoString serializedValue;
+  GetAnimPathSegList()->GetBaseValue().GetValueAsString(serializedValue);
 
-  SetAttr(kNameSpaceID_None, GetPathDataAttrName(), newStr, PR_TRUE);
+  nsAttrValue attrValue(serializedValue);
+  SetParsedAttr(kNameSpaceID_None, GetPathDataAttrName(), nsnull, attrValue, PR_TRUE);
 }
 
 void
 nsSVGElement::DidAnimatePathSegList()
 {
   NS_ABORT_IF_FALSE(GetPathDataAttrName(),
                     "Animating non-existent path data?");
 
--- a/content/svg/content/test/test_SVGPathSegList.xhtml
+++ b/content/svg/content/test/test_SVGPathSegList.xhtml
@@ -27,29 +27,82 @@ This file runs a series of SVGPathSegLis
 tests can be found in test_SVGxxxList.xhtml. Anything that can be generalized
 to other list types belongs there.
 */
 
 function run_tests()
 {
   document.getElementById('svg').pauseAnimations();
 
+  var d;
+  var seg;
   var path = document.getElementById("path");
-  var d = path.pathSegList;
-  var arc = path.createSVGPathSegArcAbs(400, 0, 200, 200, 0, 1, 1);
+  var list = path.pathSegList;
 
   // See https://bugzilla.mozilla.org/show_bug.cgi?id=611138
   // Here we are doing a replace with a segment (arc) that has more arguments
-  // than the total number of arguments in the entire path + 2 (for the two
-  // segment types).
+  // than the total number of arguments in the entire path + 2 (the +2
+  // refering to the encoding of the segment types for the two segments).
   path.setAttribute('d', 'M0,0 L100,100');
-  d.replaceItem(arc, 1);
+  var arc = path.createSVGPathSegArcAbs(400, 0, 200, 200, 0, 1, 1);
+  list.replaceItem(arc, 1);
+
+  is(list.numberOfItems, 2, 'The length of the list should be the same after a valid replaceItem() call');
+  is(list.getItem(1), arc, 'The inserted object should now be the object at the index being replaced');
+
+  // Test whether and when we normalize the 'd' attribute:
+
+  d = "  \n  M  10  ,  10  \n  L  20  10  \n  ";
+  path.setAttribute('d', d);
+  is(path.getAttribute('d'), d, "Values passed to setAttribute for the 'd' attribute should not be normalised");
+  list.getItem(1).y = 20;
+  isnot(path.getAttribute('d'), d, "The 'd' attribute should change when its underlying DOM list changes");
+
+  // Test that path manipulations still work even when the path is invalid due
+  // to it not starting with a moveto segment:
+
+  path.setAttribute('d', 'M0,0 L1,1');
+  is(list.numberOfItems, 2, 'setAttribute should result in two items')
+
+  seg = list.getItem(1);
+  list.removeItem(0);
+  ok(list.numberOfItems == 1 && list.getItem(0) == seg,
+    'If removeItem removes the initial moveto leaving an invalid path, the other items should still be left in the list')
 
-  ok(d.numberOfItems == 2, 'The number of items should be 2');
-  ok(d.getItem(1) == arc, 'The second item is the wrong object');
+  seg = path.createSVGPathSegLinetoAbs(1, 2);
+  list.appendItem(seg);
+  ok(list.numberOfItems == 2 && list.getItem(1) == seg,
+    'appendItem should be able to append to an invalid path');
+
+  seg = path.createSVGPathSegLinetoAbs(1, 2);
+  list.replaceItem(seg, 1);
+  ok(list.numberOfItems == 2 && list.getItem(1) == seg,
+    'replaceItem should be able to replace items in an invalid path');
+
+  seg = path.createSVGPathSegLinetoAbs(1, 2);
+  list.insertItemBefore(seg, 1);
+  ok(list.numberOfItems == 3 && list.getItem(1) == seg,
+    'insertItemBefore should be able insert items into an invalid path');
+
+  seg = path.createSVGPathSegLinetoAbs(1, 2);
+  list.initialize(seg);
+  ok(list.numberOfItems == 1 && list.getItem(0) == seg,
+    'initialize should be able initialize an invalid path with a non-moveto item');
+
+  d = 'M0,0 L12,34'
+  path.setAttribute('d', d);
+  function check_old_value(e) {
+    is(e.target, path, 'check mutation event is for expected node');
+    is(e.attrName, 'd', 'check mutation event is for expected attribute');
+    is(e.prevValue, d, 'check old attribute value is correctly reported');
+    isnot(e.newValue, d, 'check attribute value has changed');
+  }
+  path.addEventListener('DOMAttrModified', check_old_value, false);
+  list.getItem(1).y = 35;
+  path.removeEventListener('DOMAttrModified', check_old_value, false);
 
   SimpleTest.finish();
 }
 
 window.addEventListener("load", run_tests, false);
 
 ]]>
 </script>
new file mode 100644
--- /dev/null
+++ b/layout/reftests/svg/path-04.svg
@@ -0,0 +1,33 @@
+<!--
+     Any copyright is dedicated to the Public Domain.
+     http://creativecommons.org/licenses/publicdomain/
+-->
+<svg xmlns="http://www.w3.org/2000/svg" class="reftest-wait">
+
+  <title>Test that selectors detect changes to the 'd' attribute</title>
+
+  <!-- From https://bugzilla.mozilla.org/show_bug.cgi?id=610990 -->
+
+  <style>
+
+path[d="M0,0 V100 H100 V0 Z"] {
+  fill: lime;
+}
+
+  </style>
+  <script><![CDATA[
+
+function run()
+{
+  document.getElementById('path').setAttribute('d', 'M0,0 V100 H100 V0 Z');
+  document.documentElement.removeAttribute('class');
+}
+
+window.addEventListener('MozReftestInvalidate', run, false);
+
+  ]]></script>
+
+  <rect width="100%" height="100%" fill="lime"/>
+  <path id="path" fill="red" d="M0,0 H100 V100 H0 Z"/>
+
+</svg>
--- a/layout/reftests/svg/reftest.list
+++ b/layout/reftests/svg/reftest.list
@@ -143,16 +143,17 @@ random-if(gtk2Widget) == objectBoundingB
 == objectBoundingBox-and-pattern-03.svg objectBoundingBox-and-pattern-03-ref.svg
 == opacity-and-gradient-01.svg pass.svg
 == opacity-and-gradient-02.svg opacity-and-gradient-02-ref.svg
 == opacity-and-pattern-01.svg pass.svg
 == outer-svg-border-and-padding-01.svg outer-svg-border-and-padding-01-ref.svg 
 == path-01.svg path-01-ref.svg
 == path-02.svg pass.svg
 == path-03.svg pass.svg
+== path-04.svg pass.svg
 == pattern-invalid-01.svg pattern-invalid-01-ref.svg
 == pattern-live-01a.svg pattern-live-01-ref.svg
 == pattern-live-01b.svg pattern-live-01-ref.svg
 == pattern-live-01c.svg pattern-live-01-ref.svg
 == polygon-marker-01.svg pass.svg
 == polygon-points-negative-01.svg pass.svg
 == polyline-points-invalid-01.svg pass.svg
 == pseudo-classes-01.svg pass.svg