Fixes #2809: Throw TypeError immediately when getter/setter is not callable or when this object is null or undefined.

lib/Runtime/Library/JavascriptObject.cpp

@@ -1302,12 +1302,11 @@ namespace Js
 #if ENABLE_COPYONACCESS_ARRAY
         JavascriptLibrary::CheckAndConvertCopyOnAccessNativeIntArray<Var>(args[0]);
 #endif
-        Var thisArg = JavascriptOperators::OP_GetThisNoFastPath(args[0], 0, scriptContext);
-        RecyclableObject* obj = RecyclableObject::FromVar(thisArg);
-
-        Var propertyKey = args.Info.Count > 1 ? args[1] : obj->GetLibrary()->GetUndefined();
-        const PropertyRecord* propertyRecord;
-        JavascriptConversion::ToPropertyKey(propertyKey, scriptContext, &propertyRecord);
+        RecyclableObject* obj = nullptr;
+        if (!JavascriptConversion::ToObject(args[0], scriptContext, &obj))
+        {
+            JavascriptError::ThrowTypeError(scriptContext, JSERR_This_NullOrUndefined, _u("Object.prototype.__defineGetter__"));
+        }
 
         Var getterFunc = args.Info.Count > 2 ? args[2] : obj->GetLibrary()->GetUndefined();
 
@@ -1316,6 +1315,10 @@ namespace Js
             JavascriptError::ThrowTypeError(scriptContext, JSERR_FunctionArgument_NeedFunction, _u("Object.prototype.__defineGetter__"));
         }
 
+        Var propertyKey = args.Info.Count > 1 ? args[1] : obj->GetLibrary()->GetUndefined();
+        const PropertyRecord* propertyRecord;
+        JavascriptConversion::ToPropertyKey(propertyKey, scriptContext, &propertyRecord);
+
         PropertyDescriptor propertyDescriptor;
         propertyDescriptor.SetEnumerable(true);
         propertyDescriptor.SetConfigurable(true);
@@ -1340,12 +1343,11 @@ namespace Js
         // For browser interop, simulate LdThis by calling OP implementation directly.
         // Do not have module id here so use the global id, 0.
         //
-        Var thisArg = JavascriptOperators::OP_GetThisNoFastPath(args[0], 0, scriptContext);
-        RecyclableObject* obj = RecyclableObject::FromVar(thisArg);
-
-        Var propertyKey = args.Info.Count > 1 ? args[1] : obj->GetLibrary()->GetUndefined();
-        const PropertyRecord* propertyRecord;
-        JavascriptConversion::ToPropertyKey(propertyKey, scriptContext, &propertyRecord);
+        RecyclableObject* obj = nullptr;
+        if (!JavascriptConversion::ToObject(args[0], scriptContext, &obj))
+        {
+            JavascriptError::ThrowTypeError(scriptContext, JSERR_This_NullOrUndefined, _u("Object.prototype.__defineSetter__"));
+        }
 
         Var setterFunc = args.Info.Count > 2 ? args[2] : obj->GetLibrary()->GetUndefined();
 
@@ -1354,6 +1356,10 @@ namespace Js
             JavascriptError::ThrowTypeError(scriptContext, JSERR_FunctionArgument_NeedFunction, _u("Object.prototype.__defineSetter__"));
         }
 
+        Var propertyKey = args.Info.Count > 1 ? args[1] : obj->GetLibrary()->GetUndefined();
+        const PropertyRecord* propertyRecord;
+        JavascriptConversion::ToPropertyKey(propertyKey, scriptContext, &propertyRecord);
+
         PropertyDescriptor propertyDescriptor;
         propertyDescriptor.SetEnumerable(true);
         propertyDescriptor.SetConfigurable(true);

test/LetConst/shadowedsetter.js

@@ -5,7 +5,7 @@
 
 evaluate = WScript.LoadScript;
 
