1313869 - Assertion failure: fallibleScope_ ([OOM] Cannot allocate a new chunk in an infallible scope.), at js/src/ds/LifoAlloc.cpp:105

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

The following testcase crashes on mozilla-central revision 1561c917ee27 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --thread-count=2 --ion-eager):

See attachment.

Backtrace:

Thread 3 Crashed:: JS Helper
0   js-dbg-64-dm-clang-darwin-1561c917ee27	0x00000001024fff2c js::LifoAlloc::getOrCreateChunk(unsigned long) + 316 (LifoAlloc.cpp:105)
1   js-dbg-64-dm-clang-darwin-1561c917ee27	0x00000001022cfac7 js::LifoAlloc::allocImpl(unsigned long) + 103 (LifoAlloc.h:225)
2   js-dbg-64-dm-clang-darwin-1561c917ee27	0x00000001020305c2 js::jit::TempObject::operator new(unsigned long, js::jit::TempAllocator&) + 130 (LifoAlloc.h:291)
3   js-dbg-64-dm-clang-darwin-1561c917ee27	0x00000001020071f9 js::jit::LIRGeneratorX64::visitBox(js::jit::MBox*) + 281 (Lowering-x64.cpp:88)
4   js-dbg-64-dm-clang-darwin-1561c917ee27	0x0000000101e9a722 js::jit::LIRGenerator::visitBlock(js::jit::MBasicBlock*) + 674 (Lowering-shared-inl.h:429)
/snip

For detailed crash information, see attachment.

Comment 1

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

Attachment: Detailed Crash Information

Comment 2

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

Attachment: Testcase

Comment 3

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

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/f5d61890ecb5
user:        Jan de Mooij
date:        Sun Oct 02 15:49:57 2016 +0200
summary:     Bug 1301343 - Trace pointers stored in MIR. r=jonco,nbp

Jan, is bug 1301343 a likely regressor?

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #3)
> Jan, is bug 1301343 a likely regressor?

Answering for Jan, I don't think it is, probably forgetting to call ensureBallast in time.
I will take this bug later.

Comment 5

[Fuzzing Team]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 1196bf3032e1).

Comment 6

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

autoBisect shows this is probably related to the following changeset:

The first good revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/d27d2fec192e
user:        Tooru Fujisawa
date:        Sun Nov 13 00:40:28 2016 +0900
summary:     Bug 1021835 - Part 1: Emit JSOP_CHECKISOBJ after GetIterator in byte code. r=evilpie

Arai-san, is bug 1021835 a likely fix?

Comment 7

[Tooru Fujisawa [:arai]]

It shouldn't be a fix for JIT issue.  it just hides the underlying issue.

Comment 8

[Hannes Verschore [:h4writer]]

Attachment: Patch

Comment 10

[Nicolas B. Pierron [:nbp]]

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

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

r=me with nit applied.

::: js/src/jit/Lowering.cpp
@@ +4821,2 @@
>              MDefinition* opd = phi->getOperand(position);
>              ensureDefined(opd);

nit: It seems that only ensureDefined is capable of allocating to lower the instruction which are emitted-at-uses. Move this check under ensureDefined then-block.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa2519aaef25
IonMonkey - Ensure ballast in ensureDefined, r=nbp

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 13

[Julien Cristau [:jcristau]]

Too late for 52, is this worth uplifting to 53?

Comment 14

[Ryan VanderMeulen [:RyanVM]]

I'm curious about ESR52 as well. I know that in the past we've seen real-life crashes that have been fixed by fuzzer OOM fixes.

Comment 15

[Hannes Verschore [:h4writer]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> I'm curious about ESR52 as well. I know that in the past we've seen
> real-life crashes that have been fixed by fuzzer OOM fixes.

We could uplift it to ESR52. Patch is small and easy. Don't think the amount of this crash is huge. But could fix a real-life crash.

Comment 16

[Hannes Verschore [:h4writer]]

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

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
Possibly fix a OOM  bug

User impact if declined: 
Possibly quite low. Though this patch is also very easy and well understood.

Fix Landed on Version:
53

Risk to taking this patch (and alternatives if risky): 
Patch is in FF53/54/55 and no issues encountered. Also very well understood and easy. Just an extra check that handles the OOM failure.

String or UUID changes made by this patch: 
/

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

(This hasn't landed on 53 yet, but it probably should!)

Comment 18

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

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

Avoiding an OOM crash sounds good, let's take this for beta 3.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Julien Cristau [:jcristau]]

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

avoid oom crash, esr52+

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/eba25396310b