Bug 1531333 - Fix <svg:use> cycle detection. r=longsonr
authorEmilio Cobos Álvarez <emilio@crisal.io>
Fri, 22 Mar 2019 22:26:53 +0000
changeset 465793 88b6e96d82153bf333e1a309f5ae384906168883
parent 465792 8bfb359646bf38483cc1a4ea15abc31c40fd7ed4
child 465794 56383d31e265398b575ddb3b8c8615bb066e5ba0
push id35746
push usershindli@mozilla.com
push dateSat, 23 Mar 2019 09:46:24 +0000
treeherdermozilla-central@02b7484f316b [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerslongsonr
bugs1531333, 232774, 230866, 228958
milestone68.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 1531333 - Fix <svg:use> cycle detection. r=longsonr With the current code we'll eventually detect the cycle, but will take much more, creating many shadow trees unnecessarily. Take for example the following: <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="133" height="232774"> <style> symbol { display: block } </style> <symbol id="svg-sprite" viewBox="0 0 133 230866"> <title>svg-sprite</title> <symbol id="svg-sprite" viewBox="0 0 133 230866"> <title>svg-sprite</title> <use xlink:href="#svg-sprite" width="500" height="500" /> </symbol> <use xlink:href="#svg-sprite" y="1601" width="133" height="228958" /> </symbol> <use xlink:href="#svg-sprite" y="1601" width="133" height="230866" /> </svg> Before this patch, we'd create an svg use element subtree for #svg-sprite. That subtree will contain two other <use> elements, one under the <symbol>, one not under it. Both point to #svg-sprite, but we fail to detect we're an ancestor since the element #svg-sprite we're looking at is the clone of the #svg-sprite element. Thus we need to take a look at mOriginal instead (which is the <use> element under #svg-sprite) rather than at the clone. Yeah, I had to draw the trees, it's messy :) Blink and WebKit do something slightly different (they check the element id directly[1]). That's not 100% correct, since you can have multiple elements with the same ID. [1]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/svg/svg_use_element.cc?l=560&rcl=861855dcb8c39ba8d42497247d433277858df79b Differential Revision: https://phabricator.services.mozilla.com/D24565
dom/svg/SVGUseElement.cpp
dom/svg/SVGUseElement.h
layout/svg/tests/mochitest.ini
layout/svg/tests/test_use_tree_cycle.html
--- a/dom/svg/SVGUseElement.cpp
+++ b/dom/svg/SVGUseElement.cpp
@@ -231,16 +231,37 @@ void SVGUseElement::ContentRemoved(nsICo
   }
 }
 
 void SVGUseElement::NodeWillBeDestroyed(const nsINode* aNode) {
   nsCOMPtr<nsIMutationObserver> kungFuDeathGrip(this);
   UnlinkSource();
 }
 
+bool SVGUseElement::IsCyclicReferenceTo(const Element& aTarget) const {
+  if (&aTarget == this) {
+    return true;
+  }
+  if (mOriginal && mOriginal->IsCyclicReferenceTo(aTarget)) {
+    return true;
+  }
+  for (nsINode* parent = GetParentOrHostNode(); parent;
+       parent = parent->GetParentOrHostNode()) {
+    if (parent == &aTarget) {
+      return true;
+    }
+    if (auto* use = SVGUseElement::FromNode(*parent)) {
+      if (mOriginal && use->mOriginal == mOriginal) {
+        return true;
+      }
+    }
+  }
+  return false;
+}
+
 //----------------------------------------------------------------------
 
 void SVGUseElement::UpdateShadowTree() {
   MOZ_ASSERT(IsInComposedDoc());
 
   if (mReferencedElementTracker.get()) {
     mReferencedElementTracker.get()->RemoveMutationObserver(this);
   }
@@ -276,33 +297,20 @@ void SVGUseElement::UpdateShadowTree() {
           nsGkAtoms::text, nsGkAtoms::rect, nsGkAtoms::circle,
           nsGkAtoms::ellipse, nsGkAtoms::line, nsGkAtoms::polyline,
           nsGkAtoms::polygon, nsGkAtoms::image, nsGkAtoms::use)) {
     return;
   }
 
   // circular loop detection
 
