Closed Bug 1355123 Opened 2 years ago Closed 2 years ago

IPC Cpp unittest broken, GeckoProfiler not initialized

Categories

(Core :: IPC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: kanru, Unassigned)

Details

Attachments

(1 file)

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'
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.
The gPS check is new. Do you know where should I initialize the profiler properly?
Flags: needinfo?(n.nethercote)
Flags: needinfo?(mstange)
FYI to Nick and Markus, to build these tests, you need this in your mozconfig:
  --enable-ipdl-tests
(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
Attached patch patchSplinter Review
The MOZ_RELEASE_ASSERT in profiler_init is removed because it is run before the main-thread has been set.
Flags: needinfo?(n.nethercote)
Attachment #8856585 - Flags: review?(mstange)
Attachment #8856585 - Attachment is patch: true
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.
Flags: needinfo?(mstange)
(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 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.
Attachment #8856585 - Flags: review?(n.nethercote)
Attachment #8856585 - Flags: review?(mstange)
Attachment #8856585 - Flags: review+
Does XRE_InitParentProcess() really run off the main thread? That seems surprising.
(In reply to Nicholas Nethercote [:njn] from comment #9)
> Does XRE_InitParentProcess() really run off the main thread? That seems
> surprising.

See comment 5.
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 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.
Attachment #8856585 - Flags: review?(n.nethercote) → review+
(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.
Flags: needinfo?(n.nethercote)
> 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.
Flags: needinfo?(n.nethercote)
(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.
Pushed by kchen@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edef97549a8c
Initialize gecko profiler properly for IPDL test runner. r=mstange,njn
https://hg.mozilla.org/mozilla-central/rev/edef97549a8c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.