Bug 1486488 - Don't assume that SVGAnimationElement has a parent on bind. r=dholbert
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 28 Aug 2018 09:06:08 +0000
changeset 488681 e36b98a79cd85470eef43718b9c32c8bde54e3fd
parent 488680 683aadc6686ec5fcfb6dadc5c7a831323e06be85
child 488682 58c2e6069684e166bc4ab9807f3f240956220c63
push id9734
push usershindli@mozilla.com
push dateThu, 30 Aug 2018 12:18:07 +0000
treeherdermozilla-beta@71c71ab3afae [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert
bugs1486488, 1450250
milestone63.0a1
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 1486488 - Don't assume that SVGAnimationElement has a parent on bind. r=dholbert This is a regression from bug 1450250, which removed a if (!GetCx()) early return in this function. That early return was wrong, both because it prevented elements that were in shadow trees from targeting other elements, but also because that check was not present in AfterSetAttr, which means that dynamic updates to the attribute would work. Pass the SVGAnimationElement itself to resolve references. That's what we do for attribute mutations, and also it's the same behavior we have, since the ID lookup IDTracker does only depends on containing shadow root and containing document, and that's invariant between a kid and it's DOM parent. Some other code has been updated to take references instead of pointers so the null-safety of those methods is explicit. Differential Revision: https://phabricator.services.mozilla.com/D4349
dom/base/IDTracker.cpp
dom/base/IDTracker.h
dom/smil/nsSMILTimeValueSpec.cpp
dom/smil/nsSMILTimeValueSpec.h
dom/smil/nsSMILTimedElement.cpp
dom/smil/nsSMILTimedElement.h
dom/svg/SVGAnimationElement.cpp
dom/svg/crashtests/1486488.html
dom/svg/crashtests/crashtests.list
--- a/dom/base/IDTracker.cpp
+++ b/dom/base/IDTracker.cpp
@@ -139,31 +139,28 @@ IDTracker::Reset(nsIContent* aFromConten
     atom.swap(mWatchID);
   }
 
   mReferencingImage = aReferenceImage;
   HaveNewDocumentOrShadowRoot(docOrShadow, aWatch, ref);
 }
 
 void
-IDTracker::ResetWithID(nsIContent* aFromContent,
-                       nsAtom* aID,
-                       bool aWatch)
+IDTracker::ResetWithID(Element& aFrom, nsAtom* aID, bool aWatch)
 {
-  MOZ_ASSERT(aFromContent);
   MOZ_ASSERT(aID);
 
   if (aWatch) {
     RefPtr<nsAtom> atom = aID;
     atom.swap(mWatchID);
   }
 
   mReferencingImage = false;
 
-  DocumentOrShadowRoot* docOrShadow = DocOrShadowFromContent(*aFromContent);
+  DocumentOrShadowRoot* docOrShadow = DocOrShadowFromContent(aFrom);
   HaveNewDocumentOrShadowRoot(docOrShadow, aWatch, nsDependentAtomString(aID));
 }
 
 void
 IDTracker::HaveNewDocumentOrShadowRoot(
   DocumentOrShadowRoot* aDocOrShadow,
   bool aWatch,
   const nsString& aRef)
--- a/dom/base/IDTracker.h
+++ b/dom/base/IDTracker.h
@@ -73,17 +73,17 @@ public:
    * A variation on Reset() to set up a reference that consists of the ID of
    * an element in the same document as aFrom.
    * @param aFrom the source element for context
    * @param aID the ID of the element
    * @param aWatch if false, then we do not set up the notifications to track
    * changes, so ElementChanged won't fire and get() will always return the same
    * value, the current element for the ID.
    */
-  void ResetWithID(nsIContent* aFrom, nsAtom* aID, bool aWatch = true);
+  void ResetWithID(Element& aFrom, nsAtom* aID, bool aWatch = true);
 
   /**
    * Clears the reference. ElementChanged is not triggered. get() will return
    * null.
    */
   void Unlink();
 
   void Traverse(nsCycleCollectionTraversalCallback* aCB);
