TaskTracer doesn't aware that profiler could be initialized multiple times

NEW
Assigned to

Status

()

Core
Gecko Profiler
3 years ago
3 years ago

People

(Reporter: kanru, Assigned: cyu)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
The first time is in http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsAppRunner.cpp?rev=32b690a35683#4169 via GeckoProfilerInitRAII

The second time is in http://mxr.mozilla.org/mozilla-central/source/xpcom/build/XPCOMInit.cpp?rev=32b690a35683#494 via profiler_init

Task tracer crashes because a assert on TLS data

Breakpoint 1, mozilla::tasktracer::InitTaskTracer (aFlags=aFlags@entry=0) at ../../../gecko/tools/profiler/GeckoTaskTracer.cpp:208
208       MOZ_ASSERT(!sTraceInfoTLS);
(Assignee)

Comment 1

3 years ago
in tools/profiler/platform.cpp:
> int         sInitCount = 0; // Each init must have a matched shutdown.
I think the profiler is expected to be init'd multiple times, as long as it has the same number of shutdown calls. I am not sure why it is designed in this way. Maybe it is unavoidable. I guess we need to do the same.
(Reporter)

Updated

3 years ago
Summary: Profiler initialized twice → TaskTracer doesn't aware that profiler could be initialized multiple times
(Reporter)

Updated

3 years ago
Blocks: 995058
(Assignee)

Comment 2

3 years ago
It's not really dependent upon bug 1095433 that much. But the bug has some fundamental changes that I would like to land first so I just it to dependent bug and avoid the patches from stepping on each other's toes.
Assignee: nobody → cyu
Depends on: 1095433
(Assignee)

Comment 3

3 years ago
Created attachment 8545865 [details] [diff] [review]
Allow multiple init/shutdown calls

Try push: https://tbpl.mozilla.org/?tree=Try&rev=8831d394db5c
Attachment #8545865 - Flags: review?(tlee)

Comment 4

3 years ago
Comment on attachment 8545865 [details] [diff] [review]
Allow multiple init/shutdown calls

Review of attachment 8545865 [details] [diff] [review]:
-----------------------------------------------------------------

::: tools/profiler/GeckoTaskTracer.cpp
@@ +228,5 @@
>  ShutdownTaskTracer()
>  {
> +  sInitCount --;
> +  if (sInitCount > 0) {
> +    return;

This would make the process that call |ReinitTaskTracer()| @ https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#629 never shutting down.
Attachment #8545865 - Flags: review?(tlee) → review-
(Assignee)

Comment 5

3 years ago
(In reply to Thinker Li [:sinker] from comment #4)
> This would make the process that call |ReinitTaskTracer()| @
> https://dxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#629
> never shutting down.

The problem will not happen with bug 1095433 fixed. Calling InitTaskTracer with the flag FORKED_AFTER_NUWA only obseletes current trace info and doesn't bump sInitCount.
You need to log in before you can comment on or make changes to this bug.