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

Status: RESOLVED WONTFIX

(Core :: Gecko Profiler, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

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);

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

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.

Comment 2

[Cervantes Yu [:cyu] [:cervantes]]

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.

Comment 3

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Allow multiple init/shutdown calls

Try push: https://tbpl.mozilla.org/?tree=Try&rev=8831d394db5c

Comment 4

[Thinker Li [:sinker]]

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.

Comment 5

[Cervantes Yu [:cyu] [:cervantes]]

(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.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Task Tracer was removed in bug 1715257.