Found and fixed #2747 in JavascriptSet

lib/Runtime/Library/JavascriptMap.cpp

@@ -69,8 +69,9 @@ namespace Js
             JavascriptError::ThrowTypeErrorVar(scriptContext, JSERR_ObjectIsAlreadyInitialized, _u("Map"), _u("Map"));
         }
 
-        // ensure mapObject->map is created before trying to fetch the adder function
-        // see github#2747
+        /* Ensure mapObject->map is created before trying to fetch the adder function. If Map.prototype.set has
+           its getter set to another Map method (such as Map.prototype.get) and we try to get the function before
+           the map is initialized, it will cause a null dereference. See github#2747 */
         mapObject->map = RecyclerNew(scriptContext->GetRecycler(), MapDataMap, scriptContext->GetRecycler());
 
         RecyclableObject* iter = nullptr;

lib/Runtime/Library/JavascriptSet.cpp

@@ -64,6 +64,13 @@ namespace Js
 
         Var iterable = (args.Info.Count > 1) ? args[1] : library->GetUndefined();
 
+        if (setObject->set != nullptr)
+        {
+            JavascriptError::ThrowTypeErrorVar(scriptContext, JSERR_ObjectIsAlreadyInitialized, _u("Set"), _u("Set"));
+        }
+
+        setObject->set = RecyclerNew(scriptContext->GetRecycler(), SetDataSet, scriptContext->GetRecycler());
+
         RecyclableObject* iter = nullptr;
         RecyclableObject* adder = nullptr;
 
@@ -78,14 +85,6 @@ namespace Js
             adder = RecyclableObject::FromVar(adderVar);
         }
 
-        if (setObject->set != nullptr)
-        {
-            JavascriptError::ThrowTypeErrorVar(scriptContext, JSERR_ObjectIsAlreadyInitialized, _u("Set"), _u("Set"));
-        }
-
-
-        setObject->set = RecyclerNew(scriptContext->GetRecycler(), SetDataSet, scriptContext->GetRecycler());
-
         if (iter != nullptr)
         {
             JavascriptOperators::DoIteratorStepAndValue(iter, scriptContext, [&](Var nextItem) {

test/es6/bug_issue_2747.js

@@ -5,25 +5,64 @@
 
 WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");
 
+//
+// Check Map
+//
 var oldSet = Map.prototype.set;
 var m;
-function constructorFunc () {
+function constructMap () {
     m = new Map([["a", 1], ["b", 2]]);
 }
 
 Object.defineProperty(Map.prototype, "set", {
-    get: Map.prototype.get, // can be any Map.prototype method that depends on `this` being a valid map
+    get: Map.prototype.get, // can be any Map.prototype method that depends on `this` being valid
     configurable: true
 });
-assert.throws(function () { return Map.prototype.set });
-assert.throws(constructorFunc);
+assert.throws(function () {
+    return Map.prototype.set;
+}, TypeError, "Getting Map.prototype.set with the altered getter should throw a TypeError");
+assert.throws(constructMap, TypeError, "Constructing a Map (uses Map.prototype.set internally) should throw a TypeError");
 
 Object.defineProperty(Map.prototype, "set", {
     get: function () { return oldSet; }
 });
-assert.doesNotThrow(function () { return Map.prototype.set });
-assert.doesNotThrow(constructorFunc);
-assert.doesNotThrow(function () { m.set("a", 2); });
-assert.isTrue(m.get("a") === 2);
+assert.doesNotThrow(function () {
+    return Map.prototype.set;
+}, "Getting Map.prototype.set with the default set function should not throw");
+assert.doesNotThrow(constructMap, "Constructing a Map with the default set function should not throw");
+assert.doesNotThrow(function () {
+    m.set("a", 2);
+}, "Inserting a new key/value pair with the default set function should not through");
+assert.isTrue(m.get("a") === 2, "Inserting a new key/value pair should actually insert it");
 
-WScript.Echo("pass");
+//
+// Check Set
+//
+var oldAdd = Set.prototype.add;
+var s;
+function constructSet () {
+    s = new Set([1, 2, 3, 2, 4, 1]);
+}
+
+Object.defineProperty(Set.prototype, "add", {
+    get: Set.prototype.has, // can be any Set.prototype method that depends on `this` being valid
+    configurable: true
+});
+assert.throws(function () {
+    return Set.prototype.add;
+}, TypeError, "Getting Set.prototype.add with the altered getter should throw a TypeError");
+assert.throws(constructSet, TypeError, "Constructing a Set (uses Set.prototype.add internally) should throw a TypeError");
+
+Object.defineProperty(Set.prototype, "add", {
+    get: function () { return oldAdd; }
+});
+assert.doesNotThrow(function () {
+    return Set.prototype.add;
+}, "Getting Set.prototype.add with the default add function should not throw");
+assert.doesNotThrow(constructSet, "Constructing a Set with the default add function should not throw");
+assert.doesNotThrow(function () {
+    s.add(6);
+}, "Inserting a new item with the default set function should not throw");
+assert.isTrue(s.has(6) === true, "Inserting a new item should actually insert it");
+
+WScript.Echo("pass");