--- a/dom/smil/nsSMILTimeValueSpec.cpp
+++ b/dom/smil/nsSMILTimeValueSpec.cpp
@@ -52,17 +52,17 @@ nsSMILTimeValueSpec::~nsSMILTimeValueSpe
   if (mEventListener) {
     mEventListener->Disconnect();
     mEventListener = nullptr;
   }
 }
 
 nsresult
 nsSMILTimeValueSpec::SetSpec(const nsAString& aStringSpec,
-                             Element* aContextNode)
+                             Element& aContextElement)
 {
   nsSMILTimeValueSpecParams params;
 
   if (!nsSMILParserUtils::ParseTimeValueSpecParams(aStringSpec, params))
     return NS_ERROR_FAILURE;
 
   mParams = params;
 
@@ -75,42 +75,40 @@ nsSMILTimeValueSpec::SetSpec(const nsASt
     mOwner->AddInstanceTime(new nsSMILInstanceTime(mParams.mOffset), mIsBegin);
   }
 
   // Fill in the event symbol to simplify handling later
   if (mParams.mType == nsSMILTimeValueSpecParams::REPEAT) {
     mParams.mEventSymbol = nsGkAtoms::repeatEvent;
   }
 
-  ResolveReferences(aContextNode);
+  ResolveReferences(aContextElement);
 
   return NS_OK;
 }
 
 void
-nsSMILTimeValueSpec::ResolveReferences(nsIContent* aContextNode)
+nsSMILTimeValueSpec::ResolveReferences(Element& aContextElement)
 {
-  if (mParams.mType != nsSMILTimeValueSpecParams::SYNCBASE && !IsEventBased())
+  if (mParams.mType != nsSMILTimeValueSpecParams::SYNCBASE && !IsEventBased()) {
     return;
-
-  MOZ_ASSERT(aContextNode,
-             "null context node for resolving timing references against");
+  }
 
   // If we're not bound to the document yet, don't worry, we'll get called again
   // when that happens
-  if (!aContextNode->IsInComposedDoc())
+  if (!aContextElement.IsInComposedDoc())
     return;
 
   // Hold ref to the old element so that it isn't destroyed in between resetting
   // the referenced element and using the pointer to update the referenced
   // element.
   RefPtr<Element> oldReferencedElement = mReferencedElement.get();
 
   if (mParams.mDependentElemID) {
-    mReferencedElement.ResetWithID(aContextNode, mParams.mDependentElemID);
+    mReferencedElement.ResetWithID(aContextElement, mParams.mDependentElemID);
   } else if (mParams.mType == nsSMILTimeValueSpecParams::EVENT) {
     Element* target = mOwner->GetTargetElement();
     mReferencedElement.ResetWithElement(target);
   } else {
     MOZ_ASSERT(false, "Syncbase or repeat spec without ID");
   }
   UpdateReferencedElement(oldReferencedElement, mReferencedElement.get());
 }
--- a/dom/smil/nsSMILTimeValueSpec.h
+++ b/dom/smil/nsSMILTimeValueSpec.h
@@ -43,19 +43,19 @@ class nsSMILTimeValueSpec
 public:
   typedef mozilla::dom::Element Element;
   typedef mozilla::dom::Event Event;
   typedef mozilla::dom::IDTracker IDTracker;
 
   nsSMILTimeValueSpec(nsSMILTimedElement& aOwner, bool aIsBegin);
   ~nsSMILTimeValueSpec();
 
