[1.7>master] [1.6>1.7] [MERGE #3429 @atulkatti] Fixes #586 JsCreateNa…

…medFunction should create a tracked PropertyId for name to ensure toString() works as expected.

Merge pull request #3429 from atulkatti:Issue586.JSCreateNamedFunction.1

Fixes #586 JsCreateNamedFunction should create a tracked PropertyId for name to ensure toString() works as expected.

bin/NativeTests/JsRTApiTest.cpp

@@ -683,6 +683,18 @@ namespace JsRTApiTest
             CHECK(wcscmp(expectedName, actualName) == 0);
         };
 
+        auto testToStringResult = [=](JsValueRef function, PCWCHAR expectedResult, size_t expectedResultLength)
+        {
+            JsValueRef actualResult = JS_INVALID_REFERENCE;
+            REQUIRE(JsConvertValueToString(function, &actualResult) == JsNoError);
+
+            PCWCHAR actualResultString = nullptr;
+            size_t actualResultLength;
+            REQUIRE(JsStringToPointer(actualResult, &actualResultString, &actualResultLength) == JsNoError);
+            CHECK(expectedResultLength == actualResultLength);
+            CHECK(wcscmp(expectedResult, actualResultString) == 0);
+        };
+
         JsValueRef function = JS_INVALID_REFERENCE;
         REQUIRE(JsCreateFunction(ExternalFunctionCallback, nullptr, &function) == JsNoError);
         testConstructorName(function, _u(""), 0);
@@ -692,6 +704,11 @@ namespace JsRTApiTest
         REQUIRE(JsPointerToString(name, _countof(name) - 1, &nameString) == JsNoError);
         REQUIRE(JsCreateNamedFunction(nameString, ExternalFunctionCallback, nullptr, &function) == JsNoError);
         testConstructorName(function, name, _countof(name) - 1);
+
+        WCHAR toStringExpectedResult[] = _u("function FooName() { [native code] }");
+        testToStringResult(function, toStringExpectedResult, _countof(toStringExpectedResult) - 1);
+        // Calling toString multiple times should return the same result.
+        testToStringResult(function, toStringExpectedResult, _countof(toStringExpectedResult) - 1);
     }
 
     TEST_CASE("ApiTest_ExternalFunctionNameTest", "[ApiTest]")

lib/Runtime/Debug/DiagObjectModel.cpp

@@ -269,7 +269,7 @@ namespace Js
             }
             else
             {
-                Js::JavascriptString *builtInName = returnValue->calledFunction->GetDisplayName();
+                Js::JavascriptString *builtInName = ParseFunctionName(returnValue->calledFunction->GetDisplayName(), pResolvedObject->scriptContext);
                 swprintf_s(finalName, RETURN_VALUE_MAX_NAME, _u("[%s returned]"), builtInName->GetSz());
             }
             pResolvedObject->obj = returnValue->returnedValue;
@@ -285,6 +285,47 @@ namespace Js
         pResolvedObject->objectDisplay->SetDefaultTypeAttribute(defaultAttributes);
     }
 
