Closed
Bug 1236473
Opened 9 years ago
Closed 9 years ago
Crash [@ js::gc::MergeCompartments] with OOM
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: decoder, Unassigned)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, regression, testcase, Whiteboard: [jsbugmon:update])
Crash Data
Attachments
(1 file)
(deleted),
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
The following testcase crashes on mozilla-central revision d7a0ad85d9fb (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug, run with --fuzzing-safe):
oomTest(() => {
offThreadCompileScript(`try {} catch (NaN) {}`);
runOffThreadScript();
});
Backtrace:
Program received signal SIGSEGV, Segmentation fault.
js::gc::MergeCompartments (source=<optimized out>, target=target@entry=0x7ffff6949000) at js/src/jsobj.h:122
#0 js::gc::MergeCompartments (source=<optimized out>, target=target@entry=0x7ffff6949000) at js/src/jsobj.h:122
#1 0x0000000000a2a9b7 in js::GlobalHelperThreadState::mergeParseTaskCompartment (this=<optimized out>, rt=rt@entry=0x7ffff695d000, parseTask=0x7ffff28b1480, global=global@entry=..., dest=0x7ffff6949000) at js/src/vm/HelperThreads.cpp:1151
#2 0x0000000000a368bd in js::GlobalHelperThreadState::finishParseTask (this=<optimized out>, maybecx=maybecx@entry=0x7ffff6907800, rt=rt@entry=0x7ffff695d000, token=token@entry=0x7ffff28b1480) at js/src/vm/HelperThreads.cpp:1053
#3 0x00000000008c4cde in JS::FinishOffThreadScript (maybecx=maybecx@entry=0x7ffff6907800, rt=rt@entry=0x7ffff695d000, token=token@entry=0x7ffff28b1480) at js/src/jsapi.cpp:4111
#4 0x0000000000488773 in runOffThreadScript (cx=0x7ffff6907800, argc=<optimized out>, vp=0x7fffffffc338) at js/src/shell/js.cpp:3670
#5 0x00007ffff7ff7d28 in ?? ()
#6 0x0000000000000000 in ?? ()
rax 0x7ffff2529b30 140737258887984
rbx 0x7ffff2529b20 140737258887968
rcx 0x1 1
rdx 0x0 0
rsi 0x7ffff2600000 140737259765760
rdi 0x7ffff26911c8 140737260360136
rbp 0x7fffffffc060 140737488339040
rsp 0x7fffffffbf10 140737488338704
r8 0x210 528
r9 0x1 1
r10 0x568a6942 1451911490
r11 0x6 6
r12 0x7ffff2691160 140737260360032
r13 0x0 0
r14 0x7ffff6949000 140737330319360
r15 0x0 0
rip 0x9274bd <js::gc::MergeCompartments(JSCompartment*, JSCompartment*)+701>
=> 0x9274bd <js::gc::MergeCompartments(JSCompartment*, JSCompartment*)+701>: mov 0x0(%r13),%rax
0x9274c1 <js::gc::MergeCompartments(JSCompartment*, JSCompartment*)+705>: lea 0x1269058(%rip),%rsi # 0x1b90520 <_ZN2js10CallObject6class_E>
Updated•9 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment 1•9 years ago
|
||
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:
The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/de72e2291ae8
user: Jan de Mooij
date: Wed Dec 09 22:55:50 2015 -0500
summary: Bug 1225396 part 3 - Make %GeneratorPrototype% inherit from %IteratorPrototype%. r=jorendorff
This iteration took 283.490 seconds to run.
Jan, is bug 1225396 a likely regressor?
Blocks: 1225396
Flags: needinfo?(jdemooij)
Comment 3•9 years ago
|
||
(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #2)
> Jan, is bug 1225396 a likely regressor?
No it's a regression from bug 1213574.
The problem is that we OOM while allocating the script's shared data, *after* we call JSScript::partiallyInit. So we've set the length of the JSScript arrays for block scopes, constants, etc. but we haven't filled these arrays yet. Then in gc::MergeCompartments, we do:
if (script->hasBlockScopes()) {
BlockScopeArray* scopes = script->blockScopes();
for (uint32_t i = 0; i < scopes->length; i++) {
uint32_t scopeIndex = scopes->vector[i].index;
if (scopeIndex != BlockScopeNote::NoBlockScopeIndex) {
ScopeObject* scope = &script->getObject(scopeIndex)->as<ScopeObject>();
We use calloc, so scopeIndex is 0 and script->getObject(scopeIndex) is nullptr, so we crash.
Not sure how to fix it. The easiest and safest fix may be to initialize the block scope list with NoBlockScopeIndex instead of 0, in JSScript::partiallyInit.
Comment 4•9 years ago
|
||
Attachment #8709758 -
Flags: review?(jdemooij)
Updated•9 years ago
|
Flags: needinfo?(shu)
Comment 5•9 years ago
|
||
Comment on attachment 8709758 [details] [diff] [review]
Do not merge scripts that didn't successfully compile.
Review of attachment 8709758 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks, that's simpler than my suggestion.
::: js/src/jit-test/tests/gc/bug-1236473.js
@@ +2,5 @@
> + quit();
> +
> +oomTest(() => {
> + offThreadCompileScript(`try {} catch (NaN) {}`);
> + runOffThreadScript();
Nit: make sure the test doesn't fail with --no-threads (offThreadCompileScript will throw in that case).
::: js/src/jsgc.cpp
@@ +6857,5 @@
> script->setTypesGeneration(target->zone()->types.generation);
>
> + // If the script failed to compile, no need to fix up.
> + if (!script->code())
> + continue;
It seems a little safer to do this before we modify the script's compartment.
Attachment #8709758 -
Flags: review?(jdemooij) → review+
Comment 6•9 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #5)
> ::: js/src/jsgc.cpp
> @@ +6857,5 @@
> > script->setTypesGeneration(target->zone()->types.generation);
> >
> > + // If the script failed to compile, no need to fix up.
> > + if (!script->code())
> > + continue;
>
> It seems a little safer to do this before we modify the script's compartment.
The reason I did it after was that that was the old mergeCompartment code. I don't know enough about the invariants to say if the source compartment expects all of its things to be moved. If you know for certain there is no such invariant, then I agree it'd be a little safer.
Comment 7•9 years ago
|
||
(In reply to Shu-yu Guo [:shu] from comment #6)
> The reason I did it after was that that was the old mergeCompartment code. I
> don't know enough about the invariants to say if the source compartment
> expects all of its things to be moved. If you know for certain there is no
> such invariant, then I agree it'd be a little safer.
OK I'm not sure, and I'm fine with the patch either way. I'm just a bit worried about cross-compartment script -> object edges causing problems elsewhere.
Comment 9•9 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•