Introduce browser side FrameTreeNodeBlameContext

content/browser/frame_host/frame_tree_node_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 "content/browser/frame_host/frame_tree_node_blame_context.h"
+
+#include "base/strings/stringprintf.h"
+#include "base/trace_event/trace_event_argument.h"
+#include "content/browser/frame_host/frame_tree.h"
+#include "url/gurl.h"
+
+namespace content {
+
+const char kFrameTreeNodeBlameContextCategory[] = "navigation";
+const char kFrameTreeNodeBlameContextName[] = "FrameTreeNode";
+const char kFrameTreeNodeBlameContextType[] = "Frame";
+const char kFrameTreeNodeBlameContextScope[] = "FrameTreeNode";
+
+FrameTreeNodeBlameContext::FrameTreeNodeBlameContext(FrameTreeNode* node)
+    : base::trace_event::BlameContext(kFrameTreeNodeBlameContextCategory,
+                                      kFrameTreeNodeBlameContextName,
+                                      kFrameTreeNodeBlameContextType,
+                                      kFrameTreeNodeBlameContextScope,
+                                      node->frame_tree_node_id(),
+                                      node->parent()
+                                        ? node->parent()->blame_context()
+                                        : nullptr) {
+  UpdateArguments(node);
+}
+
+FrameTreeNodeBlameContext::~FrameTreeNodeBlameContext() {}
+
+void FrameTreeNodeBlameContext::ClearArguments() {
+  process_id_ = -1;
+  routing_id_ = -1;
+  url_.clear();
+}
+
+void FrameTreeNodeBlameContext::UpdateArguments(FrameTreeNode* node) {
+  RenderFrameHostImpl* current_frame_host = node->current_frame_host();
+  if (!current_frame_host) {
+    ClearArguments();
+    return;
+  }
+
+  // On Windows, |rph->GetHandle()| does not duplicate ownership of the
+  // process handle and the render host still retains it. Therefore, we
+  // cannot create a base::Process object, which provides a proper way to get
+  // a process id, from the handle. For a stopgap, we use this deprecated
+  // function that does not require the ownership (http://crbug.com/417532).
+  process_id_ = base::GetProcId(current_frame_host->GetProcess()->GetHandle());
+
+  routing_id_ = current_frame_host->GetRoutingID();
+  DCHECK_NE(routing_id_, MSG_ROUTING_NONE);
+
+  if (current_frame_host->last_committed_url().is_valid())
+    url_ = current_frame_host->last_committed_url().spec();
+  else
+    url_.clear();
+}
+
+void FrameTreeNodeBlameContext::AsValueInto(

[benjhayden, 2016/04/20 01:37:34]

Hm, it looks like there might be a gotcha here: this method may be called on
either the UI or the FILE thread, depending on the caller.
OnTraceLogEnabled/Disabled can be called on the FILE thread, iirc.

As it is, this doesn't look like a problem, but it definitely smells odd to me.
Most of the rest of chrome can usually rely on being called on one particular
thread. FrameTree and friends all live on the UI thread, so you can rely on the
invariant that their methods are always called on the UI thread and their
members are only accessed on the UI thread. Chrome devs are accustomed to
working under that single-thread invariant per-object or at least per-method.
Without it, data races and other threading bugs can creep in.

It's definitely neat how UpdateArguments lets you buffer the args in a
long-lived object so you don't need to worry about it. But it might be too neat
if it lets you play fast and loose with threads.

Now that I look through some of the StartTracing callers, it looks like some of
them might be on the UI thread, but not all. So perhaps the gotcha is there.
Maybe BlameContext should use a WeakPtrFactory to guarantee that AsValueInto is
always called on a specified thread? Or maybe YAGNI?

Sorry I'm not more helpful. nduca is on vacation, so maybe skyostil can help?

[Xiaocheng, 2016/04/20 02:01:05]

Thanks for pointing out the gotcha and let me think about it...

[Sami, 2016/04/20 12:15:08]

IMO the most straightforward way to avoid threading issues here is to save
copies of all necessary fields on the right thread so they can be snapshotted on
an arbitrary thread. Frames are created and modified rarely (relatively
speaking) so this should not be any measurable performance overhead. 

You might still need a lock if the snapshot values need to change (e.g., url).
Another option in that case would be to create a fresh BlameContext but that
might complicate things if we want to Enter/Leave these contexts in the future.

[benjhayden, 2016/04/20 17:29:45]

If re-creating the BlameContext when the url changes can complicate using
Enter/Leave (which seems like a logical way to attribute RenderFrame, etc. tasks
to the frame, which would simplify finding LoadExpectations), then should
BlameContext support changing the parent_id without re-creation?

Is there a reason not to make TraceLog::EnabledStateObserver run on a single
thread so that clients such as BlameContexts don't need to worry about locks and
WeakPtrFactories? Maybe that's a job for TracingV2 or the fixit?

[Xiaocheng, 2016/04/21 08:01:31]

To me, adding a lock seems to be the most painless way to ensure thread safety,
as it's easy to implement and does not introduce any architectural change. I
agree that there shouldn't be any measurable performance change with locks since
navigation is rare.

I've added a documentation stating explicitly that thread safety is achieved by
locking, so that anyone interested in eliminating blocking calls can find and
fix it.

+    base::trace_event::TracedValue* value) {
+  BlameContext::AsValueInto(value);
+
+  if (process_id_ >= 0) {
+    value->BeginDictionary("RenderFrame");
+    value->SetInteger("pid_ref", process_id_);
+    value->SetString("id_ref", base::StringPrintf("0x%x", routing_id_));
+    value->SetString("scope", "RenderFrame");
+    value->EndDictionary();
+  }
+
+  if (!url_.empty())
+    value->SetString("url", url_);
+}
+
+}  // content