+    // The debugger uses the functionNameId field instead of the "name" property to get the name of the funtion. The functionNameId field is overloaded and may contain the display name if
+    // toString() has been called on the function object. For built-in or external functions the display name can be something like "function Echo() { native code }". We will try to parse the
+    // function name out of the display name so the user will see just the function name e.g. "Echo" instead of the full display name in debugger.
+    JavascriptString * VariableWalkerBase::ParseFunctionName(JavascriptString* displayName, ScriptContext* scriptContext)
+    {
+        Assert(displayName);
+        const char16 * displayNameBuffer = displayName->GetString();
+        const charcount_t displayNameBufferLength = displayName->GetLength();
+        const charcount_t funcStringLength = _countof(JS_DISPLAY_STRING_FUNCTION_HEADER) - 1; // discount the ending null character in string literal
+        const charcount_t templateStringLength = funcStringLength + _countof(JS_DISPLAY_STRING_FUNCTION_BODY) - 1; // discount the ending null character in string literal
+        // If the string doesn't meet our expected format; return the original string.
+        if (displayNameBufferLength <= templateStringLength || (wmemcmp(displayNameBuffer, JS_DISPLAY_STRING_FUNCTION_HEADER, funcStringLength) != 0))
+        {
+            return displayName;
+        }
+
+        // Look for the left parenthesis, if we don't find one; return the original string.
+        const char16* parenChar = wcschr(displayNameBuffer, '(');
+        if (parenChar == nullptr)
+        {
+            return displayName;
+        }
+
+        charcount_t actualFunctionNameLength = displayNameBufferLength - templateStringLength;
+        uint byteLengthForCopy = sizeof(char16) * actualFunctionNameLength;
+        char16 * actualFunctionNameBuffer = AnewArray(GetArenaFromContext(scriptContext), char16, actualFunctionNameLength + 1); // The last character will be the null character.
+        if (actualFunctionNameBuffer == nullptr)
+        {
+            return displayName;
+        }
+        js_memcpy_s(actualFunctionNameBuffer, byteLengthForCopy, displayNameBuffer + funcStringLength, byteLengthForCopy);
+        actualFunctionNameBuffer[actualFunctionNameLength] = _u('\0');
+
+        JavascriptString * actualFunctionName = JavascriptString::NewWithArenaSz(actualFunctionNameBuffer, scriptContext);
+        if (actualFunctionName == nullptr)
+        {
+            return displayName;
+        }
+        return actualFunctionName;
+    }
+
     /*static*/
     BOOL VariableWalkerBase::GetReturnedValue(int &index, DiagStackFrame* frame, ResolvedObject* pResolvedObject)
     {

lib/Runtime/Debug/DiagObjectModel.h

@@ -216,6 +216,9 @@ namespace Js
         int GetMemberCount() { return pMembersList ? pMembersList->Count() : 0; }
 
         bool IsPropertyValid(PropertyId propertyId, RegSlot location, bool *isPropertyInDebuggerScope, bool* isConst, bool* isInDeadZone) const;
+
+    private:
+        static JavascriptString * ParseFunctionName(JavascriptString* displayName, ScriptContext* scriptContext);
     };
 
 

lib/Runtime/Library/JavascriptLibrary.cpp

@@ -6597,12 +6597,32 @@ namespace Js
         return CreateStdCallExternalFunction(entryPoint, TaggedInt::ToVarUnchecked(nameId), callbackState);
     }
 
-    JavascriptExternalFunction* JavascriptLibrary::CreateStdCallExternalFunction(StdCallJavascriptMethod entryPoint, Var nameId, void *callbackState)
+    JavascriptExternalFunction* JavascriptLibrary::CreateStdCallExternalFunction(StdCallJavascriptMethod entryPoint, Var name, void *callbackState)
     {
+        Var functionNameOrId = name;
+        if (JavascriptString::Is(name))
+        {
+            JavascriptString * functionName = JavascriptString::FromVar(name);
+            const char16 * functionNameBuffer = functionName->GetString();
+            int functionNameBufferLength = functionName->GetLengthAsSignedInt();
+
+            PropertyId functionNamePropertyId = scriptContext->GetOrAddPropertyIdTracked(functionNameBuffer, functionNameBufferLength);
+            functionNameOrId = TaggedInt::ToVarUnchecked(functionNamePropertyId);
+        }
+#if ENABLE_TTD
+        else if (scriptContext->GetThreadContext()->IsRuntimeInTTDMode() && TaggedInt::Is(name))
+        {
+            PropertyId pid = TaggedInt::ToInt32(name);
+            if (!scriptContext->IsTrackedPropertyId(pid))
+            {
+                scriptContext->TrackPid(pid);
+            }
+        }
+#endif
+        AssertOrFailFast(TaggedInt::Is(functionNameOrId));
         JavascriptExternalFunction* function = this->CreateIdMappedExternalFunction(entryPoint, stdCallFunctionWithDeferredPrototypeType);
-        function->SetFunctionNameId(nameId);
+        function->SetFunctionNameId(functionNameOrId);
         function->SetCallbackState(callbackState);
-
         return function;
     }
 

lib/Runtime/Library/JavascriptLibrary.h

@@ -1043,7 +1043,7 @@ namespace Js
         JavascriptExternalFunction* CreateExternalFunction(ExternalMethod entryPointer, PropertyId nameId, Var signature, JavascriptTypeId prototypeTypeId, UINT64 flags);
         JavascriptExternalFunction* CreateExternalFunction(ExternalMethod entryPointer, Var nameId, Var signature, JavascriptTypeId prototypeTypeId, UINT64 flags);
         JavascriptExternalFunction* CreateStdCallExternalFunction(StdCallJavascriptMethod entryPointer, PropertyId nameId, void *callbackState);
