Closed Bug 1313869 Opened 4 years ago Closed 4 years ago

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


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox52 --- wontfix
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed


(Reporter: gkw, Assigned: h4writer)



(Keywords: assertion, bugmon, testcase, Whiteboard: [jsbugmon:update,ignore])


(3 files)

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.


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)

For detailed crash information, see attachment.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
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?
Blocks: 1301343
Flags: needinfo?(jdemooij)
(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.
Flags: needinfo?(jdemooij) → needinfo?(nicolas.b.pierron)
Blocks: 1264948
No longer blocks: 1301343
Flags: needinfo?(nicolas.b.pierron)
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 1196bf3032e1).
autoBisect shows this is probably related to the following changeset:

The first good revision is:
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?
Flags: needinfo?(arai.unmht)
It shouldn't be a fix for JIT issue.  it just hides the underlying issue.
Flags: needinfo?(arai.unmht)
Flags: needinfo?(nicolas.b.pierron)
Attached patch PatchSplinter Review
Assignee: nobody → hv1989
Attachment #8833125 - Flags: review?(nicolas.b.pierron)
Duplicate of this bug: 1328155
Comment on attachment 8833125 [details] [diff] [review]

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.
Attachment #8833125 - Flags: review?(nicolas.b.pierron) → review+
Pushed by
IonMonkey - Ensure ballast in ensureDefined, r=nbp
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: needinfo?(nicolas.b.pierron)
Too late for 52, is this worth uplifting to 53?
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.
Flags: needinfo?(hv1989)
(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.
Flags: needinfo?(hv1989)
Comment on attachment 8833125 [details] [diff] [review]

[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:

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 for more info.
Attachment #8833125 - Flags: approval-mozilla-esr52?
Attachment #8833125 - Flags: approval-mozilla-beta?
(This hasn't landed on 53 yet, but it probably should!)
Comment on attachment 8833125 [details] [diff] [review]

Avoiding an OOM crash sounds good, let's take this for beta 3.
Attachment #8833125 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8833125 [details] [diff] [review]

avoid oom crash, esr52+
Attachment #8833125 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
You need to log in before you can comment on or make changes to this bug.