Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1390053002:
Trace FrameTreeNode Snapshots (Closed)
|
Created:
5 years, 2 months ago by benjhayden Modified:
4 years, 9 months ago CC:
carlosk, chromium-reviews, creis+watch_chromium.org, Charlie Harrison, darin-cc_chromium.org, jam, lpy, nasko+codewatch_chromium.org, nduca Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTrace FrameTreeNode Snapshots
This will provide a single ground-truth for frame structure in tracing.
FrameTreeNodes are snapshot when they are created or destroyed,
when tracing starts, and when a frame's url changes.
Each snapshot contains the frame's url,
a reference to its parent (unless it's a root),
and a cross-process scoped reference to the current RenderFrame.
The snapshots are traced at their frame_tree_node_id, not their memory address.
The RenderFrames are traced at their routing_ids, not their memory address,
so that FrameTreeNodes can use cross-process references to them.
Pending and speculative RenderFrames are not supported. If a specific
use case is found for them, then cross-process scoped references
will be added to TracedFrameTreeNode for them.
The RenderFrame references will allow correlating frame tree structure
and FrameTreeNode objects with LocalFrame Trace Contexts.
The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the
snapshot is recorded, not when the trace is dumped to JSON, because the
underlying FrameTree might change between when the snapshot is recorded
and when the trace is dumped to JSON.
RenderFrames and FrameTreeNodes are snapshot when they are created
so that there's always a snapshot for the tracing model to find when
it patches up the references.
The catapult side of this change is https://codereview.chromium.org/1736373002
The RenderFrame object snapshots that are referenced by the FrameTreeNode snapshots
are in Sami's Frameblamr CL:
https://codereview.chromium.org/1447563002/
BUG=537802
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/ff37bf3297e8cbe14c6a865e528673f0814c7da1
Cr-Commit-Position: refs/heads/master@{#379623}
Patch Set 1 : #
Total comments: 14
Patch Set 2 : comments #Patch Set 3 : comments #Patch Set 4 : comments #
Total comments: 12
Patch Set 5 : TracedFrameTreeNode #Patch Set 6 : ScopedVector<scoped_refptr> #Patch Set 7 : DidNavigate #Patch Set 8 : non-copied FrameTree #
Total comments: 6
Patch Set 9 : rewrite #Patch Set 10 : rebase #Patch Set 11 : RenderFrameHostManager::CommitPendingIfNecessary #Patch Set 12 : processId, routingId, children #Patch Set 13 : TraceSnapshot only in FrameTree #
Total comments: 3
Patch Set 14 : TracedFrameTreeNode #Patch Set 15 : . #Patch Set 16 : . #Patch Set 17 : rebase #Patch Set 18 : scoped_ptr #
Total comments: 14
Patch Set 19 : comments #Patch Set 20 : comments #Patch Set 21 : GetProcId windows handle #
Total comments: 2
Patch Set 22 : fix segfaults #
Total comments: 3
Patch Set 23 : fix windows #Patch Set 24 : fix posix #Patch Set 25 : Process(rph.GetHandle()) is broken, using GetProcId #
Messages
Total messages: 93 (40 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
benjhayden@chromium.org changed reviewers: + nduca@chromium.org
I'll send this to an owner for full review if you confirm that it's part of what you want for frame trees.
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:60001) has been deleted
benjhayden@chromium.org changed reviewers: + clamy@chromium.org, nasko@chromium.org - nduca@chromium.org
PTAL, clamy@ for ownership, and nasko@ for tracing context, though it says he might be slow to respond. Thanks!
Thanks! Could you add a link to a bug in your CL description? Also, IIUC, you want to correlate a FTN id with a RFH id, however your patch: A) does not track the pending/speculative RFH. B) does not take a trace when a new RFH is committed in the FTN. A means it will be harder to link the navigation events from the pending RFH to a FTN id before commit. I think taking the trace when the url change may mitigate B. If that's the case, it should be a conscious and documented choice not to record a trace on RFH commit. Apart form that, a few comments. https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... File content/browser/frame_host/frame_tree.h (right): https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree.h:149: void TraceSnapshot() const; This function should have a comment. https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... File content/browser/frame_host/traced_frame_tree.cc (right): https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.cc:14: base::DictionaryValue* getFrameTreeNodeAsValue(const FrameTreeNode& node) { s/getFrameTreeNodeAsValue/GetFrameTreeNodeAsValue https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.cc:18: RenderFrameHostImpl* frame_host = node.current_frame_host(); Should we also care about the pending_render_frame_host? (And the speculative in PlzNavigate?). https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.cc:34: } Add "// namespace" after the {. https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.cc:53: } Add "// content" after the {. https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... File content/browser/frame_host/traced_frame_tree.h (right): https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.h:14: class CONTENT_EXPORT TracedFrameTree : It would also be nice to have a comment to explain what that class is doing :). https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.h:30: } Add "// content" after the { (see example for an unnamed namespace: https://www.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces).
Is this the right way to trace the pending/speculative frame hosts? Is this the right way to trace when the RFH is committed? Thanks! https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... File content/browser/frame_host/frame_tree.h (right): https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_tree.h:149: void TraceSnapshot() const; On 2015/10/12 at 09:31:05, clamy wrote: > This function should have a comment. Done. https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... File content/browser/frame_host/traced_frame_tree.cc (right): https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.cc:14: base::DictionaryValue* getFrameTreeNodeAsValue(const FrameTreeNode& node) { On 2015/10/12 at 09:31:06, clamy wrote: > s/getFrameTreeNodeAsValue/GetFrameTreeNodeAsValue Done. https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.cc:18: RenderFrameHostImpl* frame_host = node.current_frame_host(); On 2015/10/12 at 09:31:06, clamy wrote: > Should we also care about the pending_render_frame_host? (And the speculative in PlzNavigate?). I'm not sure. I admit I've only skimmed go/plznavigate and a few other docs, and I haven't yet written the code that will use these trees, so there's a lot that I'm still learning and figuring out. I changed this to speculative || pending || current, but I can imagine some ways that that might not be right. Is that the right way to get the frame_host? https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.cc:34: } On 2015/10/12 at 09:31:06, clamy wrote: > Add "// namespace" after the {. Done. https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.cc:53: } On 2015/10/12 at 09:31:06, clamy wrote: > Add "// content" after the {. Done. https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... File content/browser/frame_host/traced_frame_tree.h (right): https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.h:14: class CONTENT_EXPORT TracedFrameTree : On 2015/10/12 at 09:31:06, clamy wrote: > It would also be nice to have a comment to explain what that class is doing :). Done. https://codereview.chromium.org/1390053002/diff/80001/content/browser/frame_h... content/browser/frame_host/traced_frame_tree.h:30: } On 2015/10/12 at 09:31:06, clamy wrote: > Add "// content" after the { (see example for an unnamed namespace: https://www.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces). Done.
Thanks! A few comments on tracing the pending/speculative RFH. Alos, if you remove the # from the bug number, code review generates a link to the crbug issue (nicer :). https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:925: frame_tree_->TraceSnapshot(); I would rather have this execute inside NavigatorImpl::DidNavigate. This way we can have it not executed if the renderer did not actually navigate (eg in page-navigations). https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree.cc (right): https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:19: RenderFrameHostImpl* frame_host = ( 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.
https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree.cc (right): https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:17: value->SetInteger("ftid", node.frame_tree_node_id()); nit: "ftnid", since this is a node, not the full tree. https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:19: RenderFrameHostImpl* frame_host = ( On 2015/10/13 11:48:17, clamy wrote: > 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. 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. https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:45: : value_(GetFrameTreeNodeAsValue(*tree.root())) { 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.
Description was changed from ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Whenever a FrameTreeNode is added or removed, a snapshot of the entire FrameTree is traced. There's a separate FrameTree object for each tab. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=#537802 ========== to ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Whenever a FrameTreeNode is added or removed, a snapshot of the entire FrameTree is traced. There's a separate FrameTree object for each tab. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ==========
Apologies for the delay, I'm back from a brief paternity leave. Thanks for your comments! I made a couple assumptions, so please correct me. https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:925: frame_tree_->TraceSnapshot(); On 2015/10/13 at 11:48:17, clamy wrote: > I would rather have this execute inside NavigatorImpl::DidNavigate. This way we can have it not executed if the renderer did not actually navigate (eg in page-navigations). I put it after the !did_navigate early return, but there's a comment saying not to put anything else there, so I'm not sure if I did it right. https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree.cc (right): https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:17: value->SetInteger("ftid", node.frame_tree_node_id()); On 2015/10/13 at 23:01:56, nasko (slow to review) wrote: > nit: "ftnid", since this is a node, not the full tree. Done. https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:19: RenderFrameHostImpl* frame_host = ( On 2015/10/13 at 11:48:17, clamy wrote: > 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. Done. https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:45: : value_(GetFrameTreeNodeAsValue(*tree.root())) { On 2015/10/13 at 23:01:56, nasko (slow to review) wrote: > 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. 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?
https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/render_frame_host_impl.cc:925: frame_tree_->TraceSnapshot(); On 2015/10/22 17:24:21, benjhayden_chromium wrote: > On 2015/10/13 at 11:48:17, clamy wrote: > > I would rather have this execute inside NavigatorImpl::DidNavigate. This way > we can have it not executed if the renderer did not actually navigate (eg in > page-navigations). > > I put it after the !did_navigate early return, but there's a comment saying not > to put anything else there, so I'm not sure if I did it right. Now that I think about this. Why do we need to hook this into the FrameTree itself? Why can't the tracer just be a WebContentsObserver, which tracks RenderFrameCreated/RenderFrameDeleted/DidFinishNavigation? If we go that route, it will be possible to even dump trees of nested WebContents, which is going to be even more awesome for cases of Chrome apps using <webview>, once we move it to out-of-process iframes. https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree.cc (right): https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:45: : value_(GetFrameTreeNodeAsValue(*tree.root())) { On 2015/10/22 17:24:21, benjhayden_chromium wrote: > On 2015/10/13 at 23:01:56, nasko (slow to review) wrote: > > 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. > > 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? 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.
https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree.cc (right): https://codereview.chromium.org/1390053002/diff/140001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:45: : value_(GetFrameTreeNodeAsValue(*tree.root())) { On 2015/10/23 at 15:52:04, nasko (slow to review) wrote: > On 2015/10/22 17:24:21, benjhayden_chromium wrote: > > On 2015/10/13 at 23:01:56, nasko (slow to review) wrote: > > > 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. > > > > 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? > > 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. 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.
Thanks! https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:497: render_frame_host->frame_tree_node()->frame_tree()->TraceSnapshot(); I think this should move to RenderFrameHostManager::CommitPendingIfNecessary when we commit or destroy the pending/speculative RFH. It would also be interesting to trace the FrameTree when we create/destroy either of those RFH in RFHM::GetFrameHostForNavigation and RFHM::UpdateStateForNavigate. Another option would be to trace the FrameTree when a RFH is created/destroyed (but we generally try to avoid doing a lot of work in the constructor/destructor).
The tracer code structure looks good now, but I do have some extra comments. https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:497: render_frame_host->frame_tree_node()->frame_tree()->TraceSnapshot(); On 2015/10/26 14:45:13, clamy wrote: > I think this should move to RenderFrameHostManager::CommitPendingIfNecessary > when we commit or destroy the pending/speculative RFH. It would also be > interesting to trace the FrameTree when we create/destroy either of those RFH in > RFHM::GetFrameHostForNavigation and RFHM::UpdateStateForNavigate. I would rather not trace in RFHM's method that allocate RFHs that aren't committed yet. This exposes a level of detail that only few devs in Chrome understand and is rarely something people care about. > Another option would be to trace the FrameTree when a RFH is created/destroyed > (but we generally try to avoid doing a lot of work in the > constructor/destructor). This will allow the concept of pending/speculative RFH to leak out, wouldn't it? I'm against leaking this information. https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree.cc (right): https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:19: value->SetInteger("ftid", node->frame_tree_node_id()); "ftnid" or even "ftn_id" or "ftnId", depending on which style is used in the consumer of this JSON value. https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:25: value->SetInteger("cpid", current_frame_host->GetProcess()->GetID()); Why not "procId" and "routeId"? I find those a lot more readable and meaningful. The "c" for current doesn't make much difference, as this isn't something people outside of this code should care about. https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:29: if (base::CommandLine::ForCurrentProcess()->HasSwitch( I do not think the pending and speculative RFHs need to be included. I would suggest removing this code unless there is a very compelling reason do have it.
https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree.cc (right): https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:29: if (base::CommandLine::ForCurrentProcess()->HasSwitch( On 2015/10/26 15:59:23, nasko (slow to review) wrote: > I do not think the pending and speculative RFHs need to be included. I would > suggest removing this code unless there is a very compelling reason do have it. I think it depends on what this code is supposed to be used for. If we want precise association between RFH events and FTNs, ignoring the speculative/pending RFH will mean that a bunch of events such as DidStartProvisionalLoad may not get associated with the right FTN. It would be nice to have more details about the planned use case for the tracing of FrameTrees.
Description was changed from ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Whenever a FrameTreeNode is added or removed, a snapshot of the entire FrameTree is traced. There's a separate FrameTree object for each tab. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ========== to ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The perf insights infrastructure will be the eventual consumer of this data: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of the entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. There's a separate FrameTree object for each tab. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ==========
Description was changed from ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The perf insights infrastructure will be the eventual consumer of this data: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of the entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. There's a separate FrameTree object for each tab. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ========== to ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of the entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. There's a separate FrameTree object for each tab. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ==========
On 2015/10/26 at 17:54:03, clamy wrote: > https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... > File content/browser/frame_host/traced_frame_tree.cc (right): > > https://codereview.chromium.org/1390053002/diff/220001/content/browser/frame_... > content/browser/frame_host/traced_frame_tree.cc:29: if (base::CommandLine::ForCurrentProcess()->HasSwitch( > On 2015/10/26 15:59:23, nasko (slow to review) wrote: > > I do not think the pending and speculative RFHs need to be included. I would > > suggest removing this code unless there is a very compelling reason do have it. > > I think it depends on what this code is supposed to be used for. If we want precise association between RFH events and FTNs, ignoring the speculative/pending RFH will mean that a bunch of events such as DidStartProvisionalLoad may not get associated with the right FTN. It would be nice to have more details about the planned use case for the tracing of FrameTrees. The routing id on RenderFrameImpl trace events will be used directly to collect them. As long as a navigation eventually commits during a trace, then RenderFrameImpl events like didStartProvisionalLoad will be able to be matched up with a FrameTreeNode through the LoadInteractionRecord in tracing. If a use case is eventually found for navigations that don't eventually commit during a trace, or another use case that does require the speculative/pending RFH, then it will be added then. I updated the CL description, but let me know if it's still lacking any context. I'll address the other comments before PTAL-ing. Thanks!
Description was changed from ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of the entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. There's a separate FrameTree object for each tab. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ========== to ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render frame host's process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of the entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. There's a separate FrameTree object for each tab. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ==========
Description was changed from ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render frame host's process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of the entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. There's a separate FrameTree object for each tab. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ========== to ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render frame host's process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of its entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. There's a separate FrameTree object for each tab. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ==========
PTAL Now that pending/speculative frame host ids are not traced, are there any TraceSnapshot() calls required outside of FrameTree?
The CL description can be fixed here and there, but nothing too big. The one item that should probably be skipped is "There's a separate FrameTree object for each tab.". Each WebContents has a FrameTree and lots of things are WebContents, tabs being one of them. This sentence doesn't bring any more clarity than needed and adds potentially confusing statement. Otherwise, code LGTM with one note. https://codereview.chromium.org/1390053002/diff/320001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1390053002/diff/320001/content/browser/frame_... content/browser/frame_host/frame_tree_node.cc:195: frame_tree_->TraceSnapshot(); Keep in mind that since the tracing here, it will snapshot every time there is a pushState/popState/ref fragment change. If those are not desired, the check needs to move back to Navigator.
Description was changed from ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render frame host's process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of its entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. There's a separate FrameTree object for each tab. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ========== to ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render frame host's process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of its entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ==========
Removed the confusing sentence. Thanks! https://codereview.chromium.org/1390053002/diff/320001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1390053002/diff/320001/content/browser/frame_... content/browser/frame_host/frame_tree_node.cc:195: frame_tree_->TraceSnapshot(); On 2015/10/28 at 20:17:55, nasko (slow to review) wrote: > Keep in mind that since the tracing here, it will snapshot every time there is a pushState/popState/ref fragment change. If those are not desired, the check needs to move back to Navigator. Good to know! If it turns out that these snapshots significantly affect trace file size, then I'll investigate how to capture fewer unnecessary snapshots, starting here.
Camille?
Thnaks! Lgtm with one comment. https://codereview.chromium.org/1390053002/diff/320001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree.cc (right): https://codereview.chromium.org/1390053002/diff/320001/content/browser/frame_... content/browser/frame_host/traced_frame_tree.cc:23: if (current_frame_host) { We should always have a current RFH, so I think we can just DCHECK here.
Is this going to land at some point?
On 2015/12/14 at 17:13:01, nasko wrote: > Is this going to land at some point? Sorry, maybe. We've been working on some things related to this and want to see how they all fit together first. skyostil has been working on a renderer-side version of this with blink LocalFrames and WebViewImpls instead of FrameTreeNodes: https://codereview.chromium.org/1447563002 chiniforooshan has been working on a way for one process to refer to trace objects in another process: https://docs.google.com/document/d/1e7vTbGW9EvCyMKwor7v4Q9G_5ahAjiCxp1x_pr4NQ... carlosk has been working on navigation metrics that could use these FrameTree snapshot's frame_tree_node_ids. So, for example, TracedFrameTree could change to trace each TracedFrameTreeNode separately, with its id set to the frame_tree_node_id instead of a memory address. TracedFrameTreeNode could contain a cross-process idref to a new RenderFrame trace object in the renderer process whose id is its routing_id instead of a memory address. The RenderFrame could contain an idref to one of skyostil's new LocalFrame objects, which can be used as a TraceContext to find ThreadSlices associated with that LocalFrame. That way, a metric such as carlosk's navigation metrics could start with a browser-side FrameTreeNode and follow pointers all the way to EventSets of ThreadSlices that are associated with that navigation/load, or start with any ThreadSlice in the renderer and follow pointers all the way back up to the browser-side FrameTreeNode. Some of those pointers would be automatically wired up by chiniforooshan's identifiers, instead of requiring custom code to manually match up routing ids. Or there might be a better way of connecting everything together.
On 2015/12/14 18:19:08, benjhayden_chromium wrote: > On 2015/12/14 at 17:13:01, nasko wrote: > > Is this going to land at some point? > > Sorry, maybe. > > We've been working on some things related to this and want to see how they all > fit together first. > > skyostil has been working on a renderer-side version of this with blink > LocalFrames and WebViewImpls instead of FrameTreeNodes: > https://codereview.chromium.org/1447563002 > > chiniforooshan has been working on a way for one process to refer to trace > objects in another process: > https://docs.google.com/document/d/1e7vTbGW9EvCyMKwor7v4Q9G_5ahAjiCxp1x_pr4NQ... > > carlosk has been working on navigation metrics that could use these FrameTree > snapshot's frame_tree_node_ids. > > So, for example, TracedFrameTree could change to trace each TracedFrameTreeNode > separately, with its id set to the frame_tree_node_id instead of a memory > address. TracedFrameTreeNode could contain a cross-process idref to a new > RenderFrame trace object in the renderer process whose id is its routing_id > instead of a memory address. The RenderFrame could contain an idref to one of > skyostil's new LocalFrame objects, which can be used as a TraceContext to find > ThreadSlices associated with that LocalFrame. > That way, a metric such as carlosk's navigation metrics could start with a > browser-side FrameTreeNode and follow pointers all the way to EventSets of > ThreadSlices that are associated with that navigation/load, or start with any > ThreadSlice in the renderer and follow pointers all the way back up to the > browser-side FrameTreeNode. Some of those pointers would be automatically wired > up by chiniforooshan's identifiers, instead of requiring custom code to manually > match up routing ids. > > Or there might be a better way of connecting everything together. Memory addresses are pretty bad identifiers as they can be reallocated to different object at a later point in time. This is common event and we shouldn't rely on uniqueness of pointers for objects.
Description was changed from ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render frame host's process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of its entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 ========== to ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render frame host's process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of its entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by benjhayden@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1390053002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1390053002/340001
Description was changed from ========== Trace FrameTree Snapshots This will allow tracing to analyze frame structure and correlate routing_ids from RenderFrameImpl trace events with frame_tree_node_ids from other trace events, and possibly other ids in the future. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports Each FrameTreeNode has a url, render process id, routing id, and frame tree node id. Only the current render frame host's process id and routing id are traced. If a specific use case for the pending/speculative frame host is found, then its process id and routing id will be added then. Whenever a FrameTreeNode is added or removed, a snapshot of its entire FrameTree is traced. The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. A followup CL will snapshot all FrameTrees when tracing starts, so tracing will always have a current FrameTree. BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, its parent frame_tree_node_id (unless it's a root), and a cross-process scoped reference to the current RenderFrame. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The snapshots are traced at their frame_tree_node_id, not their memory address. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, its parent frame_tree_node_id (unless it's a root), and a cross-process scoped reference to the current RenderFrame. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The snapshots are traced at their frame_tree_node_id, not their memory address. The first direct tracing consumer of the traced FrameTree will be the Load Interaction Record: http://go/loadevents The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, its parent frame_tree_node_id (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, its parent frame_tree_node_id (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The perf insights infrastructure will be the eventual consumer of the traced FrameTrees and data derived from them: http://go/performance-insights-reports The data for each FrameTreeNode is copied to a TracedFrameTree when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, a reference to its parent (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. The RenderFrames are traced at their routing_ids, not their memory address, so that FrameTreeNodes can use cross-process references to them. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, a reference to its parent (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. The RenderFrames are traced at their routing_ids, not their memory address, so that FrameTreeNodes can use cross-process references to them. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, a reference to its parent (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. The RenderFrames are traced at their routing_ids, not their memory address, so that FrameTreeNodes can use cross-process references to them. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. RenderFrames and FrameTreeNodes are snapshot when they are created so that there's always a snapshot for the tracing model to find when it patches up the references. BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, a reference to its parent (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. The RenderFrames are traced at their routing_ids, not their memory address, so that FrameTreeNodes can use cross-process references to them. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. RenderFrames and FrameTreeNodes are snapshot when they are created so that there's always a snapshot for the tracing model to find when it patches up the references. BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, a reference to its parent (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. The RenderFrames are traced at their routing_ids, not their memory address, so that FrameTreeNodes can use cross-process references to them. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. RenderFrames and FrameTreeNodes are snapshot when they are created so that there's always a snapshot for the tracing model to find when it patches up the references. The catapult side of this change is https://codereview.chromium.org/1736373002 BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by benjhayden@chromium.org to run a CQ dry run
benjhayden@chromium.org changed reviewers: + skyostil@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1390053002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1390053002/380001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
Description was changed from ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, a reference to its parent (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. The RenderFrames are traced at their routing_ids, not their memory address, so that FrameTreeNodes can use cross-process references to them. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. RenderFrames and FrameTreeNodes are snapshot when they are created so that there's always a snapshot for the tracing model to find when it patches up the references. The catapult side of this change is https://codereview.chromium.org/1736373002 BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, a reference to its parent (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. The RenderFrames are traced at their routing_ids, not their memory address, so that FrameTreeNodes can use cross-process references to them. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. RenderFrames and FrameTreeNodes are snapshot when they are created so that there's always a snapshot for the tracing model to find when it patches up the references. The catapult side of this change is https://codereview.chromium.org/1736373002 The RenderFrame object snapshots that are referenced by the FrameTreeNode snapshots are in Sami's Frameblamr CL: https://codereview.chromium.org/1447563002/ BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, a reference to its parent (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. The RenderFrames are traced at their routing_ids, not their memory address, so that FrameTreeNodes can use cross-process references to them. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. RenderFrames and FrameTreeNodes are snapshot when they are created so that there's always a snapshot for the tracing model to find when it patches up the references. The catapult side of this change is https://codereview.chromium.org/1736373002 The RenderFrame object snapshots that are referenced by the FrameTreeNode snapshots are in Sami's Frameblamr CL: https://codereview.chromium.org/1447563002/ BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure in tracing. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, a reference to its parent (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. The RenderFrames are traced at their routing_ids, not their memory address, so that FrameTreeNodes can use cross-process references to them. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. RenderFrames and FrameTreeNodes are snapshot when they are created so that there's always a snapshot for the tracing model to find when it patches up the references. The catapult side of this change is https://codereview.chromium.org/1736373002 The RenderFrame object snapshots that are referenced by the FrameTreeNode snapshots are in Sami's Frameblamr CL: https://codereview.chromium.org/1447563002/ BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by benjhayden@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1390053002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1390053002/400001
PTAL! Apologies for the long delay. I had to rewrite this CL, but I think we worked through all the issues. (Famous last words.)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
A few minor things. https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree_node.cc (right): https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. Same here, 2016. https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.cc:26: if (current_frame_host) { There should always be a current_frame_host. https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.cc:45: if (routingId_ >= 0) { routing_id != MSG_ROUTING_NONE is more correct comparison. On the other hand, this should never be the case, so at most put a DCHECK in the constructor to ensure the routing id is not NONE. https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree_node.h (right): https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. It has been an year already : ), 2016. https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.h:28: int parentNodeId_; Class members use hacker_case_.
https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/frame_tree_node.cc:192: TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID( Should we move this to a helper method to avoid repeating it three times? https://codereview.chromium.org/1390053002/diff/420001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1390053002/diff/420001/content/renderer/rende... content/renderer/render_frame_impl.cc:1045: TRACE_EVENT_OBJECT_CREATED_WITH_ID( Once my part lands we don't need the changes in this file, right?
The CQ bit was checked by benjhayden@chromium.org to run a CQ dry run
Thanks! PTAL. https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/frame_tree_node.cc:192: TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID( On 2016/03/02 at 00:29:01, Sami (slow -- travelling) wrote: > Should we move this to a helper method to avoid repeating it three times? Done. https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree_node.cc (right): https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.cc:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/02 at 00:20:20, nasko wrote: > Same here, 2016. Done. https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.cc:26: if (current_frame_host) { On 2016/03/02 at 00:20:20, nasko wrote: > There should always be a current_frame_host. Done. https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.cc:45: if (routingId_ >= 0) { On 2016/03/02 at 00:20:20, nasko wrote: > routing_id != MSG_ROUTING_NONE is more correct comparison. On the other hand, this should never be the case, so at most put a DCHECK in the constructor to ensure the routing id is not NONE. Done. https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree_node.h (right): https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.h:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2016/03/02 at 00:20:20, nasko wrote: > It has been an year already : ), 2016. Done. https://codereview.chromium.org/1390053002/diff/420001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.h:28: int parentNodeId_; On 2016/03/02 at 00:20:20, nasko wrote: > Class members use hacker_case_. Done. https://codereview.chromium.org/1390053002/diff/420001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1390053002/diff/420001/content/renderer/rende... content/renderer/render_frame_impl.cc:1045: TRACE_EVENT_OBJECT_CREATED_WITH_ID( On 2016/03/02 at 00:29:01, Sami (slow -- travelling) wrote: > Once my part lands we don't need the changes in this file, right? Right :-) The changes in this file are just so that I can test the idRefs from FrameTreeNode to RenderFrame, and to compare to your RenderFrame snapshots. Both of those are done so I'll undo these changes.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1390053002/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1390053002/460001
LGTM
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by benjhayden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1390053002/#ps480001 (title: "GetProcId windows handle")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1390053002/480001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1390053002/480001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/1390053002/diff/480001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree_node.cc (right): https://codereview.chromium.org/1390053002/diff/480001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.cc:26: process_id_ = base::GetProcId(current_frame_host->GetProcess()->GetHandle()); Why are you using GetProcId? It is documented as deprecated.
Updated to base::Process::Pid(). I also had to add some branches back in order to fix some segfaults. I guess the RenderFrameHostManager is Init()ed after the FrameTreeNode constructor returns? https://codereview.chromium.org/1390053002/diff/480001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree_node.cc (right): https://codereview.chromium.org/1390053002/diff/480001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.cc:26: process_id_ = base::GetProcId(current_frame_host->GetProcess()->GetHandle()); On 2016/03/03 at 19:03:51, nasko wrote: > Why are you using GetProcId? It is documented as deprecated. Done.
https://codereview.chromium.org/1390053002/diff/500001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1390053002/diff/500001/content/browser/frame_... content/browser/frame_host/frame_tree_node.cc:113: TraceSnapshot(); Is this call to TraceSnapshot() really needed? As you discovered, at this point in time RFHM is not initialized, so you won't have any RFH information. There will be a navigation that commits in this FTN, so maybe tracing a snapshot here isn't needed. https://codereview.chromium.org/1390053002/diff/500001/content/browser/frame_... File content/browser/frame_host/traced_frame_tree_node.cc (right): https://codereview.chromium.org/1390053002/diff/500001/content/browser/frame_... content/browser/frame_host/traced_frame_tree_node.cc:25: if (current_frame_host) { if (!current_frame_host) return;
https://codereview.chromium.org/1390053002/diff/500001/content/browser/frame_... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/1390053002/diff/500001/content/browser/frame_... content/browser/frame_host/frame_tree_node.cc:113: TraceSnapshot(); On 2016/03/03 at 21:41:32, nasko wrote: > Is this call to TraceSnapshot() really needed? As you discovered, at this point in time RFHM is not initialized, so you won't have any RFH information. There will be a navigation that commits in this FTN, so maybe tracing a snapshot here isn't needed. Maybe, maybe not. I put this TraceSnapshot() here because of a quirk in how idRefs are patched up in catapult. If a trace event E (such as another object snapshot (such as a child FrameTreeNode)) contains an idRef to a FrameTreeNode N, and E's timestamp is before the first snapshot of N, then E's idRef to N will not be able to be patched up. It'll be a broken idRef. So, it seems safer to always snapshot an object as soon as it's created. If you like, I could replace this TraceSnapshot() call with a direct call to TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID() *without* a TracedFrameTreeNode, so there'll be a snapshot for idRefs, but TracedFrameTreeNode will be able to assume that its FrameTreeNode will always be initialized.
On 2016/03/03 23:14:06, benjhayden_chromium wrote: > https://codereview.chromium.org/1390053002/diff/500001/content/browser/frame_... > File content/browser/frame_host/frame_tree_node.cc (right): > > https://codereview.chromium.org/1390053002/diff/500001/content/browser/frame_... > content/browser/frame_host/frame_tree_node.cc:113: TraceSnapshot(); > On 2016/03/03 at 21:41:32, nasko wrote: > > Is this call to TraceSnapshot() really needed? As you discovered, at this > point in time RFHM is not initialized, so you won't have any RFH information. > There will be a navigation that commits in this FTN, so maybe tracing a snapshot > here isn't needed. > > Maybe, maybe not. > I put this TraceSnapshot() here because of a quirk in how idRefs are patched up > in catapult. > If a trace event E (such as another object snapshot (such as a child > FrameTreeNode)) contains an idRef to a FrameTreeNode N, and E's timestamp is > before the first snapshot of N, then E's idRef to N will not be able to be > patched up. It'll be a broken idRef. > So, it seems safer to always snapshot an object as soon as it's created. Thanks for the details! > If you like, I could replace this TraceSnapshot() call with a direct call to > TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID() *without* a TracedFrameTreeNode, so > there'll be a snapshot for idRefs, but TracedFrameTreeNode will be able to > assume that its FrameTreeNode will always be initialized. Your call, either way is fine with me.
On 2016/03/03 at 23:18:56, nasko wrote: > On 2016/03/03 23:14:06, benjhayden_chromium wrote: > > https://codereview.chromium.org/1390053002/diff/500001/content/browser/frame_... > > File content/browser/frame_host/frame_tree_node.cc (right): > > > > https://codereview.chromium.org/1390053002/diff/500001/content/browser/frame_... > > content/browser/frame_host/frame_tree_node.cc:113: TraceSnapshot(); > > On 2016/03/03 at 21:41:32, nasko wrote: > > > Is this call to TraceSnapshot() really needed? As you discovered, at this > > point in time RFHM is not initialized, so you won't have any RFH information. > > There will be a navigation that commits in this FTN, so maybe tracing a snapshot > > here isn't needed. > > > > Maybe, maybe not. > > I put this TraceSnapshot() here because of a quirk in how idRefs are patched up > > in catapult. > > If a trace event E (such as another object snapshot (such as a child > > FrameTreeNode)) contains an idRef to a FrameTreeNode N, and E's timestamp is > > before the first snapshot of N, then E's idRef to N will not be able to be > > patched up. It'll be a broken idRef. > > So, it seems safer to always snapshot an object as soon as it's created. > > Thanks for the details! > > > If you like, I could replace this TraceSnapshot() call with a direct call to > > TRACE_EVENT_OBJECT_SNAPSHOT_WITH_ID() *without* a TracedFrameTreeNode, so > > there'll be a snapshot for idRefs, but TracedFrameTreeNode will be able to > > assume that its FrameTreeNode will always be initialized. > > Your call, either way is fine with me. I think it's possible that we might want to add other fields to TracedFrameTreeNode, and we might want to snapshot those fields before the FrameTreeNode is fully initialized, even if that early snapshot can't contain the url or RenderFrame idRef. Also, it's possible that somebody who isn't aware of the FrameTreeNode initialization process (including me in a few weeks) might come through here and add TraceSnapshot() calls willy-nilly. It looks like there were some tests that caught the segfaults, but I had to dig a little in order to see that they were directly caused by TracedFrameTreeNode. So I think I'd rather keep TracedFrameTreeNode able to handle an uninitialized FrameTreeNode if that's fine with you.
The CQ bit was checked by benjhayden@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1390053002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1390053002/500001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
I'll figure out what to do about this latest windows failure tomorrow unless anybody has any suggestions before then. FATAL:process_win.cc(25)] Check failed: handle != ::GetCurrentProcess() (FFFFFFFF vs. FFFFFFFF)
On 2016/03/04 at 02:01:08, benjhayden_chromium wrote: > I'll figure out what to do about this latest windows failure tomorrow unless anybody has any suggestions before then. > > FATAL:process_win.cc(25)] Check failed: handle != ::GetCurrentProcess() (FFFFFFFF vs. FFFFFFFF) I just used an #ifdef, if (process_win would crash) return; #endif Anybody know why Process CHECKs like this?
The CQ bit was checked by benjhayden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1390053002/#ps540001 (title: "fix posix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1390053002/540001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1390053002/540001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
The CQ bit was checked by benjhayden@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1390053002/#ps560001 (title: "Process(rph.GetHandle()) is broken, using GetProcId")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1390053002/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1390053002/560001
Message was sent while issue was closed.
Committed patchset #25 (id:560001)
Message was sent while issue was closed.
Description was changed from ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure in tracing. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, a reference to its parent (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. The RenderFrames are traced at their routing_ids, not their memory address, so that FrameTreeNodes can use cross-process references to them. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. RenderFrames and FrameTreeNodes are snapshot when they are created so that there's always a snapshot for the tracing model to find when it patches up the references. The catapult side of this change is https://codereview.chromium.org/1736373002 The RenderFrame object snapshots that are referenced by the FrameTreeNode snapshots are in Sami's Frameblamr CL: https://codereview.chromium.org/1447563002/ BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Trace FrameTreeNode Snapshots This will provide a single ground-truth for frame structure in tracing. FrameTreeNodes are snapshot when they are created or destroyed, when tracing starts, and when a frame's url changes. Each snapshot contains the frame's url, a reference to its parent (unless it's a root), and a cross-process scoped reference to the current RenderFrame. The snapshots are traced at their frame_tree_node_id, not their memory address. The RenderFrames are traced at their routing_ids, not their memory address, so that FrameTreeNodes can use cross-process references to them. Pending and speculative RenderFrames are not supported. If a specific use case is found for them, then cross-process scoped references will be added to TracedFrameTreeNode for them. The RenderFrame references will allow correlating frame tree structure and FrameTreeNode objects with LocalFrame Trace Contexts. The data for each FrameTreeNode is copied to a TracedFrameTreeNode when the snapshot is recorded, not when the trace is dumped to JSON, because the underlying FrameTree might change between when the snapshot is recorded and when the trace is dumped to JSON. RenderFrames and FrameTreeNodes are snapshot when they are created so that there's always a snapshot for the tracing model to find when it patches up the references. The catapult side of this change is https://codereview.chromium.org/1736373002 The RenderFrame object snapshots that are referenced by the FrameTreeNode snapshots are in Sami's Frameblamr CL: https://codereview.chromium.org/1447563002/ BUG=537802 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/ff37bf3297e8cbe14c6a865e528673f0814c7da1 Cr-Commit-Position: refs/heads/master@{#379623} ==========
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/ff37bf3297e8cbe14c6a865e528673f0814c7da1 Cr-Commit-Position: refs/heads/master@{#379623}
Message was sent while issue was closed.
A revert of this CL (patchset #25 id:560001) has been created in https://codereview.chromium.org/1774853003/ by benwells@chromium.org. The reason for reverting is: This has caused errors on the Linux TSAN bot. First failing build: https://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20TSan%20Test... Sample output: WARNING: ThreadSanitizer: data race (pid=29730) Read of size 8 at 0x7d540000f890 by thread T8: #0 get buildtools/third_party/libc++/trunk/include/memory:2714:69 (content_browsertests+0x000004cdffbd) #1 content::RenderProcessHostImpl::GetHandle() const content/browser/renderer_host/render_process_host_impl.cc:1648 (content_browsertests+0x000004cdffbd) #2 content::TracedFrameTreeNode::TracedFrameTreeNode(content::FrameTreeNode const&) content/browser/frame_host/traced_frame_tree_node.cc:32:5 (content_browsertests+0x000004c132f1) #3 content::FrameTreeNode::TraceSnapshot() const content/browser/frame_host/frame_tree_node.cc:470:3 (content_browsertests+0x000004bd3804) #4 content::FrameTreeNode::OnTraceLogEnabled() content/browser/frame_host/frame_tree_node.cc:466:3 (content_browsertests+0x000004bd5cf9) #5 base::trace_event::TraceLog::SetEnabled(base::trace_event::TraceConfig const&, base::trace_event::TraceLog::Mode) base/trace_event/trace_log.cc:638:5 (content_browsertests+0x000001085fad) #6 content::TracingControllerImpl::SetEnabledOnFileThread(base::trace_event::TraceConfig const&, int, base::Callback<void ()> const&) content/browser/tracing/tracing_controller_impl.cc:211:3 (content_browsertests+0x000004dca941) #7 Run<const base::trace_event::TraceConfig &, const base::trace_event::TraceLog::Mode &, const base::Callback<void ()> &> base/bind_internal.h:181:12 (content_browsertests+0x000004dd1b60) #8 MakeItSo<content::TracingControllerImpl *, const base::trace_event::TraceConfig &, const base::trace_event::TraceLog::Mode &, const base::Callback<void ()> &> base/bind_internal.h:301 (content_browsertests+0x000004dd1b60) #9 base::internal::Invoker<base::IndexSequence<0ul, 1ul, 2ul, 3ul>, base::internal::BindState<base::internal::RunnableAdapter<void (content::TracingControllerImpl::*)(base::trace_event::TraceConfig const&, int, base::Callback<void ()> const&)>, void (content::TracingControllerImpl*, base::trace_event::TraceConfig const&, int, base::Callback<void ()> const&), base::internal::UnretainedWrapper<content::TracingControllerImpl>, base::trace_event::TraceConfig const&, base::trace_event::TraceLog::Mode, base::Callback<void ()>&>, base::internal::InvokeHelper<false, void, base::internal::RunnableAdapter<void (content::TracingControllerImpl::*)(base::trace_event::TraceConfig const&, int, base::Callback<void ()> const&)> >, void ()>::Run(base::internal::BindStateBase*) base/bind_internal.h:351 (content_browsertests+0x000004dd1b60) #10 Run base/callback.h:394:12 (content_browsertests+0x000001093cbd) #11 base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask const&) base/debug/task_annotator.cc:51 (content_browsertests+0x000001093cbd) #12 base::MessageLoop::RunTask(base::PendingTask const&) base/message_loop/message_loop.cc:476:3 (content_browsertests+0x000001014762) #13 base::MessageLoop::DeferOrRunPendingTask(base::PendingTask const&) base/message_loop/message_loop.cc:485:5 (content_browsertests+0x000001014cfd) #14 base::MessageLoop::DoWork() base/message_loop/message_loop.cc:597:13 (content_browsertests+0x0000010150e5) #15 base::MessagePumpLibevent::Run(base::MessagePump::Delegate*) base/message_loop/message_pump_libevent.cc:229:21 (content_browsertests+0x000000feeac0) #16 base::MessageLoop::RunHandler() base/message_loop/message_loop.cc:440:3 (content_browsertests+0x00000101415b) #17 base::RunLoop::Run() base/run_loop.cc:35:3 (content_browsertests+0x0000010308f6) #18 base::MessageLoop::Run() base/message_loop/message_loop.cc:293:3 (content_browsertests+0x0000010139a5) #19 base::Thread::Run(base::MessageLoop*) base/threading/thread.cc:202:3 (content_browsertests+0x000001059239) #20 content::BrowserThreadImpl::FileThreadRun(base::MessageLoop*) content/browser/browser_thread_impl.cc:188:3 (content_browsertests+0x000004b7227e) #21 content::BrowserThreadImpl::Run(base::MessageLoop*) content/browser/browser_thread_impl.cc:243:14 (content_browsertests+0x000004b7281a) #22 base::Thread::ThreadMain() base/threading/thread.cc:254:3 (content_browsertests+0x000001059409) #23 base::(anonymous namespace)::ThreadFunc(void*) base/threading/platform_thread_posix.cc:68:3 (content_browsertests+0x0000010537dd) ..........
Message was sent while issue was closed.
Thanks! Relanding in https://codereview.chromium.org/1775953003 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
