commit 175f588831c3d9f76349af8c27c64ceaa4460871
author Brian Salomon <bsalomon@google.com> Fri May 12 12:02:50 2017 -0400
committer Skia Commit-Bot <skia-commit-bot@chromium.org> Fri May 12 16:49:07 2017 +0000
tree fd7733ec3dcc8caae5b4aa732ea1e665b643d909
parent ebb1b5c297e394ab19f99d807095672b7f5d8aef

Modify SkEventTracer::SetInstance to fail rather than assert if a tracer has already been installed.

Chromium usually calls SetInstance once per process. However, when run in single process more renderer threads will try to set the instance after the browser process already has done so. This allows them to fail gracefully without asserting.

Bug: skia:6603
Change-Id: Ic8a35422d787335aa67eefc07d0658f0fbe73db4
Reviewed-on: https://skia-review.googlesource.com/16664
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Mike Klein <mtklein@chromium.org>

include/utils/SkEventTracer.h

diff --git a/include/utils/SkEventTracer.h b/include/utils/SkEventTracer.h
index f4f8676..a0289ce 100644
--- a/include/utils/SkEventTracer.h
+++ b/include/utils/SkEventTracer.h
@@ -27,9 +27,18 @@
 
     typedef uint64_t Handle;
 
-    static SkEventTracer* GetInstance();
+    /**
+     * If this is the first call to SetInstance or GetInstance then the passed instance is
+     * installed and true is returned. Otherwise, false is returned. In either case ownership of the
+     * tracer is transferred and it will be deleted when no longer needed.
+     */
+    static bool SetInstance(SkEventTracer*);
 
-    static void SetInstance(SkEventTracer*);
+    /**
+     * Gets the event tracer. If this is the first call to SetInstance or GetIntance then a default
+     * event tracer is installed and returned.
+     */
+    static SkEventTracer* GetInstance();
 
     virtual ~SkEventTracer() { }
 

src/utils/SkEventTracer.cpp

diff --git a/src/utils/SkEventTracer.cpp b/src/utils/SkEventTracer.cpp
index 0a748d1..5fb60bd 100644
--- a/src/utils/SkEventTracer.cpp
+++ b/src/utils/SkEventTracer.cpp
@@ -42,11 +42,15 @@
 // We prefer gUserTracer if it's been set, otherwise we fall back on a default tracer;
 static SkEventTracer* gUserTracer = nullptr;
 
-void SkEventTracer::SetInstance(SkEventTracer* tracer) {
-    SkASSERT(nullptr == sk_atomic_load(&gUserTracer, sk_memory_order_acquire));
-    sk_atomic_store(&gUserTracer, tracer, sk_memory_order_release);
+bool SkEventTracer::SetInstance(SkEventTracer* tracer) {
+    SkEventTracer* expected = nullptr;
+    if (!sk_atomic_compare_exchange(&gUserTracer, &expected, tracer)) {
+        delete tracer;
+        return false;
+    }
     // An atomic load during process shutdown is probably overkill, but safe overkill.
-    atexit([]() { delete sk_atomic_load(&gUserTracer, sk_memory_order_acquire); });
+    atexit([]() { delete sk_atomic_load(&gUserTracer); });
+    return true;
 }
 
 SkEventTracer* SkEventTracer::GetInstance() {

tools/viewer/Viewer.cpp

diff --git a/tools/viewer/Viewer.cpp b/tools/viewer/Viewer.cpp
index 7cf8cd8..a1c0b5b 100644
--- a/tools/viewer/Viewer.cpp
+++ b/tools/viewer/Viewer.cpp
@@ -285,7 +285,7 @@
 #endif
 
     if (FLAGS_atrace) {
-        SkEventTracer::SetInstance(new SkATrace());
+        SkAssertResult(SkEventTracer::SetInstance(new SkATrace()));
     }
 
     fBackendType = get_backend_type(FLAGS_backend[0]);