1672501 - Crash in [@ mozilla::baseprofiler::profiler_shutdown]

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect, P1)

Description

[[:philipp]]

Crash report: https://crash-stats.mozilla.org/report/index/7a1d6852-5300-487c-bf88-d5fdc0201021

Top 6 frames of crashing thread:

0 mozglue.dll mozilla::baseprofiler::profiler_shutdown mozglue/baseprofiler/core/platform.cpp:2742
1 firefox.exe wmain toolkit/xre/nsWindowsWMain.cpp:131
2 firefox.exe __scrt_common_main_seh /builds/worker/workspace/obj-build/browser/app/f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:288
3 kernel32.dll BaseThreadInitThunk 
4 ntdll.dll __RtlUserThreadStart 
5 ntdll.dll _RtlUserThreadStart 

these crash reports with MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(profiler_is_main_thread()) from bug 1670954 are newly appearing in firefox 83.

Comment 1

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

It looks like I may have been wrong in assuming that static vars would be initialized from the main thread; or maybe Win32's GetCurrentThreadId() may not always work before main() starts?
Anyway, I'm working on a fix to initialize scProfilerMainThreadId from profiler_init() instead, which is all we need in the profiler, and I believe will be safe to uplift.

Comment 2

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

Attachment: Bug 1672501 - Initialize scProfilerMainThreadId from profiler_init - r?gregtatum

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3aa39fa769a2
Initialize scProfilerMainThreadId from profiler_init - r=gregtatum

Comment 4

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/3aa39fa769a2

Comment 5

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

Comment on attachment 9183115 [details]
Bug 1672501 - Initialize scProfilerMainThreadId from profiler_init - r?gregtatum

Beta/Release Uplift Approval Request

• User impact if declined: Crashes during process shutdowns.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change makes the code closer to what it was before bug 1670954 (removing the suspected cause of crashes, while keeping other changes needed by other tasks).

An alternative would be to completely revert bug 1670954 on beta 83, as I think it's not needed there: Dependent bug 1672256 landed in 84.

• String changes made/needed:

Comment 6

[Pascal Chevrel:pascalc]

Comment on attachment 9183115 [details]
Bug 1672501 - Initialize scProfilerMainThreadId from profiler_init - r?gregtatum

Crash fix, patch on nightly looks effective, uplift approved for 83 beta 5, thanks.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ea5b3ebdcb7d