-  // check 1 - check if we're a document descendent of the target
-  if (nsContentUtils::ContentIsShadowIncludingDescendantOf(this,
-                                                           targetElement)) {
+  if (IsCyclicReferenceTo(*targetElement)) {
     return;
   }
 
-  // check 2 - check if we're a clone, and if we already exist in the hierarchy
-  if (mOriginal) {
-    for (nsINode* parent = GetParentOrHostNode(); parent;
-         parent = parent->GetParentOrHostNode()) {
-      SVGUseElement* use = SVGUseElement::FromNode(*parent);
-      if (use && use->mOriginal == mOriginal) {
-        return;
-      }
-    }
-  }
-
   nsCOMPtr<nsIURI> baseURI = targetElement->GetBaseURI();
   if (!baseURI) {
     return;
   }
 
   {
     nsNodeInfoManager* nodeInfoManager = targetElement->OwnerDoc() == OwnerDoc()
                                              ? nullptr
--- a/dom/svg/SVGUseElement.h
+++ b/dom/svg/SVGUseElement.h
@@ -93,16 +93,18 @@ class SVGUseElement final : public SVGUs
   // This is needed because SMIL doesn't go through AfterSetAttr unfortunately.
   void ProcessAttributeChange(int32_t aNamespaceID, nsAtom* aAttribute);
 
   nsresult AfterSetAttr(int32_t aNamespaceID, nsAtom* aAttribute,
                         const nsAttrValue* aValue, const nsAttrValue* aOldValue,
                         nsIPrincipal* aSubjectPrincipal, bool aNotify) final;
 
  protected:
+  bool IsCyclicReferenceTo(const Element& aTarget) const;
+
   /**
    * Helper that provides a reference to the element with the ID that is
    * referenced by the 'use' element's 'href' attribute, and that will update
    * the 'use' element if the element that that ID identifies changes to a
    * different element (or none).
    */
   class ElementTracker final : public IDTracker {
    public:
@@ -140,17 +142,17 @@ class SVGUseElement final : public SVGUs
   enum { ATTR_X, ATTR_Y, ATTR_WIDTH, ATTR_HEIGHT };
   nsSVGLength2 mLengthAttributes[4];
   static LengthInfo sLengthInfo[4];
 
   enum { HREF, XLINK_HREF };
   SVGString mStringAttributes[2];
   static StringInfo sStringInfo[2];
 
-  nsCOMPtr<nsIContent> mOriginal;  // if we've been cloned, our "real" copy
+  RefPtr<SVGUseElement> mOriginal;  // if we've been cloned, our "real" copy
   ElementTracker mReferencedElementTracker;
   RefPtr<URLExtraData> mContentURLData;  // URL data for its anonymous content
 };
 
 }  // namespace dom
 }  // namespace mozilla
 
 #endif  // mozilla_dom_SVGUseElement_h
--- a/layout/svg/tests/mochitest.ini
+++ b/layout/svg/tests/mochitest.ini
@@ -6,8 +6,9 @@ support-files =
 [test_filter_crossorigin.html]
 support-files =
   filters.svg
   file_filter_crossorigin.svg
   file_black_yellow.svg
   file_yellow_black.svg
 
 [test_hover_near_text.html]
+[test_use_tree_cycle.html]
new file mode 100644
--- /dev/null
+++ b/layout/svg/tests/test_use_tree_cycle.html
@@ -0,0 +1,37 @@
+<!doctype html>
+<title>Test for bug 1531333</title>
+<script src="/tests/SimpleTest/SimpleTest.js"></script>
+<link rel="stylesheet" href="/tests/SimpleTest/test.css">
+<style>
+  symbol { display: block }
+</style>
+<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="10" height="10">
+  <symbol id="svg-sprite" viewBox="0 0 133 230866">
+    <title>svg-sprite</title>
+    <symbol id="svg-sprite" viewBox="0 0 133 230866">
+        <title>svg-sprite</title>
+        <use xlink:href="#svg-sprite" width="500" height="500" />
+    </symbol>
+    <use xlink:href="#svg-sprite" y="1601" width="133" height="228958" />
+  </symbol>
+  <use xlink:href="#svg-sprite" y="1601" width="133" height="230866" />
+</svg>
+<script>
+function countUseElements(subtreeRoot) {
+  if (!subtreeRoot)
+    return 0;
+
+  let i = 0;
+  for (const use of subtreeRoot.querySelectorAll("use"))
+    i += 1 + countUseElements(SpecialPowers.wrap(use).openOrClosedShadowRoot);
+  return i;
+}
+SimpleTest.waitForExplicitFinish();
+onload = function() {
+  document.body.offsetTop;
+  // The three in the document, plus the two created from the element that's
+  // not in the <symbol> subtree.
+  is(countUseElements(document), 5, "Shouldn't create more than 5 use elements");
+  SimpleTest.finish();
+}
+</script>