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 67648b5..59f054c 100644
--- a/bench/MutexBench.cpp
+++ b/bench/MutexBench.cpp
@@ -19,7 +19,7 @@
}
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 1839ee1..e4b43d1 100644
--- a/include/core/SkInstCnt.h
+++ b/include/core/SkInstCnt.h
@@ -73,9 +73,14 @@
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 c8f13a7..bcbc437 100644
--- a/include/core/SkThread.h
+++ b/include/core/SkThread.h
@@ -96,7 +96,6 @@
};
#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 d90708d..0376038 100644
--- a/include/gpu/GrFontScaler.h
+++ b/include/gpu/GrFontScaler.h
@@ -21,7 +21,7 @@
*/
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 48be651..01b534c 100644
--- a/src/core/SkTypeface.cpp
+++ b/src/core/SkTypeface.cpp
@@ -76,13 +76,13 @@
}
};
+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 5014c35..48904fa 100644
--- a/src/effects/gradients/SkGradientShader.cpp
+++ b/src/effects/gradients/SkGradientShader.cpp
@@ -570,6 +570,7 @@
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 @@
///////////////////////////////////
- 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 dcea79e..b979b53 100644
--- 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 5779431..652b56d 100644
--- a/src/pdf/SkPDFFont.cpp
+++ b/src/pdf/SkPDFFont.cpp
@@ -892,9 +892,9 @@
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 fa3baaf..bd8afe0 100644
--- a/src/pdf/SkPDFGraphicState.cpp
+++ b/src/pdf/SkPDFGraphicState.cpp
@@ -88,9 +88,9 @@
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 23bd203..afe6bb0 100644
--- a/src/pdf/SkPDFShader.cpp
+++ b/src/pdf/SkPDFShader.cpp
@@ -664,9 +664,9 @@
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 1323a62..94662f5 100644
--- a/src/ports/SkFontConfigInterface_android.cpp
+++ b/src/ports/SkFontConfigInterface_android.cpp
@@ -132,11 +132,11 @@
///////////////////////////////////////////////////////////////////////////////
+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 a8e1b57..2d985f9 100755
--- a/src/ports/SkFontHost_mac.cpp
+++ b/src/ports/SkFontHost_mac.cpp
@@ -526,9 +526,9 @@
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 f71016b..3bf3628 100644
--- a/src/ports/SkMutex_pthread.h
+++ b/src/ports/SkMutex_pthread.h
@@ -89,9 +89,11 @@
#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 d84b0e4..ccad063 100644
--- a/src/ports/SkMutex_win.h
+++ b/src/ports/SkMutex_win.h
@@ -73,7 +73,9 @@
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 2d74126..05d0004 100644
--- a/tests/PathOpsExtendedTest.cpp
+++ b/tests/PathOpsExtendedTest.cpp
@@ -316,6 +316,8 @@
}
}
+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 @@
}
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 @@
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 @@
}
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));