Ensure Origin Thread Affinity for BlameContext

base/trace_event/blame_context.cc

Index: base/trace_event/blame_context.cc
diff --git a/base/trace_event/blame_context.cc b/base/trace_event/blame_context.cc
index 27d2d2eb7509c2b57f161aa5263c2787cc337a3c..93355aaf52908f9b4ad5da0e78960293d7acd9de 100644
--- a/base/trace_event/blame_context.cc
+++ b/base/trace_event/blame_context.cc
@@ -5,6 +5,7 @@
 #include "base/trace_event/blame_context.h"
 
 #include "base/strings/stringprintf.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/trace_event/trace_event.h"
 #include "base/trace_event/trace_event_argument.h"
 
@@ -24,7 +25,9 @@ BlameContext::BlameContext(const char* category,
       id_(id),
       parent_scope_(parent_context ? parent_context->scope() : nullptr),
       parent_id_(parent_context ? parent_context->id() : 0),
-      category_group_enabled_(nullptr) {
+      category_group_enabled_(nullptr),
+      task_runner_(ThreadTaskRunnerHandle::Get()),
+      weak_factory_(this) {
   DCHECK(!parent_context || !std::strcmp(name_, parent_context->name()))
       << "Parent blame context must have the same name";
 }
@@ -54,6 +57,7 @@ void BlameContext::Leave() {
 }
 
 void BlameContext::TakeSnapshot() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(WasInitialized());
   if (!*category_group_enabled_)
     return;
@@ -71,9 +75,13 @@ void BlameContext::TakeSnapshot() {
                                   arg_values, TRACE_EVENT_FLAG_HAS_ID);
 }
 
+// Called by TraceLog from a non-main thread. Since access to TraceLog's list

[Sami, 2016/04/26 11:00:32]

I'd suggest talking about an "origin" thread (i.e., the thread an object was
created on) instead of a specific main thread. It might be perfectly reasonable
to have a blame context on the file thread for example.

This would also involve changing the below to

if (task_runner_->BelongsToCurrentThread() {
  TakeSnapshot();
} else {
  PostTask(...);
}

[Xiaocheng, 2016/04/26 12:48:21]

Yeah, that's a better idea.

+// of observers is lock-guarded, we can use task_runner_ and weak_factory_ from
+// a different thread safely without worrying about threading issues.

[Sami, 2016/04/26 11:00:32]

nit: task_runner_ is thread safe, as is getting a weak pointer from
weak_factory_ (while not dereferencing it).

[Xiaocheng, 2016/04/26 12:48:21]

Sorry for the ambiguity.

I just want to say that it is safe for the trace log to access members
(task_runner_ and weak_factory_) of a BlameContext via a raw pointer.

Since (almost) every access to TraceLog::enabled_state_observer_list_ is
lock-guarded, the above thread safety claim should be true. The only exception
is in the deletion of TraceLog. Can I simply assume that TraceLog is always
deleted later than any BlameContext (since TraceLog is a singleton)?

[Sami, 2016/04/26 12:57:12]

I see. When a BlameContext is deleted, it removes itself from the TraceLog's
observer list and any in-flight task to TakeSnapshot becomes a no-op. Conversely
after a TraceLog is deleted, we won't have a call to OnTraceLogEnabled in the
first place. So that is just a long way of saying that this should be safe :)

 void BlameContext::OnTraceLogEnabled() {
   DCHECK(WasInitialized());
-  TakeSnapshot();
+  task_runner_->PostTask(FROM_HERE, Bind(&BlameContext::TakeSnapshot,
+                                         weak_factory_.GetWeakPtr()));
 }
 
 void BlameContext::OnTraceLogDisabled() {}