Skip to content

Commit

Permalink
OS#15334058: Fix class extends null and this access for super pro…
Browse files Browse the repository at this point in the history 
…perty reference

We were supporting some old version of the spec language around classes which extend from null. Instead of leaving `this` undeclared, we were constructing an object and assigning it the `this` binding. This meant we were not throwing ReferenceErrors on access to `this` when we should have been.

Also fixed the case where a super property reference failed to access `this` resulting in trying to load a property from null.

Fixes:
https://microsoft.visualstudio.com/OS/_workitems/edit/15334058
  • Loading branch information
@boingoing
boingoing committed Feb 1, 2018
1 parent 7443cf6 commit 85af2a8
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 76 deletions.
51 changes: 20 additions & 31 deletions lib/Runtime/ByteCode/ByteCodeEmitter.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2135,31 +2135,20 @@ void ByteCodeGenerator::LoadThisObject(FuncInfo *funcInfo, bool thisLoadedFromPa

if (this->scriptContext->GetConfig()->IsES6ClassAndExtendsEnabled() && funcInfo->IsClassConstructor())
{
// Derived class constructors initialize 'this' to be Undecl except "extends null" cases
// Derived class constructors initialize 'this' to be Undecl
// - we'll check this value during a super call and during 'this' access
//
// Base class constructors or "extends null" cases initialize 'this' to a new object using new.target
// Base class constructors initialize 'this' to a new object using new.target
if (funcInfo->IsBaseClassConstructor())
{
EmitBaseClassConstructorThisObject(funcInfo);
Symbol* newTargetSym = funcInfo->GetNewTargetSymbol();
Assert(newTargetSym);

this->Writer()->Reg2(Js::OpCode::NewScObjectNoCtorFull, thisSym->GetLocation(), newTargetSym->GetLocation());
}
else
{
Js::ByteCodeLabel thisLabel = this->Writer()->DefineLabel();
Js::ByteCodeLabel skipLabel = this->Writer()->DefineLabel();

Js::RegSlot tmpReg = funcInfo->AcquireTmpRegister();
this->Writer()->Reg1(Js::OpCode::LdFuncObj, tmpReg);
this->Writer()->BrReg1(Js::OpCode::BrOnBaseConstructorKind, thisLabel, tmpReg); // branch when [[ConstructorKind]]=="base"
funcInfo->ReleaseTmpRegister(tmpReg);

this->m_writer.Reg1(Js::OpCode::InitUndecl, thisSym->GetLocation()); // not "extends null" case
this->Writer()->Br(Js::OpCode::Br, skipLabel);

this->Writer()->MarkLabel(thisLabel);
EmitBaseClassConstructorThisObject(funcInfo); // "extends null" case

this->Writer()->MarkLabel(skipLabel);
this->m_writer.Reg1(Js::OpCode::InitUndecl, thisSym->GetLocation());
}
}
else if (!funcInfo->IsGlobalFunction())
Expand Down Expand Up @@ -2208,11 +2197,11 @@ void ByteCodeGenerator::LoadSuperConstructorObject(FuncInfo *funcInfo)
{
Symbol* superConstructorSym = funcInfo->GetSuperConstructorSymbol();
Assert(superConstructorSym);
Assert(!funcInfo->IsLambda());
Assert(!funcInfo->IsLambda());
Assert(funcInfo->IsDerivedClassConstructor());

m_writer.Reg1(Js::OpCode::LdFuncObj, superConstructorSym->GetLocation());
}
m_writer.Reg1(Js::OpCode::LdFuncObj, superConstructorSym->GetLocation());
}

void ByteCodeGenerator::LoadSuperObject(FuncInfo *funcInfo)
{
Expand Down Expand Up @@ -2339,11 +2328,6 @@ void ByteCodeGenerator::EmitClassConstructorEndCode(FuncInfo *funcInfo)
}
}

void ByteCodeGenerator::EmitBaseClassConstructorThisObject(FuncInfo *funcInfo)
{
this->Writer()->Reg2(Js::OpCode::NewScObjectNoCtorFull, funcInfo->GetThisSymbol()->GetLocation(), funcInfo->GetNewTargetSymbol()->GetLocation());
}

void ByteCodeGenerator::EmitThis(FuncInfo *funcInfo, Js::RegSlot lhsLocation, Js::RegSlot fromRegister)
{
if (funcInfo->byteCodeFunction->GetIsStrictMode() && !funcInfo->IsGlobalFunction() && !funcInfo->IsLambda())
Expand Down Expand Up @@ -4917,14 +4901,14 @@ void ByteCodeGenerator::EmitPropLoadThis(Js::RegSlot lhsLocation, ParseNode *pno
}
else
{
this->EmitPropLoad(lhsLocation, pnode->sxPid.sym, pnode->sxPid.pid, funcInfo, true);
this->EmitPropLoad(lhsLocation, pnode->sxPid.sym, pnode->sxPid.pid, funcInfo, true);

if ((!sym || sym->GetNeedDeclaration()) && chkUndecl)
{
this->Writer()->Reg1(Js::OpCode::ChkUndecl, lhsLocation);
if ((!sym || sym->GetNeedDeclaration()) && chkUndecl)
{
this->Writer()->Reg1(Js::OpCode::ChkUndecl, lhsLocation);
}
}
}
}

