Closed Bug 1285934 Opened 5 years ago Closed 5 years ago

Assertion failure: uintptr_t(obj) > 0x1000 || uintptr_t(obj) == 0x48, at dist/include/js/Value.h:831 with OOM

Categories

(Core :: JavaScript Engine, defect)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: decoder, Assigned: bbouvier)

References

(Blocks 1 open bug)

Details

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

Attachments

(2 files)

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
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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
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.
Attachment #8769668 - Flags: review?(luke)
Attached patch oom.patchSplinter Review
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?
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
Attachment #8769687 - Flags: review?(luke)
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').
Attachment #8769687 - Flags: review?(luke) → review+
(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.
Flags: needinfo?(luke)
(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.
Flags: needinfo?(luke)
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.
https://hg.mozilla.org/mozilla-central/rev/56c832213c10
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.