Closed Bug 1236476 Opened 8 years ago Closed 8 years ago

Assertion failure: message, at js/src/jscntxt.cpp:818 with OOM

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla46
Tracking Flags:
Tracking Status
firefox46 --- fixed

People

(Reporter: decoder, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file)

MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r=jandem
58 bytes, text/x-review-board-request
jandem
: review+
Details 
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(`
      "use asm";
      return assertEq;
    `);
    runOffThreadScript();
});



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000008c3e01 in js::CallErrorReporter (cx=<optimized out>, message=<optimized out>, reportp=<optimized out>) at js/src/jscntxt.cpp:818
#0  0x00000000008c3e01 in js::CallErrorReporter (cx=<optimized out>, message=<optimized out>, reportp=<optimized out>) at js/src/jscntxt.cpp:818
#1  0x0000000000bdae63 in js::frontend::CompileError::throwError (this=<optimized out>, cx=<optimized out>) at js/src/frontend/TokenStream.cpp:583
#2  0x0000000000a36a84 in js::GlobalHelperThreadState::finishParseTask (this=<optimized out>, maybecx=maybecx@entry=0x7ffff6907800, rt=rt@entry=0x7ffff695d000, token=token@entry=0x7ffff31d5ac0) at js/src/vm/HelperThreads.cpp:1064
#3  0x00000000008c4cde in JS::FinishOffThreadScript (maybecx=maybecx@entry=0x7ffff6907800, rt=rt@entry=0x7ffff695d000, token=token@entry=0x7ffff31d5ac0) 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	0x0	0
rbx	0x7ffff6907800	140737330051072
rcx	0x7ffff6ca53cd	140737333842893
rdx	0x0	0
rsi	0x7ffff6f7a9d0	140737336814032
rdi	0x7ffff6f791c0	140737336807872
rbp	0x7fffffffc150	140737488339280
rsp	0x7fffffffc150	140737488339280
r8	0x7ffff7fe0780	140737354008448
r9	0x6372732f736a2f6c	7165916604736876396
r10	0x7fffffffbf10	140737488338704
r11	0x7ffff6c27960	140737333328224
r12	0x1	1
r13	0x7fffffffc1b0	140737488339376
r14	0x7fffffffc1d0	140737488339408
r15	0x7ffff6907808	140737330051080
rip	0x8c3e01 <js::CallErrorReporter(JSContext*, char const*, JSErrorReport*)+97>
=> 0x8c3e01 <js::CallErrorReporter(JSContext*, char const*, JSErrorReport*)+97>:	movl   $0x332,0x0
   0x8c3e0c <js::CallErrorReporter(JSContext*, char const*, JSErrorReport*)+108>:	callq  0x4a4a90 <abort()>
Not specific to asm.js, I think? 

- when encountering an error while parsing off main thread, we call cx->addPendingException, which is allocating a new CompileError https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/TokenStream.cpp#620
- then we expand the error message in this call to ExpandErrorArgumentsVA https://dxr.mozilla.org/mozilla-central/source/js/src/frontend/TokenStream.cpp#650
- at the end of ExpandErrorArgumentsVA, if there's a memory allocation failure, we explicitly free the error "message" variable, which is what's happening here
- later, when calling CallErrorReporter with this CompileError's message, it asserts because the message is empty

