Implement JsGetProxyProperties API fixes #950 #4806

rhuanjl · 2018-03-10T21:09:53Z

Responding to issue #950

This PR:

Adds a JavascriptProxy::IsRevoked method to the Javascript proxy class (necessary for the below)
Adds a JsGetProxyProperties API to Jsrt which can:
a) check if an object is a proxy -> set a provided bool to true/false
b) if it is a proxy check if it's revoked
c) if it is a revoked proxy set provided target and handler references to nullptr
d) if it is a proxy that is not revoked provide references to it's target and handler
Tracks the same API through to WScriptJsrt in ch
Adds a test that uses the ch implementation

(Targeting 1.9 as this will assist with an issue in node-chakracore nodejs/node-chakracore#488 )

jackhorton · 2018-03-11T01:10:10Z

bin/ch/WScriptJsrt.cpp

+                JsValueRef undefined = JS_INVALID_REFERENCE;
+                IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&undefined));
+                IfJsrtErrorSetGo(ChakraRTInterface::JsSetProperty(returnValue, targetProperty, undefined, true));
+                IfJsrtErrorSetGo(ChakraRTInterface::JsSetProperty(returnValue, handlerProperty, undefined, true));


Not sure if there is a benefit to doing this here -- the value of return.target will be undefined regardless of whether or not you set it. I personally would say that this method should return { target, handler} or undefined rather than {target, handler} or {}

Not changed - I think there is potential utility to this - also it enables me to explicitly test the handling of a revoked proxy.

Currently this API does the following:

if given a non-proxy -> returns undefined

if given a revoked proxy -> returns { target : undefined, handler : undefined} -> you can explicitly detect a revoked proxy this way as the return value will have two properties not 0

if given a proxy that has not been revoked returns {target : proxyTarget, handler : proxyHandler}

Sure, but then if nothing else, you still dont need to assign undefined to ret.target and ret.handler, they'll be undefined regardless.

@jackhorton They will, but then Object.keys(ret).length would then be 0 instead of 2 which seems to be what @rhuanjl was getting at.

I guess it could have {} as the return value for a revoked proxy - just seemed easier to explicitly test it this way.

The object returned from this helper can have a revoked property set to true if handler and target are null. We could use that to detect a revoked proxy. Would be easier to test that. Pretty sure real proxy can never have undefined or null handler and target unless it's revoked, though. I'm not really invested in either solution.

jackhorton · 2018-03-11T01:11:56Z

lib/Jsrt/Jsrt.cpp

+        VALIDATE_INCOMING_REFERENCE(object, scriptContext);
+        PARAM_NOT_NULL(isProxy);
+
+        *isProxy = false;


This initial assignment is probably unnecessary.

I was following the pseudo-policy I've seen of always initialising out values. But in this case considering it's set to a bool on the next line I can see your point - changed.

@rhuanjl The idea I think is that out parameters are always written to regardless of which codepath you take (in languages like C# with native support for out params it’s actually an error not to do so). If, like here, the value is unconditionally set to something then initializing it is redundant.

I think SAL/Prefast is supposed to give errors for things like that, but I am not sure if it catches everything.

jackhorton · 2018-03-11T01:13:02Z

lib/Jsrt/Jsrt.cpp

+
+        Js::JavascriptProxy* proxy = Js::JavascriptProxy::UnsafeFromVar(object);
+
+        if (target != nullptr && handler != nullptr)


Maybe check target and handler separately in case someone wanted one but not the other?

Good idea - done

jackhorton · 2018-03-11T01:15:45Z

lib/Runtime/Library/JavascriptProxy.h

@@ -52,6 +52,7 @@ namespace Js
        JavascriptProxy(DynamicType * type, ScriptContext * scriptContext, RecyclableObject* target, RecyclableObject* handler);
        static BOOL Is(_In_ Var obj);
        static BOOL Is(_In_ RecyclableObject* obj);
+        BOOL IsRevoked();


nit: I personally think all new APIs should be bool rather than BOOL. This method can also be marked const, while we are at it.

I was just following what was done in the file, changed now though.

I don’t understand the motivation for returning const bool? The const in this context seems redundant since bool is an immutable value type. What am I missing?

@fatcerberus I meant more like bool IsRevoked() const;, since it can be called on const proxies with no side effects.

oops fixed (and squashed)

Also for clarification on the BOOL vs bool, BOOL was the old (microsoft-specific?) version before bool was part of the C++ standard (and I think its still not even in C, ridiculously). Its main problem is that its typedef'd to int, which causes silly conversion warnings among other things. Old APIs will remain BOOL indefinitely since changing it would affect almost every file (not to mention some uses of BOOL are for interaction with Win32 APIs that expect it), but I think we have been generally trying to avoid it for anything new unless it needs to interop.

C does have bool now, but you have to include stdbool.h for the typedef as it’s normally called _Bool. I use it in my own code, it was a long overdue addition.

boingoing · 2018-03-13T01:25:38Z

bin/ch/WScriptJsrt.cpp

+        bool isProxy = false;
+        JsValueRef target;
+        JsValueRef handler;
+        ChakraRTInterface::JsGetProxyProperties(arguments[1], &isProxy, &target, &handler);


Wrap in IfJsrtErrorSetGo?

boingoing · 2018-03-13T01:27:06Z

bin/ch/WScriptJsrt.cpp

+            if (target == nullptr)
+            {
+                JsValueRef undefined = JS_INVALID_REFERENCE;
+                IfJsrtErrorSetGo(ChakraRTInterface::JsGetUndefinedValue(&undefined));


We already called JsGetUndefinedValue above. Can we fold these two into one call at the top?

boingoing · 2018-03-13T01:28:36Z

bin/ch/WScriptJsrt.cpp

+            IfJsrtErrorSetGo(CreatePropertyIdFromString("handler", &handlerProperty));
+            IfJsrtErrorSetGo(ChakraRTInterface::JsCreateObject(&returnValue));
+
+            if (target == nullptr)


Check to see if target == JS_INVALID_REFERENCE.

boingoing · 2018-03-13T01:30:47Z

bin/ch/WScriptJsrt.cpp

+            else
+            {
+                IfJsrtErrorSetGo(ChakraRTInterface::JsSetProperty(returnValue, targetProperty, target, true));
+                IfJsrtErrorSetGo(ChakraRTInterface::JsSetProperty(returnValue, handlerProperty, handler, true));


It's a little bit weird to me to set both of these properties in both sides of the conditional. What about setting target and handler to undefined if they're JS_INVALID_REFERENCE and having one set of JsSetProperty which always gets called?

boingoing · 2018-03-13T01:35:34Z

lib/Jsrt/ChakraCore.h

+///     If object is not a Proxy object the target and handler parameters are not touched.
+///     If nullptr is supplied for target or handler the function returns after
+///     setting the isProxy value.
+///     If the object is a revoked Proxy target and handler are made to point to nullptr.


I think we should set the target and handler to JS_INVALID_REFERENCE instead of nullptr and have this reflected in the doc here.

boingoing · 2018-03-13T01:44:50Z

lib/Jsrt/Jsrt.cpp

+
+        if (!*isProxy)
+        {
+            return JsNoError;


Unless I'm mistaken, _Out_opt_ has the same semantics as _Out_ when it comes to initialization. That means this function should not return without initializing target and handler if they are not null. We should just initialize both of them to JS_INVALID_REFERENCE before the assignment to isProxy if they're non-null.

Yeah, _Out_opt from what I've been able to gather just means the out pointer itself can be nullptr - if it's a non-null pointer then it should indeed be treated the same as an _Out_ parameter.

boingoing · 2018-03-13T01:47:55Z

test/es6/ProxyPropertiesAPI.js

+            assert.isTrue(names.includes("handler"));
+
+            assert.isUndefined(properties.target, "Target of revoked proxy should be undefined.");
+            assert.isUndefined(properties.handler, "Handler of revoekd proxy should be undefined.");


Typo in revoked.

rhuanjl · 2018-03-13T08:49:01Z

@boingoing Thanks for the review I think I've addressed all of your points. (Done in a separate commit for now to make it easy to compare)

boingoing · 2018-03-13T16:34:42Z

bin/ch/WScriptJsrt.cpp

+
+            if (target == JS_INVALID_REFERENCE)
+            {
+                ChakraRTInterface::JsGetTrueValue(&revoked);


For consistency, can we wrap this in IfJsrtErrorSetGo?

boingoing · 2018-03-13T16:34:52Z

bin/ch/WScriptJsrt.cpp

+            }
+            else
+            {
+                ChakraRTInterface::JsGetFalseValue(&revoked);


Same nitpick about IfJsrtErrorSetGo.

boingoing

Looks good to me

kfarnung

LGTM with nit

kfarnung · 2018-03-13T17:56:29Z

lib/Runtime/Library/JavascriptProxy.h

@@ -52,6 +52,7 @@ namespace Js
        JavascriptProxy(DynamicType * type, ScriptContext * scriptContext, RecyclableObject* target, RecyclableObject* handler);
        static BOOL Is(_In_ Var obj);
        static BOOL Is(_In_ RecyclableObject* obj);
+        bool IsRevoked() const;


nit: I'd move this down with the other non-static, non-virtual methods.

rhuanjl · 2018-03-13T19:47:12Z

@boingoing Thanks for the review and sorry for the mistakes. I've fixed those additional points, updated to latest and squashed. Please let me know if anything else is needed.

rhuanjl · 2018-03-13T20:47:56Z

@dotnet-bot test Windows ci_slow_x64_debug please

akroshg · 2018-03-13T21:13:54Z

test/es6/ProxyPropertiesAPI.js

+            let properties = WScript.GetProxyProperties(proxy);
+
+            let names = Object.getOwnPropertyNames(properties);
+            assert.areEqual(names.length, 3, "proxy properties names should have length 2");


length 2 [](start = 81, length = 8)

shoul have 3 ?

should*

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

boingoing · 2018-03-13T21:35:24Z

@rhuanjl Thanks once again for your contribution here! I'll start the internal testing process for this one.

@jackhorton

Merge pull request #4806 from rhuanjl:JsGetProxyProperties Responding to issue #950 This PR: 1. Adds a JavascriptProxy::IsRevoked method to the Javascript proxy class (necessary for the below) 2. Adds a JsGetProxyProperties API to Jsrt which can: a) check if an object is a proxy -> set a provided bool to true/false b) if it is a proxy check if it's revoked c) if it is a revoked proxy set provided target and handler references to nullptr d) if it is a proxy that is not revoked provide references to it's target and handler 3. Tracks the same API through to WScriptJsrt in ch 4. Adds a test that uses the ch implementation (Targeting 1.9 as this will assist with an issue in node-chakracore nodejs/node-chakracore#488 ) **CC:** @jackhorton @kfarnung @dilijev

@jackhorton

…fixes #950 Merge pull request #4806 from rhuanjl:JsGetProxyProperties Responding to issue #950 This PR: 1. Adds a JavascriptProxy::IsRevoked method to the Javascript proxy class (necessary for the below) 2. Adds a JsGetProxyProperties API to Jsrt which can: a) check if an object is a proxy -> set a provided bool to true/false b) if it is a proxy check if it's revoked c) if it is a revoked proxy set provided target and handler references to nullptr d) if it is a proxy that is not revoked provide references to it's target and handler 3. Tracks the same API through to WScriptJsrt in ch 4. Adds a test that uses the ch implementation (Targeting 1.9 as this will assist with an issue in node-chakracore nodejs/node-chakracore#488 ) **CC:** @jackhorton @kfarnung @dilijev

rhuanjl · 2018-03-14T20:24:57Z

@boingoing thanks for getting this merged I hope it's helpful.

jackhorton approved these changes Mar 11, 2018

View reviewed changes

jackhorton added the External Contributor label Mar 11, 2018

rhuanjl force-pushed the JsGetProxyProperties branch 2 times, most recently from d8e37f4 to 9bd95d3 Compare March 11, 2018 18:14

digitalinfinity requested review from boingoing and akroshg March 11, 2018 23:48

boingoing reviewed Mar 13, 2018

View reviewed changes

boingoing approved these changes Mar 13, 2018

View reviewed changes

boingoing self-assigned this Mar 13, 2018

kfarnung approved these changes Mar 13, 2018

View reviewed changes

rhuanjl force-pushed the JsGetProxyProperties branch from 39fa9db to 4c538d6 Compare March 13, 2018 19:29

akroshg reviewed Mar 13, 2018

View reviewed changes

Implement JsGetProxyProperties API

e891f8c

rhuanjl force-pushed the JsGetProxyProperties branch from 4c538d6 to e891f8c Compare March 13, 2018 21:26

chakrabot merged commit e891f8c into chakra-core:release/1.9 Mar 13, 2018

rhuanjl deleted the JsGetProxyProperties branch March 13, 2018 23:11


		Js::JavascriptProxy* proxy = Js::JavascriptProxy::UnsafeFromVar(object);

		if (target != nullptr && handler != nullptr)

Implement JsGetProxyProperties API fixes #950 #4806

Implement JsGetProxyProperties API fixes #950 #4806

Conversation

rhuanjl commented Mar 10, 2018 • edited Loading

Choose a reason for hiding this comment

rhuanjl Mar 11, 2018 • edited 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

jackhorton Mar 11, 2018 • edited 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

rhuanjl commented Mar 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing left a comment

Choose a reason for hiding this comment

kfarnung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhuanjl commented Mar 13, 2018

rhuanjl commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing commented Mar 13, 2018

rhuanjl commented Mar 14, 2018

rhuanjl commented Mar 10, 2018 •

edited

Loading

rhuanjl Mar 11, 2018 •

edited

Loading

jackhorton Mar 11, 2018 •

edited

Loading

rhuanjl commented Mar 13, 2018 •

edited

Loading