1344539 - Crash in ProcessExecutableMemory::allocate

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-2af8c4be-a324-403e-90c2-11a4f2170305.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	ProcessExecutableMemory::allocate(unsigned int, js::jit::ProtectionSetting) 	js/src/jit/ProcessExecutableMemory.cpp:553
1 	xul.dll 	js::jit::ExecutableAllocator::createPool(unsigned int) 	js/src/jit/ExecutableAllocator.cpp:221
2 	xul.dll 	js::jit::ExecutableAllocator::poolForSize(unsigned int) 	js/src/jit/ExecutableAllocator.cpp:158
3 	xul.dll 	js::jit::Linker::newCode<1>(JSContext*, js::jit::CodeKind, bool) 	js/src/jit/Linker.cpp:36
4 	xul.dll 	js::jit::IonCache::linkCode(JSContext*, js::jit::MacroAssembler&, js::jit::IonCache::StubAttacher&, js::jit::IonScript*, js::jit::JitCode**) 	js/src/jit/IonCaches.cpp:284
5 	xul.dll 	js::jit::IonCache::linkAndAttachStub(JSContext*, js::jit::MacroAssembler&, js::jit::IonCache::StubAttacher&, js::jit::IonScript*, char const*, JS::TrackedOutcome) 	js/src/jit/IonCaches.cpp:316
6 	xul.dll 	js::jit::GetPropertyIC::tryAttachNative(JSContext*, JS::Handle<JSScript*>, js::jit::IonScript*, JS::Handle<JSObject*>, JS::Handle<jsid>, void*, bool*) 	js/src/jit/IonCaches.cpp:1532
7 	xul.dll 	js::jit::GetPropertyIC::tryAttachStub(JSContext*, JS::Handle<JSScript*>, js::jit::IonScript*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, bool*) 	js/src/jit/IonCaches.cpp:2196
8 	mozglue.dll 	mozglue.dll@0x479f

this crash signature on windows 32bit & 64bit builds of firefox was first showing up in 52.0b5 and regularly thereafter. it looks related to the uplifts in bug 1334933

Comment 1

[Jan de Mooij [:jandem]]

Crash reason is MOZ_CRASH(CommitPages failed). All crashes except one were on 32-bit (and the x64 one looked like a potential physical memory OOM). I'm not sure why VirtualAlloc would fail like this, maybe some internal OOM or limit in virtual memory mappings?

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

Interestingly, almost half of the crashes with this crash reason have the "EMPTY: no crashing thread identified; ERROR_NO_MINIDUMP_HEADER" signature [1]. The available physical memory and available virtual memory don't look particularly low for those either (assuming those stats are to be believed), so I'm not sure what to make of them.

While most crashes are on 32-bit Firefox, they seem split between 32-bit and 64-bit Windows (from looking at the total virtual memory).

[1] https://crash-stats.mozilla.com/search/?moz_crash_reason=%3DMOZ_CRASH%28CommitPages%20failed%29&product=Firefox&version=52.0b&version=52.0esr&version=52.0&version=52.0a2&version=52.0a1&version=53.0a2&version=53.0a1&version=54.0a1&date=%3E%3D2017-02-07T00%3A00%3A00.000Z&date=%3C2018-02-06T00%3A00%3A00.000Z&_sort=date&_facets=signature&_facets=total_virtual_memory&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-signature

Comment 3

[(Away)]

