From 0b9e37f73e94de3dfda827dd2e191f3e265a9e6a Mon Sep 17 00:00:00 2001 From: Eric Faust Date: Thu, 10 Dec 2015 12:50:35 -0800 Subject: [PATCH] Bug 1230710 - Reenable direct eval and arrow functions in derived class constructors. (r=jorendorff, r=shu) --- js/src/builtin/Eval.cpp | 6 --- js/src/frontend/BytecodeEmitter.cpp | 28 +++++++++++- js/src/frontend/Parser.cpp | 13 +++--- js/src/frontend/SharedContext.h | 3 ++ js/src/js.msg | 1 - js/src/jsobj.cpp | 3 +- .../derivedConstructorArrowEvalBinding.js | 19 ++++++++ .../derivedConstructorArrowEvalClosed.js | 18 ++++++++ .../derivedConstructorArrowEvalEscape.js | 20 +++++++++ ...ConstructorArrowEvalEscapeUninitialized.js | 45 +++++++++++++++++++ .../derivedConstructorArrowEvalGetThis.js | 17 +++++++ ...ivedConstructorArrowEvalNestedSuperCall.js | 41 +++++++++++++++++ .../derivedConstructorArrowEvalSuperCall.js | 25 +++++++++++ .../Class/derivedConstructorDisabled.js | 38 ---------------- js/src/tests/js1_8_5/reflect-parse/classes.js | 3 -- js/src/vm/Debugger.cpp | 7 --- js/src/vm/Interpreter-inl.h | 4 ++ js/src/vm/Interpreter.cpp | 8 +--- js/src/vm/ScopeObject.h | 2 +- js/src/vm/Xdr.h | 4 +- 20 files changed, 230 insertions(+), 75 deletions(-) create mode 100644 js/src/tests/ecma_6/Class/derivedConstructorArrowEvalBinding.js create mode 100644 js/src/tests/ecma_6/Class/derivedConstructorArrowEvalClosed.js create mode 100644 js/src/tests/ecma_6/Class/derivedConstructorArrowEvalEscape.js create mode 100644 js/src/tests/ecma_6/Class/derivedConstructorArrowEvalEscapeUninitialized.js create mode 100644 js/src/tests/ecma_6/Class/derivedConstructorArrowEvalGetThis.js create mode 100644 js/src/tests/ecma_6/Class/derivedConstructorArrowEvalNestedSuperCall.js create mode 100644 js/src/tests/ecma_6/Class/derivedConstructorArrowEvalSuperCall.js delete mode 100644 js/src/tests/ecma_6/Class/derivedConstructorDisabled.js diff --git a/js/src/builtin/Eval.cpp b/js/src/builtin/Eval.cpp index b925890a7be..49b5e4d04a7 100644 --- a/js/src/builtin/Eval.cpp +++ b/js/src/builtin/Eval.cpp @@ -238,12 +238,6 @@ EvalKernel(JSContext* cx, const CallArgs& args, EvalType evalType, AbstractFrame return false; } - if (evalType == DIRECT_EVAL && caller.script()->isDerivedClassConstructor()) { - JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_DISABLED_DERIVED_CLASS, - "direct eval"); - return false; - } - // ES5 15.1.2.1 step 1. if (args.length() < 1) { args.rval().setUndefined(); diff --git a/js/src/frontend/BytecodeEmitter.cpp b/js/src/frontend/BytecodeEmitter.cpp index 7f2dac63949..271466db5ee 100644 --- a/js/src/frontend/BytecodeEmitter.cpp +++ b/js/src/frontend/BytecodeEmitter.cpp @@ -3475,10 +3475,34 @@ BytecodeEmitter::emitSetThis(ParseNode* pn) JSOp setOp = name->getOp(); + // Handle the eval case. Only accept the strict variant, as eval in a + // derived class constructor must be strict. + if (setOp == JSOP_STRICTSETNAME) { + if (!emitAtomOp(name, JSOP_GETNAME)) + return false; + if (!emit1(JSOP_CHECKTHISREINIT)) + return false; + if (!emit1(JSOP_POP)) + return false; + + if (!emitAtomOp(name, JSOP_BINDNAME)) + return false; + if (!emit1(JSOP_SWAP)) + return false; + + return emitAtomOp(name, setOp); + } + JSOp getOp; switch (setOp) { - case JSOP_SETLOCAL: getOp = JSOP_GETLOCAL; break; - case JSOP_SETALIASEDVAR: getOp = JSOP_GETALIASEDVAR; break; + case JSOP_SETLOCAL: + getOp = JSOP_GETLOCAL; + setOp = JSOP_INITLEXICAL; + break; + case JSOP_SETALIASEDVAR: + getOp = JSOP_GETALIASEDVAR; + setOp = JSOP_INITALIASEDLEXICAL; + break; default: MOZ_CRASH("Unexpected op"); } diff --git a/js/src/frontend/Parser.cpp b/js/src/frontend/Parser.cpp index ba5f3f75157..3032e976a23 100644 --- a/js/src/frontend/Parser.cpp +++ b/js/src/frontend/Parser.cpp @@ -125,8 +125,12 @@ SharedContext::computeAllowSyntax(JSObject* staticScope) // Any function supports new.target. allowNewTarget_ = true; allowSuperProperty_ = it.fun().allowSuperProperty(); - if (it.maybeFunctionBox()) + if (it.maybeFunctionBox()) { superScopeAlreadyNeedsHomeObject_ = it.maybeFunctionBox()->needsHomeObject(); + allowSuperCall_ = it.maybeFunctionBox()->isDerivedClassConstructor(); + } else { + allowSuperCall_ = it.fun().isDerivedClassConstructor(); + } break; } } @@ -7470,11 +7474,6 @@ Parser::assignExpr(InHandling inHandling, YieldHandling yieldHandl if (!tokenStream.peekToken(&ignored, TokenStream::Operand)) return null(); - if (pc->sc->isFunctionBox() && pc->sc->asFunctionBox()->isDerivedClassConstructor()) { - report(ParseError, false, null(), JSMSG_DISABLED_DERIVED_CLASS, "arrow functions"); - return null(); - } - Node arrowFunc = functionDef(inHandling, yieldHandling, nullptr, Arrow, NotGenerator); if (!arrowFunc) return null(); @@ -8890,7 +8889,7 @@ Parser::memberExpr(YieldHandling yieldHandling, TripledotHandling tt == TOK_NO_SUBS_TEMPLATE) { if (handler.isSuperBase(lhs)) { - if (!pc->sc->isFunctionBox() || !pc->sc->asFunctionBox()->isDerivedClassConstructor()) { + if (!pc->sc->allowSuperCall()) { report(ParseError, false, null(), JSMSG_BAD_SUPERCALL); return null(); } diff --git a/js/src/frontend/SharedContext.h b/js/src/frontend/SharedContext.h index 4cb879f107d..7b1256ffa8c 100644 --- a/js/src/frontend/SharedContext.h +++ b/js/src/frontend/SharedContext.h @@ -202,6 +202,7 @@ class SharedContext bool allowNewTarget_; bool allowSuperProperty_; + bool allowSuperCall_; bool inWith_; bool needsThisTDZChecks_; bool superScopeAlreadyNeedsHomeObject_; @@ -217,6 +218,7 @@ class SharedContext thisBinding_(ThisBinding::Global), allowNewTarget_(false), allowSuperProperty_(false), + allowSuperCall_(false), inWith_(false), needsThisTDZChecks_(false), superScopeAlreadyNeedsHomeObject_(false) @@ -244,6 +246,7 @@ class SharedContext bool allowNewTarget() const { return allowNewTarget_; } bool allowSuperProperty() const { return allowSuperProperty_; } + bool allowSuperCall() const { return allowSuperCall_; } bool inWith() const { return inWith_; } bool needsThisTDZChecks() const { return needsThisTDZChecks_; } diff --git a/js/src/js.msg b/js/src/js.msg index 2d976770719..389093f7452 100644 --- a/js/src/js.msg +++ b/js/src/js.msg @@ -105,7 +105,6 @@ MSG_DEF(JSMSG_INVALID_ARG_TYPE, 3, JSEXN_TYPEERR, "Invalid type: {0} can' MSG_DEF(JSMSG_TERMINATED, 1, JSEXN_ERR, "Script terminated by timeout at:\n{0}") MSG_DEF(JSMSG_PROTO_NOT_OBJORNULL, 1, JSEXN_TYPEERR, "{0}.prototype is not an object or null") MSG_DEF(JSMSG_CANT_CALL_CLASS_CONSTRUCTOR, 0, JSEXN_TYPEERR, "class constructors must be invoked with |new|") -MSG_DEF(JSMSG_DISABLED_DERIVED_CLASS, 1, JSEXN_INTERNALERR, "{0} temporarily disallowed in derived class constructors") MSG_DEF(JSMSG_UNINITIALIZED_THIS, 1, JSEXN_REFERENCEERR, "|this| used uninitialized in {0} class constructor") MSG_DEF(JSMSG_BAD_DERIVED_RETURN, 1, JSEXN_TYPEERR, "derived class constructor returned invalid value {0}") diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp index ee75835d552..e3ef0309446 100644 --- a/js/src/jsobj.cpp +++ b/js/src/jsobj.cpp @@ -2231,11 +2231,12 @@ js::LookupNameUnqualified(JSContext* cx, HandlePropertyName name, HandleObject s // See note above RuntimeLexicalErrorObject. if (pobj == scope) { - if (IsUninitializedLexicalSlot(scope, shape)) { + if (name != cx->names().dotThis && IsUninitializedLexicalSlot(scope, shape)) { scope = RuntimeLexicalErrorObject::create(cx, scope, JSMSG_UNINITIALIZED_LEXICAL); if (!scope) return false; } else if (scope->is() && !scope->is() && !shape->writable()) { + MOZ_ASSERT(name != cx->names().dotThis); scope = RuntimeLexicalErrorObject::create(cx, scope, JSMSG_BAD_CONST_ASSIGN); if (!scope) return false; diff --git a/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalBinding.js b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalBinding.js new file mode 100644 index 00000000000..bd87f8f477c --- /dev/null +++ b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalBinding.js @@ -0,0 +1,19 @@ +// Make sure it doesn't matter when we make the arrow function +var test = ` + +new class extends class { } { + constructor() { + let arrow = () => this; + assertThrowsInstanceOf(arrow, ReferenceError); + super(); + assertEq(arrow(), this); + } +}(); + +`; + +if (classesEnabled()) + eval(test); + +if (typeof reportCompare === 'function') + reportCompare(0,0,"OK"); diff --git a/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalClosed.js b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalClosed.js new file mode 100644 index 00000000000..7899aeabdd3 --- /dev/null +++ b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalClosed.js @@ -0,0 +1,18 @@ +var test = ` + +new class extends class { } { + constructor() { + let a1 = () => this; + let a2 = (() => super()); + assertThrowsInstanceOf(a1, ReferenceError); + assertEq(a2(), a1()); + } +}(); + +`; + +if (classesEnabled()) + eval(test); + +if (typeof reportCompare === 'function') + reportCompare(0,0,"OK"); diff --git a/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalEscape.js b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalEscape.js new file mode 100644 index 00000000000..8284bac9d5a --- /dev/null +++ b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalEscape.js @@ -0,0 +1,20 @@ +var test = ` + +let arrow; + +class foo extends class { } { + constructor() { + arrow = () => this; + super(); + } +} + +assertEq(new foo(), arrow()); + +`; + +if (classesEnabled()) + eval(test); + +if (typeof reportCompare === 'function') + reportCompare(0,0,"OK"); diff --git a/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalEscapeUninitialized.js b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalEscapeUninitialized.js new file mode 100644 index 00000000000..010f0dd1b7e --- /dev/null +++ b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalEscapeUninitialized.js @@ -0,0 +1,45 @@ +var test = ` + +let superArrow; +let thisArrow; + +let thisStash; + +class base { + constructor() { + // We run this constructor twice as part of the double init check + if (!thisStash) + thisStash = {prop:45}; + return thisStash; + } +} + +class foo extends base { + constructor() { + superArrow = (()=>super()); + thisArrow = ()=>this; + } +} + +// Populate the arrow function saves. Since we never invoke super(), we throw +assertThrowsInstanceOf(()=>new foo(), ReferenceError); + +// No |this| binding in the closure, yet +assertThrowsInstanceOf(thisArrow, ReferenceError); + +// call super() +superArrow(); + +// Can't call it twice +assertThrowsInstanceOf(superArrow, ReferenceError); + +// Oh look, |this| is populated, now. +assertEq(thisArrow(), thisStash); + +`; + +if (classesEnabled()) + eval(test); + +if (typeof reportCompare === 'function') + reportCompare(0,0,"OK"); diff --git a/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalGetThis.js b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalGetThis.js new file mode 100644 index 00000000000..dbc69beb8a1 --- /dev/null +++ b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalGetThis.js @@ -0,0 +1,17 @@ +var test = ` + +new class extends class { } { + constructor() { + super(); + assertEq(this, (()=>this)()); + assertEq(this, eval("this")); + } +}(); + +`; + +if (classesEnabled()) + eval(test); + +if (typeof reportCompare === 'function') + reportCompare(0,0,"OK"); diff --git a/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalNestedSuperCall.js b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalNestedSuperCall.js new file mode 100644 index 00000000000..60acffd14a6 --- /dev/null +++ b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalNestedSuperCall.js @@ -0,0 +1,41 @@ +var test = ` + +new class extends class { } { + constructor() { + (()=>eval("super()"))(); + assertEq(this, eval("this")); + assertEq(this, (()=>this)()); + } +}(); + +new class extends class { } { + constructor() { + (()=>(()=>super())())(); + assertEq(this, eval("this")); + assertEq(this, (()=>this)()); + } +}(); + +new class extends class { } { + constructor() { + eval("(()=>super())()"); + assertEq(this, eval("this")); + assertEq(this, (()=>this)()); + } +}(); + +new class extends class { } { + constructor() { + eval("eval('super()')"); + assertEq(this, eval("this")); + assertEq(this, (()=>this)()); + } +}(); + +`; + +if (classesEnabled()) + eval(test); + +if (typeof reportCompare === 'function') + reportCompare(0,0,"OK"); diff --git a/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalSuperCall.js b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalSuperCall.js new file mode 100644 index 00000000000..76bb184243c --- /dev/null +++ b/js/src/tests/ecma_6/Class/derivedConstructorArrowEvalSuperCall.js @@ -0,0 +1,25 @@ +var test = ` + +new class extends class { } { + constructor() { + assertEq(eval("super(); this"), this); + assertEq(this, eval("this")); + assertEq(this, (()=>this)()); + } +}(); + +new class extends class { } { + constructor() { + (()=>super())(); + assertEq(this, eval("this")); + assertEq(this, (()=>this)()); + } +}(); + +`; + +if (classesEnabled()) + eval(test); + +if (typeof reportCompare === 'function') + reportCompare(0,0,"OK"); diff --git a/js/src/tests/ecma_6/Class/derivedConstructorDisabled.js b/js/src/tests/ecma_6/Class/derivedConstructorDisabled.js deleted file mode 100644 index 4b86408fc12..00000000000 --- a/js/src/tests/ecma_6/Class/derivedConstructorDisabled.js +++ /dev/null @@ -1,38 +0,0 @@ -// |reftest| skip-if(!xulRuntime.shell) - -var test = ` - -class base { - constructor() { - eval(''); - (()=>0)(); - } -} - -class derived extends base { - constructor() { - eval(''); - } -} - -// Make sure eval and arrows are still valid in non-derived constructors. -new base(); - - -// Eval throws in derived class constructors, in both class expressions and -// statements. -assertThrowsInstanceOf((() => new derived()), InternalError); -assertThrowsInstanceOf((() => new class extends base { constructor() { eval('') } }()), InternalError); - -var g = newGlobal(); -var dbg = Debugger(g); -dbg.onDebuggerStatement = function(frame) { assertThrowsInstanceOf(()=>frame.eval(''), InternalError); }; - -g.eval("new class foo extends null { constructor() { debugger; return {}; } }()"); -`; - -if (classesEnabled()) - eval(test); - -if (typeof reportCompare === 'function') - reportCompare(0,0,"OK"); diff --git a/js/src/tests/js1_8_5/reflect-parse/classes.js b/js/src/tests/js1_8_5/reflect-parse/classes.js index 9a230be8db0..5fc2bc4fdf2 100644 --- a/js/src/tests/js1_8_5/reflect-parse/classes.js +++ b/js/src/tests/js1_8_5/reflect-parse/classes.js @@ -132,9 +132,6 @@ function testClasses() { assertClass("class NAME { }", []); assertClass("class NAME extends null { }", [], lit(null)); - // For now, disallow arrow functions in derived class constructors - assertClassError("class NAME extends null { constructor() { (() => 0); }", InternalError); - // Derived class constructor must have curly brackets assertClassError("class NAME extends null { constructor() 1 }", SyntaxError); diff --git a/js/src/vm/Debugger.cpp b/js/src/vm/Debugger.cpp index a9b755326a2..458e1ff4d33 100644 --- a/js/src/vm/Debugger.cpp +++ b/js/src/vm/Debugger.cpp @@ -6724,13 +6724,6 @@ DebuggerGenericEval(JSContext* cx, const char* fullMethodName, const Value& code MOZ_ASSERT_IF(iter, !scope); MOZ_ASSERT_IF(!iter, scope && IsGlobalLexicalScope(scope)); - if (iter && iter->script()->isDerivedClassConstructor()) { - MOZ_ASSERT(iter->isFunctionFrame() && iter->calleeTemplate()->isClassConstructor()); - JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_DISABLED_DERIVED_CLASS, - "debugger eval"); - return false; - } - /* Check the first argument, the eval code string. */ if (!code.isString()) { JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_NOT_EXPECTED_TYPE, diff --git a/js/src/vm/Interpreter-inl.h b/js/src/vm/Interpreter-inl.h index 498ac83a6cf..0ac4c01ceae 100644 --- a/js/src/vm/Interpreter-inl.h +++ b/js/src/vm/Interpreter-inl.h @@ -204,6 +204,10 @@ FetchName(JSContext* cx, HandleObject obj, HandleObject obj2, HandlePropertyName } } + // We do our own explicit checking for |this| + if (name == cx->names().dotThis) + return true; + // NAME operations are the slow paths already, so unconditionally check // for uninitialized lets. return CheckUninitializedLexical(cx, name, vp); diff --git a/js/src/vm/Interpreter.cpp b/js/src/vm/Interpreter.cpp index 5d3e633cbb2..fecea72d1cd 100644 --- a/js/src/vm/Interpreter.cpp +++ b/js/src/vm/Interpreter.cpp @@ -3230,10 +3230,7 @@ CASE(JSOP_SETLOCAL) { uint32_t i = GET_LOCALNO(REGS.pc); - // Derived class constructors store the TDZ Value in the .this slot - // before a super() call. - MOZ_ASSERT_IF(!script->isDerivedClassConstructor(), - !IsUninitializedLexical(REGS.fp()->unaliasedLocal(i))); + MOZ_ASSERT(!IsUninitializedLexical(REGS.fp()->unaliasedLocal(i))); REGS.fp()->unaliasedLocal(i) = REGS.sp[-1]; } @@ -4843,9 +4840,6 @@ js::ThrowUninitializedThis(JSContext* cx, AbstractFramePtr frame) { RootedFunction fun(cx, frame.callee()); - MOZ_ASSERT(fun->isClassConstructor()); - MOZ_ASSERT(fun->nonLazyScript()->isDerivedClassConstructor()); - const char* name = "anonymous"; JSAutoByteString str; if (fun->atom()) { diff --git a/js/src/vm/ScopeObject.h b/js/src/vm/ScopeObject.h index efe72bea340..26005a37621 100644 --- a/js/src/vm/ScopeObject.h +++ b/js/src/vm/ScopeObject.h @@ -953,7 +953,7 @@ class ClonedBlockObject : public BlockObject // Attempting to access anything on this scope throws the appropriate // ReferenceError. // -// ES6 'const' bindings induce a runtime assignment when assigned to outside +// ES6 'const' bindings induce a runtime error when assigned to outside // of initialization, regardless of strictness. class RuntimeLexicalErrorObject : public ScopeObject { diff --git a/js/src/vm/Xdr.h b/js/src/vm/Xdr.h index 04616576c8d..182390ef8ed 100644 --- a/js/src/vm/Xdr.h +++ b/js/src/vm/Xdr.h @@ -29,11 +29,11 @@ namespace js { * * https://developer.mozilla.org/en-US/docs/SpiderMonkey/Internals/Bytecode */ -static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 328; +static const uint32_t XDR_BYTECODE_VERSION_SUBTRAHEND = 329; static const uint32_t XDR_BYTECODE_VERSION = uint32_t(0xb973c0de - XDR_BYTECODE_VERSION_SUBTRAHEND); -static_assert(JSErr_Limit == 425, +static_assert(JSErr_Limit == 424, "GREETINGS, POTENTIAL SUBTRAHEND INCREMENTER! If you added or " "removed MSG_DEFs from js.msg, you should increment " "XDR_BYTECODE_VERSION_SUBTRAHEND and update this assertion's "