The following testcase crashes on mozilla-central revision 1bee8d2da23e (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-debug --enable-optimize, run with --fuzzing-safe --ion-offthread-compile=off): function foo() { var g = newGlobal(); g.eval(`o = Wasm.instantiateModule(wasmTextToBinary('(module (func) (export "" 0))'));`); } oomTest(foo); Backtrace: received signal SIGSEGV, Segmentation fault. 0x00000000007f5c18 in OBJECT_TO_JSVAL_IMPL (obj=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:831 #0 0x00000000007f5c18 in OBJECT_TO_JSVAL_IMPL (obj=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:831 #1 0x000000000082de59 in JS::Value::setObject (obj=..., this=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:1092 #2 js::MutableValueOperations<JS::MutableHandle<JS::Value> >::setObject (obj=..., this=<optimized out>) at /srv/jenkins/jobs/mozilla-central-build-jsshell/workspace/arch/64/compiler/gcc/sanitizer/none/type/debug/dist/include/js/Value.h:1822 #3 InstantiateModule (cx=cx@entry=0x7ffff6965000, argc=<optimized out>, vp=0x7ffff32f3120) at js/src/asmjs/WasmJS.cpp:195 #4 0x0000000000af4ff4 in js::CallJSNative (cx=cx@entry=0x7ffff6965000, native=0x82db30 <InstantiateModule(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:232 #5 0x0000000000aefc53 in js::InternalCallOrConstruct (cx=0x7ffff6965000, args=..., construct=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:453 #6 0x0000000000ae3e0f in js::CallFromStack (args=..., cx=<optimized out>) at js/src/vm/Interpreter.cpp:504 #7 Interpret (cx=0x7ffff6965000, state=...) at js/src/vm/Interpreter.cpp:2873 #8 0x0000000000aefa99 in js::RunScript (cx=cx@entry=0x7ffff6965000, state=...) at js/src/vm/Interpreter.cpp:399 #9 0x0000000000af245b in js::ExecuteKernel (cx=cx@entry=0x7ffff6965000, script=..., script@entry=..., scopeChainArg=..., newTargetValue=..., evalInFrame=..., evalInFrame@entry=..., result=result@entry=0x7fffffffbb48) at js/src/vm/Interpreter.cpp:679 #10 0x0000000000a39280 in EvalKernel (cx=cx@entry=0x7ffff6965000, v=..., evalType=evalType@entry=INDIRECT_EVAL, caller=..., scopeobj=..., pc=pc@entry=0x0, vp=...) at js/src/builtin/Eval.cpp:334 #11 0x0000000000a39a5c in js::IndirectEval (cx=cx@entry=0x7ffff6965000, argc=1, vp=0x7fffffffbb48) at js/src/builtin/Eval.cpp:434 #12 0x0000000000af4ff4 in js::CallJSNative (cx=cx@entry=0x7ffff6965000, native=0xa39980 <js::IndirectEval(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:232 #13 0x0000000000aefc53 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6965000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:453 #14 0x0000000000aeff96 in InternalCall (cx=cx@entry=0x7ffff6965000, args=...) at js/src/vm/Interpreter.cpp:498 #15 0x0000000000af00ee in js::Call (cx=cx@entry=0x7ffff6965000, fval=..., fval@entry=..., thisv=..., args=..., rval=...) at js/src/vm/Interpreter.cpp:517 #16 0x0000000000a16986 in js::Wrapper::call (this=this@entry=0x1ccf280 <js::CrossCompartmentWrapper::singleton>, cx=cx@entry=0x7ffff6965000, proxy=..., proxy@entry=..., args=...) at js/src/proxy/Wrapper.cpp:165 #17 0x0000000000a010e3 in js::CrossCompartmentWrapper::call (this=0x1ccf280 <js::CrossCompartmentWrapper::singleton>, cx=0x7ffff6965000, wrapper=..., args=...) at js/src/proxy/CrossCompartmentWrapper.cpp:329 #18 0x0000000000a0b0d3 in js::Proxy::call (cx=cx@entry=0x7ffff6965000, proxy=proxy@entry=..., args=...) at js/src/proxy/Proxy.cpp:401 #19 0x0000000000a0b1d8 in js::proxy_Call (cx=cx@entry=0x7ffff6965000, argc=<optimized out>, vp=<optimized out>) at js/src/proxy/Proxy.cpp:689 #20 0x0000000000af4ff4 in js::CallJSNative (cx=cx@entry=0x7ffff6965000, native=0xa0b140 <js::proxy_Call(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:232 #21 0x0000000000aefe47 in js::InternalCallOrConstruct (cx=cx@entry=0x7ffff6965000, args=..., construct=construct@entry=js::NO_CONSTRUCT) at js/src/vm/Interpreter.cpp:441 #22 0x0000000000aeff96 in InternalCall (cx=cx@entry=0x7ffff6965000, args=...) at js/src/vm/Interpreter.cpp:498 #23 0x0000000000af00ba in js::CallFromStack (cx=cx@entry=0x7ffff6965000, args=...) at js/src/vm/Interpreter.cpp:504 #24 0x00000000005ef199 in js::jit::DoCallFallback (cx=0x7ffff6965000, frame=0x7fffffffc228, stub_=<optimized out>, argc=<optimized out>, vp=0x7fffffffc1d0, res=...) at js/src/jit/BaselineIC.cpp:5977 #25 0x00007ffff7ff069a in ?? () [...] #59 0x0000000000000000 in ?? () rax 0x0 0 rbx 0x1 1 rcx 0x7ffff6c28a2d 140737333332525 rdx 0x0 0 rsi 0x7ffff6ef7770 140737336276848 rdi 0x7ffff6ef6540 140737336272192 rbp 0x7fffffffac20 140737488333856 rsp 0x7fffffffac20 140737488333856 r8 0x7ffff6ef7770 140737336276848 r9 0x7ffff7fdc740 140737353992000 r10 0x58 88 r11 0x7ffff6b9f750 140737332770640 r12 0x7fffffffac80 140737488333952 r13 0x7fffffffac60 140737488333920 r14 0x7ffff32f3120 140737273344288 r15 0x1 1 rip 0x7f5c18 <OBJECT_TO_JSVAL_IMPL(JSObject*)+72> => 0x7f5c18 <OBJECT_TO_JSVAL_IMPL(JSObject*)+72>: movl $0x0,0x0 0x7f5c23 <OBJECT_TO_JSVAL_IMPL(JSObject*)+83>: ud2

JSBugMon: Bisection requested, result: === Treeherder Build Bisection Results by autoBisect === The "good" changeset has the timestamp "20160303082107" and the hash "fa7a5698fced4b1e5bf67178233a2a82d2b9288f". The "bad" changeset has the timestamp "20160303083931" and the hash "6c8b2fbba88b9044bf47ac4e8a76dafeb8d629b6". Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fa7a5698fced4b1e5bf67178233a2a82d2b9288f&tochange=6c8b2fbba88b9044bf47ac4e8a76dafeb8d629b6

Review commit: https://reviewboard.mozilla.org/r/63456/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/63456/

Comment on attachment 8769668 [details] Bug 1285934: Report an error in case of OOM when copying the bytecode input; Cancelling review, there's actually another bug.

A few OOM in wasm::Eval were not reported. I am not too sure that we should set OOM if the buildIdOp failed, even if there's no exception, because of the BuildIdOp used in the CycledCollectedJSRuntime: http://searchfox.org/mozilla-central/source/xpcom/base/CycleCollectedJSRuntime.cpp#415 (the two other ones, used in the JS and XPC shell are trivial and just fallibly append something to the vector) What do you think?

Comment on attachment 8769687 [details] [diff] [review] oom.patch Review of attachment 8769687 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmCompile.cpp @@ +1233,5 @@ > alwaysBaseline = cx->options().wasmAlwaysBaseline(); > + if (assumptions.init(SignalUsage(cx), cx->buildIdOp())) > + return true; > + > + if (!cx->maybeJSContext() || cx->asJSContext()->isExceptionPending()) (and that's a !cx->asJSContext()->isExceptionPending() here, of course -- fixed locally)

Comment on attachment 8769687 [details] [diff] [review] oom.patch Review of attachment 8769687 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/asmjs/WasmCompile.cpp @@ +1234,5 @@ > + if (assumptions.init(SignalUsage(cx), cx->buildIdOp())) > + return true; > + > + if (!cx->maybeJSContext() || cx->asJSContext()->isExceptionPending()) > + ReportOutOfMemory(cx); Actually, ReportOutOfMemory() takes an ExclusiveContext (and does the Right Thing) internally, so you don't need this test. With that change, could you invert the control flow so it's: if (!assumptions.init(...)) { ReportOutOfMemory(cx); return false; } ? ::: js/src/jit-test/tests/wasm/regress/oom-eval.js @@ +1,5 @@ > +// |jit-test| allow-oom > + > +if (typeof newGlobal !== 'function' || > + typeof oomTest !== 'function' || > + typeof Wasm !== 'object') When can 'newGlobal' or 'oomTest' be missing? For 'Wasm', though, I think we should only test wasmIsSupported() and then unconditionally use Wasm so that way we get loud failures if something breaks (like, e.g., when 'Wasm' is removed we'll know to update the test to use 'WebAssembly').

(In reply to Luke Wagner [:luke] from comment #6) > Comment on attachment 8769687 [details] [diff] [review] > oom.patch > > Review of attachment 8769687 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: js/src/asmjs/WasmCompile.cpp > @@ +1234,5 @@ > > + if (assumptions.init(SignalUsage(cx), cx->buildIdOp())) > > + return true; > > + > > + if (!cx->maybeJSContext() || cx->asJSContext()->isExceptionPending()) > > + ReportOutOfMemory(cx); > > Actually, ReportOutOfMemory() takes an ExclusiveContext (and does the Right > Thing) internally, so you don't need this test. If we just use ReportOutOfMemory here, we might clobber an existing error in the case of a JSContext. What I was trying to do was the following: - if we have an ExclusiveContext, make it report out of memory. - if we have a JSContext, check there's no pending exception and make it report OOM only in this case. Not sure the BuildIdOp used by the browser could set a pending exception, hence these checks. If you think it's fine to just ReportOutOfMemory here, I can do so; please let me know. > > ::: js/src/jit-test/tests/wasm/regress/oom-eval.js > @@ +1,5 @@ > > +// |jit-test| allow-oom > > + > > +if (typeof newGlobal !== 'function' || > > + typeof oomTest !== 'function' || > > + typeof Wasm !== 'object') > > When can 'newGlobal' or 'oomTest' be missing? oomTest is defined under #ifdef DEBUG and JS_OOM_BREAKPOINT and is protected in all the JS shell tests. newGlobal seems to be defined in shell/js.cpp all the time, but the check is almost free and ensures the test won't fail if run in the browser (I think that happens?). > > For 'Wasm', though, I think we should only test wasmIsSupported() and then > unconditionally use Wasm so that way we get loud failures if something > breaks (like, e.g., when 'Wasm' is removed we'll know to update the test to > use 'WebAssembly'). Indeed, edited.

(In reply to Benjamin Bouvier [:bbouvier] from comment #7) > If we just use ReportOutOfMemory here, we might clobber an existing error in > the case of a JSContext. What I was trying to do was the following: > - if we have an ExclusiveContext, make it report out of memory. > - if we have a JSContext, check there's no pending exception and make it > report OOM only in this case. > > Not sure the BuildIdOp used by the browser could set a pending exception, > hence these checks. If you think it's fine to just ReportOutOfMemory here, I > can do so; please let me know. The inductive SM error-reporting invariant is that either you take a 'cx' and report your own error (so 'return false' implies 'cx->isExceptionPending()') OR you don't take a 'cx' and do not report your own error (so '!cx->isExceptionPending()' will be a pre- and post-condition). 'assumptions.init()' (and the underlying BuildIdOp call) fit in the latter case, so unconditionally ReportOutOfMemory() is the right thing to do. > newGlobal seems to be defined in shell/js.cpp all the time, but the check is > almost free and ensures the test won't fail if run in the browser (I think > that happens?). Only the ref tests in js/src/test run in the browser; tests in jit-tests rely on tons of shell/js.cpp (and thus non-browser) functions. I'm not at all worried about the cost of the check, I was just trying to make the test less likely to short-circuit.

Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/56c832213c10 Report an error in case of OOM in wasm::Eval; r=luke

Thanks for the explanation! Addressed the remarks in comment 6 and removed the guard for newGlobal.