Trace FrameTreeNode Snapshots

content/browser/frame_host/traced_frame_tree.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 "content/browser/frame_host/traced_frame_tree.h"
+
+#include "base/json/json_writer.h"
+#include "content/browser/frame_host/frame_tree.h"
+
+namespace content {
+
+namespace {
+
+base::DictionaryValue* GetFrameTreeNodeAsValue(const FrameTreeNode& node) {
+  base::DictionaryValue* value = new base::DictionaryValue();
+  value->SetString("url", node.current_url().spec());
+  value->SetInteger("ftid", node.frame_tree_node_id());

[nasko, 2015/10/13 23:01:56]

nit: "ftnid", since this is a node, not the full tree.

[benjhayden, 2015/10/22 17:24:21]

Done.

+  RenderFrameHostManager* render_manager = node.render_manager();
+  RenderFrameHostImpl* frame_host = (

[clamy, 2015/10/13 11:48:17]

The problem is that two of them can be non-null at the same time, and we should
probably keep track of both of them. That would require having two sets of
pid/rid.

In PlzNavigate case (aka kBrowserSideNavigation) we are interested in the
speculative and the current RFH. Otherwise, we are interested in the pending and
the current RFH.

[nasko, 2015/10/13 23:01:56]

I would suggest checking for the command line flag and using the one that
matches the intended use, i.e. only use the speculative in PlzNavigate.

[benjhayden, 2015/10/22 17:24:21]

Done.

+      render_manager->speculative_frame_host() ||
+      render_manager->pending_frame_host() ||
+      render_manager->current_frame_host());
+  if (frame_host) {
+    value->SetInteger("pid", frame_host->GetProcess()->GetID());
+    value->SetInteger("rid", frame_host->GetRoutingID());
+  }
+
+  if (node.child_count() != 0) {
+    scoped_ptr<base::ListValue> subframes(new base::ListValue());
+    for (size_t i = 0; i < node.child_count(); ++i) {
+      subframes->Append(GetFrameTreeNodeAsValue(*node.child_at(i)));
+    }
+    value->Set("subframes", subframes.Pass());
+  }
+  return value;
+}
+
+}  // namespace
+
+scoped_refptr<TracedFrameTree> TracedFrameTree::Create(const FrameTree& tree) {
+  return scoped_refptr<TracedFrameTree>(new TracedFrameTree(tree));
+}
+
+TracedFrameTree::TracedFrameTree(const FrameTree& tree)
+    : value_(GetFrameTreeNodeAsValue(*tree.root())) {

[nasko, 2015/10/13 23:01:56]

I would avoid doing a lot of work in the constructor. Since you already have a
::Create method, I'd suggest initializing the object with a very simple
constructor and calling the GetFrameTreeNodeAsValue after that.

[benjhayden, 2015/10/22 17:24:21]

I assume "a lot of work" refers to the Value-ification and not the recursion, so
I made the constructor recursively build a tree of TracedFrameTree objects with
a string and some int fields. Then AppendAsTraceFormat() uses the new AsValue()
method to recursively Value-ify the tree at trace dump time.
Is that right?

[nasko, 2015/10/23 15:52:04]

I meant actually everything. Sorry if I wasn't clear. The constructor should
just create a simple object and the work to traverse the FrameTree and generate
JSON should be performed at the time when it is needed.

[benjhayden, 2015/10/23 20:31:33]

I made TracedFrameTree store only the reference to the FrameTree, and then
recursively Value-ify it in AppendAsTraceFormat(). Apologies if that still isn't
what you're thinking.
If that is what you're thinking, then it looks like there might be a problem
with that approach.

If the FrameTree changes between this snapshot and when the trace is dumped,
then only the last state of the FrameTree will be traced for each of the
snapshots. All of the snapshots will only reflect the last state of the
FrameTree.
I recorded a trace of opening a new tab to confirm.
If I navigate away from the new tab page, then Chrome segfaults. Is the
"https://www.google.com/webhp?sourceid=chrome-instant&ion=1&espv=2&ie=UTF-8"
frame destroyed quickly?

Maybe there's another option I'm not seeing, but it seems to me like the
FrameTreeNode fields need to be copied recursively at snapshot time, and not at
trace dump time, because the FrameTreeNodes might change between the two times.

+}
+
+TracedFrameTree::~TracedFrameTree() {
+}
+
+void TracedFrameTree::AppendAsTraceFormat(std::string* out) const {
+  std::string tmp;
+  base::JSONWriter::Write(*value_, &tmp);
+  *out += tmp;
+}
+
+}  // content