jsrt: update context api wrapping by obastemur · Pull Request #3207 · chakra-core/ChakraCore

obastemur · 2017-06-22T12:36:14Z

When JSRT call ends up creating an object etc. (recycler allocation) we better wrap using ContextAPIWrapper. Oppositely, we could use a lighter version of it (ContextAPINoScriptWrapper) for simpler tasks.

Especially JsHasProperty is a very hot call for node-chakracore.

MSLaguana · 2017-06-22T15:54:12Z

 CHAKRA_API JsCreateObject(_Out_ JsValueRef *object)
 {
-    return ContextAPINoScriptWrapper([&](Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+    return ContextAPIWrapper<true>([&](Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {


Why do these calls need to change from ContextAPINoScriptWrapper to ContextAPIWrapper? Is this fixing a correctness bug? It looks to me like creating an object doesn't rely on executing any script.

My thinking (explained a bit on PR comment) Don't we allocate memory?

It does allocate memory yes, but ContextAPINoScriptWrapper sets up handlers for OOM exceptions (and I believe we also failfast on OOM as well) so I'm not sure why we specifically need the script-handling version here.

@MSLaguana is right. JsCreateObject() won't execute the user script and hence we want to keep it under ContextAPINoScriptWrapper. Additionally ContextAPINoScriptWrapper is lighter version of ContextApiWrapper as it doesn't have BEGIN_ENTER_SCRIPT(scriptContext, true, true, true) and END_ENTER_SCRIPT.

BTW, we do set up handler for OOM in both cases.

MSLaguana · 2017-06-22T15:57:51Z


 CHAKRA_API JsInstanceOf(_In_ JsValueRef object, _In_ JsValueRef constructor, _Out_ bool *result) {
-    return ContextAPIWrapper<true>([&](Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+    return ContextAPINoScriptWrapper([&](Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {


Are you sure that this is correct? It is possible to provide a custom hasInstance method which may have arbitrary side effects, so surely we need to allow for the possibility of script executing here?

Yes right, I was 50/50 on this, will update

MSLaguana · 2017-06-22T16:01:08Z

+    return ContextAPINoScriptWrapper([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTHasProperty, (Js::PropertyRecord *)propertyId, object);

        VALIDATE_INCOMING_OBJECT(object, scriptContext);


While you are modifying this area, I just noticed the line *hasProperty = nullptr which seems strange since hasProperty is a bool* so if anything we should be doing *hasProperty = false, or just removing that line completely and letting the following line set the value.

MSLaguana · 2017-06-22T16:14:33Z

 CHAKRA_API JsHasIndexedProperty(_In_ JsValueRef object, _In_ JsValueRef index, _Out_ bool *result)
 {
-    return ContextAPIWrapper<true>([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+    return ContextAPINoScriptWrapper([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {


I'm not convinced that this is safe, given the ability to write a proxy with a custom accessor.

MSLaguana · 2017-06-22T16:29:28Z

 {
-    return ContextAPIWrapper<true>([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+    return ContextAPINoScriptWrapper([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
        PERFORM_JSRT_TTD_RECORD_ACTION(scriptContext, RecordJsRTEquals, object1, object2, false);


This can also call script if one of the objects has a custom valueOf provided.

MSLaguana · 2017-06-22T16:30:21Z

 CHAKRA_API JsStrictEquals(_In_ JsValueRef object1, _In_ JsValueRef object2, _Out_ bool *result)
 {
-    return ContextAPIWrapper<true>([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+    return ContextAPINoScriptWrapper([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {


I believe that this one is probably fine.

kunalspathak · 2017-06-22T22:39:17Z


 CHAKRA_API JsGetPrototype(_In_ JsValueRef object, _Out_ JsValueRef *prototypeObject)
 {
-    return ContextAPIWrapper<true>([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {


ContextAPIWrapper [](start = 11, length = 17)

can call user script code if have getter?

kunalspathak · 2017-06-22T22:45:43Z

@@ -1342,7 +1342,7 @@ CHAKRA_API JsInstanceOf(_In_ JsValueRef object, _In_ JsValueRef constructor, _Ou

 CHAKRA_API JsGetExtensionAllowed(_In_ JsValueRef object, _Out_ bool *value)


JsGetExtensionAllowed [](start = 11, length = 21)

For most of the APIs below can call into user code if the object is proxy and you have handler for respective methods. See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy

When JSRT call ends up creating an object etc. (recycler allocation) we better wrap using ContextAPIWrapper. Oppositely, we could use a lighter version of it (ContextAPINoScriptWrapper) for simpler tasks

MSLaguana · 2017-07-10T23:55:59Z

-        *hasProperty = nullptr;
+        *hasProperty = false;

        *hasProperty = Js::JavascriptOperators::OP_HasProperty(object, ((Js::PropertyRecord *)propertyId)->GetPropertyId(), scriptContext) != 0;


I'm curious now, how does OP_HasProperty interact with proxies? It looks like it walks the prototype chain, but I'm not sure whether it can potentially trigger proxies (e.g. custom get functions on the proxy) or if it only checks for normal properties.

Yes, I will double check this because I see OP_HasProperty calls into HasPropertyQuery and Proxy has an implementation for it.

In reply to: 126565457 [](ancestors = 126565457)

Well, don't spend much time on this. I've been working on another approach that might cover the rest as well. I will close this PR for now.

MSLaguana · 2017-07-11T00:12:30Z

 CHAKRA_API JsDefineProperty(_In_ JsValueRef object, _In_ JsPropertyIdRef propertyId, _In_ JsValueRef propertyDescriptor, _Out_ bool *result)
 {
-    return ContextAPIWrapper<true>([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {
+    return ContextAPINoScriptWrapper([&] (Js::ScriptContext *scriptContext, TTDRecorder& _actionEntryPopper) -> JsErrorCode {


DefineOwnPropertyDescriptor can also be proxied.

obastemur requested review from MSLaguana, digitalinfinity and kunalspathak June 22, 2017 12:36

msftclas added the cla-already-signed label Jun 22, 2017

MSLaguana reviewed Jun 22, 2017

View reviewed changes

kunalspathak reviewed Jun 22, 2017

View reviewed changes

obastemur force-pushed the wrap_right branch from 464034b to 07da7b4 Compare June 23, 2017 11:26

jsrt: update context api wrapping

8e615c4

When JSRT call ends up creating an object etc. (recycler allocation) we better wrap using ContextAPIWrapper. Oppositely, we could use a lighter version of it (ContextAPINoScriptWrapper) for simpler tasks

obastemur force-pushed the wrap_right branch from 07da7b4 to 8e615c4 Compare June 23, 2017 11:28

digitalinfinity requested a review from curtisman June 26, 2017 23:54

MSLaguana reviewed Jul 10, 2017

View reviewed changes

MSLaguana reviewed Jul 11, 2017

View reviewed changes

kfarnung mentioned this pull request Jul 11, 2017

Expose JsHasOwnProperty #3316

Merged

obastemur closed this Jul 11, 2017

		@@ -1342,7 +1342,7 @@ CHAKRA_API JsInstanceOf(_In_ JsValueRef object, _In_ JsValueRef constructor, _Ou

		CHAKRA_API JsGetExtensionAllowed(_In_ JsValueRef object, _Out_ bool *value)

Conversation

obastemur commented Jun 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants