Bug 1715387 - Flush style properly for DOM APIs which use d property. r=jwatt,hiro,emilio

In order to make sure these APIs work properly, we have to flush style before
building the path to make sure the d property is up-to-date.
1. isPointInFill()
2. isPointInStroke()
3. getTotalLength()
4. getPointAtLength()
5. getPathSegAtLength() (note: Legacy API, only Gecko and WebKit support.)

Differential Revision: https://phabricator.services.mozilla.com/D133434

dom/svg/SVGGeometryElement.cpp

@@ -178,27 +178,31 @@ static Point GetPointFrom(const DOMPointInit& aPoint) {
 }
 
 bool SVGGeometryElement::IsPointInFill(const DOMPointInit& aPoint) {
-  auto point = GetPointFrom(aPoint);
+  // d is a presentation attribute, so make sure style is up to date:
+  FlushStyleIfNeeded();
 
   RefPtr<Path> path = GetOrBuildPathForHitTest();
   if (!path) {
     return false;
   }
 
+  auto point = GetPointFrom(aPoint);
   return path->ContainsPoint(point, {});
 }
 
 bool SVGGeometryElement::IsPointInStroke(const DOMPointInit& aPoint) {
-  auto point = GetPointFrom(aPoint);
+  // stroke-* attributes and the d attribute are presentation attributes, so we
+  // flush the layout before building the path.
+  if (nsCOMPtr<Document> doc = GetComposedDoc()) {
+    doc->FlushPendingNotifications(FlushType::Layout);
+  }
 
   RefPtr<Path> path = GetOrBuildPathForHitTest();
   if (!path) {
     return false;
   }
-  if (nsCOMPtr<Document> doc = GetComposedDoc()) {
-    doc->FlushPendingNotifications(FlushType::Layout);
-  }
 
+  auto point = GetPointFrom(aPoint);
   bool res = false;
   SVGGeometryProperty::DoForComputedStyle(this, [&](const ComputedStyle* s) {
     // Per spec, we should take vector-effect into account.
@@ -224,13 +228,16 @@ bool SVGGeometryElement::IsPointInStroke(const DOMPointInit& aPoint) {
   return res;
 }
 
-float SVGGeometryElement::GetTotalLength() {
+float SVGGeometryElement::GetTotalLengthForBinding() {
-  RefPtr<Path> flat = GetOrBuildPathForMeasuring();
+  // d is a presentation attribute, so make sure style is up to date:
-  return flat ? flat->ComputeLength() : 0.f;
+  FlushStyleIfNeeded();
+  return GetTotalLength();
 }
 
 already_AddRefed<DOMSVGPoint> SVGGeometryElement::GetPointAtLength(
     float distance, ErrorResult& rv) {
+  // d is a presentation attribute, so make sure style is up to date:
+  FlushStyleIfNeeded();
   RefPtr<Path> path = GetOrBuildPathForMeasuring();
   if (!path) {
     rv.ThrowInvalidStateError("No path available for measuring");
@@ -274,5 +281,27 @@ already_AddRefed<DOMSVGAnimatedNumber> SVGGeometryElement::PathLength() {
   return mPathLength.ToDOMAnimatedNumber(this);
 }
 
+float SVGGeometryElement::GetTotalLength() {
+  RefPtr<Path> flat = GetOrBuildPathForMeasuring();
+  return flat ? flat->ComputeLength() : 0.f;
+}
+
+void SVGGeometryElement::FlushStyleIfNeeded() {
+  // Note: we still can set d property on other elements which don't have d
+  // attribute, but we don't look at the d property on them, so here we only
+  // care about the element with d attribute, i.e. SVG path element.
+  if (GetPathDataAttrName() != nsGkAtoms::d ||
+      !StaticPrefs::layout_css_d_property_enabled()) {
+    return;
+  }
+
+  RefPtr<Document> doc = GetComposedDoc();
+  if (!doc) {
+    return;
+  }
+
+  doc->FlushPendingNotifications(FlushType::Style);
+}
+
 }  // namespace dom
 }  // namespace mozilla

dom/svg/SVGGeometryElement.h

@@ -227,22 +227,28 @@ class SVGGeometryElement : public SVGGeometryElementBase {
 
   // WebIDL
   already_AddRefed<DOMSVGAnimatedNumber> PathLength();
-  bool IsPointInFill(const DOMPointInit& aPoint);
+  MOZ_CAN_RUN_SCRIPT bool IsPointInFill(const DOMPointInit& aPoint);
-  bool IsPointInStroke(const DOMPointInit& aPoint);
+  MOZ_CAN_RUN_SCRIPT bool IsPointInStroke(const DOMPointInit& aPoint);
-  float GetTotalLength();
+  MOZ_CAN_RUN_SCRIPT float GetTotalLengthForBinding();
-  already_AddRefed<DOMSVGPoint> GetPointAtLength(float distance,
+  MOZ_CAN_RUN_SCRIPT already_AddRefed<DOMSVGPoint> GetPointAtLength(
-                                                 ErrorResult& rv);
+      float distance, ErrorResult& rv);
 
  protected:
   // SVGElement method
   virtual NumberAttributesInfo GetNumberInfo() override;
 
+  // d is a presentation attribute, so we would like to make sure style is
+  // up-to-date. This function flushes the style if the path attribute is d.
+  MOZ_CAN_RUN_SCRIPT void FlushStyleIfNeeded();
+
   SVGAnimatedNumber mPathLength;
   static NumberInfo sNumberInfo;
   mutable RefPtr<Path> mCachedPath;
 
  private:
   already_AddRefed<Path> GetOrBuildPathForHitTest();
+
+  float GetTotalLength();
 };
 
 }  // namespace dom

dom/svg/SVGPathElement.cpp

@@ -66,9 +66,11 @@ uint32_t SVGPathElement::GetPathSegAtLength(float distance) {
     }
   };
 
-  if (StaticPrefs::layout_css_d_property_enabled() &&
+  if (StaticPrefs::layout_css_d_property_enabled()) {
-      SVGGeometryProperty::DoForComputedStyle(this, callback)) {
+    FlushStyleIfNeeded();
-    return seg;
+    if (SVGGeometryProperty::DoForComputedStyle(this, callback)) {
+      return seg;
+    }
   }
   return mD.GetAnimValue().GetPathSegAtLength(distance);
 }

dom/svg/SVGPathElement.h

@@ -46,12 +46,23 @@ class SVGPathElement final : public SVGPathElementBase {
   virtual bool AttributeDefinesGeometry(const nsAtom* aName) override;
   virtual bool IsMarkable() override;
   virtual void GetMarkPoints(nsTArray<SVGMark>* aMarks) override;
+  /*
+   * Note: This function maps d attribute to CSS d property, and we don't flush
+   * style in this function because some callers don't need it, so if the caller
+   * needs style to be flushed (e.g. DOM APIs), the caller should flush style
+   * before calling this.
+   */
   virtual already_AddRefed<Path> BuildPath(PathBuilder* aBuilder) override;
 
   /**
    * This returns a path without the extra little line segments that
    * ApproximateZeroLengthSubpathSquareCaps can insert if we have square-caps.
    * See the comment for that function for more info on that.
+   *
+   * Note: This function maps d attribute to CSS d property, and we don't flush
+   * style in this function because some callers don't need it, so if the caller
+   * needs style to be flushed (e.g. DOM APIs), the caller should flush style
+   * before calling this.
    */
   virtual already_AddRefed<Path> GetOrBuildPathForMeasuring() override;
 
@@ -68,7 +79,7 @@ class SVGPathElement final : public SVGPathElementBase {
   }
 
   // WebIDL
-  uint32_t GetPathSegAtLength(float distance);
+  MOZ_CAN_RUN_SCRIPT uint32_t GetPathSegAtLength(float distance);
   already_AddRefed<DOMSVGPathSegClosePath> CreateSVGPathSegClosePath();
   already_AddRefed<DOMSVGPathSegMovetoAbs> CreateSVGPathSegMovetoAbs(float x,
                                                                      float y);

dom/svg/crashtests/1715387.html

@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<style>
+svg {
+  width: 10%;
+  height: 10%;
+  background: #eee;
+}
+svg path {
+  fill: none;
+  stroke: #000;
+}
+
+</style>
+<script>
+  function run() {
+    const target = document.createElementNS("http://www.w3.org/2000/svg", "path");
+    const root = document.getElementById('svgroot');
+    root.appendChild(target);
+    target.style.d = 'path("M0,0 L2,2")';
+
+    var m = new MutationObserver(function () {
+      // This will destroy the oringal document.
+      document.write("<html><body></body></html>");
+      SpecialPowers.forceGC();
+      SpecialPowers.forceCC();
+
+      document.documentElement.classList.remove("reftest-wait");
+    });
+
+    m.observe(target, { attributes: true });
+    target.setAttribute("d", "none");
+
+    // Calling these APIs flushes the style, which may run the script in the
+    // callback function above that destroys the composed document.
+    target.getTotalLength();
+    target.getPointAtLength(1);
+    target.isPointInFill({x: 1, y: 1});
+    target.isPointInStroke({x: 1, y: 1});
+    target.getPathSegAtLength(0);
+  }
+</script>
+<body onload="run()">
+<svg viewBox="0 0 20 20" id="svgroot">
+</svg>

dom/svg/crashtests/crashtests.list

@@ -98,3 +98,4 @@ load 1555795.html
 load 1560179.html
 load 1572904.html
 load 1683907.html
+pref(layout.css.d-property.enabled,true) pref(dom.svg.pathSeg.enabled,true) load 1715387.html

dom/svg/test/test_getPathSegListAtLength_with_d_property.html

@@ -42,6 +42,12 @@ test(function() {
 
 test(function() {
   let target = document.getElementById('target1');
+  // Note: in order to make sure we flush style properly, we call
+  // getComputedStyle after setting an initial value first, so if
+  // getPathSegAtLength(5) doesn't flush style, it returns 0.
+  target.style.d = 'none';
+  assert_equals(getComputedStyle(target).d, "none");
+
   target.style.d = 'path("M2,2 h3 v5")';
   assert_equals(target.getPathSegAtLength(5), 2);
 }, "getPathSegAtLength works properly after updating d property");

dom/webidl/SVGGeometryElement.webidl

@@ -18,6 +18,7 @@ interface SVGGeometryElement : SVGGraphicsElement {
   boolean isPointInFill(optional DOMPointInit point = {});
   boolean isPointInStroke(optional DOMPointInit point = {});
 
+  [BinaryName="getTotalLengthForBinding"]
   float getTotalLength();
   [NewObject, Throws]
   SVGPoint getPointAtLength(float distance);

testing/web-platform/tests/svg/path/property/test_style_flush_on_dom_api_with_d_property.html

@@ -0,0 +1,80 @@
+<!DOCTYPE html>
+<title>Test DOM APIs which flush the style properly, with d property</title>
+<meta charset=utf-8>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+
+<style>
+svg {
+  width: 10%;
+  height: 10%;
+  background: #eee;
+}
+svg path {
+  fill: none;
+  stroke: #000;
+}
+</style>
+
+<div id="log"></div>
+
+<svg viewBox="0 0 20 20" id="svgroot"></svg>
+
+<script>
+'use strict';
+
+function createPath(t) {
+  const target = document.createElementNS("http://www.w3.org/2000/svg", "path");
+  document.getElementById('svgroot').appendChild(target);
+
+  if (t && typeof t.add_cleanup === "function") {
+    t.add_cleanup(function() {
+      target.remove();
+    });
+  }
+
+  // Set an initial d and flush style first to make sure the following DOM APIs
+  // do flush again.
+  target.style.d = 'path("M 0 0")';
+  assert_equals(getComputedStyle(target).d, 'path("M 0 0")', "the initial d");
+  return target;
+}
+
+test(function(t) {
+  const target = createPath(t);
+  target.style.d = 'path("M5,5 L10,5 L10,10")';
+  assert_equals(target.getTotalLength(), 10, "the total length");
+}, "getTotalLength() with d property");
+
+test(function(t) {
+  const target = createPath(t);
+  target.style.d = 'path("M5,5 L10,5 L10,10")';
+  // The first segment of path is (5,5) to (10,5), its length is 5.
+  // The second segment of path is (10,5) to (10,10), its length is 5.
+  // So, the length 8 is on the 2nd segment, at (10,8).
+  const point = target.getPointAtLength(8);
+  assert_equals(point.x, 10, "x-axis position");
+  assert_equals(point.y, 8, "y-axis position");
+}, "getPointAtLength() with d property");
+
+test(function(t) {
+  const target = createPath(t);
+  const svgPoint = document.getElementById("svgroot").createSVGPoint();
+  svgPoint.x = 10;
+  svgPoint.y = 8;
+
+  target.style.d = 'path("M5,5 L10,5 L10,10")';
+  assert_equals(target.isPointInFill(svgPoint), true);
+}, "isPointInFill() with d property");
+
+test(function(t) {
+  const target = createPath(t);
+  const svgPoint = document.getElementById("svgroot").createSVGPoint();
+  svgPoint.x = 10;
+  svgPoint.y = 8;
+
+  target.style.d = 'path("M5,5 L10,5 L10,10")';
+  assert_equals(target.isPointInStroke(svgPoint), true);
+}, "isPointInStroke() with d property");
+
+</script>