Access FileTracing::g_provider atomically to prevent data races

base/files/file_tracing.cc

Index: base/files/file_tracing.cc
diff --git a/base/files/file_tracing.cc b/base/files/file_tracing.cc
index 6d11cbc746ed268a357e0d2ba08d463c501e5d96..c909569e692e8bf5c2567b515e321740c38547f1 100644
--- a/base/files/file_tracing.cc
+++ b/base/files/file_tracing.cc
@@ -4,8 +4,11 @@
 
 #include "base/files/file_tracing.h"
 
+#include "base/atomicops.h"
 #include "base/files/file.h"
 
+using base::subtle::AtomicWord;

[danakj, 2016/07/07 22:40:36]

nit: you could put this in the method that uses it.

[Dan Beam, 2016/07/07 23:13:48]

Done.

+
 namespace base {
 
 namespace {
@@ -19,7 +22,8 @@ bool FileTracing::IsCategoryEnabled() {
 
 // static
 void FileTracing::SetProvider(FileTracing::Provider* provider) {
-  g_provider = provider;
+  base::subtle::NoBarrier_Store(reinterpret_cast<AtomicWord*>(&g_provider),

[danakj, 2016/07/07 22:40:36]

I think the way most people do this would be with a static
base::LazyInstance<base::Lock> beside the g_provider. And that would be easier
to understand/verify for sure, and avoids subtle, so yay. I *think* this is
right though (I haven't actually reviewed/used NoBarrier_Store before, fun!),
because you want to ensure the pointer is set, but you don't care if another
thread set it at the same timeish, and that you get their value?

But the call in
https://codereview.chromium.org/1072133006/diff/810001/content/browser/tracin...
is setting it to a newly malloced thing. If multiple threads are doing this,
they are setting it to different values. Do you care which one gets used in all
the places that use g_provider below? Why/not?

[Dan Beam, 2016/07/07 23:13:48]

g_provider should only be set once. ever. i can verify that with a CHECK()
maybe.

[danakj, 2016/07/07 23:21:04]

Ah, so it's just threads are reading null.. reading null.. reading half-set
pointer..

A DCHECK might be nice sure :) CHECK is probably overkill.

Why don't you want a memory barrier so other threads pick up the value you set
here? using lock lets us not ask these questions.

+                                reinterpret_cast<AtomicWord>(provider));
 }
 
 FileTracing::ScopedEnabler::ScopedEnabler() {