Closed Bug 1131846 Opened 9 years ago Closed 9 years ago

Crash [@ js::jit::MInstruction::setResumePoint] involving oomAfterAllocations

Categories

(Core :: JavaScript Engine: JIT, defect)

x86_64
macOS
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox36 --- wontfix
firefox37 --- wontfix
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: gkw, Assigned: nbp)

References

Details

(Keywords: crash, regression, testcase, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(2 files)

s = newGlobal()
try {
    evalcx("for (v of this) {}", s)
} catch (e) {
    "" + e
}
evalcx("\
    do {\
        for (let y = 0; y < 5; ++y) {\
            oomAfterAllocations(70);\
        }\
    } while (x)\
", s)

crashes js debug shell on m-c changeset ee093ca70666 with --fuzzing-safe --no-threads --ion-eager at js::jit::MInstruction::setResumePoint.

Debug configure options:

CC="clang -Qunused-arguments" CXX="clang++ -Qunused-arguments" AR=ar AUTOCONF=/usr/local/Cellar/autoconf213/2.13/bin/autoconf213 sh /Users/skywalker/trees/mozilla-central/js/src/configure --target=x86_64-apple-darwin12.5.0 --enable-debug --enable-nspr-build --enable-more-deterministic --with-ccache --enable-gczeal --enable-debug-symbols --disable-tests
Flags: needinfo?
Attached file stack
(lldb) bt 5
* thread #1: tid = 0x26c3b7, 0x000000010061d4b1 js-dbg-64-dm-nsprBuild-darwin-ee093ca70666`js::jit::MInstruction::setResumePoint(js::jit::MResumePoint*) [inlined] js::jit::MResumePoint::setInstruction(this=0x0000000000000000, ins=0x0000000104083440) at MIR.h:11735, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x40)
  * frame #0: 0x000000010061d4b1 js-dbg-64-dm-nsprBuild-darwin-ee093ca70666`js::jit::MInstruction::setResumePoint(js::jit::MResumePoint*) [inlined] js::jit::MResumePoint::setInstruction(this=0x0000000000000000, ins=0x0000000104083440) at MIR.h:11735
    frame #1: 0x000000010061d4b1 js-dbg-64-dm-nsprBuild-darwin-ee093ca70666`js::jit::MInstruction::setResumePoint(this=0x0000000104083440, resumePoint=0x0000000000000000) + 17 at MIR.cpp:310
    frame #2: 0x0000000100634ba0 js-dbg-64-dm-nsprBuild-darwin-ee093ca70666`js::jit::MBasicBlock::linkOsrValues(this=0x0000000104082e78, start=<unavailable>) + 608 at MIRGraph.cpp:573
    frame #3: 0x000000010052ad34 js-dbg-64-dm-nsprBuild-darwin-ee093ca70666`js::jit::IonBuilder::newOsrPreheader(this=0x0000000104081658, predecessor=0x0000000104082690, loopEntry=<unavailable>) + 2724 at IonBuilder.cpp:6578
    frame #4: 0x000000010052955a js-dbg-64-dm-nsprBuild-darwin-ee093ca70666`js::jit::IonBuilder::forLoop(this=0x0000000104081658, op=<unavailable>, sn=<unavailable>) + 1594 at IonBuilder.cpp:3112
(lldb)
Flags: needinfo?
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/55f95c3b5dbe
user:        Kannan Vijayan
date:        Wed Aug 13 11:59:55 2014 -0400
summary:     Bug 1004831 - Part 4 - Register native to bytecode mappings when new IonCode is generated. r=h4writer

Kannan, is bug 1004831 a likely regressor?
Blocks: 1004831
Flags: needinfo?(kvijayan)
This is almost definitely a coincidental regressor, not the real cause.  The offending line is:

    cloneRp->setResumePoint(MResumePoint::Copy(graph().alloc(), res));

here, MResumePoint::Copy is returning |nullptr| and eventually causes a nullptr access.

'hg blame' identifies the following patch as adding that line:

 changeset:   200866:9a48a86bf2ef
 user:        Nicolas B. Pierron <nicolas.b.pierron@mozilla.com>
 date:        Thu Aug 21 20:47:03 2014 +0200
 summary:     Bug 1042729 part 1 - Make resume point unique to each instruction. r=h4writer
Flags: needinfo?(kvijayan)
Nicolas, is bug 1042729 a likely regressor?
Blocks: 1042729
No longer blocks: 1004831
Flags: needinfo?(nicolas.b.pierron)
Assignee: nobody → nicolas.b.pierron
Status: NEW → ASSIGNED
Flags: needinfo?(nicolas.b.pierron)
Attachment #8576138 - Flags: review?(hv1989) → review+
Bug 1042729 landed in Gecko 34+.
https://hg.mozilla.org/mozilla-central/rev/550637e50cd8
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment on attachment 8576138 [details] [diff] [review]
Check the return value of MResumePoint::Copy.

Approval Request Comment
[Feature/regressing bug #]:
bug 1042729

[User impact if declined]:
Unlikely OOM during compilation.

[Describe test coverage new/current, TreeHerder]:
Landed, no test case, as there is no good way to consistently check for it.

[Risks and why]: 
None?

[String/UUID change made/needed]:
N/A
Attachment #8576138 - Flags: approval-mozilla-beta?
Attachment #8576138 - Flags: approval-mozilla-aurora?
(In reply to Nicolas B. Pierron [:nbp] from comment #9)
> [User impact if declined]:
> Unlikely OOM during compilation.

We've shipped 3 releases with this bug and are pretty late in the 37 Beta cycle. How unlikely is it to hit this OOM condition? Do you think we need to fix this in 37 or can it wait for 38?
Flags: needinfo?(nicolas.b.pierron)
(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #10)
> (In reply to Nicolas B. Pierron [:nbp] from comment #9)
> > [User impact if declined]:
> > Unlikely OOM during compilation.
> 
> We've shipped 3 releases with this bug and are pretty late in the 37 Beta
> cycle. How unlikely is it to hit this OOM condition? Do you think we need to
> fix this in 37 or can it wait for 38?

I think this is a nice to have. I do not think it is frequent enough to make it mandatory.
Flags: needinfo?(nicolas.b.pierron)
Comment on attachment 8576138 [details] [diff] [review]
Check the return value of MResumePoint::Copy.

Given where we are in the cycle, let's skip this fix for 37. I think it's reasonable to take this fix in 38. Beta- Aurora+
Attachment #8576138 - Flags: approval-mozilla-beta?
Attachment #8576138 - Flags: approval-mozilla-beta-
Attachment #8576138 - Flags: approval-mozilla-aurora?
Attachment #8576138 - Flags: approval-mozilla-aurora+
(In reply to Nicolas B. Pierron [:nbp] from comment #11)
> I think this is a nice to have. I do not think it is frequent enough to make
> it mandatory.

For the record,

https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=js%3A%3Ajit%3A%3AMInstruction%3A%3AsetResumePoint%28js%3A%3Ajit%3A%3AMResumePoint*%29#tab-reports

I see 115 crashes reported to crash-stat over the last 28 days.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: