Bug 783915 - Make SVG Fragment identifier processing more robust. r=dholbert
authorRobert Longson <longsonr@gmail.com>
Sat, 25 Aug 2012 10:02:34 -0700
changeset 105473 9245897374c1a8647716680c89b4fdf49dc8c01c
parent 105472 4b46b309c12631fc1412cef7526fcc5dca5cb86e
child 105474 368de938d9e1e58e20673a34d6db9cc20c2a4f00
push id55
push usershu@rfrn.org
push dateThu, 30 Aug 2012 01:33:09 +0000
reviewersdholbert
bugs783915
milestone17.0a1
Bug 783915 - Make SVG Fragment identifier processing more robust. r=dholbert
content/svg/content/src/SVGAnimatedPreserveAspectRatio.cpp
content/svg/content/src/SVGAnimatedPreserveAspectRatio.h
content/svg/content/src/SVGFragmentIdentifier.cpp
content/svg/content/src/nsSVGElement.cpp
content/svg/content/src/nsSVGViewBox.cpp
content/svg/content/src/nsSVGViewBox.h
content/svg/content/test/test_fragments.html
--- a/content/svg/content/src/SVGAnimatedPreserveAspectRatio.cpp
+++ b/content/svg/content/src/SVGAnimatedPreserveAspectRatio.cpp
@@ -196,36 +196,41 @@ ToPreserveAspectRatio(const nsAString &a
   }
 
   *aValue = val;
   return NS_OK;
 }
 
 nsresult
 SVGAnimatedPreserveAspectRatio::SetBaseValueString(
-  const nsAString &aValueAsString, nsSVGElement *aSVGElement)
+  const nsAString &aValueAsString, nsSVGElement *aSVGElement, bool aDoSetAttr)
 {
   SVGPreserveAspectRatio val;
   nsresult res = ToPreserveAspectRatio(aValueAsString, &val);
   if (NS_FAILED(res)) {
     return res;
   }
 
+  nsAttrValue emptyOrOldValue;
+  if (aDoSetAttr) {
+    emptyOrOldValue = aSVGElement->WillChangePreserveAspectRatio();
+  }
+
   mBaseVal = val;
   mIsBaseSet = true;
+
   if (!mIsAnimated) {
     mAnimVal = mBaseVal;
   }
-  else {
+  if (aDoSetAttr) {
+    aSVGElement->DidChangePreserveAspectRatio(emptyOrOldValue);
+  }
+  if (mIsAnimated) {
     aSVGElement->AnimationNeedsResample();
   }
-
-  // We don't need to call DidChange* here - we're only called by
-  // nsSVGElement::ParseAttribute under nsGenericElement::SetAttr,
-  // which takes care of notifying.
   return NS_OK;
 }
 
 void
 SVGAnimatedPreserveAspectRatio::GetBaseValueString(
   nsAString& aValueAsString) const
 {
   nsAutoString tmpString;
--- a/content/svg/content/src/SVGAnimatedPreserveAspectRatio.h
+++ b/content/svg/content/src/SVGAnimatedPreserveAspectRatio.h
@@ -87,17 +87,18 @@ public:
     mBaseVal.mMeetOrSlice = nsIDOMSVGPreserveAspectRatio::SVG_MEETORSLICE_MEET;
     mBaseVal.mDefer = false;
     mAnimVal = mBaseVal;
     mIsAnimated = false;
     mIsBaseSet = false;
   }
 
   nsresult SetBaseValueString(const nsAString& aValue,
-                              nsSVGElement *aSVGElement);
+                              nsSVGElement *aSVGElement,
+                              bool aDoSetAttr);
   void GetBaseValueString(nsAString& aValue) const;
 
   void SetBaseValue(const SVGPreserveAspectRatio &aValue,
                     nsSVGElement *aSVGElement);
   nsresult SetBaseAlign(uint16_t aAlign, nsSVGElement *aSVGElement) {
     if (aAlign < nsIDOMSVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_NONE ||
         aAlign > nsIDOMSVGPreserveAspectRatio::SVG_PRESERVEASPECTRATIO_XMAXYMAX) {
       return NS_ERROR_FAILURE;
--- a/content/svg/content/src/SVGFragmentIdentifier.cpp
+++ b/content/svg/content/src/SVGFragmentIdentifier.cpp
@@ -1,31 +1,36 @@
 /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "SVGFragmentIdentifier.h"
-#include "mozilla/CharTokenizer.h"
 #include "nsIDOMSVGDocument.h"
 #include "nsSVGSVGElement.h"
 #include "nsSVGViewElement.h"
 
 using namespace mozilla;
 
 static bool
 IsMatchingParameter(const nsAString &aString, const nsAString &aParameterName)
 {
   // The first two tests ensure aString.Length() > aParameterName.Length()
   // so it's then safe to do the third test
   return StringBeginsWith(aString, aParameterName) &&
          aString.Last() == ')' &&
          aString.CharAt(aParameterName.Length()) == '(';
 }
 
+inline bool
+IgnoreWhitespace(PRUnichar aChar)
+{
+  return false;
+}
+
 static nsSVGViewElement*
 GetViewElement(nsIDocument *aDocument, const nsAString &aId)
 {
   dom::Element* element = aDocument->GetElementById(aId);
   return (element && element->IsSVG(nsGkAtoms::view)) ?
             static_cast<nsSVGViewElement*>(element) : nullptr;
 }
 
@@ -89,29 +94,30 @@ SVGFragmentIdentifier::RestoreOldZoomAnd
 bool
 SVGFragmentIdentifier::ProcessSVGViewSpec(const nsAString &aViewSpec,
                                           nsSVGSVGElement *root)
 {
   if (!IsMatchingParameter(aViewSpec, NS_LITERAL_STRING("svgView"))) {
     return false;
   }
 
-  // SVGViewAttribute may occur in any order, but each type may only occur at most one time
-  // in a correctly formed SVGViewSpec.
-  // If we encounter any element more than once or get any syntax errors we're going to
-  // return without updating the root element
-
-  const nsAString *viewBoxParams = nullptr;
-  const nsAString *preserveAspectRatioParams = nullptr;
-  const nsAString *zoomAndPanParams = nullptr;
+  // SVGViewAttributes may occur in any order, but each type may only occur
+  // at most one time in a correctly formed SVGViewSpec.
+  // If we encounter any attribute more than once or get any syntax errors
+  // we're going to return false and cancel any changes.
+  
+  bool viewBoxFound = false;
+  bool preserveAspectRatioFound = false;
+  bool zoomAndPanFound = false;
 
   // Each token is a SVGViewAttribute
   int32_t bracketPos = aViewSpec.FindChar('(');
-  CharTokenizer<';'>tokenizer(
-    Substring(aViewSpec, bracketPos + 1, aViewSpec.Length() - bracketPos - 2));
+  uint32_t lengthOfViewSpec = aViewSpec.Length() - bracketPos - 2;
+  nsCharSeparatedTokenizerTemplate<IgnoreWhitespace> tokenizer(
+    Substring(aViewSpec, bracketPos + 1, lengthOfViewSpec), ';');
 
   if (!tokenizer.hasMoreTokens()) {
     return false;
   }
   do {
 
     nsAutoString token(tokenizer.nextToken());
 
@@ -120,60 +126,72 @@ SVGFragmentIdentifier::ProcessSVGViewSpe
       // invalid SVGViewAttribute syntax
       return false;
     }
 
     const nsAString &params =
       Substring(token, bracketPos + 1, token.Length() - bracketPos - 2);
 
     if (IsMatchingParameter(token, NS_LITERAL_STRING("viewBox"))) {
-      if (viewBoxParams) {
+      if (viewBoxFound ||
+          NS_FAILED(root->mViewBox.SetBaseValueString(
+                      params, root, true))) {
+        return false;
+      }
+      viewBoxFound = true;
+    } else if (IsMatchingParameter(token, NS_LITERAL_STRING("preserveAspectRatio"))) {
+      if (preserveAspectRatioFound ||
+          NS_FAILED(root->mPreserveAspectRatio.SetBaseValueString(
+                      params, root, true))) {
+        return false;
+      }
+      preserveAspectRatioFound = true;
+    } else if (IsMatchingParameter(token, NS_LITERAL_STRING("zoomAndPan"))) {
+      if (zoomAndPanFound) {
         return false;
       }
-      viewBoxParams = &params;
-    } else if (IsMatchingParameter(token, NS_LITERAL_STRING("preserveAspectRatio"))) {
-      if (preserveAspectRatioParams) {
+      nsIAtom *valAtom = NS_GetStaticAtom(params);
+      if (!valAtom) {
         return false;
       }
-      preserveAspectRatioParams = &params;
-    } else if (IsMatchingParameter(token, NS_LITERAL_STRING("zoomAndPan"))) {
-      if (zoomAndPanParams) {
-        return false;
+      const nsSVGEnumMapping *mapping = nsSVGSVGElement::sZoomAndPanMap;
+      while (mapping->mKey) {
+        if (valAtom == *(mapping->mKey)) {
+          // If we've got a valid zoomAndPan value, then set it on our root element.
+          if (NS_FAILED(root->mEnumAttributes[nsSVGSVGElement::ZOOMANDPAN].SetBaseValue(
+                          mapping->mVal, root))) {
+            return false;
+          }
+          break;
+        }
+        mapping++;
       }
-      zoomAndPanParams = &params;
+      if (!mapping->mKey) {
+          // Unrecognised zoomAndPan value
+          return false;
+      }
+      zoomAndPanFound = true;
     } else {
       // We don't support transform or viewTarget currently
       return false;
     }
   } while (tokenizer.hasMoreTokens());
 
-  if (viewBoxParams) {
-    root->mViewBox.SetBaseValueString(*viewBoxParams, root);
-  } else {
-    RestoreOldViewBox(root);
-  }
-
-  if (preserveAspectRatioParams) {
-    root->mPreserveAspectRatio.SetBaseValueString(*preserveAspectRatioParams, root);
-  } else {
-    RestoreOldPreserveAspectRatio(root);
-  }
-
-  if (zoomAndPanParams) {
-    nsCOMPtr<nsIAtom> valAtom = do_GetAtom(*zoomAndPanParams);
-    const nsSVGEnumMapping *mapping = root->sZoomAndPanMap;
-    while (mapping->mKey) {
-      if (valAtom == *(mapping->mKey)) {
-        root->mEnumAttributes[nsSVGSVGElement::ZOOMANDPAN].SetBaseValue(mapping->mVal, root);
-        break;
-      }
-      mapping++;
+  if (root->mUseCurrentView) {
+    // A previous SVGViewSpec may have overridden some attributes.
+    // If they are no longer overridden we need to restore the old values.
+    if (!viewBoxFound) {
+      RestoreOldViewBox(root);
     }
-  } else {
-    RestoreOldZoomAndPan(root);
+    if (!preserveAspectRatioFound) {
+      RestoreOldPreserveAspectRatio(root);
+    }
+    if (!zoomAndPanFound) {
+      RestoreOldZoomAndPan(root);
+    }
   }
 
   return true;
 }
 
 bool
 SVGFragmentIdentifier::ProcessFragmentIdentifier(nsIDocument *aDocument,
                                                  const nsAString &aAnchorName)
--- a/content/svg/content/src/nsSVGElement.cpp
+++ b/content/svg/content/src/nsSVGElement.cpp
@@ -516,31 +516,31 @@ nsSVGElement::ParseAttribute(int32_t aNa
       }
     }
 
     if (!foundMatch) {
       // Check for nsSVGViewBox attribute
       if (aAttribute == nsGkAtoms::viewBox) {
         nsSVGViewBox* viewBox = GetViewBox();
         if (viewBox) {
-          rv = viewBox->SetBaseValueString(aValue, this);
+          rv = viewBox->SetBaseValueString(aValue, this, false);
           if (NS_FAILED(rv)) {
             viewBox->Init();
           } else {
             aResult.SetTo(*viewBox, &aValue);
             didSetResult = true;
           }
           foundMatch = true;
         }
       // Check for SVGAnimatedPreserveAspectRatio attribute
       } else if (aAttribute == nsGkAtoms::preserveAspectRatio) {
         SVGAnimatedPreserveAspectRatio *preserveAspectRatio =
           GetPreserveAspectRatio();
         if (preserveAspectRatio) {
-          rv = preserveAspectRatio->SetBaseValueString(aValue, this);
+          rv = preserveAspectRatio->SetBaseValueString(aValue, this, false);
           if (NS_FAILED(rv)) {
             preserveAspectRatio->Init();
           } else {
             aResult.SetTo(*preserveAspectRatio, &aValue);
             didSetResult = true;
           }
           foundMatch = true;
         }
--- a/content/svg/content/src/nsSVGViewBox.cpp
+++ b/content/svg/content/src/nsSVGViewBox.cpp
@@ -143,30 +143,35 @@ ToSVGViewBoxRect(const nsAString& aStr, 
   aViewBox->width = vals[2];
   aViewBox->height = vals[3];
 
   return NS_OK;
 }
 
 nsresult
 nsSVGViewBox::SetBaseValueString(const nsAString& aValue,
-                                 nsSVGElement *aSVGElement)
+                                 nsSVGElement *aSVGElement,
+                                 bool aDoSetAttr)
 {
   nsSVGViewBoxRect viewBox;
   nsresult res = ToSVGViewBoxRect(aValue, &viewBox);
   if (NS_SUCCEEDED(res)) {
+    nsAttrValue emptyOrOldValue;
+    if (aDoSetAttr) {
+      emptyOrOldValue = aSVGElement->WillChangeViewBox();
+    }
     mBaseVal = nsSVGViewBoxRect(viewBox.x, viewBox.y, viewBox.width, viewBox.height);
     mHasBaseVal = true;
 
+    if (aDoSetAttr) {
+      aSVGElement->DidChangeViewBox(emptyOrOldValue);
+    }
     if (mAnimVal) {
       aSVGElement->AnimationNeedsResample();
     }
-    // We don't need to call Will/DidChange* here - we're only called by
-    // nsSVGElement::ParseAttribute under nsGenericElement::SetAttr,
-    // which takes care of notifying.
   }
   return res;
 }
 
 void
 nsSVGViewBox::GetBaseValueString(nsAString& aValue) const
 {
   PRUnichar buf[200];
--- a/content/svg/content/src/nsSVGViewBox.h
+++ b/content/svg/content/src/nsSVGViewBox.h
@@ -59,17 +59,18 @@ public:
                     nsSVGElement *aSVGElement)
     { SetBaseValue(nsSVGViewBoxRect(aX, aY, aWidth, aHeight), aSVGElement); }
   const nsSVGViewBoxRect& GetAnimValue() const
     { return mAnimVal ? *mAnimVal : mBaseVal; }
   void SetAnimValue(float aX, float aY, float aWidth, float aHeight,
                     nsSVGElement *aSVGElement);
 
   nsresult SetBaseValueString(const nsAString& aValue,
-                              nsSVGElement *aSVGElement);
+                              nsSVGElement *aSVGElement,
+                              bool aDoSetAttr);
   void GetBaseValueString(nsAString& aValue) const;
 
   nsresult ToDOMAnimatedRect(nsIDOMSVGAnimatedRect **aResult,
                              nsSVGElement *aSVGElement);
   // Returns a new nsISMILAttr object that the caller must delete
   nsISMILAttr* ToSMILAttr(nsSVGElement* aSVGElement);
   
 private:
--- a/content/svg/content/test/test_fragments.html
+++ b/content/svg/content/test/test_fragments.html
@@ -14,46 +14,69 @@ https://bugzilla.mozilla.org/show_bug.cg
 <div id="content" style="display: none"></div>
 
 <iframe id="svg" src="fragments-helper.svg"></iframe>
 
 <pre id="test">
 <script class="testbody" type="application/javascript">
 SimpleTest.waitForExplicitFinish();
 
-function Test(svgFragmentIdentifier, valid) {
+function Test(svgFragmentIdentifier, valid, viewBoxString,
+              preserveAspectRatioString, zoomAndPanString)
+{
     this.svgFragmentIdentifier = svgFragmentIdentifier;
     this.valid = valid;
+    this.viewBoxString = viewBoxString;
+    this.preserveAspectRatioString = preserveAspectRatioString;
+    this.zoomAndPanString = zoomAndPanString;
 }
 
 function runTests()
 {
   var svg = $("svg");
   var doc = svg.contentWindow.document;
   
   var tests = [
-      new Test("view", true),
-      new Test("unknown", false),
-      new Test("svgView(viewBox(0,0,200,200)))", true),
-      new Test("svgView(preserveAspectRatio(xMaxYMin slice))", true),
-      new Test("svgView(viewBox(1,2,3,4);preserveAspectRatio(xMaxYMin))", true),
-      new Test("svgView(zoomAndPan(disable))", true),
-      new Test("svgView", false),
-      new Test("svgView(", false),
-      new Test("svgView()", false)
+      new Test("view", true, "0 200 100 100", "none", null),
+      new Test("unknown", false, null, null, null),
+      new Test("svgView(viewBox(0,0,200,200))", true, "0 0 200 200", null, null),
+      new Test("svgView(preserveAspectRatio(xMaxYMin slice))", true, null, "xMaxYMin slice", null),
+      new Test("svgView(viewBox(1,2,3,4);preserveAspectRatio(xMinYMax))", true, "1 2 3 4", "xMinYMax meet", null),
+      new Test("svgView(zoomAndPan(disable))", true, null, null, "disable"),
+      new Test("svgView(viewBox(bad)", false, null, null, null),
+      new Test("svgView(preserveAspectRatio(bad))", false, null, null, null),
+      new Test("svgView(zoomAndPan(bad))", false, null, null, null),
+      new Test("svgView", false, null, null, null),
+      new Test("svgView(", false, null, null, null),
+      new Test("svgView()", false, null, null, null),
+      // Be sure we verify that there's a closing paren for svgView()
+      // (and not too many closing parens)
+      new Test("svgView(zoomAndPan(disable)", false, null, null, null),
+      new Test("svgView(zoomAndPan(disable) ", false, null, null, null),
+      new Test("svgView(zoomAndPan(disable)]", false, null, null, null),
+      new Test("svgView(zoomAndPan(disable)))", false, null, null, null)
   ];
 
- var src = svg.getAttribute("src");
- for (var i = 0; i < tests.length; i++) {
-   var test = tests[i];
-   svg.setAttribute("src", src + "#" + test.svgFragmentIdentifier);
-   is(doc.rootElement.useCurrentView, test.valid,
-      "Expected " + test.svgFragmentIdentifier + " to be " +
-      (test.valid ? "valid" : "invalid"));
- }
+  var src = svg.getAttribute("src");
+  for (var i = 0; i < tests.length; i++) {
+    var test = tests[i];
+    svg.setAttribute("src", src + "#" + test.svgFragmentIdentifier);
+    is(doc.rootElement.useCurrentView, test.valid,
+       "Expected " + test.svgFragmentIdentifier + " to be " +
+       (test.valid ? "valid" : "invalid"));
+
+    is(doc.rootElement.getAttribute("viewBox"),
+       test.viewBoxString, "unexpected viewBox");
+
+    is(doc.rootElement.getAttribute("preserveAspectRatio"),
+       test.preserveAspectRatioString, "unexpected preserveAspectRatio");
+
+    is(doc.rootElement.getAttribute("zoomAndPan"),
+       test.zoomAndPanString, "unexpected zoomAndPan");
+  }
 
   SimpleTest.finish();
 }
 
 window.addEventListener("load", runTests, false);
 </script>
 </pre>
 </body>