chakrashim: Fix bug in repl scenario

Earlier whenever repl was created (`node.exe` with zero parameters), global context was reused. However nodejs/node#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.
kunalspathak · Jul 12, 2016 · 28894b0 · 28894b0
1 parent 97e8928
commit 28894b0
Show file tree

Hide file tree

Showing 3 changed files with 81 additions and 62 deletions.
diff --git a/deps/chakrashim/src/jsrtcontextshim.cc b/deps/chakrashim/src/jsrtcontextshim.cc
@@ -368,11 +368,34 @@ bool ContextShim::InitializeProxyOfGlobal() {
   });
 }
 
-bool ContextShim::EnsureInitialized() {
+
+bool ContextShim::InitializeCurrentContextShim() {
   if (initialized) {
     return true;
   }
 
+  JsContextRef oldContext;
+  if (JsGetCurrentContext(&oldContext) != JsNoError) {
+    return false;
+  }
+  JsContextRef thisContext = GetContextRef();
+  if (JsSetCurrentContext(thisContext) != JsNoError) {
+    return false;
+  }
+  if (!EnsureInitialized()) {
+    Fatal("Failed to initialize context");
+  }
+  if (JsSetCurrentContext(oldContext) != JsNoError) {
+    return false;
+  }
+  return true;
+}
+
+bool ContextShim::EnsureInitialized() {
+  if (initialized) {
+    return true;
+  }
+  initializing = true;
   if (jsrt::InitializePromise() != JsNoError) {
     return false;
   }
@@ -390,6 +413,7 @@ bool ContextShim::EnsureInitialized() {
   }
 
   initialized = true;
+  initializing = false;
 
   // Following is a special one-time initialization that needs to marshal
   // objects to this context. Do it after marking initialized = true.
@@ -482,76 +506,69 @@ void ContextShim::RunMicrotasks() {
   }
 }
 
-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];
-}
-
-JsValueRef ContextShim::GetNumberObjectConstructor() {
-  return globalConstructor[GlobalType::Number];
-}
-
-JsValueRef ContextShim::GetStringObjectConstructor() {
-  return globalConstructor[GlobalType::String];
-}
-
-JsValueRef ContextShim::GetDateConstructor() {
-  return globalConstructor[GlobalType::Date];
-}
-
-JsValueRef ContextShim::GetProxyConstructor() {
-  return globalConstructor[GlobalType::Proxy];
-}
+// 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() { \
+  if(!(initialized || initializing)) { \
+    if (!InitializeCurrentContextShim()) { \
+      Fatal("Failed to initialize current context"); \
+    } \
+  } \
+  return object; \
+} \
 
+DECLARE_GETOBJECT(True, trueRef)
+DECLARE_GETOBJECT(False, falseRef)
+DECLARE_GETOBJECT(Undefined, undefinedRef)
+DECLARE_GETOBJECT(Null, nullRef)
+DECLARE_GETOBJECT(Zero, zero)
+DECLARE_GETOBJECT(ProxyOfGlobal, proxyOfGlobal)
+DECLARE_GETOBJECT(ReflectObject, reflectObject)
+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::GetGlobalType(GlobalType index) {
+  if(!(initialized || initializing)) {
+    if (!InitializeCurrentContextShim()) {
+      Fatal("Failed to initialize current context");
+    }
+  }
   return globalConstructor[index];
 }
 
-JsValueRef ContextShim::GetGetOwnPropertyDescriptorFunction() {
-  return getOwnPropertyDescriptorFunction;
-}
-
-JsValueRef ContextShim::GetStringConcatFunction() {
-  return globalPrototypeFunction[GlobalPrototypeFunction::String_concat];
-}
-
 JsValueRef ContextShim::GetGlobalPrototypeFunction(
     GlobalPrototypeFunction index) {
+  if (!(initialized || initializing)) {
+    if (!InitializeCurrentContextShim()) {
+      Fatal("Failed to initialize current context");
+    }
+  }
   return globalPrototypeFunction[index];
 }
 
-JsValueRef ContextShim::GetProxyOfGlobal() {
-  return proxyOfGlobal;
-}
-
-JsValueRef ContextShim::GetReflectObject() {
-  return reflectObject;
-}
-
 JsValueRef ContextShim::GetReflectFunctionForTrap(ProxyTraps trap) {
+  if (!(initialized || initializing)) {
+    if (!InitializeCurrentContextShim()) {
+      Fatal("Failed to initialize current context");
+    }
+  }
   return reflectFunctions[trap];
 }
 

diff --git a/deps/chakrashim/src/jsrtcontextshim.h b/deps/chakrashim/src/jsrtcontextshim.h
@@ -53,6 +53,7 @@ class ContextShim {
                            JsValueRef globalObjectTemplateInstance);
   ~ContextShim();
   bool EnsureInitialized();
+  bool InitializeCurrentContextShim();
 
   IsolateShim * GetIsolateShim();
   JsContextRef GetContextRef();
@@ -105,7 +106,8 @@ class ContextShim {
 
   IsolateShim * isolateShim;
   JsContextRef context;
-  bool initialized;
+  bool initialized = false;
+  bool initializing = false;
   bool exposeGC;
   JsValueRef keepAliveObject;
   int builtInCount;

diff --git a/deps/chakrashim/src/v8context.cc b/deps/chakrashim/src/v8context.cc
@@ -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;