Add JSRT support for native callbacks with `new.target` #4529

boingoing · 2018-01-10T22:10:40Z

Existing JsNativeFunction callbacks have no concept of new.target and, as a consequence, native functions cannot be used as the base class for a derived class constructor in Javascript. An API like JsGetNewTarget doesen't make a lot of sense so we propose here a new type of callback which will be called with new.target directly.

JsCreateEnhancedFunction acts similarly to the way JsCreateFunction did but takes a JsEnhancedNativeFunction callback instead. This callback is called passing a struct (JsNativeFunctionInfo containing new.target, this, and isConstructCall) as well as ordinary callee, arguments, and argumentsCount.

To simplify JavascriptExternalFunction and handling of native callback functions in ChakraCore, I refactored JsCreateFunction to use a JsEnhancedNativeFunction wrapper function which forwards to the JsNativeFunction callback.

Add native unit tests where an enhanced function (JavascriptExternalFunction) is used as the base class for a Javascript derived class.

Fixes: #3762

MSLaguana · 2018-01-10T22:16:18Z

bin/NativeTests/JsRTApiTest.cpp

@@ -660,6 +660,215 @@ namespace JsRTApiTest
        JsRTApiTest::RunWithAttributes(JsRTApiTest::ExternalFunctionTest);
    }

+    JsValueRef CALLBACK ExternaEnhancedFunctionTestCallback(JsValueRef callee, JsValueRef *arguments, unsigned short argumentCount, JsNativeFunctionInfo *info, void *callbackData)


Typo: should be ExternalEnhancedFunctionTestCallback

Thanks for that, good catch.

MSLaguana · 2018-01-10T22:23:45Z

lib/Jsrt/ChakraCore.h

+{
+    JsValueRef thisArg;
+    JsValueRef newTargetArg;
+    bool isConstructCall;


Is it worth documenting how these work? e.g. if isConstructCall is true, then newTargetArg will be non-null, otherwise it will be null?

I think newTargetArg cannot be non-null unless isConstructCall is true but not sure if the example above is always true. Thinking about it, new.target should really be undefined and not null, I believe.

MSLaguana · 2018-01-10T22:26:57Z

lib/Jsrt/ChakraCore.h

+///     Requires an active script context.
+/// </remarks>
+/// <param name="nativeFunction">The method to call when the function is invoked.</param>
+/// <param name="metadata">If this is a string, it is used as the name of the function.</param>


What if this is not a string? If it must be a string (or undefined/not provided) perhaps name it functionName instead of metadata?

For now it's acting as a stand-in for the name parameter from JsCreateNamedFunction but the idea is to extend it so that it could be a Var with properties like length, name, or constructor.

Should we enforce the shape to be an Object with a single name property right now to make upgrading this easier in the future?

MSLaguana · 2018-01-10T22:29:17Z

lib/Jsrt/Jsrt.cpp

@@ -2791,44 +2791,65 @@ CHAKRA_API JsConstructObject(_In_ JsValueRef function, _In_reads_(cargs) JsValue
    });
 }

-CHAKRA_API JsCreateFunction(_In_ JsNativeFunction nativeFunction, _In_opt_ void *callbackState, _Out_ JsValueRef *function)
+#ifndef _CHAKRACOREBUILD
+struct JsNativeFunctionInfo


Having this duplicated between the header and here feels like it could cause issues in future (e.g. someone updates one spot but not the other). Since it seems that this struct is required in both core and full (since it is used internally there), is it worth moving the struct definition to chakra.h instead of charkacore.h?

I took the conservative approach of not adding this struct or the declaration for JsEnhancedNativeFunction in ChakraCommon.h because I don't want them to be considered part of the Windows API surface and have to go through the API review board, etc.

Makes sense. I do wish there was a better way than having essentially the same structure duplicated in 3 places.

Could it be in a separate header for the time being that is just included in each place? Its also a bit hacky but the pattern is used elsewhere in ChakraCore.

MSLaguana · 2018-01-10T23:13:24Z

lib/Runtime/Library/JavascriptExternalFunction.cpp

