Reland Trace FrameTreeNode Snapshots

Reland Trace FrameTreeNode Snapshots

This is a reland of
https://codereview.chromium.org/1390053002

That CL was reverted due to a data race in RenderProcessHostImpl::GetHandle.

This CL removes the FrameTreeNode::TraceSnapshot() call in FrameTreeNode's
constructor so that FrameTreeNodes are only snapshot after they are initialized.

This CL also removes the TraceLog observation so that snapshots are not traced
when tracing starts. There's an easy fix for the data race that it caused,
but it might not be the right fix.

BUG=537802
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d4da63d8b5c49d7fa7a2ec9821400350a0670059
Cr-Commit-Position: refs/heads/master@{#380737}

[benjhayden, 2016-03-08 19:16:12]

Description was changed from

==========
Reland Trace FrameTreeNode Snapshots

This is a reland of
https://codereview.chromium.org/1390053002

That CL was reverted due to a data race in RenderProcessHostImpl::GetHandle.

This CL removes the FrameTreeNode::TraceSnapshot() call in FrameTreeNode's
constructor so that FrameTreeNodes are only snapshot after they are initialized.

BUG=537802
==========

to

==========
Reland Trace FrameTreeNode Snapshots

This is a reland of
https://codereview.chromium.org/1390053002

That CL was reverted due to a data race in RenderProcessHostImpl::GetHandle.

This CL removes the FrameTreeNode::TraceSnapshot() call in FrameTreeNode's
constructor so that FrameTreeNodes are only snapshot after they are initialized.

BUG=537802
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[benjhayden, 2016-03-08 19:21:10]

The CQ bit was checked by benjhayden@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-08 19:21:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1775953003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1775953003/1

[benjhayden, 2016-03-08 19:22:38]

benjhayden@chromium.org changed reviewers:
+ clamy@chromium.org, nasko@chromium.org, skyostil@chromium.org

[benjhayden, 2016-03-08 19:22:39]

Sorry, it looks like Nasko was right about waiting to snapshot until after the
RenderFrameHostImpl is initialized.

PTAL?

[commit-bot: I haz the power, 2016-03-08 21:09:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-08 21:10:00]

Dry run: This issue passed the CQ dry run.

[nasko, 2016-03-09 22:33:00]

LGTM with a nit.

https://codereview.chromium.org/1775953003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1775953003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:113: // Don't TraceSnapshot()
until the RenderManager is initialized and
nit: |RenderManager| is not the name of a type that exists, did you mean
RenderFrameHostManager?

[benjhayden, 2016-03-10 00:05:21]

Thanks!
I'll be sure to upload the original patch next time I reland something.

https://codereview.chromium.org/1775953003/diff/1/content/browser/frame_host/...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1775953003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:113: // Don't TraceSnapshot()
until the RenderManager is initialized and
On 2016/03/09 at 22:33:00, nasko (slow) wrote:
> nit: |RenderManager| is not the name of a type that exists, did you mean
RenderFrameHostManager?

Done.

[benjhayden, 2016-03-11 01:11:31]

I dug into the TSan data race in
TracingControllerTest.EnableAndStopTracingWithEmptyFileAndNullCallback.
I found that TraceLog calls OnTraceLogEnabled on the FILE thread, whereas the
RenderFrameHostImpl and friends are on the UI thread. My years on the downloads
team is coming in handy. :-)
Adding a WeakPtrFactory to FrameTreeNode and posting a task from
OnTraceLogEnabled() to call TraceSnapshot() on the UI thread resolves the data
race, but might open a can of architectural worms that I don't want to open
right now.

This is an instance of a more general issue -- lots of other traced objects and
async events want to be buffered around OnTraceLogEnabled. So, for example, we
might to define an abstraction layer in the tracing system to allow events such
as object creation and snapshots to indicate via a flag that the event should be
buffered until a corresponding object deletion event. Or we might want to extend
the TraceLog observers with thread-specific observer lists to make it easier for
object snapshots and async events to buffer themselves without needing to use a
WeakPtrFactory just for that. Or something else. It's an architectural issue for
another day.

