Skip to content

Commit

Permalink
chakrashim: Fix bug in repl scenario
Browse files Browse the repository at this point in the history 
Earlier whenever repl was created (`node.exe` with zero parameters),
global context was reused. However https://github.com/nodejs/node/
pull/5703 fixed it by creating new context for repl. This broke the
chakrashim because the context was getting collected immediately. The
reason was we were adding the reference of global context to the
sandboxed object instead of newly created context. Fixed it by
ensuring we add reference to right context.  Also contextShim is
always initialized when current context was pushed on the scope.
However for scenarios like this, we might just create the context and
access the objects like global, etc. of that context without going to
push scope path. In that case ensure that things are initialized in
the new context.  But, @jianchun brought a good point offline of
perf impact because of additional checks if context is initialized in
getters. So I re-verified all the references of getters and noticed
that all of them are called on ` ContextShim::GetCurrent()`. Context
returned through this API is always the one that is set in PushScope/
PopScope code path, where we do initialize contextShim. Hence I have
reverted `EnsureInitialize()` call in all the getters except `
GetProxyOfGlobal()`. Once nodejs#95 is fixed, this need to be
revisited and `EnsureInitialized` will need to be added in getters
that can be called from different context.

PR-URL: nodejs/node-chakracore#96
Reviewed-By: Jianchun Xu <Jianchun.Xu@microsoft.com>
  • Loading branch information
