Fix thread unsafe mutex initialization.

BUG=skia:2779
R=robertphillips@google.com, mtklein@google.com, reed@android.com, bsalomon@google.com

Author: bungeman@google.com

Review URL: https://codereview.chromium.org/419113002

diff --git a/bench/MutexBench.cpp b/bench/MutexBench.cpp
index 67648b5decf31bbf257eba39f495c00e1d4dbf2d..59f054c32120e8df8f9000d3d8b76b5114e6a7e1 100644 (file)
--- a/bench/MutexBench.cpp
+++ b/bench/MutexBench.cpp
@@ -19,7 +19,7 @@ protected:
     }
 
     virtual void onDraw(const int loops, SkCanvas*) {
-        SK_DECLARE_STATIC_MUTEX(mu);
+        SkMutex mu;
         for (int i = 0; i < loops; i++) {
             mu.acquire();
             mu.release();

diff --git a/include/core/SkInstCnt.h b/include/core/SkInstCnt.h
index 1839ee1dbdc6720d9c6b0870ea10e52dbe901c7a..e4b43d130b94b6a2ab9303c3c6c1170f7c3722ff 100644 (file)
--- a/include/core/SkInstCnt.h
+++ b/include/core/SkInstCnt.h
@@ -73,9 +73,14 @@ extern bool gPrintInstCount;
             return gChildren;                                               \
         }                                                                   \
                                                                             \
+        static void create_mutex(SkMutex** mutex) {                         \
+            *mutex = SkNEW(SkMutex);                                        \
+        }                                                                   \
         static SkBaseMutex& GetChildrenMutex() {                            \
-            SK_DECLARE_STATIC_MUTEX(childrenMutex);                         \
-            return childrenMutex;                                           \
+            static SkMutex* childrenMutex;                                  \
+            SK_DECLARE_STATIC_ONCE(once);                                   \
+            SkOnce(&once, className::SkInstanceCountHelper::create_mutex, &childrenMutex);\
+            return *childrenMutex;                                          \
         }                                                                   \
                                                                             \
     } fInstanceCountHelper;                                                 \

diff --git a/include/core/SkThread.h b/include/core/SkThread.h
index c8f13a709263f92736c0bebcac1687691cd838ce..bcbc43754f69e986eaadfbb3195bfbbff8fef7a6 100644 (file)
--- a/include/core/SkThread.h
+++ b/include/core/SkThread.h
@@ -96,7 +96,6 @@ public:
 };
 
 #define SK_DECLARE_STATIC_MUTEX(name) static SkBaseMutex name = ...
-#define SK_DECLARE_GLOBAL_MUTEX(name) SkBaseMutex name = ...
 */
 
 #include SK_MUTEX_PLATFORM_H

diff --git a/include/gpu/GrFontScaler.h b/include/gpu/GrFontScaler.h
index d90708de8f1f74703f9f55fd38d909f344cb870a..037603813561bd59f2997235e4f6383422b61825 100644 (file)
--- a/include/gpu/GrFontScaler.h
+++ b/include/gpu/GrFontScaler.h
@@ -21,7 +21,7 @@ class SkPath;
  */
 class GrFontDescKey : public SkRefCnt {
 public:
-    SK_DECLARE_INST_COUNT(SkGrDescKey)
+    SK_DECLARE_INST_COUNT(GrFontDescKey)
     
     typedef uint32_t Hash;
     

diff --git a/src/core/SkTypeface.cpp b/src/core/SkTypeface.cpp
index 48be651fd6a8e80c5467d86c5320067d45fe40a1..01b534c95fa4adb28f216b4818e4ef9c42b957cd 100644 (file)
--- a/src/core/SkTypeface.cpp
+++ b/src/core/SkTypeface.cpp
@@ -76,13 +76,13 @@ protected:
     }
 };
 
