Don't mess with the function name/displayname for __chakraLibrary functions, and allow __chakraLibrary functions to be constructors

jackhorton · jackhorton · commit c523feaf0e19 · 2018-09-16T11:27:07.000-07:00
diff --git a/lib/Runtime/Library/EngineInterfaceObject.cpp b/lib/Runtime/Library/EngineInterfaceObject.cpp
@@ -272,21 +272,6 @@ namespace Js
     {
         ScriptContext *scriptContext = scriptFunction->GetScriptContext();
 
-        // Use GetSz rather than GetString because we use wcsrchr below, which expects a null-terminated string
-        // Callers can pass in a string like "get compare" or "Intl.Collator.prototype.resolvedOptions" -- only for the
-        // latter do we extract a shortName.
-        const char16 *methodNameBuf = displayName->GetSz();
-        charcount_t methodNameLength = displayName->GetLength();
-        const char16 *shortName = wcsrchr(methodNameBuf, _u('.'));
-        charcount_t shortNameOffset = 0;
-        if (shortName != nullptr)
-        {
-            shortName++;
-            shortNameOffset = static_cast<charcount_t>(shortName - methodNameBuf);
-        }
-
-        scriptFunction->GetFunctionProxy()->EnsureDeserialized()->SetDisplayName(methodNameBuf, methodNameLength, shortNameOffset);
-
         if (!isConstructor)
         {
             // set the ErrorOnNew attribute to disallow construction. JsBuiltIn/Intl functions are usually regular ScriptFunctions
@@ -308,29 +293,49 @@ namespace Js
             AssertMsg((scriptFunction->GetFunctionInfo()->GetAttributes() & FunctionInfo::Attributes::ErrorOnNew) == 0, "Why is the function not constructable by default?");
         }
 
-        // handle the name property AFTER handling isConstructor, because this can initialize the function's deferred type
-        Var existingName = nullptr;
-        if (JavascriptOperators::GetOwnProperty(scriptFunction, PropertyIds::name, &existingName, scriptContext, nullptr))
+        if (isPublic)
         {
-            JavascriptString *existingNameString = JavascriptString::FromVar(existingName);
-            if (existingNameString->GetLength() == 0)
+            // Use GetSz rather than GetString because we use wcsrchr below, which expects a null-terminated string
+            // Callers can pass in a string like "get compare" or "Intl.Collator.prototype.resolvedOptions" -- only for the
+            // latter do we extract a shortName.
+            const char16 *methodNameBuf = displayName->GetSz();
+            charcount_t methodNameLength = displayName->GetLength();
+            const char16 *shortName = wcsrchr(methodNameBuf, _u('.'));
+            charcount_t shortNameOffset = 0;
+            if (shortName != nullptr)
             {
-                // Only overwrite the name of the function object if it was anonymous coming in
-                // If the input function was named, it is likely intentional
-                existingName = nullptr;
+                shortName++;
+                shortNameOffset = static_cast<charcount_t>(shortName - methodNameBuf);
             }
-        }
 
-        if (existingName == nullptr || JavascriptOperators::IsUndefined(existingName))
-        {
-            // It is convenient to set the name here rather than in script, since it is often duplicated.
-            JavascriptString *funcName = displayName;
-            if (shortName)
+            scriptFunction->GetFunctionProxy()->EnsureDeserialized()->SetDisplayName(methodNameBuf, methodNameLength, shortNameOffset);
+
+            // handle the name property AFTER handling isConstructor, because this can initialize the function's deferred type
+            Var existingName = nullptr;
+            if (JavascriptOperators::GetOwnProperty(scriptFunction, PropertyIds::name, &existingName, scriptContext, nullptr))
             {
-                funcName = JavascriptString::NewCopyBuffer(shortName, methodNameLength - shortNameOffset, scriptContext);
+                JavascriptString *existingNameString = JavascriptString::FromVar(existingName);
+                if (existingNameString->GetLength() == 0)
+                {
+                    // Only overwrite the name of the function object if it was anonymous coming in
+                    // If the input function was named, it is likely intentional
+                    existingName = nullptr;
+                }
             }
 
-            scriptFunction->SetPropertyWithAttributes(PropertyIds::name, funcName, PropertyConfigurable, nullptr);
+            if (existingName == nullptr || JavascriptOperators::IsUndefined(existingName))
+            {
+                // It is convenient to set the name here rather than in script, since it is often duplicated.
+                JavascriptString *funcName = displayName;
+                if (shortName)
+                {
+                    funcName = JavascriptString::NewCopyBuffer(shortName, methodNameLength - shortNameOffset, scriptContext);
+                }
+
+                scriptFunction->SetPropertyWithAttributes(PropertyIds::name, funcName, PropertyConfigurable, nullptr);
+            }
+
+            scriptFunction->GetFunctionProxy()->SetIsPublicLibraryCode();
         }
 
         if (isJsBuiltIn)
@@ -342,15 +347,11 @@ namespace Js
             scriptFunction->SetEnvironment(const_cast<FrameDisplay *>(&StrictNullFrameDisplay));
 
             // TODO(jahorto): investigate force-inlining Intl code
+            scriptFunction->GetFunctionProxy()->EnsureDeserialized();
             AssertOrFailFast(scriptFunction->HasFunctionBody());
             scriptFunction->GetFunctionBody()->SetJsBuiltInForceInline();
         }
 
-        if (isPublic)
-        {
-            scriptFunction->GetFunctionProxy()->SetIsPublicLibraryCode();
-        }
-
         return scriptFunction;
     }
 
diff --git a/lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp b/lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp
@@ -259,7 +259,15 @@ namespace Js
         JavascriptLibrary * library = scriptContext->GetLibrary();
 
         JavascriptString* methodName = JavascriptString::UnsafeFromVar(args.Values[1]);
-        ScriptFunction* func = EngineInterfaceObject::CreateLibraryCodeScriptFunction(ScriptFunction::UnsafeFromVar(args.Values[2]), methodName, false /* isConstructor */, true /* isJsBuiltIn */, false /* isPublic */);
+
+        // chakra library functions, since they aren't public, can be constructors (__chakraLibrary.ArrayIterator is one)
+        ScriptFunction* func = EngineInterfaceObject::CreateLibraryCodeScriptFunction(
+            ScriptFunction::UnsafeFromVar(args.Values[2]),
+            methodName,
+            true /* isConstructor */,
+            true /* isJsBuiltIn */,
+            false /* isPublic */
+        );
 
         PropertyIds functionIdentifier = JavascriptOperators::GetPropertyId(methodName, scriptContext);
 