+
+        StdCallJavascriptMethodInfo info = {
+            callInfo.Count > 0 ? args[0] : scriptContext->GetLibrary()->GetNull(),
+            args.HasNewTarget() ? args.GetNewTarget() : args.IsNewCall() ? function : scriptContext->GetLibrary()->GetNull(),


As discussed in another comment; perhaps this should default to undefined instead of null? And is it worth asserting that args.HasNewTarget() implies that args.IsNewCall()?

I'll make the change to undefined rather than null - this is per spec even. I don't think it's worth adding that assert here. It might actually be possible to get new.target without being in a new call. Don't think there are any ways for it to happen yet, but spec doesn't forbid that case.

MSLaguana · 2018-01-10T23:14:40Z

lib/Runtime/Library/JavascriptExternalFunction.cpp

+        Var result = nullptr;
+
+        StdCallJavascriptMethodInfo info = {
+            callInfo.Count > 0 ? args[0] : scriptContext->GetLibrary()->GetNull(),


I believe that we should always have a this parameter, so args[0] should always be passed; I'd prefer an assert that callInfo.count > 0 and just putting args[0] here rather than null (which isn't possible in normal JS scenarios)

I'm onboard with this.

MSLaguana · 2018-01-10T23:18:36Z

lib/Runtime/Library/JavascriptExternalFunction.cpp

@@ -424,9 +430,15 @@ namespace Js
            TTD::TTDNestingDepthAutoAdjuster logPopper(scriptContext->GetThreadContext());
            TTD::NSLogEvents::EventLogEntry* callEvent = elog->RecordExternalCallEvent(externalFunction, scriptContext->GetThreadContext()->TTDRootNestingCount, args.Info.Count, args.Values, true);


Do these TTD events need to be updated with the new.target information? Right now only the arguments are passed in.

I think we need to do something more here for TTD cases because RecordExternalCallEvent doesn't even record the flags for the call at all right now. new.target is located in the args, so not sure it should be handled specially but we should probably care if the function was called as a new expression or not.

Since the TTD tests are passing again, this may not need to change. I think that since the call flags aren't directly visible in JS, when replaying we probably don't need to know the flags, the external function is a black-box that has some js-visible side effects and a return value, all of which are currently recorded.

MSLaguana · 2018-01-10T23:19:36Z

lib/Runtime/Library/JavascriptLibrary.h

@@ -1091,6 +1091,7 @@ namespace Js
        JavascriptExternalFunction* CreateExternalFunction(ExternalMethod entryPointer, Var nameId, Var signature, UINT64 flags, bool isLengthAvailable = false);
        JavascriptExternalFunction* CreateStdCallExternalFunction(StdCallJavascriptMethod entryPointer, PropertyId nameId, void *callbackState);
        JavascriptExternalFunction* CreateStdCallExternalFunction(StdCallJavascriptMethod entryPointer, Var name, void *callbackState);
+


Is this intentional?

No, actually. Leftover from reverted changes, I guess.

MSLaguana · 2018-01-10T23:23:19Z

lib/Jsrt/ChakraCore.h

+/// <summary>
+///     A structure containing information about a native function callback.
+/// </summary>
+struct JsNativeFunctionInfo


You should also typedef this struct as JsNativeFunctionInfo since clang won't automatically do so for you.

Ok, thanks.

MSLaguana · 2018-01-10T23:26:32Z

lib/Jsrt/Jsrt.cpp

-        PERFORM_JSRT_TTD_RECORD_ACTION_RESULT(scriptContext, function);
+struct JsNativeFunctionWrapperHolder
+{
+    void *callbackState;


There's also xplat compile errors claiming that these two values need a Field barrier, since they are in recycler memory. I think they both actually be a FieldNoBarrier since neither value is recycler allocated.

@digitalinfinity I'd appreciate if you could double-check my thoughts here.

Hmm I'll add FieldNoBarrier. 👍

MSLaguana · 2018-01-10T23:30:36Z

lib/Jsrt/Jsrt.cpp

+
+CHAKRA_API JsCreateFunction(_In_ JsNativeFunction nativeFunction, _In_opt_ void *callbackState, _Out_ JsValueRef *function)
+{
+    return JsCreateNamedFunction(JS_INVALID_REFERENCE, nativeFunction, callbackState, function);


JsCreateNamedFunction states that the name property is an _In_ parameter, but JS_INVALID_REFERENCE is null so prefast is complaining here.

Noticed and this is unfortunate. I'll change JsCreateFunction to call JsCreateEnhancedFunctionHelper directly since the metadata argument is marked as _In_opt_.

MSLaguana · 2018-01-10T23:32:00Z

bin/NativeTests/JsRTApiTest.cpp

+        JsValueRef name;
+        REQUIRE(JsCreateString("BaseClass", 10, &name) == JsNoError);
+        JsValueRef base = JS_INVALID_REFERENCE;
+        REQUIRE(JsCreateEnhancedFunction(ExternaEnhancedBaseClassFunctionTestCallback, name, &info, &base) == JsNoError);


Prefast complains that info is uninitialized here

Thanks, have fixed this one.

MSLaguana · 2018-01-10T23:36:36Z

lib/Jsrt/Jsrt.cpp

-        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTAllocateFunction, true, name);
-
-        VALIDATE_INCOMING_REFERENCE(name, scriptContext);
+        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTAllocateFunction, false, nullptr);


You probably still need to record the metadata variable if it is provided. There is a TTD test which is failing, but I'm not sure if it's for this reason.

Ah good catch, thank you.

agarwal-sandeep · 2018-01-10T23:40:42Z

lib/Jsrt/ChakraCore.h

+///     Requires an active script context.
+/// </remarks>
+/// <param name="nativeFunction">The method to call when the function is invoked.</param>
+/// <param name="metadata">If this is a string, it is used as the name of the function.</param>


If this is a string [](start = 27, length = 19)

Consider rewording as if this is not null you are calling ToString, so any non-null is converted to a string.

Thanks, I made a note here as such.

jackhorton · 2018-01-10T23:40:44Z

bin/ch/ChakraRtInterface.h

@@ -318,6 +320,7 @@ class ChakraRTInterface
    static JsErrorCode WINAPI JsCreateObject(JsValueRef *object) { return HOOK_JS_API(CreateObject(object)); }
    static JsErrorCode WINAPI JsCreateExternalObject(void *data, JsFinalizeCallback callback, JsValueRef *object) { return HOOK_JS_API(CreateExternalObject(data, callback, object)); }
    static JsErrorCode WINAPI JsCreateFunction(JsNativeFunction nativeFunction, void *callbackState, JsValueRef *function) { return HOOK_JS_API(CreateFunction(nativeFunction, callbackState, function)); }
+    static JsErrorCode WINAPI JsCreateEnhancedFunction(JsEnhancedNativeFunction nativeFunction, JsValueRef metadata, void *callbackState, JsValueRef *function) { return HOOK_JS_API(CreateEnhancedFunction(nativeFunction, metadata, callbackState, function)); }


I don't know if this is a particularly productive bikeshed, but how set in stone are we on this name? I feel like we could instead call these ES6Functions rather than EnhancedFunctions? I fear of a situation like https://msdn.microsoft.com/en-us/library/dd317812(v=vs.85).aspx, where we might need to change this API again in the future and we are stuck with CreateFunction, CreateEnhancedFunction, and... CreateExtraEnhancedFunction? ES6Function might be confused with arrow functions, so maybe this doesn't have a good solution.

Agree with the sentiment. We can not change JsCreateFunction or JsNativeFunction easily, though. I considered just naming the new one JsCreateFunction2 to avoid giving it a name and just acknowledge that it's newer. Don't think ES6Function means much, though. I had named it JsNativeFunctionWithInfo but the function JsCreateFunctionWithInfo sounds like you need to pass info in order to create the function. JsNativeFunctionWithNewTarget is too specific. Maybe JsNativeSubclassableFunction or JsSubclassableNativeFunction but that may discourage use for ordinary function callbacks?

My thinking for JsCreateES6Function was that the important difference here compared to JsCreateFunction was that you gained access to an ES6 feature, new.target. If the eventual goal is to get this into Windows, is there any value in actually following the naming scheme with JsCreateFunctionEx? The only concern is that a new spec will add a new feature and we will be in the same bad situation right now, except that we will have a new naming ambiguity.

As long as it hasn't shipped in Windows, we're treating these API as experimental. So I suppose we can change the name until that point. Not that we should change it many times or anything, but the option is open.

jackhorton · 2018-01-10T23:45:04Z

lib/Jsrt/ChakraCore.h

+/// <summary>
+///     A structure containing information about a native function callback.
+/// </summary>
+struct JsNativeFunctionInfo


Is this struct one that we will be able to add fields to in the future, even if this API becomes part of Windows? (Along the same lines as my other comment about needing future updates to this API surface).

We should be able to add more fields to the struct later if we want to. If it becomes part of Windows API surface, changing it will become very difficult. We'll have to go through Windows API review board.

jackhorton · 2018-01-10T23:47:15Z

lib/Jsrt/ChakraCore.h

+///     Requires an active script context.
+/// </remarks>
+/// <param name="nativeFunction">The method to call when the function is invoked.</param>
+/// <param name="metadata">If this is a string, it is used as the name of the function.</param>


Should we enforce the shape to be an Object with a single name property right now to make upgrading this easier in the future?

jackhorton · 2018-01-10T23:48:35Z

lib/Jsrt/Jsrt.cpp

@@ -2791,44 +2791,65 @@ CHAKRA_API JsConstructObject(_In_ JsValueRef function, _In_reads_(cargs) JsValue
    });
 }

