the following sample can crash safari 10.1. class A { }; class B extends A { constructor(a, b) { var f = () => b ? this : {}; if (a) { var val = f() == super(); } else { super(); var val = f(); } } }; for (var i=0; i < 10000; i++) { try { new B(true, true); } catch (e) { } var a = new B(false, true); var c = new B(true, false); }
Created attachment 312144 [details] crash log
Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 ??? 0x00004e19b5e03ca6 0 + 85872332520614 1 com.apple.JavaScriptCore 0x00007fff9877bca0 llint_entry + 27522 2 com.apple.JavaScriptCore 0x00007fff98774f3b vmEntryToJavaScript + 299 3 com.apple.JavaScriptCore 0x00007fff9865017e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 158 4 com.apple.JavaScriptCore 0x00007fff9861dd38 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*) + 15192 5 com.apple.JavaScriptCore 0x00007fff982e3f31 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 337 6 com.apple.WebCore 0x00007fff9db351c9 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*) + 313
<rdar://problem/32602704>
This bug is really interesting. We have code like this: ``` block#1: n: GetClosureVar(..., |this|) // this will load empty JSValue() SetLocal(Cell:@n, locFoo) // Cell check succeeds because JSValue() looks like a cell Branch(#2, #3) Block#3: x: GetLocal(locFoo) y: CheckNotEmpty(@x) ``` However, AI will think that @x is a cell, hence not empty, and we'll crash because we eliminate node @y
(In reply to Saam Barati from comment #4) > This bug is really interesting. We have code like this: > > ``` > block#1: > n: GetClosureVar(..., |this|) // this will load empty JSValue() > SetLocal(Cell:@n, locFoo) // Cell check succeeds because JSValue() looks > like a cell > Branch(#2, #3) > > Block#3: > x: GetLocal(locFoo) > y: CheckNotEmpty(@x) > ``` > > However, AI will think that @x is a cell, hence not empty, and we'll crash > because we eliminate node @y Sounds like AI should model cell checks as admitting empty on 64-bit.
(In reply to Filip Pizlo from comment #5) > (In reply to Saam Barati from comment #4) > > This bug is really interesting. We have code like this: > > > > ``` > > block#1: > > n: GetClosureVar(..., |this|) // this will load empty JSValue() > > SetLocal(Cell:@n, locFoo) // Cell check succeeds because JSValue() looks > > like a cell > > Branch(#2, #3) > > > > Block#3: > > x: GetLocal(locFoo) > > y: CheckNotEmpty(@x) > > ``` > > > > However, AI will think that @x is a cell, hence not empty, and we'll crash > > because we eliminate node @y > > Sounds like AI should model cell checks as admitting empty on 64-bit. I was thinking of either doing this, or we can change the value representation for TDZ sentinel.
Created attachment 312274 [details] WIP still a bunch of assertion failures, I need to audit more code
Created attachment 312337 [details] WIP
Created attachment 312338 [details] WIP
Might be all that's needed
(In reply to Saam Barati from comment #10) > Might be all that's needed it passes release JSC tests
Comment on attachment 312338 [details] WIP Attachment 312338 [details] did not pass jsc-ews (mac): Output: http://webkit-queues.webkit.org/results/3895910 New failing tests: jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-no-ftl jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-no-ftl jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-no-ftl jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-no-llint jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-no-ftl jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-ftl-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-ftl-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-ftl-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-no-llint jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-no-ftl jsc-layout-tests.yaml/js/script-tests/arrowfunction-tdz.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-ftl-no-cjit jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-ftl-no-cjit jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-no-llint jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-no-ftl jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout-dfg-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-no-llint jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout-ftl-eager-no-cjit jsc-layout-tests.yaml/js/script-tests/arrowfunction-supercall.js.layout jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/class-syntax-name.js.layout jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-no-llint jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-no-ftl jsc-layout-tests.yaml/js/script-tests/arrowfunction-superproperty.js.layout-no-llint jsc-layout-tests.yaml/js/script-tests/class-syntax-extends.js.layout-no-cjit jsc-layout-tests.yaml/js/script-tests/class-syntax-super.js.layout-ftl-no-cjit jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-ftl-no-cjit jsc-layout-tests.yaml/js/script-tests/class-constructor-return.js.layout-no-llint
Created attachment 312357 [details] patch
Comment on attachment 312357 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=312357&action=review > Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h:134 > + WTFReportBacktrace(); oops, this should be removed.
Created attachment 312359 [details] patch
Created attachment 312361 [details] patch
Comment on attachment 312361 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=312361&action=review > Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13447 > + FTL_TYPE_CHECK(jsValueValue(cell), edge, SpecSymbol, isNotSymbol(cell)); We can go either way for all these various speculateX functions. They all do lowCell prior to being called, so we're guaranteed that ~SpecCell is not in the set. However, we could leave this code as is (this is how the DFG writes many of its speculation checks). This would leave these functions open to folks doing their own manual checks that haven't updated AI state.
Comment on attachment 312361 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=312361&action=review >> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13447 >> + FTL_TYPE_CHECK(jsValueValue(cell), edge, SpecSymbol, isNotSymbol(cell)); > > We can go either way for all these various speculateX functions. They all do lowCell prior to being called, so we're guaranteed that ~SpecCell is not in the set. However, we could leave this code as is (this is how the DFG writes many of its speculation checks). This would leave these functions open to folks doing their own manual checks that haven't updated AI state. Alternatively, we could assert against the current state of things. So if folks started using this in the way described above, they'd hit the assert, and be forced to write the more general version.
Comment on attachment 312361 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=312361&action=review I think it's correct, but I think there are two style goofs: - The cellCheckTypePassthrough is not specific to the DFG - it's just how cell checks work everywhere, including LLInt, Baseline, and JSValue. I think it should be defined in SpeculatedType.h and follow the same naming convention as everything else (so SpecCellBlahBlah). - There seem to be places where you don't use this type constant and instead you introduce #if's. You should probably use the constant you introduced. > Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h:63 > +// When doing a cell check, this is the type that passes through > +static constexpr SpeculatedType cellCheckTypePassthrough = is64Bit() ? SpecCellOrEmpty : SpecCell; > + I think this should be SpecCellCheck, defined in SpeculatedType.h. Even JSValue's isCell() has this issue, so it's not DFG-specific. I'd call it SpecCellCheck, but I'm also OK with SpecCellChekTypePassthrough, or whatever. Shorter names are better IMO, but I don't care too much. I care more about this not being DFG-specific, and being named SpecBlah like all the other SpeculatedType constants. > Source/JavaScriptCore/dfg/DFGUseKind.h:120 > +#if USE(JSVALUE64) > + return SpecCellOrEmpty; > +#else > + return SpecCell; > +#endif Shouldn't this use SpecCellCheck, or whatever it ends up being called? > Source/JavaScriptCore/dfg/DFGUseKind.h:128 > +#if USE(JSVALUE64) > + return SpecCellOrEmpty | SpecOther; > +#else > return SpecCell | SpecOther; > +#endif Ditto. > Source/JavaScriptCore/dfg/DFGUseKind.h:171 > +#if USE(JSVALUE64) > + return ~SpecCellOrEmpty; > +#else > return ~SpecCell; > +#endif Ditto. >>> Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp:13447 >>> + FTL_TYPE_CHECK(jsValueValue(cell), edge, SpecSymbol, isNotSymbol(cell)); >> >> We can go either way for all these various speculateX functions. They all do lowCell prior to being called, so we're guaranteed that ~SpecCell is not in the set. However, we could leave this code as is (this is how the DFG writes many of its speculation checks). This would leave these functions open to folks doing their own manual checks that haven't updated AI state. > > Alternatively, we could assert against the current state of things. So if folks started using this in the way described above, they'd hit the assert, and be forced to write the more general version. I guess speculateSymbol was modeled after speculateString.
Created attachment 312617 [details] patch Addressed Fil's comments. I also like the name "SpecCellCheck", and decided to use that.
Comment on attachment 312617 [details] patch Clearing flags on attachment: 312617 Committed r218137: <http://trac.webkit.org/changeset/218137>
All reviewed patches have been landed. Closing bug.