1515229 - Too many frames are skipped by MozStackWalk

Status: RESOLVED FIXED

(Core :: mozglue, defect, P3)

Description

[Mike Hommey [:glandium]]

This is what I get if I add a MOZ_CRASH early in XREMain in a debug build:

- on Windows x86-64:
Hit MOZ_CRASH(foobar) at z:/build/build/src/toolkit/xre/nsAppRunner.cpp:3453
#01: XRE_main (z:\build\build\src\toolkit\xre\nsAppRunner.cpp:4841)
#02: NS_internal_main (z:\build\build\src\browser\app\nsBrowserApp.cpp:293)
#03: wmain (z:\build\build\src\toolkit\xre\nsWindowsWMain.cpp:129)
#04: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
#05: BaseThreadInitThunk[C:\WINDOWS\System32\KERNEL32.DLL +0x13034]
#06: RtlUserThreadStart[C:\WINDOWS\SYSTEM32\ntdll.dll +0x73691]

Removing all frame skipping, I get those additional frames:
#01: MozStackWalkThread[C:\Users\glandium\Desktop\foo\firefox\mozglue.dll +0x333f3]
#02: MozStackWalk[C:\Users\glandium\Desktop\foo\firefox\mozglue.dll +0x33821]
#03: MOZ_ReportCrash (z:\build\build\src\obj-firefox\dist\include\mozilla\Assertions.h:184)
#04: XREMain::XRE_mainInit (z:\build\build\src\toolkit\xre\nsAppRunner.cpp:3453)

- on Windows x86:
Hit MOZ_CRASH(foobar) at z:/build/build/src/toolkit/xre/nsAppRunner.cpp:3453
#01: XREMain::XRE_main (z:\build\build\src\toolkit\xre\nsAppRunner.cpp:4729)
#02: XRE_main (z:\build\build\src\toolkit\xre\nsAppRunner.cpp:4841)
#03: mozilla::BootstrapImpl::XRE_main (z:\build\build\src\toolkit\xre\Bootstrap.cpp:39)
#04: NS_internal_main (z:\build\build\src\browser\app\nsBrowserApp.cpp:293)
#05: wmain (z:\build\build\src\toolkit\xre\nsWindowsWMain.cpp:129)
#06: __scrt_common_main_seh (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:288)
#07: BaseThreadInitThunk[C:\WINDOWS\System32\KERNEL32.DLL +0x18484]
#08: RtlAreBitsSet[C:\WINDOWS\SYSTEM32\ntdll.dll +0x63ab8]
#09: RtlAreBitsSet[C:\WINDOWS\SYSTEM32\ntdll.dll +0x63a88]

Without any skipping:
#01: MozStackWalkThread[C:\Users\glandium\Desktop\bar\firefox\mozglue.dll +0x2ab6b]
#02: MozStackWalk[C:\Users\glandium\Desktop\bar\firefox\mozglue.dll +0x2af28]
#03: nsTraceRefcnt::WalkTheStack (z:\build\build\src\xpcom\base\nsTraceRefcnt.cpp:773)
#04: MOZ_ReportCrash (z:\build\build\src\obj-firefox\dist\include\mozilla\Assertions.h:182)
#05: XREMain::XRE_mainInit (z:\build\build\src\toolkit\xre\nsAppRunner.cpp:3453)

At the very least, the XREMain::XRE_mainInit frame should not be skipped. But it also seems that WalkTheStack shouldn't be assuming that its caller should be skipped, too. Also, this is all fragile in the face of inlining changing frame counts.

The weird thing is that the number of skipped frame is different, although it's supposed to be the same, per:
https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/xpcom/base/nsTraceRefcnt.cpp#839
https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/mozglue/misc/StackWalk.cpp#254

Comment 1

[Mike Hommey [:glandium]]

The XREMain::XRE_mainInit frame is also skipped over on Linux.

> Also, this is all fragile in the face of inlining changing frame counts.

... or tail call optimizations or whatever... as a result, the same crash yields, on aarch64 windows, the following frames:
#01: MozStackWalkThread
#02: MOZ_ReportCrash
#03: XREMain::XRE_mainInit
#04: XREMain::XRE_main

Comment 2

[Mike Hommey [:glandium]]

The XREMain::XRE_mainInit frame is also skipped over on OSX.

Comment 5

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

Attachment: Bug 1695604 - Fix MozStackWalk self-skipping, add gtests - r?mstange!,canaltinova

Tests on Windows now verify that the leaf stack frames are as expected (preset, in the correct order, starting from the leaf-most frame, and excluding the stack walker code itself).
This confirmed that Windows' MozStackWalk and MozStackWalkThread self-skipping were incorrect at 3. These are now fixed to be 2 and 1 respectively.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1515229 - Make MozStackWalk/MozWalkTheStack frame skipping more reliable.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f7b0cdc3aeb0
Make MozStackWalk/MozWalkTheStack frame skipping more reliable. r=gerald,nika,bobowen,jld

Comment 8

[Sandor Molnar[:smolnar]]

Backed out for causing xpc failures in test_feature_stackwalking

Backout link: https://hg.mozilla.org/integration/autoland/rev/ec57ffc15db2def96ec16e5892c6ae6edf6e255e

Push with failures

Failure log

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5c6b15fcea71
Make MozStackWalk/MozWalkTheStack frame skipping more reliable. r=gerald,nika,bobowen,jld

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/5c6b15fcea71

Comment 11

[Atila Butkovits]

Backed out for causing GTest failures.

Backout link: https://hg.mozilla.org/integration/autoland/rev/c84361fe4a1bfb0f643a5e99e45b2baf091a1b82

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&fromchange=5c6b15fcea71ab5c56d6e796a1b93fa8971115ed&searchStr=gtest%2Cshippable&tochange=5c6b15fcea71ab5c56d6e796a1b93fa8971115ed&selectedTaskRun=XxxvPgavReSYghvrz4I_6Q.0

Failure log: https://treeherder.mozilla.org/logviewer?job_id=336615609&repo=autoland&lineNumber=2390

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backout merged: https://hg.mozilla.org/mozilla-central/rev/c84361fe4a1b

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a00cdb29df1f
Make MozStackWalk/MozWalkTheStack frame skipping more reliable. r=gerald,nika,bobowen,jld

Comment 14

[Sandor Molnar[:smolnar]]

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