chakrashim: Fix GetOwnPropertyNames and GetPropertyNames

As per spec `Object.getOwnPropertyNames()` and `Object.getPropertyNames()` should return an array of strings. However there is no standardization of what should happen if these methods are called natively (through NaN for example). For these 2 APIs, v8 returns an array containing mix of numbers for numeric properties and strings for normal string properties. This breaks [grpc](https://github.com/grpc/grpc) module because it [expects](https://github.com/grpc/grpc/blob/master/src/node/ext/call.cc#L673-L683) returned values to be numeric. Fix: Once we get the result from, iterate over the result and call `parseInt` to save numeric value of the key. PR-URL: nodejs/node#169 Reviewed-By: Jianchun Xu <[email protected]>
kunalspathak · Jan 23, 2017 · 9716837 · 9716837
1 parent 8e66382
commit 9716837
Show file tree

Hide file tree

Showing 5 changed files with 39 additions and 10 deletions.
diff --git a/deps/chakrashim/lib/chakra_shim.js b/deps/chakrashim/lib/chakra_shim.js
@@ -37,7 +37,8 @@
     Set_entries = Set.prototype.entries,
     Set_values = Set.prototype.values,
     Symbol_keyFor = Symbol.keyFor,
-    Symbol_for = Symbol.for;
+    Symbol_for = Symbol.for,
+    Global_ParseInt = parseInt;
   var BuiltInError = Error;
   var global = this;
 
@@ -276,7 +277,7 @@
       Error, EvalError, RangeError, ReferenceError, SyntaxError, TypeError,
       URIError
     ].forEach(function(type) {
-      var newType = function __newType() {
+      var newType = function newType() {
         var e = withStackTraceLimitOffset(
           3, () => Reflect_construct(type, arguments, new.target || newType));
         // skip 3 frames: lambda, withStackTraceLimitOffset, this frame
@@ -428,7 +429,7 @@
   var microTasks = [];
 
   function patchUtils(utils) {
-    var isUintRegex = /^(0|[1-9]\\d*)$/;
+    var isUintRegex = /^(0|[1-9]\d*)$/;
 
     var isUint = function(value) {
       var result = isUintRegex.test(value);
@@ -449,7 +450,11 @@
     utils.getPropertyNames = function(a) {
       var names = [];
       for (var propertyName in a) {
-        names.push(propertyName);
+        if (isUint(propertyName)) {
+          names.push(Global_ParseInt(propertyName));
+        } else {
+          names.push(propertyName);
+        }
       }
       return names;
     };
@@ -588,6 +593,22 @@
       }
       return attributes;
     };
+    utils.getOwnPropertyNames = function(obj) {
+      var ownPropertyNames = Object_getOwnPropertyNames(obj);
+      var i = 0;
+      while (i < ownPropertyNames.length) {
+        var item = ownPropertyNames[i];
+        if (isUint(item)) {
+          ownPropertyNames[i] = Global_ParseInt(item);
+          i++;
+          continue;
+        }
+        // As per spec, getOwnPropertyNames() first include
+        // numeric properties followed by non-numeric
+        break;
+      }
+      return ownPropertyNames;
+    };
   }
 
   patchErrorTypes();

diff --git a/deps/chakrashim/src/jsrtcachedpropertyidref.inc b/deps/chakrashim/src/jsrtcachedpropertyidref.inc
@@ -75,6 +75,7 @@ DEF(enqueueMicrotask)
 DEF(dequeueMicrotask)
 DEF(saveInHandleScope)
 DEF(getPropertyAttributes)
+DEF(getOwnPropertyNames)
 DEF(getFunctionName)
 DEF(getFileName)
 DEF(getColumnNumber)

diff --git a/deps/chakrashim/src/jsrtcontextshim.cc b/deps/chakrashim/src/jsrtcontextshim.cc
@@ -99,7 +99,8 @@ ContextShim::ContextShim(IsolateShim * isolateShim,
       ensureDebugFunction(JS_INVALID_REFERENCE),
       enqueueMicrotaskFunction(JS_INVALID_REFERENCE),
       dequeueMicrotaskFunction(JS_INVALID_REFERENCE),
-      getPropertyAttributesFunction(JS_INVALID_REFERENCE) {
+      getPropertyAttributesFunction(JS_INVALID_REFERENCE),
+      getOwnPropertyNamesFunction(JS_INVALID_REFERENCE) {
   memset(globalConstructor, 0, sizeof(globalConstructor));
   memset(globalPrototypeFunction, 0, sizeof(globalPrototypeFunction));
 }
@@ -613,6 +614,7 @@ CHAKRASHIM_FUNCTION_GETTER(ensureDebug)
 CHAKRASHIM_FUNCTION_GETTER(enqueueMicrotask);
 CHAKRASHIM_FUNCTION_GETTER(dequeueMicrotask);
 CHAKRASHIM_FUNCTION_GETTER(getPropertyAttributes);
+CHAKRASHIM_FUNCTION_GETTER(getOwnPropertyNames);
 
 #define DEF_IS_TYPE(F) CHAKRASHIM_FUNCTION_GETTER(F)
 #include "jsrtcachedpropertyidref.inc"

diff --git a/deps/chakrashim/src/jsrtcontextshim.h b/deps/chakrashim/src/jsrtcontextshim.h
@@ -53,7 +53,7 @@ class ContextShim {
   };
 
   static ContextShim * New(IsolateShim * isolateShim, bool exposeGC,
-                           bool useGlobalTTState, 
+                           bool useGlobalTTState,
                            JsValueRef globalObjectTemplateInstance);
   ~ContextShim();
   void EnsureInitialized();
@@ -156,6 +156,7 @@ private: \
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(enqueueMicrotask);
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(dequeueMicrotask);
   DECLARE_CHAKRASHIM_FUNCTION_GETTER(getPropertyAttributes);
+  DECLARE_CHAKRASHIM_FUNCTION_GETTER(getOwnPropertyNames);
 };
 
 }  // namespace jsrt
diff --git a/deps/chakrashim/src/v8object.cc b/deps/chakrashim/src/v8object.cc
@@ -405,13 +405,17 @@ Local<Array> Object::GetPropertyNames() {
 }
 
 MaybeLocal<Array> Object::GetOwnPropertyNames(Local<Context> context) {
-  JsValueRef arrayRef;
+  ContextShim* contextShim = ContextShim::GetCurrent();
+  JsValueRef getOwnPropertyNamesFunction =
+    contextShim->GetgetOwnPropertyNamesFunction();
+
+  JsValueRef result;
 
-  if (JsGetOwnPropertyNames((JsValueRef)this, &arrayRef) != JsNoError) {
+  if (jsrt::CallFunction(getOwnPropertyNamesFunction, (JsValueRef)this,
+                         &result) != JsNoError) {
     return Local<Array>();
   }
-
-  return Local<Array>::New(arrayRef);
+  return Local<Array>::New(result);
 }
 
 Local<Array> Object::GetOwnPropertyNames() {