+SK_DECLARE_STATIC_MUTEX(gCreateDefaultMutex);
 SkTypeface* SkTypeface::CreateDefault(int style) {
     // If backed by fontconfig, it's not safe to call SkFontHost::CreateTypeface concurrently.
     // To be safe, we serialize here with a mutex so only one call to
     // CreateTypeface is happening at any given time.
     // TODO(bungeman, mtklein): This is sad.  Make our fontconfig code safe?
-    SK_DECLARE_STATIC_MUTEX(mutex);
-    SkAutoMutexAcquire lock(&mutex);
+    SkAutoMutexAcquire lock(&gCreateDefaultMutex);
 
     SkTypeface* t = SkFontHost::CreateTypeface(NULL, NULL, (Style)style);
     return t ? t : SkEmptyTypeface::Create();

diff --git a/src/effects/gradients/SkGradientShader.cpp b/src/effects/gradients/SkGradientShader.cpp
index 5014c35ee0e647ce65b989513c59f480d554c9d9..48904fa8fd6c83ce06be296f8d7f1d8e2fc10e0a 100644 (file)
--- a/src/effects/gradients/SkGradientShader.cpp
+++ b/src/effects/gradients/SkGradientShader.cpp
@@ -570,6 +570,7 @@ SkGradientShaderBase::GradientShaderCache* SkGradientShaderBase::refCache(U8CPU
     return fCache;
 }
 
+SK_DECLARE_STATIC_MUTEX(gGradientCacheMutex);
 /*
  *  Because our caller might rebuild the same (logically the same) gradient
  *  over and over, we'd like to return exactly the same "bitmap" if possible,
@@ -605,11 +606,10 @@ void SkGradientShaderBase::getGradientTableBitmap(SkBitmap* bitmap) const {
 
     ///////////////////////////////////
 
-    SK_DECLARE_STATIC_MUTEX(gMutex);
     static SkBitmapCache* gCache;
     // each cache cost 1K of RAM, since each bitmap will be 1x256 at 32bpp
     static const int MAX_NUM_CACHED_GRADIENT_BITMAPS = 32;
-    SkAutoMutexAcquire ama(gMutex);
+    SkAutoMutexAcquire ama(gGradientCacheMutex);
 
     if (NULL == gCache) {
         gCache = SkNEW_ARGS(SkBitmapCache, (MAX_NUM_CACHED_GRADIENT_BITMAPS));

diff --git a/src/gpu/GrDrawTargetCaps.h b/src/gpu/GrDrawTargetCaps.h
index dcea79e0915af95ae15d805effb9dd48e021a9b6..b979b534c40528d083f24feec908fae5c714b812 100644 (file)
--- a/src/gpu/GrDrawTargetCaps.h
+++ b/src/gpu/GrDrawTargetCaps.h
@@ -17,7 +17,7 @@
  */
 class GrDrawTargetCaps : public SkRefCnt {
 public:
-    SK_DECLARE_INST_COUNT(Caps)
+    SK_DECLARE_INST_COUNT(GrDrawTargetCaps)
 
     GrDrawTargetCaps() { this->reset(); }
     GrDrawTargetCaps(const GrDrawTargetCaps& other) : INHERITED() { *this = other; }

diff --git a/src/pdf/SkPDFFont.cpp b/src/pdf/SkPDFFont.cpp
index 5779431a563a933b43704d9c89dd48200bbe55f0..652b56d706683d26a7f848e5ffafbc2b851ddd41 100644 (file)
--- a/src/pdf/SkPDFFont.cpp
+++ b/src/pdf/SkPDFFont.cpp
@@ -892,9 +892,9 @@ SkTDArray<SkPDFFont::FontRec>& SkPDFFont::CanonicalFonts() {
     return gCanonicalFonts;
 }
 
+SK_DECLARE_STATIC_MUTEX(gCanonicalFontsMutex);
 // static
 SkBaseMutex& SkPDFFont::CanonicalFontsMutex() {
-    SK_DECLARE_STATIC_MUTEX(gCanonicalFontsMutex);
     return gCanonicalFontsMutex;
 }
 

diff --git a/src/pdf/SkPDFGraphicState.cpp b/src/pdf/SkPDFGraphicState.cpp
index fa3baaf95e3bb9b5e1e48e25933585b0cb8cdebe..bd8afe0684365fe64059df1f69cb530cd96254c5 100644 (file)
--- a/src/pdf/SkPDFGraphicState.cpp
+++ b/src/pdf/SkPDFGraphicState.cpp
@@ -88,9 +88,9 @@ SkTDArray<SkPDFGraphicState::GSCanonicalEntry>& SkPDFGraphicState::CanonicalPain
     return gCanonicalPaints;
 }
 
+SK_DECLARE_STATIC_MUTEX(gCanonicalPaintsMutex);
 // static
 SkBaseMutex& SkPDFGraphicState::CanonicalPaintsMutex() {
-    SK_DECLARE_STATIC_MUTEX(gCanonicalPaintsMutex);
     return gCanonicalPaintsMutex;
 }
 

diff --git a/src/pdf/SkPDFShader.cpp b/src/pdf/SkPDFShader.cpp
index 23bd203f9d064cb6ecb069cfff220e15d42b5768..afe6bb06b33704139ca2cde0991a91c13e22c45b 100644 (file)
--- a/src/pdf/SkPDFShader.cpp
+++ b/src/pdf/SkPDFShader.cpp
@@ -664,9 +664,9 @@ SkTDArray<SkPDFShader::ShaderCanonicalEntry>& SkPDFShader::CanonicalShaders() {
     return gCanonicalShaders;
 }
 
+SK_DECLARE_STATIC_MUTEX(gCanonicalShadersMutex);
 // static
 SkBaseMutex& SkPDFShader::CanonicalShadersMutex() {
-    SK_DECLARE_STATIC_MUTEX(gCanonicalShadersMutex);
     return gCanonicalShadersMutex;
 }
 

diff --git a/src/ports/SkFontConfigInterface_android.cpp b/src/ports/SkFontConfigInterface_android.cpp
index 1323a62d7a5f37489100eb8ade2a5d6f34c9dd58..94662f5d5949844ec9a77fee69dcfd924c73f1ff 100644 (file)
--- a/src/ports/SkFontConfigInterface_android.cpp
+++ b/src/ports/SkFontConfigInterface_android.cpp
@@ -132,11 +132,11 @@ private:
 
 ///////////////////////////////////////////////////////////////////////////////
 
+SK_DECLARE_STATIC_MUTEX(gGetSingletonInterfaceMutex);
 static SkFontConfigInterfaceAndroid* getSingletonInterface() {
-    SK_DECLARE_STATIC_MUTEX(gMutex);
     static SkFontConfigInterfaceAndroid* gFontConfigInterface;
 
-    SkAutoMutexAcquire ac(gMutex);
+    SkAutoMutexAcquire ac(gGetSingletonInterfaceMutex);
     if (NULL == gFontConfigInterface) {
         // load info from a configuration file that we can use to populate the
         // system/fallback font structures

diff --git a/src/ports/SkFontHost_mac.cpp b/src/ports/SkFontHost_mac.cpp
index a8e1b5793e604cca91f4fdbda1bf6dd8a27731ed..2d985f94638c07a980aeed23e4af552c7bcf00e3 100755 (executable)
--- a/src/ports/SkFontHost_mac.cpp
+++ b/src/ports/SkFontHost_mac.cpp
@@ -526,9 +526,9 @@ static SkTypeface* NewFromName(const char familyName[], SkTypeface::Style theSty
     return ctFont ? NewFromFontRef(ctFont, familyName, false) : NULL;
 }
 
+SK_DECLARE_STATIC_MUTEX(gGetDefaultFaceMutex);
 static SkTypeface* GetDefaultFace() {
-    SK_DECLARE_STATIC_MUTEX(gMutex);
-    SkAutoMutexAcquire ma(gMutex);
+    SkAutoMutexAcquire ma(gGetDefaultFaceMutex);
 
     static SkTypeface* gDefaultFace;
 

diff --git a/src/ports/SkMutex_pthread.h b/src/ports/SkMutex_pthread.h
index f71016b31e88a28b3cc1932a2e4bf1d0f0c93abd..3bf3628588cd3b66b085325697fb98f61008ae2d 100644 (file)
--- a/src/ports/SkMutex_pthread.h
+++ b/src/ports/SkMutex_pthread.h
@@ -89,9 +89,11 @@ private:
 #define SK_BASE_MUTEX_INIT { PTHREAD_MUTEX_INITIALIZER, SkDEBUGCODE(0) }
 
 // Using POD-style initialization prevents the generation of a static initializer.
-#define SK_DECLARE_STATIC_MUTEX(name) static SkBaseMutex name = SK_BASE_MUTEX_INIT
-
-// Special case used when the static mutex must be available globally.
-#define SK_DECLARE_GLOBAL_MUTEX(name) SkBaseMutex name = SK_BASE_MUTEX_INIT
+// Without magic statics there are no thread safety guarantees on initialization
+// of local statics (even POD).
+// As a result, it is illegal to SK_DECLARE_STATIC_MUTEX in a function.
+#define SK_DECLARE_STATIC_MUTEX(name) \
+    static inline void SK_MACRO_APPEND_LINE(name)(){} \
+    static SkBaseMutex name = SK_BASE_MUTEX_INIT
 
 #endif

diff --git a/src/ports/SkMutex_win.h b/src/ports/SkMutex_win.h
index d84b0e42fd0da5fe0607544c6453001283d62a03..ccad063c2796c6b9ffea899ff6aa35971499e6ee 100644 (file)
--- a/src/ports/SkMutex_win.h
+++ b/src/ports/SkMutex_win.h
@@ -73,7 +73,9 @@ private:
 class SkMutex : public SkBaseMutex { };
 
 // Windows currently provides no documented means of POD initializing a CRITICAL_SECTION.
-#define SK_DECLARE_STATIC_MUTEX(name) static SkBaseMutex name
-#define SK_DECLARE_GLOBAL_MUTEX(name) SkBaseMutex name
+// As a result, it is illegal to SK_DECLARE_STATIC_MUTEX in a function.
+#define SK_DECLARE_STATIC_MUTEX(name) \
+    static inline void SK_MACRO_APPEND_LINE(name)(){} \
+    static SkBaseMutex name
 
 #endif

diff --git a/tests/PathOpsExtendedTest.cpp b/tests/PathOpsExtendedTest.cpp
index 2d741261a6dc9b4de60a703bee5efad5b2ea5403..05d00045b97e7d3d5b72800b05c5cd8f3ceb147e 100644 (file)
--- a/tests/PathOpsExtendedTest.cpp
+++ b/tests/PathOpsExtendedTest.cpp
@@ -316,6 +316,8 @@ void ShowTestArray() {
     }
 }
 
+SK_DECLARE_STATIC_MUTEX(compareDebugOut3);
+SK_DECLARE_STATIC_MUTEX(compareDebugOut4);
 static int comparePaths(skiatest::Reporter* reporter, const char* testName, const SkPath& one,
         const SkPath& scaledOne, const SkPath& two, const SkPath& scaledTwo, SkBitmap& bitmap,
         const SkPath& a, const SkPath& b, const SkPathOp shapeOp, const SkMatrix& scale) {
@@ -329,13 +331,11 @@ static int comparePaths(skiatest::Reporter* reporter, const char* testName, cons
     }
     const int MAX_ERRORS = 8;
     if (errors2x2 > MAX_ERRORS && gComparePathsAssert) {
-        SK_DECLARE_STATIC_MUTEX(compareDebugOut3);
         SkAutoMutexAcquire autoM(compareDebugOut3);
         SkDebugf("\n*** this test fails ***\n");
         showPathOpPath(testName, one, two, a, b, scaledOne, scaledTwo, shapeOp, scale);
         REPORTER_ASSERT(reporter, 0);
     } else if (gShowPath || errors2x2 == MAX_ERRORS || errors2x2 == MAX_ERRORS - 1) {
-        SK_DECLARE_STATIC_MUTEX(compareDebugOut4);
         SkAutoMutexAcquire autoM(compareDebugOut4);
         showPathOpPath(testName, one, two, a, b, scaledOne, scaledTwo, shapeOp, scale);
     }
@@ -402,6 +402,7 @@ static void outputToStream(const char* pathStr, const char* pathPrefix, const ch
     outFile.flush();
 }
 
+SK_DECLARE_STATIC_MUTEX(simplifyDebugOut);
 bool testSimplify(SkPath& path, bool useXor, SkPath& out, PathOpsThreadState& state,
                   const char* pathStr) {
     SkPath::FillType fillType = useXor ? SkPath::kEvenOdd_FillType : SkPath::kWinding_FillType;
@@ -421,7 +422,6 @@ bool testSimplify(SkPath& path, bool useXor, SkPath& out, PathOpsThreadState& st
     }
     int result = comparePaths(state.fReporter, NULL, path, out, *state.fBitmap);
     if (result && gPathStrAssert) {
-        SK_DECLARE_STATIC_MUTEX(simplifyDebugOut);
         SkAutoMutexAcquire autoM(simplifyDebugOut);
         char temp[8192];
         sk_bzero(temp, sizeof(temp));