So I just removed the TraceLog observation and OnTraceLogEnabled() method from
FrameTreeNode. This might mean that some idRefs will be broken in some traces.
If this turns out to be a problem, then I can add the TraceLog observation and
WeakPtrFactory in another CL, or accelerate fixing the more general issue, or
try to address it in catapult. We'll know if it's a problem when we try to build
TBMv2 metrics with these snapshots, after Frameblamr lands, probably in a couple
weeks.

TSan still reports a lock-order-inversion for that test, but it's in dbus and
seems to be there without this patch.

[benjhayden, 2016-03-11 01:12:54]

Description was changed from

==========
Reland Trace FrameTreeNode Snapshots

This is a reland of
https://codereview.chromium.org/1390053002

That CL was reverted due to a data race in RenderProcessHostImpl::GetHandle.

This CL removes the FrameTreeNode::TraceSnapshot() call in FrameTreeNode's
constructor so that FrameTreeNodes are only snapshot after they are initialized.

BUG=537802
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reland Trace FrameTreeNode Snapshots

This is a reland of
https://codereview.chromium.org/1390053002

That CL was reverted due to a data race in RenderProcessHostImpl::GetHandle.

This CL removes the FrameTreeNode::TraceSnapshot() call in FrameTreeNode's
constructor so that FrameTreeNodes are only snapshot after they are initialized.

This CL also removes the TraceLog observation so that snapshots are not traced
when tracing starts. There's an easy fix for the data race that it caused,
but it might not be the right fix.

BUG=537802
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[benjhayden, 2016-03-11 01:13:08]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-03-11 01:13:09]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1775953003/#ps40001
(title: "remove TraceLog observation")

[commit-bot: I haz the power, 2016-03-11 01:13:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1775953003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1775953003/40001

[commit-bot: I haz the power, 2016-03-11 01:46:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 01:46:27]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[benjhayden, 2016-03-11 18:47:16]

The CQ bit was checked by benjhayden@chromium.org

[benjhayden, 2016-03-11 18:47:17]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1775953003/#ps60001
(title: "rebase")

[commit-bot: I haz the power, 2016-03-11 18:47:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1775953003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1775953003/60001

[commit-bot: I haz the power, 2016-03-11 19:28:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-03-11 19:28:43]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[benjhayden, 2016-03-11 20:43:33]

The CQ bit was checked by benjhayden@chromium.org

[commit-bot: I haz the power, 2016-03-11 20:44:22]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1775953003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1775953003/60001

[commit-bot: I haz the power, 2016-03-11 21:29:41]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-11 21:31:03]

Description was changed from

==========
Reland Trace FrameTreeNode Snapshots

This is a reland of
https://codereview.chromium.org/1390053002

That CL was reverted due to a data race in RenderProcessHostImpl::GetHandle.

This CL removes the FrameTreeNode::TraceSnapshot() call in FrameTreeNode's
constructor so that FrameTreeNodes are only snapshot after they are initialized.

This CL also removes the TraceLog observation so that snapshots are not traced
when tracing starts. There's an easy fix for the data race that it caused,
but it might not be the right fix.

BUG=537802
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Reland Trace FrameTreeNode Snapshots

This is a reland of
https://codereview.chromium.org/1390053002

That CL was reverted due to a data race in RenderProcessHostImpl::GetHandle.

This CL removes the FrameTreeNode::TraceSnapshot() call in FrameTreeNode's
constructor so that FrameTreeNodes are only snapshot after they are initialized.

This CL also removes the TraceLog observation so that snapshots are not traced
when tracing starts. There's an easy fix for the data race that it caused,
but it might not be the right fix.

BUG=537802
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/d4da63d8b5c49d7fa7a2ec9821400350a0670059
Cr-Commit-Position: refs/heads/master@{#380737}
==========

[commit-bot: I haz the power, 2016-03-11 21:31:04]

Patchset 4 (id:??) landed as
https://crrev.com/d4da63d8b5c49d7fa7a2ec9821400350a0670059
Cr-Commit-Position: refs/heads/master@{#380737}