Ensure Origin Thread Affinity for BlameContext

base/trace_event/blame_context.cc

 // Copyright 2016 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/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"
 
 namespace base {
 namespace trace_event {
 
 BlameContext::BlameContext(const char* category,
                            const char* name,
                            const char* type,
                            const char* scope,
                            int64_t id,
                            const BlameContext* parent_context)
     : category_(category),
       name_(name),
       type_(type),
       scope_(scope),
       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";
 }
 
 BlameContext::~BlameContext() {
   DCHECK(WasInitialized());
   TRACE_EVENT_API_ADD_TRACE_EVENT(
       TRACE_EVENT_PHASE_DELETE_OBJECT, category_group_enabled_, type_, scope_,
       id_, 0, nullptr, nullptr, nullptr, nullptr, TRACE_EVENT_FLAG_HAS_ID);
   trace_event::TraceLog::GetInstance()->RemoveEnabledStateObserver(this);
 }
 
 void BlameContext::Enter() {
   DCHECK(WasInitialized());
   TRACE_EVENT_API_ADD_TRACE_EVENT(TRACE_EVENT_PHASE_ENTER_CONTEXT,
                                   category_group_enabled_, name_, scope_, id_,
                                   0 /* num_args */, nullptr, nullptr, nullptr,
                                   nullptr, TRACE_EVENT_FLAG_HAS_ID);
 }
 
 void BlameContext::Leave() {
   DCHECK(WasInitialized());
   TRACE_EVENT_API_ADD_TRACE_EVENT(TRACE_EVENT_PHASE_LEAVE_CONTEXT,
                                   category_group_enabled_, name_, scope_, id_,
                                   0 /* num_args */, nullptr, nullptr, nullptr,
                                   nullptr, TRACE_EVENT_FLAG_HAS_ID);
 }
 
 void BlameContext::TakeSnapshot() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(WasInitialized());
   if (!*category_group_enabled_)
     return;
   std::unique_ptr<trace_event::TracedValue> snapshot(
       new trace_event::TracedValue);
   AsValueInto(snapshot.get());
   static const char* kArgName = "snapshot";
   const int kNumArgs = 1;
   unsigned char arg_types[1] = {TRACE_VALUE_TYPE_CONVERTABLE};
   std::unique_ptr<trace_event::ConvertableToTraceFormat> arg_values[1] = {
       std::move(snapshot)};
   TRACE_EVENT_API_ADD_TRACE_EVENT(TRACE_EVENT_PHASE_SNAPSHOT_OBJECT,
                                   category_group_enabled_, type_, scope_, id_,
                                   kNumArgs, &kArgName, arg_types, nullptr,
                                   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() {}
 
 void BlameContext::AsValueInto(trace_event::TracedValue* state) {
   DCHECK(WasInitialized());
   if (!parent_id_)
     return;
   state->BeginDictionary("parent");
   state->SetString("id_ref", StringPrintf("0x%" PRIx64, parent_id_));
@@ skipping 10 matching lines @@
   trace_event::TraceLog::GetInstance()->AddEnabledStateObserver(this);
   TakeSnapshot();
 }
 
 bool BlameContext::WasInitialized() const {
   return category_group_enabled_ != nullptr;
 }
 
 }  // namespace trace_event
 }  // namespace base