-__defineSetter__("x", function () { });
+this.__defineSetter__("x", function () { });
 
 evaluate(`
   let x = 'let';

test/es6/definegettersetter.baseline

@@ -16,7 +16,7 @@ PASSED
 PASSED
 *** Running test #9 (test09): __defineGetter__ and __defineSetter__ both have length 2 and __lookupGetter__ and __lookupSetter__ both have length 1
 PASSED
-*** Running test #10 (test10): __defineGetter__ and __defineSetter__ should convert null/undefined this argument to global object
+*** Running test #10 (test10): __defineGetter__ and __defineSetter__ should throw TypeError with null/undefined this argument
 PASSED
 *** Running test #11 (test11): __lookupGetter__ and __lookupSetter__ find getters and setters of the given name on the calling object respectively
 PASSED
@@ -25,4 +25,6 @@ PASSED
 *** Running test #13 (test13): __lookupGetter__ and __lookupSetter__ should look for accessors up the prototype chain
 undefined
 PASSED
-Summary of tests: total executed: 13; passed: 13; failed: 0
+*** Running test #14 (test14): __defineGetter__ and __defineSetter__ should throw TypeError when the object specified as getter/setter is not callable
+PASSED
+Summary of tests: total executed: 14; passed: 14; failed: 0

test/es6/definegettersetter.js

@@ -206,22 +206,17 @@ var tests = {
         }
     },
     test10: {
-        name: "__defineGetter__ and __defineSetter__ should convert null/undefined this argument to global object",
+        name: "__defineGetter__ and __defineSetter__ should throw TypeError with null/undefined this argument",
         body: function () {
-            Object.prototype.__defineGetter__.call(undefined, "test10_undefined_getter", function () { return undefined; });
-            Object.prototype.__defineGetter__.call(null, "test10_null_getter", function () { return undefined; });
-            Object.prototype.__defineSetter__.call(undefined, "test10_undefined_setter", function (v) { });
-            Object.prototype.__defineSetter__.call(null, "test10_null_setter", function (v) { });
-
-            assert.isTrue(globalObject.hasOwnProperty("test10_undefined_getter"), "global object should now have a getter named test10_undefined_getter");
-            assert.isTrue(globalObject.hasOwnProperty("test10_null_getter"), "global object should now have a getter named test10_null_getter");
-            assert.isTrue(globalObject.hasOwnProperty("test10_undefined_setter"), "global object should now have a setter named test10_undefined_setter");
-            assert.isTrue(globalObject.hasOwnProperty("test10_null_setter"), "global object should now have a setter named test10_null_setter");
-
-            delete globalObject["test10_undefined_getter"];
-            delete globalObject["test10_null_getter"];
-            delete globalObject["test10_undefined_setter"];
-            delete globalObject["test10_null_setter"];
+            assert.throws(() => { Object.prototype.__defineGetter__.call(undefined, "test10_undefined_getter", function () { return undefined; }); }, TypeError, "__defineGetter__ should throw TypeError when this object is Undefined.");
+            assert.throws(() => { Object.prototype.__defineGetter__.call(null, "test10_null_getter", function () { return undefined; }); }, TypeError, "__defineGetter__ should throw TypeError when this object is Null.");
+            assert.throws(() => { Object.prototype.__defineSetter__.call(undefined, "test10_undefined_setter", function (v) { }); }, TypeError, "__defineSetter__ should throw TypeError when this object is Undefined.");
+            assert.throws(() => { Object.prototype.__defineSetter__.call(null, "test10_null_setter", function (v) { }); }, TypeError, "__defineSetter__ should throw TypeError when this object is Null.");          
+
+            assert.isFalse(globalObject.hasOwnProperty("test10_undefined_getter"), "global object should now have a getter named test10_undefined_getter");
+            assert.isFalse(globalObject.hasOwnProperty("test10_null_getter"), "global object should now have a getter named test10_null_getter");
+            assert.isFalse(globalObject.hasOwnProperty("test10_undefined_setter"), "global object should now have a setter named test10_undefined_setter");
+            assert.isFalse(globalObject.hasOwnProperty("test10_null_setter"), "global object should now have a setter named test10_null_setter");
         }
     },
     test11: {
@@ -298,6 +293,13 @@ var tests = {
             assert.isTrue(o.__lookupSetter__("setb") === setfn, "data property on o shadows accessor property on prototype but __lookupGetter__ looks for the first accessor property, skipping all others, so should return setfn");
         }
     },
+    test14: {
+        name: "__defineGetter__ and __defineSetter__ should throw TypeError when the object specified as getter/setter is not callable",
+        body: function () {
+            assert.throws(() => { __defineGetter__.call(this, 'x', 23); }, TypeError, "__defineGetter__ should throw TypeError when the function object is not callable.");
+            assert.throws(() => { this.__defineSetter__('y', {}); }, TypeError, "__defineGetter__ should throw TypeError when the function object is not callable.");
+        }
+    },
 };
 
 testRunner.runTests(tests);