New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 706416 link

Starred by 0 users
Status: Fixed
Owner: ----
Closed: Mar 2017
Cc:
skyos...@chromium.org
█ alexclarke@chromium.org
ice...@yandex-team.ru
altimin@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

task_queue_impl creates empty tracing categories

Project Member Reported by primiano@chromium.org, Mar 29 2017 
Follow up from https://codereview.chromium.org/2777203004/ which got unfortunately reverted hitting a new DCHECK.

It looks like that the code in work_queue_unittest.cc end up using an empty string "" as category name.
Context form the CL that got reverted.

> I see that in Blink tests categoty_group_name is set to an empty string("")
> https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/scheduler/base/work_queue_unittest.cc?q=work_queue_unittest.cc:28+package:%5Echromium$&l=29
> 
> Then it will be used here
> https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/scheduler/base/task_queue_impl.cc?q=TaskQueueImpl+package:%5Echromium$&l=451

I suppose that is not intended as empty categories make little sense in trace world.

Also, looks like the trace events in task_queue_impl.cc use a non-long-lived string as category name, which is something tracing has never being designed for.
From trace_event_common.h:
// Notes: The category must always be in a long-lived char* (i.e. static const).

Not sure what the code there is intended to do, but in general the category itself should be a fixed string as it gets propagated to the UI.

Comment 1 by skyos...@chromium.org, Mar 29 2017

Components: Blink>Scheduling
Yeah this is a bit broken. The category string passed to trace events in task_queue_impl.cc is actually always a long lived pointer so there are no use-after-free issues. However the problem is that a single trace point can ever be initialized once, so it will always have the category from the first time it was encountered.

I guess there are no plans for dynamic categories from the tracing side? If not, I think our options are 1) templatize everything based on the trace category (yuck) or 2) just hard code the strings.

Comment 2 by ice...@yandex-team.ru, Mar 29 2017 

Should we just use something like |TRACE_DISABLED_BY_DEFAULT("")| instead of empty string literal "" in that test code?

Comment 3 by skyos...@chromium.org, Mar 29 2017 

I think we can just come up with a dummy category name for the test.
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 29 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/20b661f1dffc812e1432ce1d506cc367ad5c24eb

commit 20b661f1dffc812e1432ce1d506cc367ad5c24eb
Author: skyostil <skyostil@chromium.org>
Date: Wed Mar 29 19:21:21 2017

scheduler: Use a non-empty tracing category in test

BUG= 706416 
TBR=primiano@chromium.org

Review-Url: https://codereview.chromium.org/2786713002
Cr-Commit-Position: refs/heads/master@{#460484}

[modify] https://crrev.com/20b661f1dffc812e1432ce1d506cc367ad5c24eb/third_party/WebKit/Source/platform/scheduler/base/work_queue_unittest.cc

Comment 5 by skyos...@chromium.org, Mar 30 2017

Status: Fixed (was: Untriaged)
Filed  bug 706775  for the follow-up.
►

Sign in to add a comment