1841250 - Assertion failure: sJitCodeRegionStart && sJitCodeRegionStart == aStart && sJitCodeRegionSize == aSize, at mozglue/misc/StackWalk.cpp:157

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Download a "Windows 2012 AArch64 debug" js shell from treeherder via the "B" link, download the target.jsshell.zip file (I tested a couple of revs before m-c rev 68fc9ccb5d1021ae9291069aada9262ed4975872), then extract and run ./js -e "quit()" ; on an aarch64 Win machine - you'll see this:

Assertion failure: sJitCodeRegionStart && sJitCodeRegionStart == aStart && sJitCodeRegionSize == aSize, at /builds/worker/checkouts/gecko/mozglue/misc/StackWalk.cpp:157
#01: UnregisterJitCodeRegion[C:\Users\gary_\Downloads\target.jsshell\mozglue.dll +0x7f550]
#02: ???[C:\Users\gary_\Downloads\target.jsshell\js.exe +0xeebfac]
#03: ???[C:\Users\gary_\Downloads\target.jsshell\js.exe +0x28a48c]
#04: ???[C:\Users\gary_\Downloads\target.jsshell\js.exe +0xd1fc]
#05: ???[C:\Users\gary_\Downloads\target.jsshell\js.exe +0x162211c]
#06: ???[C:\Users\gary_\Downloads\target.jsshell\js.exe +0x16221c0]
#07: BaseThreadInitThunk[C:\Windows\System32\KERNEL32.DLL +0x123f0]
#08: RtlUserThreadStart[C:\Windows\SYSTEM32\ntdll.dll +0x72eac]

The opt shell crashes on shutdown as well. Jan, who is the best person to look at this? This makes other forms of testing virtually impossible.

Comment 1

[Jan de Mooij [:jandem]]

Hey Gary, thanks for reporting this.

Do you happen to know if this is a relatively recent regression? Or did you just start testing those builds?

Comment 2

[Jan de Mooij [:jandem]]

I suspect the assertion fails because aSize is off by a page size.

In ReserveProcessExecutableMemory, we add the size of a page to bytes. If we don't have an exception handler installed and are on ARM64, we then pass that modified size to RegisterJitCodeRegion.

In DeallocateProcessExecutableMemory, we call UnregisterJitCodeRegion with bytes (this is MaxCodeBytesPerProcess passed by the caller).

Comment 3

[Jan de Mooij [:jandem]]

Attachment: WIP: Bug 1841250 - Untested fix.

Comment 4

[Jan de Mooij [:jandem]]

Can you see if the attached patch helps? I haven't tested it yet though.

Comment 5

[Jan de Mooij [:jandem]]

I also started some Windows ARM64 try builds in case downloading one of those is easier: https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=96c5841d3700652e8cf2ed3e823a7e6e8907c595

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Jan de Mooij [:jandem] from comment #5)

I also started some Windows ARM64 try builds in case downloading one of those is easier: https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=96c5841d3700652e8cf2ed3e823a7e6e8907c595

This patch works great!

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I'm going to guess this is related to bug 1530552 as a possible regressor, looking at the lines the patch changed. Jan, would you agree?

Comment 8

[Jan de Mooij [:jandem]]

Yes that makes sense.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1841250 - Always skip first code page on Windows 64-bit platforms. r?iain!

On Win64 platforms (NEED_JIT_UNWIND_HANDLING), we reserve an extra page in
ReserveProcessExecutableMemory for the generated exception handler.

Before this patch we'd skip the first page if we generated an exception handler there.
If we didn't generate an exception handler (for example JS shell builds on ARM64)
we'd not skip the first page and instead have an unused page at the end of the JIT
code region.

With this patch we always skip the first page if we reserved one. This fixes an
assertion failure in UnregisterExecutableMemory for Windows ARM64 JS shell builds.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e810f2c019ba
Always skip first code page on Windows 64-bit platforms. r=iain

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1530552

Comment 12

[Cosmin Sabou [:CosminS]]

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

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jandem, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox116 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Jan de Mooij [:jandem]]

Probably makes sense to fix this on ESR115 because that will be supported for a while still, but for beta it can just ride the trains.

Comment 15

[Jan de Mooij [:jandem]]

Comment on attachment 9342227 [details]
Bug 1841250 - Always skip first code page on Windows 64-bit platforms. r?iain!

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: This fixes a shutdown crash with SpiderMonkey builds for Windows ARM64.

ESR115 will be supported for quite a while still so it probably makes sense to fix it there too.

• User impact if declined:
• Fix Landed on Version: 117
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Pretty local change and this has been tested in Nightly.

Comment 16

[(PTO June 19-July 7) Ryan VanderMeulen]

Comment on attachment 9342227 [details]
Bug 1841250 - Always skip first code page on Windows 64-bit platforms. r?iain!

Approved for 115.1esr.

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr115/rev/06a2542ef431

Comment 18

[(PTO June 19-July 7) Ryan VanderMeulen]

https://hg.mozilla.org/releases/mozilla-esr115/rev/06a2542ef431