Access FileTracing::g_provider atomically to prevent data races

base/files/file_tracing.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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 {
 FileTracing::Provider* g_provider = nullptr;
 }
 
 // static
 bool FileTracing::IsCategoryEnabled() {
   return g_provider && g_provider->FileTracingCategoryIsEnabled();
 }
 
 // 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() {
   if (g_provider)
     g_provider->FileTracingEnable(this);
 }
 
 FileTracing::ScopedEnabler::~ScopedEnabler() {
   if (g_provider)
     g_provider->FileTracingDisable(this);
 }
 
 FileTracing::ScopedTrace::ScopedTrace() : id_(nullptr) {}
 
 FileTracing::ScopedTrace::~ScopedTrace() {
   if (id_ && g_provider)
     g_provider->FileTracingEventEnd(name_, id_);
 }
 
 void FileTracing::ScopedTrace::Initialize(const char* name,
                                           File* file,
                                           int64_t size) {
   id_ = &file->trace_enabler_;
   name_ = name;
   g_provider->FileTracingEventBegin(name_, id_, file->tracing_path_, size);
 }
 
 }  // namespace base