(In reply to Jan de Mooij [:jandem] from comment #1)
> Crash reason is MOZ_CRASH(CommitPages failed). All crashes except one were
> on 32-bit (and the x64 one looked like a potential physical memory OOM). I'm
> not sure why VirtualAlloc would fail like this, maybe some internal OOM or
> limit in virtual memory mappings?

If you have minidump access, can you try "!gle" (GetLastError) on one of the 32-bit reports? I don't remember whether that works in minidumps. If not, maybe we could log GetLastError explicitly.

https://msdn.microsoft.com/en-us/library/windows/desktop/aa366887(v=vs.85).aspx only mentions one error by name, ERROR_INVALID_ADDRESS, but who knows what else may turn up.

Comment 4

[(Away)]

xc(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #2)
> Interestingly, almost half of the crashes with this crash reason have the
> "EMPTY: no crashing thread identified; ERROR_NO_MINIDUMP_HEADER" signature
> [1]. The available physical memory and available virtual memory don't look
> particularly low for those either (assuming those stats are to be believed),
> so I'm not sure what to make of them.
> 
> While most crashes are on 32-bit Firefox, they seem split between 32-bit and
> 64-bit Windows (from looking at the total virtual memory).
> 
> [1]
> https://crash-stats.mozilla.com/search/
> ?moz_crash_reason=%3DMOZ_CRASH%28CommitPages%20failed%29&product=Firefox&vers
> ion=52.0b&version=52.0esr&version=52.0&version=52.0a2&version=52.
> 0a1&version=53.0a2&version=53.0a1&version=54.0a1&date=%3E%3D2017-02-
> 07T00%3A00%3A00.000Z&date=%3C2018-02-06T00%3A00%3A00.
> 000Z&_sort=date&_facets=signature&_facets=total_virtual_memory&_columns=date&
> _columns=signature&_columns=product&_columns=version&_columns=build_id&_colum
> ns=platform#facet-signature

For the most part, those crashes ran out of pagefile.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 52 and likely too late for 53 but we could still take a patch for aurora 54.

Comment 6

[Naveed Ihsanullah [:naveed]]

dmajor is the implication that they are effective out of virtual memory and this an unactionable terminal OOM from our perspective?

Comment 7

[(Away)]

More likely out of physical memory than virtual, but yes.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like it is not actionable, at least for 54.  There are currently about 350 crashes per week with this signature in 53 release.

Comment 9

[Intermittent Failures Robot]

1 failures in 718 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 1

Platform breakdown:
* windows7-32: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1344539&startday=2017-06-26&endday=2017-07-02&tree=all

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking 57 wontfix since we're a few days away from 57 release. This is still a moderate volume crash on release, around 500 crashes per week.

Comment 11

[Nicholas Nethercote [inactive]]

This jumped in frequency in mid-November, from ~200 per day to ~600 per day.

Comment 12

[Trevor Skywalker]

Firefox 	59.0a1 	141 	11.4% 	37
Firefox 	58.0b13 278 	22.5% 	209
Firefox 	58.0b12 34 	2.8% 	32
Firefox 	58.0b14 22 	1.8% 	20
Firefox 	58.0b10 4 	0.3% 	3

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Log GetLastError

Let's log GetLastError() when the VirtualAlloc in CommitPages fails, as suggested in comment 3. Cargo culted from [0].

[0] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/js/src/gc/Memory.cpp#859-860

Comment 14

[Emanuel Hoogeveen [:ehoogeveen]]

Comment on attachment 8940683 [details] [diff] [review]
Log GetLastError

Review of attachment 8940683 [details] [diff] [review]:
-----------------------------------------------------------------

Worth a shot.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/32371cb3206a
Log GetLastError() when we fail to commit executable pages on Windows. r=ehoogeveen

Comment 16

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/32371cb3206a

Comment 17

[Jan de Mooij [:jandem]]

So far, all crashes (since the instrumentation landed) have error code 1455. According to [0], that means:

ERROR_COMMITMENT_LIMIT
    1455 (0x5AF)
    The paging file is too small for this operation to complete.

That matches comment 4.

[0] https://msdn.microsoft.com/en-us/library/windows/desktop/ms681385(v=vs.85).aspx

Comment 18

[Nicolas B. Pierron [:nbp]]

Since this patch landed, crash stat reported the following "moz crash reason" (and the equivalent error reasons[0]):

436	CommitPages failed! Error code: 1455 (ERROR_COMMITMENT_LIMIT: The paging file is too small for this operation to complete.)
1	CommitPages failed! Error code: 87   (ERROR_INVALID_PARAMETER: The parameter is incorrect.)

What would be the next step?

[0] https://msdn.microsoft.com/en-us/library/windows/desktop/ms681381(v=vs.85).aspx

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Patch

We could try to propagate OOM from CommitPages. This might move (some of) the crashes elsewhere, but ProcessExecutableMemory::allocate already returns nullptr when we allocate too much executable code so it's pretty easy to make this fallible.

Comment 20

[Luke Wagner [:luke]]

Comment on attachment 8951566 [details] [diff] [review]
Patch

Review of attachment 8951566 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9680ddb02478
Make committing executable memory fallible. r=luke

Comment 22

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/9680ddb02478

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider backporting to 59?

Comment 24

[Jan de Mooij [:jandem]]

Comment on attachment 8951566 [details] [diff] [review]
Patch

(In reply to Ryan VanderMeulen [:RyanVM] from comment #23)
> Is this something we should consider backporting to 59?

Considering the volume it might make sense.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1334933.
[User impact if declined]: Crashes on OOM.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Only affects what happens when we OOM here.
[String changes made/needed]: None.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8951566 [details] [diff] [review]
Patch

Not a new issue, but fixes a long-running OOM crash. Taking for 59b12.

Comment 26

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

https://hg.mozilla.org/releases/mozilla-beta/rev/00c3d85230c8