Should the ParseTask have a flag outOfMemory (there is overRecursed already)? Or should we just ReportOutOfMemory if there's no message allocated? The former sounds more general...
cc'ing people who might know what to do here (see comment 1 for analysis)
Agreed this seems unrelated to asm.js.  This looks related to bug 1232676 where the AUtoEnterOOMUnsafeRegion was added to ExclusiveContext::addPendingCompileError().  Jan: do you know why we have to crash here instead of simply making addPendingCompileError() fallible as bbouvier suggested?  Also, ideally I think we'd only add finished CompileErrors to the cx instead of adding them before they are initialized.
Flags: needinfo?(jdemooij)
(In reply to Luke Wagner [:luke] from comment #3)
> This looks related to bug 1232676
> where the AUtoEnterOOMUnsafeRegion was added to
> ExclusiveContext::addPendingCompileError().  Jan: do you know why we have to
> crash here instead of simply making addPendingCompileError() fallible as
> bbouvier suggested?

There used to be a MOZ_CRASH there, I just replaced it with AutoEnterOOMUnsafeRegion because the fuzzers know how to handle those. Making addPendingCompileError fallible is fine, I think.
Flags: needinfo?(jdemooij)
The issue is actually not in addPendingCompileError: it is that ExpandErrorArgumentsVA fails with oom but never reports it, so the crafted error is malformed. Patch incoming.
Previous patch adds an outOfMemory flag to the parse task, that is set the same way that the overrecursed flag.

I've tried in another patch to make ExpandErrorArgumentsVA less of a mess by using UniquePtrs, but it's a mess as in one case, you'd have to have a vector of UniquePtrs...
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
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 0.459 seconds to run.
Comment on attachment 8704103 [details]
MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r=jandem

https://reviewboard.mozilla.org/r/29569/#review26373

Thanks for fixing this! I've one question below, let me know what you think.

::: js/src/jscntxt.cpp:602
(Diff revision 1)
> +                    ReportOutOfMemory(cx);

The cx->pod_malloc call is supposed to report OOM on the context when it fails. That's not happening here because ExclusiveContext::onOutOfMemory does:

    if (!isJSContext())
        return nullptr;
    return runtime_->onOutOfMemory(allocFunc, nbytes, reallocPtr, asJSContext());

JSRuntime::onOutOfMemory will call ReportOutOfMemory, if we have a JSContext. It seems wrong to call ReportOutOfMemory here as well.

Should we, instead of reporting OOM here, change ExclusiveContext::onOutOfMemory to do addPendingOutOfMemory()?
Attachment #8704103 - Flags: review?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #9)
> Should we, instead of reporting OOM here, change
> ExclusiveContext::onOutOfMemory to do addPendingOutOfMemory()?

It's a bit tricky because some callers of ExclusiveContext::pod_malloc and friends may call recoverFromOutOfMemory, or rely on these calls not reporting anything if it's an ExclusiveContext and handle the nullptr case.

I think it makes sense to do this though, and to also handle the ExclusiveContext case in ExclusiveContext::recoverFromOutOfMemory...
Attachment #8704103 - Flags: review?(jdemooij)
Comment on attachment 8704103 [details]
MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r=jandem

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29569/diff/1-2/
(In reply to Jan de Mooij [:jandem] from comment #10)
> (In reply to Jan de Mooij [:jandem] from comment #9)
> > Should we, instead of reporting OOM here, change
> > ExclusiveContext::onOutOfMemory to do addPendingOutOfMemory()?
> 
> It's a bit tricky because some callers of ExclusiveContext::pod_malloc and
> friends may call recoverFromOutOfMemory, or rely on these calls not
> reporting anything if it's an ExclusiveContext and handle the nullptr case.
> 
> I think it makes sense to do this though, and to also handle the
> ExclusiveContext case in ExclusiveContext::recoverFromOutOfMemory...

Agreed: updated the patch to do so. I will send the patch to try to make sure we don't have something strange showing up.
Comment on attachment 8704103 [details]
MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r=jandem

https://reviewboard.mozilla.org/r/29569/#review26941

Thanks!

::: js/src/jscntxt.cpp:949
(Diff revision 2)
> +            task->outOfMemory = false;

Can we do `task->outOfMemory = false` unconditionally?

::: js/src/vm/HelperThreads.cpp:1355
(Diff revision 2)
> -    if (!error || !helperThread()->parseTask()->errors.append(error))
> +           helperThread()->parseTask()->errors.append(*error);

I think this will leak the CompileError if the append() fails.

Also, can we use cx->new_ (to make sure the OOM is reported!) and return a pointer?

```
frontend::CompileError* addPendingCompileError() {
    frontend::CompileError* error = new_<frontend::CompileError>();
    if (!error)
        return nullptr;
    if (!errors.append(*error)) {
        js_delete(error);
        return nullptr;
    }
    return error;
}
```

Bonus points for using UniquePtr :)
Attachment #8704103 - Flags: review?(jdemooij) → review+
Attachment #8704103 - Attachment description: MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r?jandem → MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r=jandem
Comment on attachment 8704103 [details]
MozReview Request: Bug 1236476: Report out of memory in ExpandErrorArgumentsVA; r=jandem

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29569/diff/2-3/
https://hg.mozilla.org/mozilla-central/rev/115da8bc01ad
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.