Add a thread-safety assertion in DEFINE_STATIC_LOCAL

Add a thread-safety assertion in DEFINE_STATIC_LOCAL

Asserts if DEFINE_STATIC_LOCAL's usage is safe:
- Its first call is made before we create other threads, or
- It's called only on a single thread, or
- It's called inside a AtomicallyInitializedStatic

BUG=367982
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=189547

[kinuko, 2015-01-27 09:19:51]

Patchset #2 (id:20001) has been deleted

[kinuko, 2015-01-27 09:19:57]

Patchset #1 (id:1) has been deleted

[kinuko, 2015-01-27 14:42:26]

kinuko@chromium.org changed reviewers:
+ eroman@chromium.org, jochen@chromium.org

[kinuko, 2015-01-27 14:42:33]

Patchset #2 (id:60001) has been deleted

[kinuko, 2015-01-27 14:42:39]

Patchset #1 (id:40001) has been deleted

[kinuko, 2015-01-27 14:50:37]

I'm thinking about adding an assertion like this (based on eroman@'s strawman),
and am fixing unsafe ones that crash with this patch.

Wdyt?

https://codereview.chromium.org/874203002/diff/70001/Source/platform/WebThrea...
File Source/platform/WebThreadSupportingGC.cpp (right):

https://codereview.chromium.org/874203002/diff/70001/Source/platform/WebThrea...
Source/platform/WebThreadSupportingGC.cpp:15: WTF::willCreateThread();
Adding a hook here feels a bit slippery, suggestions are welcome.

[jochen (gone - plz use gerrit), 2015-01-28 19:09:50]

which tests are failing with this?

[kinuko, 2015-01-29 05:42:20]

On 2015/01/28 19:09:50, jochen (slow) wrote:
> which tests are failing with this?

I think I've fixed most, though there're still some:
https://codereview.chromium.org/875053004/

[jochen (gone - plz use gerrit), 2015-01-29 15:16:03]

guess that's a good thing then once it passes everywhere

you should make sure that switching to atomic initialization doesn't affect
performance.

and alternative to using an atomic initializer is invoking the codepath once
before we start threads...

[eroman, 2015-01-30 18:02:31]

lgtm

https://codereview.chromium.org/874203002/diff/90001/Source/wtf/StdLibExtras.h
File Source/wtf/StdLibExtras.h (right):

https://codereview.chromium.org/874203002/diff/90001/Source/wtf/StdLibExtras....
Source/wtf/StdLibExtras.h:64: static StaticLocalVerifier
name##StaticLocalVerifier; \
How about making this static itself threadsafe?

https://codereview.chromium.org/874203002/diff/90001/Source/wtf/ThreadingPthr...
File Source/wtf/ThreadingPthreads.cpp (right):

https://codereview.chromium.org/874203002/diff/90001/Source/wtf/ThreadingPthr...
Source/wtf/ThreadingPthreads.cpp:255: ASSERT(!s_threadCreated);
Not sure how valuable this assertion is. Probably sufficient to just assign
s_threadCreated = true unconditionally as part of willCreateThread.

[kinuko, 2015-02-03 06:57:34]

Eric: Thanks, updated.

Jochen@: now we're getting ready to land this one (win_blink_rel looks a bit
flaky, not sure due to this patch yet). Can you take a look?

https://codereview.chromium.org/874203002/diff/90001/Source/wtf/StdLibExtras.h
File Source/wtf/StdLibExtras.h (right):

https://codereview.chromium.org/874203002/diff/90001/Source/wtf/StdLibExtras....
Source/wtf/StdLibExtras.h:64: static StaticLocalVerifier
name##StaticLocalVerifier; \
On 2015/01/30 18:02:30, eroman wrote:
> How about making this static itself threadsafe?

I wondered if we should do so, but this one's called (initialized) before
initializing thread in some cases. We can work around but I'd like to keep it as
is for simplicity.

https://codereview.chromium.org/874203002/diff/90001/Source/wtf/ThreadingPthr...
File Source/wtf/ThreadingPthreads.cpp (right):

https://codereview.chromium.org/874203002/diff/90001/Source/wtf/ThreadingPthr...
Source/wtf/ThreadingPthreads.cpp:255: ASSERT(!s_threadCreated);
On 2015/01/30 18:02:30, eroman wrote:
> Not sure how valuable this assertion is. Probably sufficient to just assign
> s_threadCreated = true unconditionally as part of willCreateThread.

Done.

[jochen (gone - plz use gerrit), 2015-02-04 12:41:05]

lgtm

https://codereview.chromium.org/874203002/diff/110001/Source/wtf/StdLibExtras.h
File Source/wtf/StdLibExtras.h (right):

https://codereview.chromium.org/874203002/diff/110001/Source/wtf/StdLibExtras...
Source/wtf/StdLibExtras.h:37: public:
add WTF_MAKE_NONCOPYABLE?

[kinuko, 2015-02-05 04:13:45]

New patchsets have been uploaded after l-g-t-m from jochen@chromium.org

[kinuko, 2015-02-05 08:58:25]

Thanks,

https://codereview.chromium.org/874203002/diff/110001/Source/wtf/StdLibExtras.h
File Source/wtf/StdLibExtras.h (right):

https://codereview.chromium.org/874203002/diff/110001/Source/wtf/StdLibExtras...
Source/wtf/StdLibExtras.h:37: public:
On 2015/02/04 12:41:05, jochen (slow) wrote:
> add WTF_MAKE_NONCOPYABLE?

Done.

[kinuko, 2015-02-05 08:58:29]

The CQ bit was checked by kinuko@chromium.org

[commit-bot: I haz the power, 2015-02-05 09:00:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/874203002/130001

[commit-bot: I haz the power, 2015-02-05 10:29:58]

Committed patchset #4 (id:130001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=189547

[kbalazs, 2015-02-11 05:08:47]

b.kelemen@samsung.com changed reviewers:
+ b.kelemen@samsung.com

[kbalazs, 2015-02-11 05:08:47]

https://codereview.chromium.org/874203002/diff/130001/Source/platform/WebThre...
File Source/platform/WebThreadSupportingGC.cpp (right):

https://codereview.chromium.org/874203002/diff/130001/Source/platform/WebThre...
Source/platform/WebThreadSupportingGC.cpp:15: WTF::willCreateThread();
How will people not forget to do this when they happen to add a new thread? Can
it be part of creation of WebThread?

Btw imho it would be nicer to #if guard the body of the function.

[kinuko, 2015-02-12 02:31:23]

https://codereview.chromium.org/874203002/diff/130001/Source/platform/WebThre...
File Source/platform/WebThreadSupportingGC.cpp (right):

https://codereview.chromium.org/874203002/diff/130001/Source/platform/WebThre...
Source/platform/WebThreadSupportingGC.cpp:15: WTF::willCreateThread();
On 2015/02/11 05:08:47, kbalazs wrote:
> How will people not forget to do this when they happen to add a new thread?
Can
> it be part of creation of WebThread?

I assumed that in blink we'll be anyway using WebThreadSupportingGC in most
cases, if not all.

> Btw imho it would be nicer to #if guard the body of the function.

Yup, might be. Feel free to create a patch :)