commit acd2f5cddc631d5008a04da483d630968df307c9
author fmalita <fmalita@chromium.org> Tue Nov 08 07:13:45 2016 -0800
committer Commit bot <commit-bot@chromium.org> Tue Nov 08 07:13:45 2016 -0800
tree c0e57e4d5c07d2762f2d6bebe096a80be51e3866
parent 9075634d60cc8ce979e3007d4ba1d0e57afc6644

Fix DM race in SVGSrc

The SVG source currently attempts to build the DOM lazily, in response
to draw() or size() calls.  But apprently DM may call these
concurrently.

Build the DOM in the constructor instead, to avoid races (and the need
for synchronization).

R=robertphillips@google.com,mtklein@google.com
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2482123003

Review-Url: https://codereview.chromium.org/2482123003

dm/DMSrcSink.cpp

diff --git a/dm/DMSrcSink.cpp b/dm/DMSrcSink.cpp
index 8bd80e3..2c5ee16 100644
--- a/dm/DMSrcSink.cpp
+++ b/dm/DMSrcSink.cpp
@@ -1098,46 +1098,43 @@
 // Used to force-scale tiny fixed-size images.
 static const SkSize kMinimumSVGSize = SkSize::Make(128, 128);
 
-SVGSrc::SVGSrc(Path path) : fPath(path), fScale(1) {}
+SVGSrc::SVGSrc(Path path)
+    : fName(SkOSPath::Basename(path.c_str()))
+    , fScale(1) {
 
-Error SVGSrc::ensureDom() const {
+  SkFILEStream stream(path.c_str());
+  if (!stream.isValid()) {
+      return;
+  }
+  fDom = SkSVGDOM::MakeFromStream(stream);
+  if (!fDom) {
+      return;
+  }
+
+  const SkSize& sz = fDom->containerSize();
+  if (sz.isEmpty()) {
+      // no intrinsic size
+      fDom->setContainerSize(kDefaultSVGSize);
+  } else {
+      fScale = SkTMax(1.f, SkTMax(kMinimumSVGSize.width()  / sz.width(),
+                                  kMinimumSVGSize.height() / sz.height()));
+  }
+}
+
+Error SVGSrc::draw(SkCanvas* canvas) const {
     if (!fDom) {
-        SkFILEStream stream(fPath.c_str());
-        if (!stream.isValid()) {
-            return SkStringPrintf("Unable to open file: %s", fPath.c_str());
-        }
-        fDom = SkSVGDOM::MakeFromStream(stream);
-        if (!fDom) {
-            return SkStringPrintf("Unable to parse file: %s", fPath.c_str());
-        }
-
-        const SkSize& sz = fDom->containerSize();
-        if (sz.isEmpty()) {
-            // no intrinsic size
-            fDom->setContainerSize(kDefaultSVGSize);
-        } else {
-            fScale = SkTMax(1.f, SkTMax(kMinimumSVGSize.width()  / sz.width(),
-                                        kMinimumSVGSize.height() / sz.height()));
-        }
+        return SkStringPrintf("Unable to parse file: %s", fName.c_str());
     }
 
+    SkAutoCanvasRestore acr(canvas, true);
+    canvas->scale(fScale, fScale);
+    fDom->render(canvas);
+
     return "";
 }
 
-Error SVGSrc::draw(SkCanvas* canvas) const {
-    Error err = this->ensureDom();
-    if (err.isEmpty()) {
-        SkAutoCanvasRestore acr(canvas, true);
-        canvas->scale(fScale, fScale);
-        fDom->render(canvas);
-    }
-
-    return err;
-}
-
 SkISize SVGSrc::size() const {
-    Error err = this->ensureDom();
-    if (!err.isEmpty()) {
+    if (!fDom) {
         return SkISize::Make(0, 0);
     }
 
@@ -1145,7 +1142,7 @@
                         fDom->containerSize().height() * fScale).toRound();
 }
 
-Name SVGSrc::name() const { return SkOSPath::Basename(fPath.c_str()); }
+Name SVGSrc::name() const { return fName; }
 
 bool SVGSrc::veto(SinkFlags flags) const {
     // No need to test to non-(raster||gpu) or indirect backends.