Closed
Bug 1201124
Opened 9 years ago
Closed 8 years ago
Assertion failure: (ValidateSimdType(cx, global, globalVal, &v))
Categories
(Core :: JavaScript Engine: JIT, defect, P5)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla52
People
(Reporter: anba, Unassigned)
Details
Attachments
(1 file)
Test case: --- var stdlib = new (newGlobal().Proxy)(this, new Proxy({ simdGet: 0, getOwnPropertyDescriptor(t, pk) { if (pk === "SIMD" && this.simdGet++ === 1) { return {}; } return Reflect.getOwnPropertyDescriptor(t, pk); } }, { get(t, pk, r) { print("trap", pk); return Reflect.get(t, pk, r); } })); function asm(stdlib, foreign, heap) { "use asm"; var i4 = stdlib.SIMD.Int32x4; var i4add = i4.add; return {}; } asm(stdlib, {}, new ArrayBuffer(0x10000)); --- Assertion failure: (ValidateSimdType(cx, global, globalVal, &v)), at /home/andre/git/mozilla-central/js/src/asmjs/AsmJSLink.cpp:368 Stack trace: --- #0 0x000000000062569b in ValidateSimdOperation (cx=0x7ffff6908c00, global=..., globalVal=...) at /home/andre/git/mozilla-central/js/src/asmjs/AsmJSLink.cpp:368 #1 0x00000000006229bb in DynamicallyLinkModule (cx=0x7ffff6908c00, args=..., module=...) at /home/andre/git/mozilla-central/js/src/asmjs/AsmJSLink.cpp:592 #2 0x00000000005676bb in LinkAsmJS (cx=0x7ffff6908c00, argc=3, vp=0x7ffff3df40a8) at /home/andre/git/mozilla-central/js/src/asmjs/AsmJSLink.cpp:1077 #3 0x00000000007dea28 in js::CallJSNative (cx=0x7ffff6908c00, native=0x567540 <LinkAsmJS(JSContext*, unsigned int, JS::Value*)>, args=...) at /home/andre/git/mozilla-central/js/src/jscntxtinlines.h:235 #4 0x0000000000772faf in js::Invoke (cx=0x7ffff6908c00, args=..., construct=js::NO_CONSTRUCT) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:763 #5 0x000000000078d55b in Interpret (cx=0x7ffff6908c00, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:3067 #6 0x000000000077f974 in js::RunScript (cx=0x7ffff6908c00, state=...) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:704 #7 0x000000000079aada in js::ExecuteKernel (cx=0x7ffff6908c00, script=..., scopeChainArg=..., thisv=..., newTargetValue=..., type=js::EXECUTE_GLOBAL, evalInFrame=..., result=0x7fffffffd350) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:978 #8 0x000000000079ae41 in js::Execute (cx=0x7ffff6908c00, script=..., scopeChainArg=..., rval=0x7fffffffd350) at /home/andre/git/mozilla-central/js/src/vm/Interpreter.cpp:1011 #9 0x0000000000e96eba in ExecuteScript (cx=0x7ffff6908c00, scope=..., script=..., rval=0x7fffffffd350) at /home/andre/git/mozilla-central/js/src/jsapi.cpp:4356 #10 0x0000000000e96d20 in JS_ExecuteScript (cx=0x7ffff6908c00, scriptArg=..., rval=...) at /home/andre/git/mozilla-central/js/src/jsapi.cpp:4381 #11 0x000000000043a6e5 in EvalAndPrint (cx=0x7ffff6908c00, bytes=0x7ffff3dfbcc0 "asm(stdlib, {}, new ArrayBuffer(0x10000));\n", '\245' <repeats 21 times>, " if (pk === \"SIMD\" && this.simdGet++ === 1) {", length=43, lineno=23, compileOnly=false, out=0x7ffff6f6a740 <_IO_2_1_stdout_>) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:487 #12 0x000000000043a03b in ReadEvalPrintLoop (cx=0x7ffff6908c00, in=0x7ffff6f69980 <_IO_2_1_stdin_>, out=0x7ffff6f6a740 <_IO_2_1_stdout_>, compileOnly=false) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:550 #13 0x0000000000439a09 in Process (cx=0x7ffff6908c00, filename=0x0, forceTTY=true) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:582 #14 0x000000000043901f in ProcessArgs (cx=0x7ffff6908c00, op=0x7fffffffd998) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:5801 #15 0x0000000000435f40 in Shell (cx=0x7ffff6908c00, op=0x7fffffffd998, envp=0x7fffffffdbb8) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:6121 #16 0x00000000004335e0 in main (argc=1, argv=0x7fffffffdba8, envp=0x7fffffffdbb8) at /home/andre/git/mozilla-central/js/src/shell/js.cpp:6470 ---
Updated•8 years ago
|
Priority: -- → P5
Updated•8 years ago
|
Flags: needinfo?(bbouvier)
Comment hidden (mozreview-request) |
Updated•8 years ago
|
Flags: needinfo?(bbouvier)
Reporter | ||
Comment 2•8 years ago
|
||
IIRC, the underlying issue for this bug is actually in [1]. Do you think it's possible to amend the check to include scripted proxy from other compartments? [1] https://dxr.mozilla.org/mozilla-central/rev/7452437b3ab571b1d60aed4e973d82a1471f72b2/js/src/asmjs/AsmJS.cpp#7418-7419
Comment 3•8 years ago
|
||
(In reply to André Bargull from comment #2) > IIRC, the underlying issue for this bug is actually in [1]. Do you think > it's possible to amend the check to include scripted proxy from other > compartments? > > [1] > https://dxr.mozilla.org/mozilla-central/rev/ > 7452437b3ab571b1d60aed4e973d82a1471f72b2/js/src/asmjs/AsmJS.cpp#7418-7419 Is it the actual underlying issue? What I was observing here is that when validating a SIMD field (like SIMD.Int32x4.add), we redo a getprop SIMD.Int32x4; if it is not the expected one, we should simply fail. To reach the validation of an operation, we must have validated the type itself, at compile-time. So this was assuming that the type had been already validated, and could not change. Your test case showed that it can actually change, thanks to a proxy. I am not sure to see how this relates to other compartments?
Reporter | ||
Comment 4•8 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #3) > Your test case showed that it can actually > change, thanks to a proxy. I am not sure to see how this relates to other > compartments? The test case only fails because the proxy is from a different compartment. So if you replace `(newGlobal().Proxy)` with just `Proxy`, the test case won't throw an assertion error.
Comment 5•8 years ago
|
||
mozreview-review |
Comment on attachment 8801072 [details] Bug 1201124: Unwrap objects before getting their fields in asm.js; https://reviewboard.mozilla.org/r/85880/#review84558 Thanks!
Attachment #8801072 -
Flags: review?(luke) → review+
Comment 6•8 years ago
|
||
Oops, I failed to read the comments; right, it looks like we're failing to detect a scripted proxy here.
Comment 7•8 years ago
|
||
(In reply to André Bargull from comment #4) > The test case only fails because the proxy is from a different compartment. > So if you replace `(newGlobal().Proxy)` with just `Proxy`, the test case > won't throw an assertion error. I see; though I am not too sure about the right fix, then: is it just to unwrap the object given in GetDataProperty? This seems to work here (and prevent the first validation).
Comment hidden (mozreview-request) |
Comment 9•8 years ago
|
||
I *think* the fix is just improving IsScriptedProxy() to correctly recognize cross-compartment-wrapped scripted proxies by adding an UncheckedUnwrap, similar to: http://searchfox.org/mozilla-central/source/js/src/jsexn.cpp#280
Comment 10•8 years ago
|
||
Most callers of IsScriptedProxy do their own unwrapping, and handle unwrapping differently: http://searchfox.org/mozilla-central/source/js/xpconnect/src/ExportHelpers.cpp#420 http://searchfox.org/mozilla-central/source/js/xpconnect/src/ExportHelpers.cpp#502 http://searchfox.org/mozilla-central/source/js/xpconnect/src/Sandbox.cpp#341 http://searchfox.org/mozilla-central/source/js/xpconnect/src/XPCComponents.cpp#2765 So it sounds like we want to keep IsScriptedProxy and the call to Unwrap separated, to be able to have the right behavior in case we're not allowed to access the wrapped object. Having another call to UncheckedUnwrap in IsScriptedProxy would be inefficient (all the callers doing it already). (also a technicality, but the js::{Unc,C}heckedUnwrap functions are not visible from public/Proxy.h where IsScriptedProxy is defined). So I think the right fix is to call unwrap ourselves?
Comment 11•8 years ago
|
||
Oh, I had been assuming IsScriptedProxy() was our own function in AsmJS.cpp; yes I agree then.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 14•8 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3a6d735bef26 Unwrap objects before getting their fields in asm.js; r=luke
Comment 15•8 years ago
|
||
Pushed by bbouvier@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c14157737409 Protect SIMD on arm to prevent bustage; r=me
Comment 16•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3a6d735bef26 https://hg.mozilla.org/mozilla-central/rev/c14157737409
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in
before you can comment on or make changes to this bug.
Description
•