chakrashim: fix some cross-context bugs

Change ContextShim::RegisterCrossContextObject() to use a finalizer instead of JsSetObjectBeforeCollectCallback. The later callback does not guarantee object collection. The object can be subsequently revived as a result of other objects being revived. When this happens we would clear the registered cross-context mapping info prematurely. Change UnwrapIfCrossContext() to use JsGetOwnPropertyDescriptor to read CrossContextTargetSymbol property. Original JsGetProperty could read from a prototype object and incorrectly unwraps the prototype object instead of the current object. Change MarshalObjectToContext() to re-enter correct fromContext when the object was cross-context and unwrapped. Change GetOwnPropertyDescriptorCallback() to return "undefined" instead of "false" for missing cases. Returning "false" is wrong. Reviewed-by: kunalspathak, curtisman
nodejs · Jun 29, 2015 · 26ff29c · 26ff29c
1 parent 93adbaa
commit 26ff29c
Show file tree

Hide file tree

Showing 6 changed files with 111 additions and 50 deletions.
diff --git a/deps/chakrashim/src/jsrtcontextshim.cc b/deps/chakrashim/src/jsrtcontextshim.cc
@@ -461,19 +461,31 @@ bool ContextShim::ExecuteChakraShimJS() {
 bool ContextShim::RegisterCrossContextObject(JsValueRef fakeTarget,
                                              const CrossContextMapInfo& info) {
   // Ensure fakeTarget lifetime encloses proxy lifetime
-  if (jsrt::SetProperty(fakeTarget, L"proxy", info.proxy) != JsNoError) {
+  if (JsSetProperty(fakeTarget,
+                    isolateShim->GetProxySymbolPropertyIdRef(),
+                    info.proxy, false) != JsNoError) {
     return false;
   }
 
   try {
     CrossContextMapInfo* mapInfoCopy = new CrossContextMapInfo(info);
-    if (JsSetObjectBeforeCollectCallback(fakeTarget,
-                    mapInfoCopy,
-                    CrossContextFakeTargeBeforeCollectCallback) != JsNoError) {
+
+    // Install a finalizer to clean up the map entry when proxy/fakeTarget are
+    // collected by GC.
+    JsValueRef finalizeObj;
+    if (JsCreateExternalObject(mapInfoCopy,
+                               CrossContextFakeTargetFinalizeCallback,
+                               &finalizeObj) != JsNoError) {
       delete mapInfoCopy;
       return false;
     }
 
+    if (JsSetProperty(fakeTarget,
+                      isolateShim->GetFinalizerSymbolPropertyIdRef(),
+                      finalizeObj, false) != JsNoError) {
+      return false;
+    }
+
     crossContextObjects[info.object].push_back(mapInfoCopy);
     return true;  // success
   } catch (const std::bad_alloc&) {
@@ -525,8 +537,8 @@ bool ContextShim::TryGetCrossContextObject(JsValueRef object,
   return false;
 }
 
-void CALLBACK ContextShim::CrossContextFakeTargeBeforeCollectCallback(
-    JsRef ref, void *callbackState) {
+void CALLBACK ContextShim::CrossContextFakeTargetFinalizeCallback(
+    void *callbackState) {
   CrossContextMapInfo* info = static_cast<CrossContextMapInfo*>(callbackState);
 
   ContextShim* fromContext = info->fromContext;

diff --git a/deps/chakrashim/src/jsrtcontextshim.h b/deps/chakrashim/src/jsrtcontextshim.h
@@ -138,8 +138,8 @@ class ContextShim {
   bool UnregisterCrossContextObject(const CrossContextMapInfo& info);
   bool TryGetCrossContextObject(JsValueRef object, ContextShim* toContext,
                                 JsValueRef* proxy);
-  static void CALLBACK CrossContextFakeTargeBeforeCollectCallback(
-    JsRef ref, void *callbackState);
+  static void CALLBACK CrossContextFakeTargetFinalizeCallback(
+    void *callbackState);
 
   friend JsValueRef MarshalObjectToContext(JsValueType valueType,
                                            JsValueRef valueRef,

diff --git a/deps/chakrashim/src/jsrtcrosscontext.cc b/deps/chakrashim/src/jsrtcrosscontext.cc
@@ -71,27 +71,43 @@ static CrossContextInfo * GetCrossContextInfo(JsValueRef targetObject) {
 
 static bool UnwrapIfCrossContext(
     JsValueRef valueRef, CrossContextInfo ** info) {
-  JsValueRef crossContextTarget;
-  if (CheckMarshalFailed(JsGetProperty(valueRef,
-      IsolateShim::GetCurrent()->GetCrossContextTargetSymbolPropertyIdRef(),
-      &crossContextTarget))) {
+  IsolateShim* iso = IsolateShim::GetCurrent();
+
+  // Try get own property CrossContextTargetSymbol
+  JsValueRef desc;
+  if (JsGetOwnPropertyDescriptor(valueRef,
+                                iso->GetCrossContextTargetSymbolPropertyIdRef(),
+                                &desc) != JsNoError) {
     return false;
   }
 
-  bool notCrossContextTarget;
-  if (CheckMarshalFailed(IsUndefined(crossContextTarget,
-                                     &notCrossContextTarget))) {
+  bool isUndefined;
+  if (CheckMarshalFailed(IsUndefined(desc, &isUndefined))) {
     return false;
   }
 
   *info = nullptr;
-  if (!notCrossContextTarget) {
-    CrossContextInfo * crossContextInfo =
-      GetCrossContextInfo(crossContextTarget);
-    if (crossContextInfo == nullptr) {
+  if (!isUndefined) {
+    JsValueRef crossContextTarget;
+    if (CheckMarshalFailed(JsGetProperty(desc,
+                        iso->GetCachedPropertyIdRef(CachedPropertyIdRef::value),
+                        &crossContextTarget))) {
+      return false;
+    }
+
+    if (CheckMarshalFailed(IsUndefined(crossContextTarget,
+                                       &isUndefined))) {
       return false;
     }
-    *info = crossContextInfo;
+
+    if (!isUndefined) {
+      CrossContextInfo * crossContextInfo =
+        GetCrossContextInfo(crossContextTarget);
+      if (crossContextInfo == nullptr) {
+        return false;
+      }
+      *info = crossContextInfo;
+    }
   }
 
   return true;
@@ -368,7 +384,7 @@ static JsValueRef CALLBACK CrossContextCallback(
     _In_opt_ void *callbackState) {
   ContextShim * currentContextShim = ContextShim::GetCurrent();
   JsValueRef fakeTargetObject = arguments[1];
-  if (trap == ProxyTraps::GetTrap) {
+  if (trap == ProxyTraps::GetOwnPropertyDescriptorTrap) {
     // Check for unwrapping
     JsValueRef prop = arguments[2];
     JsValueType type;
@@ -378,7 +394,18 @@ static JsValueRef CALLBACK CrossContextCallback(
         JsGetPropertyIdFromSymbol(prop, &propertyIdRef) == JsNoError &&
         propertyIdRef == currentContextShim->GetIsolateShim()
                                 ->GetCrossContextTargetSymbolPropertyIdRef()) {
-      return fakeTargetObject;
+      JsValueRef desc;
+      if (jsrt::CreatePropertyDescriptor(
+          jsrt::PropertyDescriptorOptionValues::None,
+          jsrt::PropertyDescriptorOptionValues::None,
+          jsrt::PropertyDescriptorOptionValues::True,  // configurable
+          fakeTargetObject,  // value
+          JS_INVALID_REFERENCE,
+          JS_INVALID_REFERENCE,
+          &desc) != JsNoError) {
+        return JS_INVALID_REFERENCE;
+      }
+      return desc;
     }
   }
 
@@ -478,6 +505,7 @@ JsValueRef MarshalObjectToContext(JsValueType valueType,
   JsValueRef crossContextObject = valueRef;
   ContextShim * fromContextShim = contextShim;
 
+  bool wasCrossContext = false;
   if (valueType == JsObject) {
     // Proxy's value type is JsObject
     // Unwrap the object if it is already cross site.
@@ -492,12 +520,8 @@ JsValueRef MarshalObjectToContext(JsValueType valueType,
       if (fromContextShim == toContextShim) {
         return crossContextObject;
       }
-    }
 
-    // Update valueType
-    if (CheckMarshalFailed(JsGetValueType(crossContextObject,
-                                          &valueType))) {
-      return JS_INVALID_REFERENCE;
+      wasCrossContext = true;
     }
   }
 
@@ -511,26 +535,39 @@ JsValueRef MarshalObjectToContext(JsValueType valueType,
   }
 
   JsValueRef valueFunctionType = JS_INVALID_REFERENCE;
-  if (valueType == JsFunction) {
-    // Special marshalling for throwAccessorErrorFunctions
-    int index;
-    if (fromContextShim->FindThrowAccessorErrorFunction(crossContextObject,
-                                                        &index)) {
-      ContextShim::Scope scope(toContextShim);
-      return toContextShim->GetThrowAccessorErrorFunction(index);
-    }
+  {
+    // Ensure re-enter correct "fromContextShim" which might have changed
+    // during above unwrapping
+    ContextShim::Scope scope(fromContextShim);
 
-    JsValueRef function = fromContextShim->GetTestFunctionTypeFunction();
-    JsValueRef args[] = {
-      fromContextShim->GetUndefined(),
-      crossContextObject };
-    if (CheckMarshalFailed(JsCallFunction(function,
-                                          args, _countof(args),
-                                          &valueFunctionType))) {
+    // Update valueType if wasCrossContext
+    if (wasCrossContext &&
+        CheckMarshalFailed(JsGetValueType(crossContextObject,
+                                          &valueType))) {
       return JS_INVALID_REFERENCE;
     }
-    valueFunctionType = MarshalJsValueRefToContext(
-      valueFunctionType, fromContextShim, toContextShim);
+
+    if (valueType == JsFunction) {
+      // Special marshalling for throwAccessorErrorFunctions
+      int index;
+      if (fromContextShim->FindThrowAccessorErrorFunction(crossContextObject,
+                                                          &index)) {
+        ContextShim::Scope scope(toContextShim);
+        return toContextShim->GetThrowAccessorErrorFunction(index);
+      }
+
+      JsValueRef function = fromContextShim->GetTestFunctionTypeFunction();
+      JsValueRef args[] = {
+        fromContextShim->GetUndefined(),
+        crossContextObject };
+      if (CheckMarshalFailed(JsCallFunction(function,
+                                            args, _countof(args),
+                                            &valueFunctionType))) {
+        return JS_INVALID_REFERENCE;
+      }
+      valueFunctionType = MarshalJsValueRefToContext(
+        valueFunctionType, fromContextShim, toContextShim);
+    }
   }
 
   // If the cross site object is a function, we need the target object to be

diff --git a/deps/chakrashim/src/jsrtisolateshim.cc b/deps/chakrashim/src/jsrtisolateshim.cc
@@ -35,6 +35,8 @@ IsolateShim::IsolateShim(JsRuntimeHandle runtime)
       selfSymbolPropertyIdRef(JS_INVALID_REFERENCE),
       crossContextTargetSymbolPropertyIdRef(JS_INVALID_REFERENCE),
       keepAliveObjectSymbolPropertyIdRef(JS_INVALID_REFERENCE),
+      proxySymbolPropertyIdRef(JS_INVALID_REFERENCE),
+      finalizerSymbolPropertyIdRef(JS_INVALID_REFERENCE),
       cachedPropertyIdRefs(),
       tryCatchStackTop(nullptr) {
   // CHAKRA-TODO: multithread locking for s_isolateList?
@@ -240,6 +242,14 @@ JsPropertyIdRef IsolateShim::GetKeepAliveObjectSymbolPropertyIdRef() {
   // return EnsurePrivateSymbol(&keepAliveObjectSymbolPropertyIdRef);
 }
 
+JsPropertyIdRef IsolateShim::GetProxySymbolPropertyIdRef() {
+  return EnsurePrivateSymbol(&proxySymbolPropertyIdRef);
+}
+
+JsPropertyIdRef IsolateShim::GetFinalizerSymbolPropertyIdRef() {
+  return EnsurePrivateSymbol(&finalizerSymbolPropertyIdRef);
+}
+
 JsPropertyIdRef IsolateShim::EnsurePrivateSymbol(
     JsPropertyIdRef * propertyIdRefPtr) {
   assert(this->GetCurrentContextShim() != nullptr);

diff --git a/deps/chakrashim/src/jsrtisolateshim.h b/deps/chakrashim/src/jsrtisolateshim.h
@@ -63,12 +63,14 @@ class IsolateShim {
 
   ContextShim * GetCurrentContextShim();
 
-  // Symbols proeprtyIdRef
+  // Symbols propertyIdRef
   JsPropertyIdRef GetSelfSymbolPropertyIdRef();
   JsPropertyIdRef GetCrossContextTargetSymbolPropertyIdRef();
   JsPropertyIdRef GetKeepAliveObjectSymbolPropertyIdRef();
+  JsPropertyIdRef GetProxySymbolPropertyIdRef();
+  JsPropertyIdRef GetFinalizerSymbolPropertyIdRef();
 
-  // String proeprtyIdRef
+  // String propertyIdRef
   JsPropertyIdRef GetProxyTrapPropertyIdRef(ProxyTraps trap);
   JsPropertyIdRef GetCachedPropertyIdRef(
     CachedPropertyIdRef cachedPropertyIdRef);
@@ -105,13 +107,13 @@ class IsolateShim {
     _In_ JsRef contextRef, _In_opt_ void *data);
 
   JsPropertyIdRef EnsurePrivateSymbol(JsPropertyIdRef * propertyIdRefPtr);
-  JsPropertyIdRef EnsurePropertyIdRef(
-    const wchar_t * name, JsPropertyIdRef * propertyIdRefPtr);
 
   JsRuntimeHandle runtime;
   JsPropertyIdRef selfSymbolPropertyIdRef;
   JsPropertyIdRef crossContextTargetSymbolPropertyIdRef;
   JsPropertyIdRef keepAliveObjectSymbolPropertyIdRef;
+  JsPropertyIdRef proxySymbolPropertyIdRef;
+  JsPropertyIdRef finalizerSymbolPropertyIdRef;
   JsPropertyIdRef cachedPropertyIdRefs[CachedPropertyIdRef::Count];
 
   ContextShim::Scope * contextScopeStack;

diff --git a/deps/chakrashim/src/v8objecttemplate.cc b/deps/chakrashim/src/v8objecttemplate.cc
@@ -571,15 +571,15 @@ JsValueRef CALLBACK GetOwnPropertyDescriptorCallback(
   JsValueRef descriptor;
 
   if (JsGetExternalData(object, &externalData) != JsNoError) {
-    return GetFalse();
+    return GetUndefined();
   }
 
   ObjectData *objectData = reinterpret_cast<ObjectData*>(externalData);
 
   bool isPropIntType;
   unsigned int index;
   if (TryParseUInt32(prop, &isPropIntType, &index) != JsNoError) {
-    return GetFalse();
+    return GetUndefined();
   }
 
   if (isPropIntType) {