-  nsresult SetSpec(const nsAString& aStringSpec, Element* aContextNode);
-  void     ResolveReferences(nsIContent* aContextNode);
-  bool     IsEventBased() const;
+  nsresult SetSpec(const nsAString& aStringSpec, Element& aContextElement);
+  void ResolveReferences(Element& aContextElement);
+  bool IsEventBased() const;
 
   void     HandleNewInterval(nsSMILInterval& aInterval,
                              const nsSMILTimeContainer* aSrcContainer);
   void     HandleTargetElementChange(Element* aNewTarget);
 
   // For created nsSMILInstanceTime objects
   bool     DependsOnBegin() const;
   void     HandleChangedInstanceTime(const nsSMILInstanceTime& aBaseTime,
--- a/dom/smil/nsSMILTimedElement.cpp
+++ b/dom/smil/nsSMILTimedElement.cpp
@@ -848,30 +848,31 @@ namespace
   bool
   RemoveNonDOM(nsSMILInstanceTime* aInstanceTime)
   {
     return !aInstanceTime->FromDOM() && !aInstanceTime->ShouldPreserve();
   }
 } // namespace
 
 bool
-nsSMILTimedElement::SetAttr(nsAtom* aAttribute, const nsAString& aValue,
+nsSMILTimedElement::SetAttr(nsAtom* aAttribute,
+                            const nsAString& aValue,
                             nsAttrValue& aResult,
-                            Element* aContextNode,
+                            Element& aContextElement,
                             nsresult* aParseResult)
 {
   bool foundMatch = true;
   nsresult parseResult = NS_OK;
 
   if (aAttribute == nsGkAtoms::begin) {
-    parseResult = SetBeginSpec(aValue, aContextNode, RemoveNonDOM);
+    parseResult = SetBeginSpec(aValue, aContextElement, RemoveNonDOM);
   } else if (aAttribute == nsGkAtoms::dur) {
     parseResult = SetSimpleDuration(aValue);
   } else if (aAttribute == nsGkAtoms::end) {
-    parseResult = SetEndSpec(aValue, aContextNode, RemoveNonDOM);
+    parseResult = SetEndSpec(aValue, aContextElement, RemoveNonDOM);
   } else if (aAttribute == nsGkAtoms::fill) {
     parseResult = SetFillMode(aValue);
   } else if (aAttribute == nsGkAtoms::max) {
     parseResult = SetMax(aValue);
   } else if (aAttribute == nsGkAtoms::min) {
     parseResult = SetMin(aValue);
   } else if (aAttribute == nsGkAtoms::repeatCount) {
     parseResult = SetRepeatCount(aValue);
@@ -923,36 +924,36 @@ nsSMILTimedElement::UnsetAttr(nsAtom* aA
   return foundMatch;
 }
 
 //----------------------------------------------------------------------
 // Setters and unsetters
 
 nsresult
 nsSMILTimedElement::SetBeginSpec(const nsAString& aBeginSpec,
-                                 Element* aContextNode,
+                                 Element& aContextElement,
                                  RemovalTestFunction aRemove)
 {
-  return SetBeginOrEndSpec(aBeginSpec, aContextNode, true /*isBegin*/,
+  return SetBeginOrEndSpec(aBeginSpec, aContextElement, true /*isBegin*/,
                            aRemove);
 }
 
 void
 nsSMILTimedElement::UnsetBeginSpec(RemovalTestFunction aRemove)
 {
   ClearSpecs(mBeginSpecs, mBeginInstances, aRemove);
   UpdateCurrentInterval();
 }
 
 nsresult
 nsSMILTimedElement::SetEndSpec(const nsAString& aEndSpec,
-                               Element* aContextNode,
+                               Element& aContextElement,
                                RemovalTestFunction aRemove)
 {
-  return SetBeginOrEndSpec(aEndSpec, aContextNode, false /*!isBegin*/,
+  return SetBeginOrEndSpec(aEndSpec, aContextElement, false /*!isBegin*/,
                            aRemove);
 }
 
 void
 nsSMILTimedElement::UnsetEndSpec(RemovalTestFunction aRemove)
 {
   ClearSpecs(mEndSpecs, mEndInstances, aRemove);
   UpdateCurrentInterval();
@@ -1200,17 +1201,17 @@ nsSMILTimedElement::IsTimeDependent(cons
 
   if (!thisBegin || !otherBegin)
     return false;
 
   return thisBegin->IsDependentOn(*otherBegin);
 }
 
 void
-nsSMILTimedElement::BindToTree(nsIContent* aContextNode)
+nsSMILTimedElement::BindToTree(Element& aContextElement)
 {
   // Reset previously registered milestone since we may be registering with
   // a different time container now.
   mPrevRegisteredMilestone = sMaxMilestone;
 
   // If we were already active then clear all our timing information and start
   // afresh
   if (mElementState != STATE_STARTUP) {
@@ -1220,22 +1221,22 @@ nsSMILTimedElement::BindToTree(nsIConten
 
   // Scope updateBatcher to last only for the ResolveReferences calls:
   {
     AutoIntervalUpdateBatcher updateBatcher(*this);
 
     // Resolve references to other parts of the tree
     uint32_t count = mBeginSpecs.Length();
     for (uint32_t i = 0; i < count; ++i) {
-      mBeginSpecs[i]->ResolveReferences(aContextNode);
+      mBeginSpecs[i]->ResolveReferences(aContextElement);
     }
 
     count = mEndSpecs.Length();
     for (uint32_t j = 0; j < count; ++j) {
-      mEndSpecs[j]->ResolveReferences(aContextNode);
+      mEndSpecs[j]->ResolveReferences(aContextElement);
     }
   }
 
   RegisterMilestone();
 }
 
 void
 nsSMILTimedElement::HandleTargetElementChange(Element* aNewTarget)
@@ -1299,17 +1300,17 @@ nsSMILTimedElement::Unlink()
   mTimeDependents.Clear();
 }
 
 //----------------------------------------------------------------------
 // Implementation helpers
 
 nsresult
 nsSMILTimedElement::SetBeginOrEndSpec(const nsAString& aSpec,
-                                      Element* aContextNode,
+                                      Element& aContextElement,
                                       bool aIsBegin,
                                       RemovalTestFunction aRemove)
 {
   TimeValueSpecList& timeSpecsList = aIsBegin ? mBeginSpecs : mEndSpecs;
   InstanceTimeList& instances = aIsBegin ? mBeginInstances : mEndInstances;
 
   ClearSpecs(timeSpecsList, instances, aRemove);
 
@@ -1318,17 +1319,17 @@ nsSMILTimedElement::SetBeginOrEndSpec(co
   nsCharSeparatedTokenizer tokenizer(aSpec, ';');
   if (!tokenizer.hasMoreTokens()) { // Empty list
     return NS_ERROR_FAILURE;
   }
 
   bool hadFailure = false;
   while (tokenizer.hasMoreTokens()) {
     auto spec = MakeUnique<nsSMILTimeValueSpec>(*this, aIsBegin);
-    nsresult rv = spec->SetSpec(tokenizer.nextToken(), aContextNode);
+    nsresult rv = spec->SetSpec(tokenizer.nextToken(), aContextElement);
     if (NS_SUCCEEDED(rv)) {
       timeSpecsList.AppendElement(std::move(spec));
     } else {
       hadFailure = true;
     }
   }
 
   // The return value from this function is only used to determine if we should
@@ -1476,23 +1477,23 @@ nsSMILTimedElement::RebuildTimingState(R
              "Attempting to enable a timed element not attached to an "
              "animation element");
   MOZ_ASSERT(mElementState == STATE_STARTUP,
              "Rebuilding timing state from non-startup state");
 
   if (mAnimationElement->HasAttr(nsGkAtoms::begin)) {
     nsAutoString attValue;
     mAnimationElement->GetAttr(nsGkAtoms::begin, attValue);
-    SetBeginSpec(attValue, mAnimationElement, aRemove);
+    SetBeginSpec(attValue, *mAnimationElement, aRemove);
   }
 
   if (mAnimationElement->HasAttr(nsGkAtoms::end)) {
     nsAutoString attValue;
     mAnimationElement->GetAttr(nsGkAtoms::end, attValue);
-    SetEndSpec(attValue, mAnimationElement, aRemove);
+    SetEndSpec(attValue, *mAnimationElement, aRemove);
   }
 
   mPrevRegisteredMilestone = sMaxMilestone;
   RegisterMilestone();
 }
 
 void
 nsSMILTimedElement::DoPostSeek()
--- a/dom/smil/nsSMILTimedElement.h
+++ b/dom/smil/nsSMILTimedElement.h
@@ -55,17 +55,17 @@ public:
    * nullptr if it is not associated with a time container.
    */
   nsSMILTimeContainer* GetTimeContainer();
 
   /*
    * Returns the element targeted by the animation element. Needed for
    * registering event listeners against the appropriate element.
    */
-  mozilla::dom::Element* GetTargetElement();
+  Element* GetTargetElement();
 
   /**
    * Methods for supporting the ElementTimeControl interface.
    */
 
   /*
    * Adds a new begin instance time at the current container time plus or minus
    * the specified offset.
@@ -260,27 +260,27 @@ public:
    * @param aAttribute  The name of the attribute to set. The namespace of this
    *                    attribute is not specified as it is checked by the host
    *                    element. Only attributes in the namespace defined for
    *                    SMIL attributes in the host language are passed to the
    *                    timed element.
    * @param aValue      The attribute value.
    * @param aResult     The nsAttrValue object that may be used for storing the
    *                    parsed result.
-   * @param aContextNode The element to use for context when resolving
-   *                     references to other elements.
+   * @param aContextElement The element to use for context when resolving
+   *                        references to other elements.
    * @param[out] aParseResult The result of parsing the attribute. Will be set
    *                          to NS_OK if parsing is successful.
    *
    * @return true if the given attribute is a timing attribute, false
    * otherwise.
    */
   bool SetAttr(nsAtom* aAttribute, const nsAString& aValue,
-                 nsAttrValue& aResult, Element* aContextNode,
-                 nsresult* aParseResult = nullptr);
+               nsAttrValue& aResult, Element& aContextElement,
+               nsresult* aParseResult = nullptr);
 
   /**
    * Attempts to unset an attribute on this timed element.
    *
    * @param aAttribute  The name of the attribute to set. As with SetAttr the
    *                    namespace of the attribute is not specified (see
    *                    SetAttr).
    *
@@ -321,28 +321,27 @@ public:
    * otherwise.
    */
   bool IsTimeDependent(const nsSMILTimedElement& aOther) const;
 
   /**
    * Called when the timed element has been bound to the document so that
    * references from this timed element to other elements can be resolved.
    *
-   * @param aContextNode  The node which provides the necessary context for
-   *                      resolving references. This is typically the element in
-   *                      the host language that owns this timed element. Should
-   *                      not be null.
+   * @param aContextElement The element which provides the necessary context for
+   *                        resolving references. This is typically the element
+   *                        in the host language that owns this timed element.
    */
-  void BindToTree(nsIContent* aContextNode);
+  void BindToTree(Element& aContextElement);
 
   /**
    * Called when the target of the animation has changed so that event
    * registrations can be updated.
    */
-  void HandleTargetElementChange(mozilla::dom::Element* aNewTarget);
+  void HandleTargetElementChange(Element* aNewTarget);
 
   /**
    * Called when the timed element has been removed from a document so that
    * references to other elements can be broken.
    */
   void DissolveReferences() { Unlink(); }
 
   // Cycle collection
@@ -372,20 +371,20 @@ protected:
   template <class TestFunctor>
   void RemoveInstanceTimes(InstanceTimeList& aArray, TestFunctor& aTest);
 
   //
   // Implementation helpers
   //
 
   nsresult          SetBeginSpec(const nsAString& aBeginSpec,
-                                 Element* aContextNode,
+                                 Element& aContextElement,
                                  RemovalTestFunction aRemove);
   nsresult          SetEndSpec(const nsAString& aEndSpec,
-                               Element* aContextNode,
+                               Element& aContextElement,
                                RemovalTestFunction aRemove);
   nsresult          SetSimpleDuration(const nsAString& aDurSpec);
   nsresult          SetMin(const nsAString& aMinSpec);
   nsresult          SetMax(const nsAString& aMaxSpec);
   nsresult          SetRestart(const nsAString& aRestartSpec);
   nsresult          SetRepeatCount(const nsAString& aRepeatCountSpec);
   nsresult          SetRepeatDur(const nsAString& aRepeatDurSpec);
   nsresult          SetFillMode(const nsAString& aFillModeSpec);
@@ -396,17 +395,17 @@ protected:
   void              UnsetMin();
   void              UnsetMax();
   void              UnsetRestart();
   void              UnsetRepeatCount();
   void              UnsetRepeatDur();
   void              UnsetFillMode();
 
   nsresult          SetBeginOrEndSpec(const nsAString& aSpec,
-                                      Element* aContextNode,
+                                      Element& aContextElement,
                                       bool aIsBegin,
                                       RemovalTestFunction aRemove);
   void              ClearSpecs(TimeValueSpecList& aSpecs,
                                InstanceTimeList& aInstances,
                                RemovalTestFunction aRemove);
   void              ClearIntervals();
   void              DoSampleAt(nsSMILTime aContainerTime, bool aEndOnly);
 
--- a/dom/svg/SVGAnimationElement.cpp
+++ b/dom/svg/SVGAnimationElement.cpp
@@ -180,17 +180,17 @@ SVGAnimationElement::BindToTree(nsIDocum
         : mAttrs.GetAttr(nsGkAtoms::href, kNameSpaceID_XLink);
     if (href) {
       nsAutoString hrefStr;
       href->ToString(hrefStr);
 
       UpdateHrefTarget(hrefStr);
     }
 
-    mTimedElement.BindToTree(aParent);
+    mTimedElement.BindToTree(*this);
   }
 
   AnimationNeedsResample();
 
   return NS_OK;
 }
 
 void
@@ -229,17 +229,17 @@ SVGAnimationElement::ParseAttribute(int3
     // First let the animation function try to parse it...
     bool foundMatch =
       AnimationFunction().SetAttr(aAttribute, aValue, aResult, &rv);
 
     // ... and if that didn't recognize the attribute, let the timed element
     // try to parse it.
     if (!foundMatch) {
       foundMatch =
-        mTimedElement.SetAttr(aAttribute, aValue, aResult, this, &rv);
+        mTimedElement.SetAttr(aAttribute, aValue, aResult, *this, &rv);
     }
 
     if (foundMatch) {
       AnimationNeedsResample();
       if (NS_FAILED(rv)) {
         ReportAttributeParseFailure(OwnerDoc(), aAttribute, aValue);
         return false;
       }
new file mode 100644
--- /dev/null
+++ b/dom/svg/crashtests/1486488.html
@@ -0,0 +1,11 @@
+<script>
+function start() {
+  document.replaceChild(id_0, document.childNodes[0]);
+}
+</script>
+<body onload="start()">
+    <svg>
+        <animate id="id_0" begin="s" />
+        <ellipse id="id_1" />
+    </svg>
+</body>
--- a/dom/svg/crashtests/crashtests.list
+++ b/dom/svg/crashtests/crashtests.list
@@ -83,8 +83,9 @@ load 1329093-2.html
 load 1343147.svg
 load 1347617-1.svg
 load 1347617-2.svg
 load 1347617-3.svg
 load 1402798.html
 load 1419250-1.html
 load 1420492.html
 load 1477853.html
+load 1486488.html