1355123 - IPC Cpp unittest broken, GeckoProfiler not initialized

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

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

In debug build when I tried to run IPDL unit test I got

make: Entering directory '/home/kanru/mozilla/gecko/obj-x86_64-pc-linux-gnu/ipc/ipdl/test/cxx'
Assertion failure: gPS, at /home/kanru/mozilla/gecko/tools/profiler/core/platform.cpp:2642
#01: ???[../../../../dist/bin/libxul.so +0x4cef506]
#02: ???[../../../../dist/bin/libxul.so +0x1187382]
#03: ???[../../../../dist/bin/libxul.so +0x116a407]
#04: ???[/lib/x86_64-linux-gnu/libpthread.so.0 +0x7424]
#05: clone[/lib/x86_64-linux-gnu/libc.so.6 +0xe89bf]
#06: ??? (???:???)
Segmentation fault (core dumped)
Makefile:34: recipe for target 'check-proc' failed
make: *** [check-proc] Error 139
make: Leaving directory '/home/kanru/mozilla/gecko/obj-x86_64-pc-linux-gnu/ipc/ipdl/test/cxx'

Comment 1

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

I have no idea how to make this work. It looks like I have to call NS_SetMainThread and profiler_init somehere before we call the test main function.

Comment 2

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

The gPS check is new. Do you know where should I initialize the profiler properly?

Comment 3

[Andrew McCreight [:mccr8]]

FYI to Nick and Markus, to build these tests, you need this in your mozconfig:
  --enable-ipdl-tests

Comment 4

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

(In reply to Andrew McCreight [:mccr8] from comment #3)
> FYI to Nick and Markus, to build these tests, you need this in your
> mozconfig:
>   --enable-ipdl-tests

And you can run the test via

  make -C obj-x86_64-pc-linux-gnu/ipc/ipdl/test/cxx/ check

Comment 5

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

Attachment: patch

The MOZ_RELEASE_ASSERT in profiler_init is removed because it is run before the main-thread has been set.

Comment 6

[Markus Stange [:mstange]]

Does this patch make us double-initialize the profiler in regular Firefox builds?

Ideally we should initialize it in a function that is only called in this type of test.

Comment 7

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

(In reply to Markus Stange [:mstange] from comment #6)
> Does this patch make us double-initialize the profiler in regular Firefox
> builds?

Nope. The function XRE_InitParentProcess is only used by the IPDL test harness. The name might suggest it's there for a reason but it's not used anymore. We should probably rename it to something else.

> Ideally we should initialize it in a function that is only called in this
> type of test.

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8856585 [details] [diff] [review]
patch

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

Ok, sounds good to me then. Forwarding review to njn to see whether he has thoughts about the assert.

Comment 9

[Nicholas Nethercote [inactive]]

Does XRE_InitParentProcess() really run off the main thread? That seems surprising.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to Nicholas Nethercote [:njn] from comment #9)
> Does XRE_InitParentProcess() really run off the main thread? That seems
> surprising.

See comment 5.

Comment 11

[Markus Stange [:mstange]]

I think the problem is that NS_IsMainThread() is called too soon. Or in other words, NS_SetMainThread() hasn't been called at that point. I wonder what does end up calling NS_SetMainThread() later; searchfox shows a bunch of call paths (mostly through NS_LogInit) but I don't know which one is the one that we end up calling for these tests.

Or maybe we should just add a call to NS_SetMainThread just before the GeckoProfilerInitRAII object.

Comment 12

[Nicholas Nethercote [inactive]]

Comment on attachment 8856585 [details] [diff] [review]
patch

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

::: toolkit/xre/nsEmbedFunctions.cpp
@@ +772,1 @@
>    ScopedXREEmbed embed;

ScopedXREEmbed's constructor calls NS_LogInit(), which calls NS_SetMainThread(). So you can move the profiler initialization below |embed| and then you don't need to remove the assertion in profiler_init().

r=me with that change.

Comment 13

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

(In reply to Nicholas Nethercote [:njn] from comment #12)
> Comment on attachment 8856585 [details] [diff] [review]
> patch
> 
> Review of attachment 8856585 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/xre/nsEmbedFunctions.cpp
> @@ +772,1 @@
> >    ScopedXREEmbed embed;
> 
> ScopedXREEmbed's constructor calls NS_LogInit(), which calls
> NS_SetMainThread(). So you can move the profiler initialization below
> |embed| and then you don't need to remove the assertion in profiler_init().
> 
> r=me with that change.

Hmm, move the profiler initialization below embed doesn't work because the ScopedXREEmbed's dtor calls XRE_TermEmbedding() which calls profiler_call_enter() via profiler label http://searchfox.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#1218

I think I can call NS_SetMainThread() before profiler initialization as it's OK (doesn't assert) to call NS_SetMainThread() multiple times.

Comment 14

[Nicholas Nethercote [inactive]]

> Hmm, move the profiler initialization below embed doesn't work because the
> ScopedXREEmbed's dtor calls XRE_TermEmbedding() which calls
> profiler_call_enter() via profiler label

Why is ScopedXREEmbed's dtor relevant?

> I think I can call NS_SetMainThread() before profiler initialization as it's
> OK (doesn't assert) to call NS_SetMainThread() multiple times.

Yes, calling NS_SetMainThread() twice should be fine, so do that if it's necessary. But I still don't understand why moving profiler_init() lower doesn't work.

Comment 15

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

(In reply to Nicholas Nethercote [:njn] from comment #14)
> > Hmm, move the profiler initialization below embed doesn't work because the
> > ScopedXREEmbed's dtor calls XRE_TermEmbedding() which calls
> > profiler_call_enter() via profiler label
> 
> Why is ScopedXREEmbed's dtor relevant?
> 
> > I think I can call NS_SetMainThread() before profiler initialization as it's
> > OK (doesn't assert) to call NS_SetMainThread() multiple times.
> 
> Yes, calling NS_SetMainThread() twice should be fine, so do that if it's
> necessary. But I still don't understand why moving profiler_init() lower
> doesn't work.

The problem is that the profiler shuts down too early.

:njn gives r+ with calling NS_SetMainThread() twice on IRC.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edef97549a8c
Initialize gecko profiler properly for IPDL test runner. r=mstange,njn

Comment 17

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/edef97549a8c