-        JavascriptExternalFunction* CreateStdCallExternalFunction(StdCallJavascriptMethod entryPointer, Var nameId, void *callbackState);
+        JavascriptExternalFunction* CreateStdCallExternalFunction(StdCallJavascriptMethod entryPointer, Var name, void *callbackState);
         JavascriptPromiseAsyncSpawnExecutorFunction* CreatePromiseAsyncSpawnExecutorFunction(JavascriptMethod entryPoint, JavascriptGenerator* generator, Var target);
         JavascriptPromiseAsyncSpawnStepArgumentExecutorFunction* CreatePromiseAsyncSpawnStepArgumentExecutorFunction(JavascriptMethod entryPoint, JavascriptGenerator* generator, Var argument, Var resolve = nullptr, Var reject = nullptr, bool isReject = false);
         JavascriptPromiseCapabilitiesExecutorFunction* CreatePromiseCapabilitiesExecutorFunction(JavascriptMethod entryPoint, JavascriptPromiseCapability* capability);

lib/Runtime/Library/RuntimeFunction.cpp

@@ -31,13 +31,16 @@ namespace Js
         {
             if (TaggedInt::Is(this->functionNameId))
             {
-                if (this->GetScriptContext()->GetConfig()->IsES6FunctionNameEnabled() && this->GetTypeHandler()->IsDeferredTypeHandler())
+                if (scriptContext->GetConfig()->IsES6FunctionNameEnabled() && this->GetTypeHandler()->IsDeferredTypeHandler())
                 {
                     JavascriptString* functionName = nullptr;
                     DebugOnly(bool status = ) this->GetFunctionName(&functionName);
                     Assert(status);
                     this->SetPropertyWithAttributes(PropertyIds::name, functionName, PropertyConfigurable, nullptr);
                 }
+
+                // This has a side-effect where any other code (such as debugger) that uses functionNameId value will now get the value like "function foo() { native code }"
+                // instead of just "foo". Alternative ways will need to be devised; if it's not desirable to use this full display name value in those cases.
                 this->functionNameId = GetNativeFunctionDisplayString(scriptContext, scriptContext->GetPropertyString(TaggedInt::ToInt32(this->functionNameId)));
             }
         }

lib/Runtime/Library/RuntimeFunction.h

@@ -18,10 +18,13 @@ namespace Js
 
         void SetFunctionNameId(Var nameId);
 
-        // this is for cached source string for the function. Possible values are:
+        // This is for cached source string for the function. Possible values are:
         // NULL; initialized for anonymous methods.
         // propertyId in Int31 format; this is used for fastDOM function as well as library function
         // JavascriptString: composed using functionname from the propertyId, or fixed string for anonymous functions.
+        // NOTE: This has a side-effect that after toString() is called for the first time on a built-in function the functionNameId gets replaced with a string like "function foo() { native code }".
+        // As a result any code like debugger(F12) that shows the functionNameId to the user will need to pre-process this string as it may not be desirable to use this as-is in some cases.
+        // See RuntimeFunction::EnsureSourceString() for details.
         Field(Var) functionNameId;
         virtual Var GetSourceString() const { return functionNameId; }
         virtual Var EnsureSourceString();

test/DebuggerCommon/returnedvaluetests3.js

@@ -6,6 +6,9 @@
 // Validating return values with built-ins and host objects.
 
 function test1() {
+    // Calling toString() shouldn't affect the function name being seen by the debugger. Used to show function name as "function Date() { [native code] }" instead of just "Date".
+    Date.toString();
+    WScript.Echo.toString();
     var a = Date(); /**bp:locals();resume('step_over');locals();resume('step_over');locals()**/
     WScript.Echo("Pass") + WScript.SetTimeout("1", 10);
 }

test/Intl/IntlReturnedValueTests.js

