Escape name and category of trace events.

Escape name and category of trace events.

Some calls to trace an event pass a source location as name
(e.g. in BeginFrameTracker::Start). On Windows, these location paths are
separated by backslashes which were not escaped when saving as JSON.
This lead to unparsable trace files created by chrome://tracing.

R=primiano@chromium.org
BUG=599176
TEST=TraceEventTestFixture.NameAndCategoryEscapedIfNeeded

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

[Primiano Tucci (use gerrit), 2016-04-07 13:30:08]

Thanks, this makes sense. Although, can you please cover this with a test?

Also you just made me realize that the code in BeginFrameTracker::Start is racy.
The const char* arguments passed to tracing should be long lived, not c_str of
temporary objects. BUt this is unrelated with this CL.

https://codereview.chromium.org/1870563002/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/1870563002/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:298: EscapeJSONString(category_group_name,
true, out);
In which case does a category name contain quotes?
This is just in case right?

[Mario Pistrich, 2016-04-07 13:50:42]

On 2016/04/07 13:30:08, Primiano Tucci wrote:
> Thanks, this makes sense. Although, can you please cover this with a test?
> 
> Also you just made me realize that the code in BeginFrameTracker::Start is
racy.
> The const char* arguments passed to tracing should be long lived, not c_str of
> temporary objects. BUt this is unrelated with this CL.
> 
>
https://codereview.chromium.org/1870563002/diff/1/base/trace_event/trace_even...
> File base/trace_event/trace_event_impl.cc (right):
> 
>
https://codereview.chromium.org/1870563002/diff/1/base/trace_event/trace_even...
> base/trace_event/trace_event_impl.cc:298:
EscapeJSONString(category_group_name,
> true, out);
> In which case does a category name contain quotes?
> This is just in case right?

Thanks for reviewing.
Can you please explain which test case needs to be covered? There is a unit test
in patch set 1 to check the escaping of name and category.

BeginFrameTracker::Finish is probably racy too as it also passes a c_str. Could
this be the reason why the name property of these async events in
BeginFrameTracker::Start and BeginFrameTracker::End sometimes contain binary
garbage instead of the real location? This is observed on Mac and Linux and is
described in the original issue 59917.

[Mario Pistrich, 2016-04-07 13:50:58]

https://codereview.chromium.org/1870563002/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/1870563002/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:298: EscapeJSONString(category_group_name,
true, out);
On 2016/04/07 13:30:08, Primiano Tucci wrote:
> In which case does a category name contain quotes?
> This is just in case right?

Yeah, I've added it just to be sure - should I remove it?

[Primiano Tucci (use gerrit), 2016-04-07 16:17:56]

Sorry I completely missed the unittests. It is fine!
LGTM, just not sure if we really need escaping of the category name.
Naming a category with a "quote is evil++ :)

https://codereview.chromium.org/1870563002/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/1870563002/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:298: EscapeJSONString(category_group_name,
true, out);
On 2016/04/07 13:50:58, Mario Pistrich wrote:
> On 2016/04/07 13:30:08, Primiano Tucci wrote:
> > In which case does a category name contain quotes?
> > This is just in case right?
> 
> Yeah, I've added it just to be sure - should I remove it?

I am pretty sure that if somebody adds quotes to the category lot of other
things will break, so I'd probably avoid the perf hit of escaping here.

[Mario Pistrich, 2016-04-07 18:59:52]

https://codereview.chromium.org/1870563002/diff/1/base/trace_event/trace_even...
File base/trace_event/trace_event_impl.cc (right):

https://codereview.chromium.org/1870563002/diff/1/base/trace_event/trace_even...
base/trace_event/trace_event_impl.cc:298: EscapeJSONString(category_group_name,
true, out);
On 2016/04/07 16:17:56, Primiano Tucci wrote:
> On 2016/04/07 13:50:58, Mario Pistrich wrote:
> > On 2016/04/07 13:30:08, Primiano Tucci wrote:
> > > In which case does a category name contain quotes?
> > > This is just in case right?
> > 
> > Yeah, I've added it just to be sure - should I remove it?
> 
> I am pretty sure that if somebody adds quotes to the category lot of other
> things will break, so I'd probably avoid the perf hit of escaping here.

Okay, sounds reasonable, I removed escaping the trace event's category.

[Mario Pistrich, 2016-04-07 19:00:27]

The CQ bit was checked by m.pistrich@gmail.com

[Mario Pistrich, 2016-04-07 19:00:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
primiano@chromium.org
Link to the patchset: https://codereview.chromium.org/1870563002/#ps20001
(title: "Remove escaping of trace event category")

[commit-bot: I haz the power, 2016-04-07 19:01:13]

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

[commit-bot: I haz the power, 2016-04-07 21:07:02]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-07 21:09:12]

Description was changed from

==========
Escape name and category of trace events.

Some calls to trace an event pass a source location as name
(e.g. in BeginFrameTracker::Start). On Windows, these location paths are
separated by backslashes which were not escaped when saving as JSON.
This lead to unparsable trace files created by chrome://tracing.

R=primiano@chromium.org
BUG=599176
TEST=TraceEventTestFixture.NameAndCategoryEscapedIfNeeded
==========

to

==========
Escape name and category of trace events.

Some calls to trace an event pass a source location as name
(e.g. in BeginFrameTracker::Start). On Windows, these location paths are
separated by backslashes which were not escaped when saving as JSON.
This lead to unparsable trace files created by chrome://tracing.

R=primiano@chromium.org
BUG=599176
TEST=TraceEventTestFixture.NameAndCategoryEscapedIfNeeded

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

[commit-bot: I haz the power, 2016-04-07 21:09:13]

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