Closed Bug 938144 Opened 6 years ago Closed 6 years ago

Crash [@ js::jit::MBasicBlock::setLoopDepth] or random assertions with OOM

Categories

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

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: decoder, Assigned: decoder)

References

(Blocks 1 open bug)

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 7b014f0f3b03 (threadsafe build, run with --ion-eager --fuzzing-safe --thread-count=2 --ion-parallel-compile=on --ion-regalloc=backtracking):


evaluate("\
if (typeof oomAfterAllocations == 'function') {\
    oomAfterAllocations(1);\
} \
");
This OOM crash seems to depend on thread scheduling, you might need to run it multiple times (use a loop of 100 to be sure). Most of the time, I get the mentioned crash but sometimes, also random assertions pop up. It would be very helpful to kill this one as it causes lots of signatures.
Ok, the setLoopDepth crash seems to be simple:

>    // Create a new block inheriting from the predecessor.
>    MBasicBlock *split = MBasicBlock::NewSplitEdge(graph, block->info(), *block);
>    split->setLoopDepth(block->loopDepth());

NewSplitEdge returns NULL here due to OOM, and we don't check it. Adding a check and returning false fixes the issue. However, nobody calls JS_ReportOutOfMemory here and we just exit with code 0 (no OOM reported). 

Jandem, can you advise me how to fix this? Should I just add the check to return false without other modifications?
Flags: needinfo?(jdemooij)
(In reply to Christian Holler (:decoder) from comment #3)
> Jandem, can you advise me how to fix this? Should I just add the check to
> return false without other modifications?

Yes, that's the right fix.

This code may run off-thread so we can't report OOM here. It looks like we ignore OOM during compilation, but we make sure we don't crash or leak in this case, so that's fine I think.
Flags: needinfo?(jdemooij)
Assignee: general → choller
Status: NEW → ASSIGNED
Attachment #8344628 - Flags: review?(jdemooij)
Comment on attachment 8344628 [details] [diff] [review]
js-setLoopDepth-oom.patch

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

Thanks!
Attachment #8344628 - Flags: review?(jdemooij) → review+
https://hg.mozilla.org/mozilla-central/rev/c227b066f842
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Keywords: verifyme
Can't reproduce this locally, on a Ubuntu 13.04 x86 machine. I did look for the signature from this report in Socorro though and I didn't find any crashes for the last 4 weeks.
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.