tracing: get rid of files in TracingController interface

tracing: get rid of files in TracingController interface

The only way to get the tracing data used to be by reading the file
provided in TracingController::DisableTracing() interface. This does
not fit well for most usages, so this introduces a TraceDataSink
interface that is backed either by a file, string, or, in case of
DevTools, by the protocol client.

This resolves the OOM in browser and imporves performance while
recording very large traces through DevTools, streamlines
TracingController core and reduces coplexity of code for most
TracingController clients.

BUG=409733, 361045
Committed: https://crrev.com/b85bb8fc41a192f207fa2361a8edcfa42b47076f
Cr-Commit-Position: refs/heads/master@{#294801}

[caseq, 2014-09-04 14:22:45]

caseq@chromium.org changed reviewers:
+ dsinclair@chromium.org, nduca@chromium.org, pfeldman@chromium.org,
yurys@chromium.org

[caseq, 2014-09-04 14:22:45]

As I previously proposed, this switches TracingController to expose a
stream-like interface for trace events consumption. Note this actually reduces
the amount of code in both the implementation and most clients.

[dsinclair, 2014-09-04 17:48:45]

dsinclair@chromium.org changed reviewers:
+ haraken@chromium.org, wangxianzhu@chromium.org

[dsinclair, 2014-09-04 17:48:46]

+wangxianzhu +haraken as they've done the most work on the file based trace
controller.

[dsinclair, 2014-09-04 18:03:13]

Overall I like the idea. Can you take a look at the sink ownership issue and
then I'll take a deeper look?

https://codereview.chromium.org/541763002/diff/2/content/browser/tracing/trac...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/541763002/diff/2/content/browser/tracing/trac...
content/browser/tracing/tracing_controller_impl.cc:139: delete this;
I'm not a fan of the delete this on the Close() methods. It's going to lead to
leaks (the File sync above does not delete this).

I think the places where a raw TraceDataSink are pass should be changed to
scoped pointers. The Close() methods can go away and we move the contents into
the destructor.

Then, the tracing controller can free the object at the right time and we can
maintain clear ownership of the sink's as they're passed around.

[nduca, 2014-09-04 18:27:40]

i'm good as long as dan lg's

[caseq, 2014-09-05 09:44:12]

On 2014/09/04 18:03:13, dsinclair wrote:
> Overall I like the idea. Can you take a look at the sink ownership issue and
> then I'll take a deeper look?

Thanks, Dan! I've switched TraceDataSink to be RefCountedThreadSafe (using
simple
scoped_ptr does not seem to be an option due to the fact that some
implementations
(File in our case) operate on multiple threads and may linger past the end of
the
recording -- more on that below...

>
https://codereview.chromium.org/541763002/diff/2/content/browser/tracing/trac...
> File content/browser/tracing/tracing_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/541763002/diff/2/content/browser/tracing/trac...
> content/browser/tracing/tracing_controller_impl.cc:139: delete this;
> I'm not a fan of the delete this on the Close() methods. It's going to lead to
> leaks (the File sync above does not delete this).

It actually does delete, although asynchronously (i.e. Close ->
CloseOnFileThread() -> FinalizeOnUIThread()).

> I think the places where a raw TraceDataSink are pass should be changed to
> scoped pointers. The Close() methods can go away and we move the contents into
> the destructor.

I don't think we should be putting the contents of Close() into the destructor
-- the primary
purpose of Close() is to signal the completion of tracing to the trace consumer,
and I think
we rather want it explicit, especially given that it may result in a complex
async/cross thread
behavior for the consumer (FileTraceDataSink is a good illustration of that).


> Then, the tracing controller can free the object at the right time and we can
> maintain clear ownership of the sink's as they're passed around.

The notion of "right time" gets a bit blurred here :-) Again, if we take File
case into consideration,
this would require an async callback from the sink to the tracing controller to
signal actual completion
of Close(), which, in my view, would unnecessarily complicate the protocol
compared to the sink managing
its own lifetime.

[dsinclair, 2014-09-05 13:58:29]

On 2014/09/05 09:44:12, caseq wrote:
> > Then, the tracing controller can free the object at the right time and we
can
> > maintain clear ownership of the sink's as they're passed around.
> 
> The notion of "right time" gets a bit blurred here :-) Again, if we take File
> case into consideration,
> this would require an async callback from the sink to the tracing controller
to
> signal actual completion
> of Close(), which, in my view, would unnecessarily complicate the protocol
> compared to the sink managing
> its own lifetime.


It's a lot harder to reason about ownership in the self managed world. If the
tracing controller owns the sink then the tracing controller has to cleanup the
sink. If the sink owns itself, then who knows if we've leaked it?

You're already doing the same call in the File case, you're just calling back to
the FileSink to delete itself, so if the call goes to the tracing controller,
why is that more compilicated?

[caseq, 2014-09-05 15:13:03]

On 2014/09/05 13:58:29, dsinclair wrote:

> It's a lot harder to reason about ownership in the self managed world. If the
> tracing controller owns the sink then the tracing controller has to cleanup
the
> sink. If the sink owns itself, then who knows if we've leaked it?

Doesn't the ps3/ps4 address ownership concern by refcounting the sink?

> You're already doing the same call in the File case, you're just calling back
to
> the FileSink to delete itself, so if the call goes to the tracing controller,
> why is that more compilicated?

Because it gets exposed on the interface level and turns a one way interface
into a two way, hence making TracingController depend on sink's behavior. If the
sync does not invoke the callback, it's not just a memory leak, but will also
make TracingControllerImpl fail the next TracingController::DisableTracing()
invocation 'cause of the way can_disable_recording() is implemented.