-CHAKRA_API JsCreateFunction(_In_ JsNativeFunction nativeFunction, _In_opt_ void *callbackState, _Out_ JsValueRef *function)
+#ifndef _CHAKRACOREBUILD
+struct JsNativeFunctionInfo


Could it be in a separate header for the time being that is just included in each place? Its also a bit hacky but the pattern is used elsewhere in ChakraCore.

jackhorton · 2018-01-10T23:59:44Z

lib/Runtime/Library/JavascriptExternalFunction.cpp

@@ -424,9 +430,15 @@ namespace Js
            TTD::TTDNestingDepthAutoAdjuster logPopper(scriptContext->GetThreadContext());
            TTD::NSLogEvents::EventLogEntry* callEvent = elog->RecordExternalCallEvent(externalFunction, scriptContext->GetThreadContext()->TTDRootNestingCount, args.Info.Count, args.Values, true);

+            StdCallJavascriptMethodInfo info = {
+                callInfo.Count > 0 ? args[0] : scriptContext->GetLibrary()->GetNull(),


Just a comment to note that the above conversation should also be updated here (re: null or undefined, callInfo.Count > 0)

boingoing · 2018-01-11T02:56:09Z

@dotnet-bot test Windows ci_lite_x64_debug please

MSLaguana · 2018-01-11T04:30:23Z

@Cellule looks like the new bytecode test is failing here, but I don't think this PR should have modified bytecode generation. Do you know if master is in a good state?

MSLaguana · 2018-01-11T04:53:55Z

@Cellule Ah, it looks like this is comparing the base version with the nojit version

jackhorton · 2018-01-11T17:22:08Z

Should 322242a be its own commit that goes into 1.7, where the bytecode tests went, to make sure we don't have this issue elsewhere?

MSLaguana · 2018-01-11T18:47:50Z

I've backported the commit undoing the bytecode verification, so it shouldn't be necessary on this PR any more.

boingoing · 2018-01-11T19:51:13Z

Thanks, @kfarnung, @jackhorton, and @MSLaguana for porting back that change. I removed the change from my PR and rebased.

Existing `JsNativeFunction` callbacks have no concept of `new.target` and, as a consequence, native functions cannot be used as the base class for a derived class constructor in Javascript. An API like `JsGetNewTarget` doesen't make a lot of sense so we propose here a new type of callback which will be called with `new.target` directly. `JsCreateEnhancedFunction` acts similarly to the way `JsCreateFunction` did but takes a `JsEnhancedNativeFunction` callback instead. This callback is called passing a struct (`JsNativeFunctionInfo` containing `new.target`, `this`, and `isConstructCall`) as well as ordinary `callee`, `arguments`, and `argumentsCount`. To simplify `JavascriptExternalFunction` and handling of native callback functions in ChakraCore, I refactored `JsCreateFunction` to use a `JsEnhancedNativeFunction` wrapper function which forwards to the `JsNativeFunction` callback. Add native unit tests where an enhanced function (`JavascriptExternalFunction`) is used as the base class for a Javascript derived class. Fixes: chakra-core#3762

…new.target` Merge pull request #4529 from boingoing:NativeCallbackSupportNewTarget Existing `JsNativeFunction` callbacks have no concept of `new.target` and, as a consequence, native functions cannot be used as the base class for a derived class constructor in Javascript. An API like `JsGetNewTarget` doesen't make a lot of sense so we propose here a new type of callback which will be called with `new.target` directly. `JsCreateEnhancedFunction` acts similarly to the way `JsCreateFunction` did but takes a `JsEnhancedNativeFunction` callback instead. This callback is called passing a struct (`JsNativeFunctionInfo` containing `new.target`, `this`, and `isConstructCall`) as well as ordinary `callee`, `arguments`, and `argumentsCount`. To simplify `JavascriptExternalFunction` and handling of native callback functions in ChakraCore, I refactored `JsCreateFunction` to use a `JsEnhancedNativeFunction` wrapper function which forwards to the `JsNativeFunction` callback. Add native unit tests where an enhanced function (`JavascriptExternalFunction`) is used as the base class for a Javascript derived class. Fixes: #3762

fatcerberus · 2018-01-11T23:13:38Z

Existing JsNativeFunction callbacks have no concept of new.target and, as a consequence, native functions cannot be used as the base class for a derived class constructor in Javascript.

This isn’t 100% true as I found out - calling a JsNativeFunction with new (even as a subclass) results in new.target being passed in as the first argument, instead of this. I took advantage of that fact to support subclassing in miniSphere already.

That said, this way is much cleaner so I welcome the change nonetheless. :)

MSLaguana reviewed Jan 10, 2018

View reviewed changes

agarwal-sandeep reviewed Jan 10, 2018

View reviewed changes

jackhorton reviewed Jan 11, 2018

View reviewed changes

boingoing requested review from digitalinfinity and curtisman January 11, 2018 04:36

boingoing force-pushed the NativeCallbackSupportNewTarget branch from 58d6375 to 322242a Compare January 11, 2018 07:04

kfarnung approved these changes Jan 11, 2018

View reviewed changes

MSLaguana approved these changes Jan 11, 2018

View reviewed changes

boingoing force-pushed the NativeCallbackSupportNewTarget branch from 322242a to 7bba9a4 Compare January 11, 2018 19:50

boingoing added 2 commits January 11, 2018 14:12

Respond to review feedback

ac00296

boingoing force-pushed the NativeCallbackSupportNewTarget branch from 7bba9a4 to ac00296 Compare January 11, 2018 22:13

chakrabot merged commit ac00296 into chakra-core:master Jan 11, 2018

		@@ -424,9 +430,15 @@ namespace Js
		TTD::TTDNestingDepthAutoAdjuster logPopper(scriptContext->GetThreadContext());
		TTD::NSLogEvents::EventLogEntry* callEvent = elog->RecordExternalCallEvent(externalFunction, scriptContext->GetThreadContext()->TTDRootNestingCount, args.Info.Count, args.Values, true);

Add JSRT support for native callbacks with new.target #4529

Add JSRT support for native callbacks with new.target #4529

Conversation

boingoing commented Jan 10, 2018 • edited by jackhorton Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing commented Jan 11, 2018

MSLaguana commented Jan 11, 2018 • edited Loading

MSLaguana commented Jan 11, 2018

jackhorton commented Jan 11, 2018

MSLaguana commented Jan 11, 2018

boingoing commented Jan 11, 2018

fatcerberus commented Jan 11, 2018

Add JSRT support for native callbacks with `new.target` #4529

Add JSRT support for native callbacks with `new.target` #4529

boingoing commented Jan 10, 2018 •

edited by jackhorton

Loading

MSLaguana commented Jan 11, 2018 •

edited

Loading