@kunalspathak
kunalspathak committed Jul 13, 2016
1 parent 11db4f2 commit 983c0b8
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 107 deletions.
167 changes: 75 additions & 92 deletions deps/chakrashim/src/jsrtcontextshim.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,13 @@ ContextShim::ContextShim(IsolateShim * isolateShim,
context(context),
initialized(false),
exposeGC(exposeGC),
trueRef(JS_INVALID_REFERENCE),
falseRef(JS_INVALID_REFERENCE),
undefinedRef(JS_INVALID_REFERENCE),
nullRef(JS_INVALID_REFERENCE),
zero(JS_INVALID_REFERENCE),
globalObject(JS_INVALID_REFERENCE),
proxyOfGlobal(JS_INVALID_REFERENCE),
globalObjectTemplateInstance(globalObjectTemplateInstance),
promiseContinuationFunction(JS_INVALID_REFERENCE),
#include "jsrtcachedpropertyidref.inc"
Expand Down Expand Up @@ -305,10 +312,6 @@ bool ContextShim::InitializeBuiltIns() {
return false;
}

if (!InitializeReflect()) {
return false;
}

if (DefineProperty(globalObject,
GetIsolateShim()->GetKeepAliveObjectSymbolPropertyIdRef(),
PropertyDescriptorOptionValues::False,
Expand Down Expand Up @@ -336,27 +339,6 @@ static JsValueRef CALLBACK ProxyOfGlobalGetPrototypeOfCallback(
return arguments[1];
}

bool ContextShim::InitializeReflect() {
if (!InitializeBuiltIn(&reflectObject,
[](JsValueRef * value) {
return GetPropertyOfGlobal(L"Reflect", value);
})) {
return false;
}

for (unsigned int i = 0; i < ProxyTraps::TrapCount; i++) {
if (!InitializeBuiltIn(&reflectFunctions[i],
[this, i](JsValueRef * value) {
return JsGetProperty(reflectObject,
this->GetIsolateShim()->GetProxyTrapPropertyIdRef((ProxyTraps)i),
value);
})) {
return false;
}
}
return true;
}

bool ContextShim::InitializeProxyOfGlobal() {
return InitializeBuiltIn(&proxyOfGlobal,
[this](JsValueRef * value) {
Expand All @@ -368,9 +350,18 @@ bool ContextShim::InitializeProxyOfGlobal() {
});
}

bool ContextShim::EnsureInitialized() {
if (initialized) {
return true;

bool ContextShim::DoInitializeContextShim() {
JsContextRef currentContext = IsolateShim::GetCurrent()
->GetCurrentContextShim()->GetContextRef();

JsContextRef thisContext = GetContextRef();
bool needToSwitchContext = currentContext != thisContext;

if (needToSwitchContext) {
if (JsSetCurrentContext(thisContext) != JsNoError) {
return false;
}
}

if (jsrt::InitializePromise() != JsNoError) {
Expand All @@ -389,8 +380,6 @@ bool ContextShim::EnsureInitialized() {
return false;
}

initialized = true;

// Following is a special one-time initialization that needs to marshal
// objects to this context. Do it after marking initialized = true.
if (!CheckConfigGlobalObjectTemplate()) {
Expand All @@ -399,12 +388,30 @@ bool ContextShim::EnsureInitialized() {

// add idleGC callback into prepareQueue
if (IsolateShim::IsIdleGcEnabled()) {
uv_prepare_start(IsolateShim::GetCurrent()->idleGc_prepare_handle(), PrepareIdleGC);
uv_prepare_start(IsolateShim::GetCurrent()->idleGc_prepare_handle(),
PrepareIdleGC);
}

if (needToSwitchContext) {
if (JsSetCurrentContext(currentContext) != JsNoError) {
return false;
}
}
return true;
}

void ContextShim::EnsureInitialized() {
if (initialized) {
return;
}

initialized = true;

if (!DoInitializeContextShim()) {
Fatal("Failed to initialize context");
}
}

bool ContextShim::ExposeGc() {
JsValueRef collectGarbageRef;

Expand Down Expand Up @@ -477,84 +484,60 @@ void ContextShim::RunMicrotasks() {

JsValueRef notUsed;
if (jsrt::CallFunction(task, &notUsed) != JsNoError) {
JsGetAndClearException(&notUsed); // swallow any exception from task
JsGetAndClearException(&notUsed); // swallow any exception from task
}
}
}

JsValueRef ContextShim::GetUndefined() {
return undefinedRef;
}

JsValueRef ContextShim::GetTrue() {
return trueRef;
}

JsValueRef ContextShim::GetFalse() {
return falseRef;
}

JsValueRef ContextShim::GetNull() {
return nullRef;
}

JsValueRef ContextShim::GetZero() {
return zero;
}

JsValueRef ContextShim::GetObjectConstructor() {
return globalConstructor[GlobalType::Object];
}

JsValueRef ContextShim::GetBooleanObjectConstructor() {
return globalConstructor[GlobalType::Boolean];
}
// check initialization state first instead of calling
// InitializeCurrentContextShim to save a function call for cases where
// contextshim is already initialized
#define DECLARE_GETOBJECT(name, object) \
JsValueRef ContextShim::Get##name() { \
CHAKRA_ASSERT(object != JS_INVALID_REFERENCE); \
return object; \
} \

JsValueRef ContextShim::GetNumberObjectConstructor() {
return globalConstructor[GlobalType::Number];
}
DECLARE_GETOBJECT(True, trueRef)
DECLARE_GETOBJECT(False, falseRef)
DECLARE_GETOBJECT(Undefined, undefinedRef)
DECLARE_GETOBJECT(Null, nullRef)
DECLARE_GETOBJECT(Zero, zero)
DECLARE_GETOBJECT(ObjectConstructor,
globalConstructor[GlobalType::Object])
DECLARE_GETOBJECT(BooleanObjectConstructor,
globalConstructor[GlobalType::Boolean])
DECLARE_GETOBJECT(NumberObjectConstructor,
globalConstructor[GlobalType::Number])
DECLARE_GETOBJECT(StringObjectConstructor,
globalConstructor[GlobalType::String])
DECLARE_GETOBJECT(DateConstructor,
globalConstructor[GlobalType::Date])
DECLARE_GETOBJECT(ProxyConstructor,
globalConstructor[GlobalType::Proxy])
DECLARE_GETOBJECT(GetOwnPropertyDescriptorFunction,
getOwnPropertyDescriptorFunction)
DECLARE_GETOBJECT(StringConcatFunction,
globalPrototypeFunction[GlobalPrototypeFunction
::String_concat])

JsValueRef ContextShim::GetStringObjectConstructor() {
return globalConstructor[GlobalType::String];
}

JsValueRef ContextShim::GetDateConstructor() {
return globalConstructor[GlobalType::Date];
}

JsValueRef ContextShim::GetProxyConstructor() {
return globalConstructor[GlobalType::Proxy];
JsValueRef ContextShim::GetProxyOfGlobal() {
EnsureInitialized();
CHAKRA_ASSERT(proxyOfGlobal != JS_INVALID_REFERENCE);
return proxyOfGlobal;
}

JsValueRef ContextShim::GetGlobalType(GlobalType index) {
CHAKRA_ASSERT(globalConstructor[index] != JS_INVALID_REFERENCE);
return globalConstructor[index];
}

JsValueRef ContextShim::GetGetOwnPropertyDescriptorFunction() {
return getOwnPropertyDescriptorFunction;
}

JsValueRef ContextShim::GetStringConcatFunction() {
return globalPrototypeFunction[GlobalPrototypeFunction::String_concat];
}

JsValueRef ContextShim::GetGlobalPrototypeFunction(
GlobalPrototypeFunction index) {
CHAKRA_ASSERT(globalPrototypeFunction[index] != JS_INVALID_REFERENCE);
return globalPrototypeFunction[index];
}

JsValueRef ContextShim::GetProxyOfGlobal() {
return proxyOfGlobal;
}

JsValueRef ContextShim::GetReflectObject() {
return reflectObject;
}

JsValueRef ContextShim::GetReflectFunctionForTrap(ProxyTraps trap) {
return reflectFunctions[trap];
}

JsValueRef ContextShim::GetCachedShimFunction(CachedPropertyIdRef id,
JsValueRef* func) {
if (*func == JS_INVALID_REFERENCE) {
Expand Down
17 changes: 6 additions & 11 deletions deps/chakrashim/src/jsrtcontextshim.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class ContextShim {
static ContextShim * New(IsolateShim * isolateShim, bool exposeGC,
JsValueRef globalObjectTemplateInstance);
~ContextShim();
bool EnsureInitialized();
void EnsureInitialized();

IsolateShim * GetIsolateShim();
JsContextRef GetContextRef();
Expand All @@ -74,9 +74,6 @@ class ContextShim {
JsValueRef GetStringConcatFunction();
JsValueRef GetGlobalPrototypeFunction(GlobalPrototypeFunction index);
JsValueRef GetProxyOfGlobal();
JsValueRef GetReflectObject();
JsValueRef GetReflectFunctionForTrap(ProxyTraps traps);


void * GetAlignedPointerFromEmbedderData(int index);
void SetAlignedPointerInEmbedderData(int index, void * value);
Expand All @@ -87,9 +84,9 @@ class ContextShim {
private:
ContextShim(IsolateShim * isolateShim, JsContextRef context, bool exposeGC,
JsValueRef globalObjectTemplateInstance);
bool DoInitializeContextShim();
bool InitializeBuiltIns();
bool InitializeProxyOfGlobal();
bool InitializeReflect();
bool InitializeGlobalPrototypeFunctions();
bool InitializeObjectPrototypeToStringShim();

Expand Down Expand Up @@ -119,8 +116,6 @@ class ContextShim {
JsValueRef globalConstructor[GlobalType::_TypeCount];
JsValueRef globalObject;
JsValueRef proxyOfGlobal;
JsValueRef reflectObject;
JsValueRef reflectFunctions[ProxyTraps::TrapCount];

JsValueRef globalPrototypeFunction[GlobalPrototypeFunction::_FunctionCount];
JsValueRef getOwnPropertyDescriptorFunction;
Expand All @@ -129,10 +124,10 @@ class ContextShim {
std::vector<void*> embedderData;

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

DECLARE_CHAKRASHIM_FUNCTION_GETTER(cloneObject);
DECLARE_CHAKRASHIM_FUNCTION_GETTER(getPropertyNames);
Expand Down
4 changes: 1 addition & 3 deletions deps/chakrashim/src/jsrtisolateshim.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -225,9 +225,7 @@ void IsolateShim::PushScope(
JsErrorCode errorCode = JsSetCurrentContext(contextShim->GetContextRef());
CHAKRA_ASSERT(errorCode == JsNoError);

if (!contextShim->EnsureInitialized()) {
Fatal("Failed to initialize context");
}
contextShim->EnsureInitialized();
}

void IsolateShim::PopScope(ContextShim::Scope * scope) {
Expand Down
2 changes: 1 addition & 1 deletion deps/chakrashim/src/v8context.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ Local<Object> Context::Global() {
// V8 Global is actually proxy where the actual global is it's prototype.
// No need to create handle here, the context will keep it alive
return Local<Object>(static_cast<Object *>(
jsrt::ContextShim::GetCurrent()->GetProxyOfGlobal()));
jsrt::IsolateShim::GetContextShim((JsContextRef*)this)->GetProxyOfGlobal()));
}

extern bool g_exposeGC;
Expand Down

0 comments on commit 983c0b8

Please # to comment.