@@ -0,0 +1,73 @@
+//-------------------------------------------------------------------------------------------------------
+// Copyright (C) Microsoft. All rights reserved.
+// Licensed under the MIT license. See LICENSE.txt file in the project root for full license information.
+//-------------------------------------------------------------------------------------------------------
+
+// Return values from Intl function calls show the function name correctly in debugger.
+
+/////////////////// DateFormat ////////////////////
+var options = { ca: "gregory", hour12: true, timeZone:"UTC" }; 
+// Bug: GitHub Issue#3438: Intl.DateTimeFormat constructor call has 2 entries in the debugger functionCallsReturn.
+// https://github.com/Microsoft/ChakraCore/issues/3438
+// Actual: 
+    // "functionCallsReturn": {
+    //   "[Anonymous function returned]": "undefined undefined",
+    //   "[Intl.DateTimeFormat returned]": "Object {...}"
+// Expected:
+    // "functionCallsReturn": {
+    //   "[Intl.DateTimeFormat returned]": "Object {...}"
+var dateFormat1 = new Intl.DateTimeFormat("en-US", options);    /**bp:resume('step_over');locals();**/
+WScript.Echo("");  // Dummy line to get desired debugger logging behavior. Required due to the above bug.
+var date1 = new Date(2000, 1, 1);
+
+// Bug: GitHub Issue#3439: Intl function call has 2 entries in the debugger functionCallsReturn.
+// https://github.com/Microsoft/ChakraCore/issues/3439
+// Actual: 
+    // "functionCallsReturn": {
+    //   "[get format returned]": "function <large string>",
+    //   "[Intl.DateTimeFormat.prototype.format returned]": "string ‎2‎/‎1‎/‎2000"
+// Expected:
+    // "functionCallsReturn": {
+    //   "[Intl.DateTimeFormat.prototype.format returned]": "string ‎2‎/‎1‎/‎2000"
+var date2 = dateFormat1.format(date1);                          /**bp:resume('step_over');locals();**/
+WScript.Echo("");  // Dummy line to get desired debugger logging behavior. Required due to the above bug.
+var stringDate1 = date1.toLocaleString("en-us");             /**bp:resume('step_over');locals();resume('step_over');locals();resume('step_over');locals();**/
+Intl.DateTimeFormat.supportedLocalesOf(["en-US"], { localeMatcher: "best fit" });
+dateFormat1.resolvedOptions();
+WScript.Echo("");  // Dummy line to get desired debugger logging behavior. Required due to the above bug.
+
+/////////////////// NumberFormat ////////////////////
+var numberFormat1 = new Intl.NumberFormat();                    /**bp:resume('step_over');locals();**/
+WScript.Echo("");  // Dummy line to get desired debugger logging behavior. Required due to the above bug.
+// Bug: GitHub Issue#3439: Intl function call has 2 entries in the debugger functionCallsReturn.
+// https://github.com/Microsoft/ChakraCore/issues/3439
+// Actual: 
+    // "functionCallsReturn": {
+    //   "[get format returned]": "function <large string>",
+    //   "[Intl.DateTimeFormat.prototype.format returned]": "string ‎2‎/‎1‎/‎2000"
+// Expected:
+    // "functionCallsReturn": {
+    //   "[Intl.DateTimeFormat.prototype.format returned]": "string ‎2‎/‎1‎/‎2000"
+var formattedNumber1 = numberFormat1.format(123.456);           /**bp:resume('step_over');locals();**/
+WScript.Echo("");  // Dummy line to get desired debugger logging behavior. Required due to the above bug.
+Intl.NumberFormat.supportedLocalesOf(["en-US"], { localeMatcher: "lookup" });   /**bp:resume('step_over');locals();resume('step_over');locals();**/
+numberFormat1.resolvedOptions();   /**bp:locals();resume('step_over');locals();**/
+WScript.Echo("");  // Dummy line to get desired debugger logging behavior. Required due to the above bug.
+
+/////////////////// Collator ////////////////////
+var collator1 = Intl.Collator();                                /**bp:resume('step_over');locals();resume('step_over');locals();**/
+// Bug: GitHub Issue#3439: Intl function call has 2 entries in the debugger functionCallsReturn.
+// https://github.com/Microsoft/ChakraCore/issues/3439
+// Actual: 
+    // "functionCallsReturn": {
+    //   "[get compare returned]": "function <large string>",
+    //   "[Intl.Collator.prototype.compare returned]": "number -1"
+// Expected:
+    // "functionCallsReturn": {
+    //   "[Intl.Collator.prototype.compare returned]": "number -1"
+var compare1 = collator1.compare('a', 'b');
+WScript.Echo("");  // Dummy line to get desired debugger logging behavior. Required due to the above bug.
+Intl.Collator.supportedLocalesOf(["en-US"], { localeMatcher: "best fit" });     /**bp:resume('step_over');locals();resume('step_over');locals();**/
+collator1.resolvedOptions();
+
+WScript.Echo("Pass");