Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

Commit

Permalink
chakrashim: Fixed multiple unittest bugs
Browse files Browse the repository at this point in the history
* There was a bug in SetCallback/DeleteCallback where we were not returning a
boolean value that set/delete traps expect. This fixes below 3 unittest failures.

`
parallel/test-fs-readfile-error
sequential/test-stdin-script-child
sequential/test-util-debug
`

* While working on `parallel/test-util` realized that we don't behave
correctly for following:
```javascript
utils.isRegExp(Object.create(RegExp.prototype));
```
Above code should fail because object returned from `Object.create()`
is a normal object whose prototype is `RegExp` which is different than
actual RegExp object. `utils.isRegExp()` checks if value is actual RegExp.
We implemented by calling `instanceof ` operator that passes because
the prototype of RegExp and value (which is Object) matches. This is
problem for other data types like Date, Array, etc.

[This](http://web.mit.edu/jwalden/www/isArray.html) is a good article
that explains how we should do such checks correctly. I implemented all
of our is*() functions in `chakra_shim.js` instead of `instanceof` operator
we had earlier. The implementation is basically :
``` javascript
Object.prototype.toString.call(obj) === "[object RegExp]"
```
I have also moved all of the existing Is* methods
Also while working on this, I realized that there was lot of duplicate code
getting added, so I wrote bunch of MACROS to refactored the code for
better readability.

This fixes unit test - `parallel/test-util`

Reviewed-By : @jianchun
  • Loading branch information
kunalspathak committed Jan 12, 2016
1 parent ee68330 commit 6c70170
Show file tree
Hide file tree
Showing 11 changed files with 188 additions and 214 deletions.
40 changes: 40 additions & 0 deletions deps/chakrashim/lib/chakra_shim.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Object.defineProperty(this, 'Debug',
Object_getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor,
Object_getOwnPropertyNames = Object.getOwnPropertyNames,
Object_keys = Object.keys,
Object_prototype_toString = Object.prototype.toString,
Map_keys = Map.prototype.keys,
Map_values = Map.prototype.values,
Map_entries = Map.prototype.entries,
Expand Down Expand Up @@ -315,6 +316,45 @@ Object.defineProperty(this, 'Debug',
utils.isSetIterator = function(value) {
return value[setIteratorProperty] == true;
};
function compareType(o, expectedType) {
return Object_prototype_toString.call(o) === '[object ' +
expectedType + ']';
};
utils.isBooleanObject = function(obj) {
return compareType(obj, 'Boolean');
};
utils.isDate = function(obj) {
return compareType(obj, 'Date');
};
utils.isMap = function(obj) {
return compareType(obj, 'Map');
};
utils.isNativeError = function(obj) {
return compareType(obj, 'Error') ||
obj instanceof Error ||
obj instanceof EvalError ||
obj instanceof RangeError ||
obj instanceof ReferenceError ||
obj instanceof SyntaxError ||
obj instanceof TypeError ||
obj instanceof URIError;
};
utils.isPromise = function(obj) {
return compareType(obj, 'Object') && obj instanceof Promise;
};
utils.isRegExp = function(obj) {
return compareType(obj, 'RegExp');
};
utils.isSet = function(obj) {
return compareType(obj, 'Set');
};
utils.isStringObject = function(obj) {
return compareType(obj, 'String');
};
utils.isNumberObject = function(obj) {
return compareType(obj, 'Number');
};

}

// patch console
Expand Down
20 changes: 17 additions & 3 deletions deps/chakrashim/src/jsrtcachedpropertyidref.inc
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@
#define DEFSYMBOL(x)
#endif

#ifndef DEF_IS_TYPE
#define DEF_IS_TYPE(x) DEF(x)
#endif

DEF(apply)
DEF(concat)
Expand Down Expand Up @@ -70,15 +73,26 @@ DEF(getColumnNumber)
DEF(getLineNumber)
DEF(prototype)
DEF(toString)
DEF(isMapIterator)
DEF(isSetIterator)


DEFSYMBOL(self)
DEFSYMBOL(__external__)
DEFSYMBOL(__hiddenvalues__)
DEFSYMBOL(__isexternal__)
DEFSYMBOL(__keepalive__)

DEF_IS_TYPE(isMapIterator)
DEF_IS_TYPE(isSetIterator)
DEF_IS_TYPE(isBooleanObject)
DEF_IS_TYPE(isDate)
DEF_IS_TYPE(isMap)
DEF_IS_TYPE(isNativeError)
DEF_IS_TYPE(isPromise)
DEF_IS_TYPE(isRegExp)
DEF_IS_TYPE(isSet)
DEF_IS_TYPE(isStringObject)
DEF_IS_TYPE(isNumberObject)


#undef DEF
#undef DEFSYMBOL
#undef DEF_IS_TYPE
14 changes: 1 addition & 13 deletions deps/chakrashim/src/jsrtcontextcachedobj.inc
Original file line number Diff line number Diff line change
Expand Up @@ -27,21 +27,12 @@
#endif

// These global constructors will be cached
DEFTYPE(ArrayBuffer)
DEFTYPE(Boolean)
DEFTYPE(Date)
DEFTYPE(Error)
DEFTYPE(EvalError)
DEFTYPE(Function)
DEFTYPE(Number)
DEFTYPE(Object)
DEFTYPE(Proxy)
DEFTYPE(RangeError)
DEFTYPE(ReferenceError)
DEFTYPE(RegExp)
DEFTYPE(String)
DEFTYPE(SyntaxError)
DEFTYPE(TypeError)
DEFTYPE(Uint8Array)
DEFTYPE(Uint8ClampedArray)
DEFTYPE(Int8Array)
Expand All @@ -51,10 +42,7 @@ DEFTYPE(Uint32Array)
DEFTYPE(Int32Array)
DEFTYPE(Float32Array)
DEFTYPE(Float64Array)
DEFTYPE(URIError)
DEFTYPE(Promise)
DEFTYPE(Map)
DEFTYPE(Set)



// These prototype functions will be cached/shimmed
Expand Down
92 changes: 25 additions & 67 deletions deps/chakrashim/src/jsrtcontextshim.cc
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ ContextShim * ContextShim::New(IsolateShim * isolateShim, bool exposeGC,
globalObjectTemplateInstance);
}

#define DEF_IS_TYPE(F) F##Function(JS_INVALID_REFERENCE),

ContextShim::ContextShim(IsolateShim * isolateShim,
JsContextRef context,
bool exposeGC,
Expand All @@ -60,19 +62,18 @@ ContextShim::ContextShim(IsolateShim * isolateShim,
exposeGC(exposeGC),
globalObjectTemplateInstance(globalObjectTemplateInstance),
promiseContinuationFunction(JS_INVALID_REFERENCE),
#include "jsrtcachedpropertyidref.inc"
#undef DEF_IS_TYPE
cloneObjectFunction(JS_INVALID_REFERENCE),
getPropertyNamesFunction(JS_INVALID_REFERENCE),
getOwnPropertyDescriptorFunction(JS_INVALID_REFERENCE),
getEnumerableNamedPropertiesFunction(JS_INVALID_REFERENCE),
getEnumerableIndexedPropertiesFunction(JS_INVALID_REFERENCE),
createEnumerationIteratorFunction(JS_INVALID_REFERENCE),
createPropertyDescriptorsEnumerationIteratorFunction(
JS_INVALID_REFERENCE),
createPropertyDescriptorsEnumerationIteratorFunction(JS_INVALID_REFERENCE),
getNamedOwnKeysFunction(JS_INVALID_REFERENCE),
getIndexedOwnKeysFunction(JS_INVALID_REFERENCE),
getStackTraceFunction(JS_INVALID_REFERENCE),
isMapIteratorFunction(JS_INVALID_REFERENCE),
isSetIteratorFunction(JS_INVALID_REFERENCE) {
getStackTraceFunction(JS_INVALID_REFERENCE) {
memset(globalConstructor, 0, sizeof(globalConstructor));
memset(globalPrototypeFunction, 0, sizeof(globalPrototypeFunction));
}
Expand Down Expand Up @@ -507,10 +508,6 @@ JsValueRef ContextShim::GetDateConstructor() {
return globalConstructor[GlobalType::Date];
}

JsValueRef ContextShim::GetRegExpConstructor() {
return globalConstructor[GlobalType::RegExp];
}

JsValueRef ContextShim::GetProxyConstructor() {
return globalConstructor[GlobalType::Proxy];
}
Expand Down Expand Up @@ -557,64 +554,25 @@ JsValueRef ContextShim::GetCachedShimFunction(CachedPropertyIdRef id,
return *func;
}

JsValueRef ContextShim::GetCloneObjectFunction() {
return GetCachedShimFunction(CachedPropertyIdRef::cloneObject,
&cloneObjectFunction);
}

JsValueRef ContextShim::GetGetPropertyNamesFunction() {
return GetCachedShimFunction(CachedPropertyIdRef::getPropertyNames,
&getPropertyNamesFunction);
}

JsValueRef ContextShim::GetGetEnumerableNamedPropertiesFunction() {
return GetCachedShimFunction(
CachedPropertyIdRef::getEnumerableNamedProperties,
&getEnumerableNamedPropertiesFunction);
}

JsValueRef ContextShim::GetGetEnumerableIndexedPropertiesFunction() {
return GetCachedShimFunction(
CachedPropertyIdRef::getEnumerableIndexedProperties,
&getEnumerableIndexedPropertiesFunction);
}

JsValueRef ContextShim::GetCreateEnumerationIteratorFunction() {
return GetCachedShimFunction(CachedPropertyIdRef::createEnumerationIterator,
&createEnumerationIteratorFunction);
}

JsValueRef
ContextShim::GetCreatePropertyDescriptorsEnumerationIteratorFunction() {
return GetCachedShimFunction(
CachedPropertyIdRef::createPropertyDescriptorsEnumerationIterator,
&createPropertyDescriptorsEnumerationIteratorFunction);
}

JsValueRef ContextShim::GetGetNamedOwnKeysFunction() {
return GetCachedShimFunction(CachedPropertyIdRef::getNamedOwnKeys,
&getNamedOwnKeysFunction);
}

JsValueRef ContextShim::GetGetIndexedOwnKeysFunction() {
return GetCachedShimFunction(CachedPropertyIdRef::getIndexedOwnKeys,
&getIndexedOwnKeysFunction);
}

JsValueRef ContextShim::GetGetStackTraceFunction() {
return GetCachedShimFunction(CachedPropertyIdRef::getStackTrace,
&getStackTraceFunction);
}

JsValueRef ContextShim::GetIsMapIteratorFunction() {
return GetCachedShimFunction(CachedPropertyIdRef::isMapIterator,
&isMapIteratorFunction);
}

JsValueRef ContextShim::GetIsSetIteratorFunction() {
return GetCachedShimFunction(CachedPropertyIdRef::isSetIterator,
&isSetIteratorFunction);
}
#define CHAKRASHIM_FUNCTION_GETTER(F) \
JsValueRef ContextShim::Get##F##Function() { \
return GetCachedShimFunction(CachedPropertyIdRef::F, \
&##F##Function); \
} \

CHAKRASHIM_FUNCTION_GETTER(cloneObject)
CHAKRASHIM_FUNCTION_GETTER(getPropertyNames)
CHAKRASHIM_FUNCTION_GETTER(getEnumerableNamedProperties)
CHAKRASHIM_FUNCTION_GETTER(getEnumerableIndexedProperties)
CHAKRASHIM_FUNCTION_GETTER(createEnumerationIterator)
CHAKRASHIM_FUNCTION_GETTER(createPropertyDescriptorsEnumerationIterator)
CHAKRASHIM_FUNCTION_GETTER(getNamedOwnKeys)
CHAKRASHIM_FUNCTION_GETTER(getIndexedOwnKeys)
CHAKRASHIM_FUNCTION_GETTER(getStackTrace)

#define DEF_IS_TYPE(F) CHAKRASHIM_FUNCTION_GETTER(F)
#include "jsrtcachedpropertyidref.inc"
#undef DEF_IS_TYPE

} // namespace jsrt

Expand Down
47 changes: 21 additions & 26 deletions deps/chakrashim/src/jsrtcontextshim.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
#include <vector>

namespace jsrt {

class ContextShim {
public:
// This has the same layout as v8::Context::Scope
Expand Down Expand Up @@ -79,17 +78,6 @@ class ContextShim {
JsValueRef GetReflectObject();
JsValueRef GetReflectFunctionForTrap(ProxyTraps traps);

JsValueRef GetCloneObjectFunction();
JsValueRef GetGetPropertyNamesFunction();
JsValueRef GetGetEnumerableNamedPropertiesFunction();
JsValueRef GetGetEnumerableIndexedPropertiesFunction();
JsValueRef GetCreateEnumerationIteratorFunction();
JsValueRef GetCreatePropertyDescriptorsEnumerationIteratorFunction();
JsValueRef GetGetNamedOwnKeysFunction();
JsValueRef GetGetIndexedOwnKeysFunction();
JsValueRef GetGetStackTraceFunction();
JsValueRef GetIsMapIteratorFunction();
JsValueRef GetIsSetIteratorFunction();

void * GetAlignedPointerFromEmbedderData(int index);
void SetAlignedPointerInEmbedderData(int index, void * value);
Expand Down Expand Up @@ -138,21 +126,28 @@ class ContextShim {
JsValueRef getOwnPropertyDescriptorFunction;

JsValueRef promiseContinuationFunction;

JsValueRef cloneObjectFunction;
JsValueRef getPropertyNamesFunction;

JsValueRef getEnumerableNamedPropertiesFunction;
JsValueRef getEnumerableIndexedPropertiesFunction;
JsValueRef createEnumerationIteratorFunction;
JsValueRef createPropertyDescriptorsEnumerationIteratorFunction;
JsValueRef getNamedOwnKeysFunction;
JsValueRef getIndexedOwnKeysFunction;
JsValueRef getStackTraceFunction;
JsValueRef isMapIteratorFunction;
JsValueRef isSetIteratorFunction;

std::vector<void*> embedderData;

#define DECLARE_CHAKRASHIM_FUNCTION_GETTER(F) \
public: \
JsValueRef Get##F##Function(); \
private: \
JsValueRef F##Function; \

DECLARE_CHAKRASHIM_FUNCTION_GETTER(cloneObject);
DECLARE_CHAKRASHIM_FUNCTION_GETTER(getPropertyNames);
DECLARE_CHAKRASHIM_FUNCTION_GETTER(getEnumerableNamedProperties);
DECLARE_CHAKRASHIM_FUNCTION_GETTER(getEnumerableIndexedProperties);
DECLARE_CHAKRASHIM_FUNCTION_GETTER(createEnumerationIterator);
DECLARE_CHAKRASHIM_FUNCTION_GETTER
(createPropertyDescriptorsEnumerationIterator);
DECLARE_CHAKRASHIM_FUNCTION_GETTER(getNamedOwnKeys);
DECLARE_CHAKRASHIM_FUNCTION_GETTER(getIndexedOwnKeys);
DECLARE_CHAKRASHIM_FUNCTION_GETTER(getStackTrace);

#define DEF_IS_TYPE(F) DECLARE_CHAKRASHIM_FUNCTION_GETTER(F)
#include "jsrtcachedpropertyidref.inc"
#undef DEF_IS_TYPE
};

} // namespace jsrt
Loading

0 comments on commit 6c70170

Please sign in to comment.