If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

IPC Cpp unittest broken, GeckoProfiler not initialized

RESOLVED FIXED in Firefox 55

Status

()

Core
IPC
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: kanru, Unassigned)

Tracking

Trunk
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 months ago
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'
(Reporter)

Comment 1

5 months ago
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.
(Reporter)

Comment 2

5 months ago
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
(Reporter)

Comment 4

5 months ago
(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
(Reporter)

Comment 5

5 months ago
Created attachment 8856585 [details] [diff] [review]
patch

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)
(Reporter)

Comment 7

5 months ago
(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+
(Reporter)

Comment 13

5 months ago
(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)
(Reporter)

Comment 15

5 months ago
(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

5 months ago
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

Comment 17

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/edef97549a8c
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.