Closed Bug 1236473 Opened 4 years ago Closed 4 years ago

Crash [@ js::gc::MergeCompartments] with OOM

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: decoder, Unassigned)

References

(Blocks 2 open bugs)

Details

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

Crash Data

Attachments

(1 file)

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>
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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)
(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.
Blocks: 1213574
No longer blocks: 1225396
Flags: needinfo?(jdemooij) → needinfo?(shu)
Flags: needinfo?(shu)
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+
(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.
(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.
https://hg.mozilla.org/mozilla-central/rev/755c6f29cbae
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.