void ByteCodeGenerator::EmitPropStoreForSpecialSymbol(Js::RegSlot rhsLocation, Symbol *sym, IdentPtr pid, FuncInfo *funcInfo, bool init)
{
Expand Down Expand Up @@ -6805,6 +6789,11 @@ void EmitAssignment(

if (ByteCodeGenerator::IsSuper(lhs->sxBin.pnode1))
{
// We need to emit the 'this' node for the super reference even if we aren't planning to use the 'this' value.
// This is because we might be in a derived class constructor where we haven't yet called super() to bind the 'this' value.
// See ecma262 abstract operation 'MakeSuperPropertyReference'
Emit(lhs->sxSuperReference.pnodeThis, byteCodeGenerator, funcInfo, false);
funcInfo->ReleaseLoc(lhs->sxSuperReference.pnodeThis);
targetLocation = byteCodeGenerator->EmitLdObjProto(Js::OpCode::LdHomeObjProto, targetLocation, funcInfo);
}

Expand Down
5 changes: 0 additions & 5 deletions lib/Runtime/Language/JavascriptOperators.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7093,11 +7093,6 @@ namespace Js
ctorProtoObj->EnsureProperty(Js::PropertyIds::constructor);
ctorProtoObj->SetEnumerable(Js::PropertyIds::constructor, FALSE);

if (ScriptFunctionBase::Is(constructor))
{
ScriptFunctionBase::FromVar(constructor)->GetFunctionInfo()->SetBaseConstructorKind();
}

break;
}

Expand Down
25 changes: 0 additions & 25 deletions test/Basics/SpecialSymbolCapture.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -516,31 +516,6 @@ var tests = [
}
}
assert.throws(() => new DerivedSuper(), TypeError, "Class derived from null can't make a super call", "Function is not a constructor");

class DerivedEmpty extends null {
constructor() { }
}
assert.areEqual(DerivedEmpty, new DerivedEmpty().constructor, "Default instance for 'extends null' case is a real instance of the derived class");

var called = false;
class DerivedVerifyNewTarget extends null {
constructor() {
assert.areEqual(DerivedVerifyNewTarget, new.target, "Derived class called as new expression gets new.target");
called = true;
}
}
assert.areEqual(DerivedVerifyNewTarget, new DerivedVerifyNewTarget().constructor, "Default instance for 'extends null' case is a real instance of the derived class");
assert.isTrue(called, "Constructor was actually called");

called = false;
class DerivedVerifyThis extends null {
constructor() {
assert.areEqual(DerivedVerifyThis, this.constructor, "Derived from null class called as new expression gets this instance of the derived class");
called = true;
}
}
assert.areEqual(DerivedVerifyThis, new DerivedVerifyThis().constructor, "Default instance for 'extends null' case is a real instance of the derived class");
assert.isTrue(called, "Constructor was actually called");
}
},
{
Expand Down
40 changes: 25 additions & 15 deletions test/es6/ES6Class_BaseClassConstruction.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,16 +108,13 @@ var tests = [
}
},
{
name: "Class that extends null binds this in constructor",
name: "Class that extends null doesn't bind 'this' implicitly",
body: function () {
var thisVal;
class B extends null {
constructor() { thisVal = this; }
constructor() { }
}

var b = new B();
assert.areEqual(true, b instanceof B);
assert.areEqual(thisVal, b);
assert.throws(() => new B(), ReferenceError, "implicit return of 'this' throws", "Use before declaration");
}
},
{
Expand All @@ -140,27 +137,40 @@ var tests = [
}
},
{
name: "Class that extends null with implicit return in constructor",
name: "Class that extends null with explicit return in constructor",
body: function () {
class A extends null {
constructor() {}
constructor() { return {}; }
}

var a;
assert.doesNotThrow(()=>{a = new A()});
assert.areEqual(A.prototype, Object.getPrototypeOf(a));
assert.areEqual(Object.prototype, Object.getPrototypeOf(a));
}
},
{
name: "Class that extends null with explicit return in constructor",
name: "Class that extends null with super references",
body: function () {
class A extends null {
constructor() { return {}; }
constructor() { super['prop'] = 'something'; return {}; }
}

var a;
assert.doesNotThrow(()=>{a = new A()});
assert.areEqual(Object.prototype, Object.getPrototypeOf(a));
assert.throws(() => new A(), ReferenceError, "super reference loads 'this' and throws if it's undecl ", "Use before declaration");

var prop = 'prop';
class B extends null {
constructor() { super[prop] = 'something'; return {}; }
}
assert.throws(() => new B(), ReferenceError, "super reference loads 'this' and throws if it's undecl ", "Use before declaration");

class C extends null {
constructor() { super['prop']; return {}; }
}
assert.throws(() => new C(), ReferenceError, "super reference loads 'this' and throws if it's undecl ", "Use before declaration");

class D extends null {
constructor() { super[prop]; return {}; }
}
assert.throws(() => new D(), ReferenceError, "super reference loads 'this' and throws if it's undecl ", "Use before declaration");
}
},
];
Expand Down

0 comments on commit 85af2a8

Please sign in to comment.