[Xianzhu, 2014-09-05 16:39:17]

I agree with caseq. I think scoped_refptr clearly states shared ownership and
leaving the last ref to the async task looks reasonable. Letting
TracingController control the whole life of sink looks complicated.

[dsinclair, 2014-09-05 19:27:43]

Ok, fair enough, current system for ownership is good, just a couple of
questions inline.

https://codereview.chromium.org/541763002/diff/50001/components/feedback/trac...
File components/feedback/tracing_manager.cc (right):

https://codereview.chromium.org/541763002/diff/50001/components/feedback/trac...
components/feedback/tracing_manager.cc:15: class StringTraceDataSink : public
content::TracingController::TraceDataSink {
Is this the same as the StringTraceDataSink in TracingControllerImpl? Can we
re-use that one?

https://codereview.chromium.org/541763002/diff/50001/content/browser/tracing/...
File content/browser/tracing/tracing_controller_impl.cc (right):

https://codereview.chromium.org/541763002/diff/50001/content/browser/tracing/...
content/browser/tracing/tracing_controller_impl.cc:147: };
Should these have DISALLOW_COPY_AND_ASSIGN entries?

[caseq, 2014-09-08 09:16:09]

On 2014/09/05 19:27:43, dsinclair wrote:
> Ok, fair enough, current system for ownership is good, just a couple of
> questions inline.
> 
>
https://codereview.chromium.org/541763002/diff/50001/components/feedback/trac...
> File components/feedback/tracing_manager.cc (right):
> 
>
https://codereview.chromium.org/541763002/diff/50001/components/feedback/trac...
> components/feedback/tracing_manager.cc:15: class StringTraceDataSink : public
> content::TracingController::TraceDataSink {
> Is this the same as the StringTraceDataSink in TracingControllerImpl? Can we
> re-use that one?

Done by exposing Create*Sink via TracingController, though I'm not sure if we
really should reuse such things
across the content API.

>
https://codereview.chromium.org/541763002/diff/50001/content/browser/tracing/...
> File content/browser/tracing/tracing_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/541763002/diff/50001/content/browser/tracing/...
> content/browser/tracing/tracing_controller_impl.cc:147: };
> Should these have DISALLOW_COPY_AND_ASSIGN entries?

Sure, fixed!

[dsinclair, 2014-09-08 13:35:33]

lgtm

[caseq, 2014-09-08 14:49:43]

caseq@chromium.org changed reviewers:
+ jochen@chromium.org

[caseq, 2014-09-08 14:49:44]

+jochen for content/ -- could you please take a look?

[yurys, 2014-09-09 11:36:41]

lgtm

[nduca, 2014-09-09 23:51:22]

lgtm

[caseq, 2014-09-10 07:27:35]

jochen, ping ;)

[jochen (gone - plz use gerrit), 2014-09-10 07:33:56]

https://codereview.chromium.org/541763002/diff/90001/content/public/browser/t...
File content/public/browser/tracing_controller.h (right):

https://codereview.chromium.org/541763002/diff/90001/content/public/browser/t...
content/public/browser/tracing_controller.h:37: : public
base::RefCountedThreadSafe<TraceDataSink> {
why threadsafe if all happens on the UI thread?

https://codereview.chromium.org/541763002/diff/90001/content/public/browser/t...
content/public/browser/tracing_controller.h:109: const
scoped_refptr<TraceDataSink> &trace_data_sink) = 0;
nit please clang-format (& should be before the space, not after)

[caseq, 2014-09-10 07:53:52]

On 2014/09/10 07:33:56, jochen wrote:
>
https://codereview.chromium.org/541763002/diff/90001/content/public/browser/t...
> File content/public/browser/tracing_controller.h (right):
> 
>
https://codereview.chromium.org/541763002/diff/90001/content/public/browser/t...
> content/public/browser/tracing_controller.h:37: : public
> base::RefCountedThreadSafe<TraceDataSink> {
> why threadsafe if all happens on the UI thread?

Not quite -- some implementations may require multiple threads (e.g.
FileTraceDataSink)

>
https://codereview.chromium.org/541763002/diff/90001/content/public/browser/t...
> content/public/browser/tracing_controller.h:109: const
> scoped_refptr<TraceDataSink> &trace_data_sink) = 0;
> nit please clang-format (& should be before the space, not after)

Ouch, fixed!

[caseq, 2014-09-11 05:45:12]

jochen, could you please take another look?

[jochen (gone - plz use gerrit), 2014-09-12 14:00:39]

lgtm

[caseq, 2014-09-15 08:03:29]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2014-09-15 08:04:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541763002/130001

[commit-bot: I haz the power, 2014-09-15 08:12:08]

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

[commit-bot: I haz the power, 2014-09-15 08:12:10]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/55876)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)

[caseq, 2014-09-15 09:06:03]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2014-09-15 09:06:32]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541763002/150001

[commit-bot: I haz the power, 2014-09-15 09:20:51]

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

[commit-bot: I haz the power, 2014-09-15 09:20:53]

Try jobs failed on following builders:
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/60777)

[caseq, 2014-09-15 09:54:03]

The CQ bit was checked by caseq@chromium.org

[commit-bot: I haz the power, 2014-09-15 09:54:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/541763002/170001

[commit-bot: I haz the power, 2014-09-15 10:53:10]

Committed patchset #10 (id:170001) as b986b8073ff3c9499ce24957dba8c1e427f047ee

[commit-bot: I haz the power, 2014-09-15 10:55:20]

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