From a1a1a729b0e8f2cfaaca392e5ce929d2e49812ec Mon Sep 17 00:00:00 2001
From: Kunal Pathak <Kunal.Pathak@microsoft.com>
Date: Thu, 6 Aug 2015 12:32:01 -0700
Subject: [PATCH 1/4] chakrashim: Remove SetOutOfMemoryErrorIfExist

Removed SetOutOfMemoryErrorIfExist from chakrashim now that chakra
supports returning OutOfMemory exception object.
---
 deps/chakrashim/src/jsrtutils.cc  | 12 ------------
 deps/chakrashim/src/jsrtutils.h   |  5 -----
 deps/chakrashim/src/v8function.cc | 20 +++++---------------
 deps/chakrashim/src/v8script.cc   | 10 ++--------
 4 files changed, 7 insertions(+), 40 deletions(-)

diff --git a/deps/chakrashim/src/jsrtutils.cc b/deps/chakrashim/src/jsrtutils.cc
index 3d9bc704e72..57242d3513d 100644
--- a/deps/chakrashim/src/jsrtutils.cc
+++ b/deps/chakrashim/src/jsrtutils.cc
@@ -897,16 +897,4 @@ void Fatal(const char * format, ...) {
 
   abort();
 }
-
-void SetOutOfMemoryErrorIfExist(_In_ JsErrorCode errorCode) {
-  if (errorCode == JsErrorOutOfMemory) {
-    const wchar_t txt[] = L"process out of memory";
-    JsValueRef msg, err;
-    if (JsPointerToString(txt, _countof(txt) - 1, &msg) == JsNoError &&
-      JsCreateError(msg, &err) == JsNoError) {
-      JsSetException(err);
-    }
-  }
-}
-
 }  // namespace jsrt
diff --git a/deps/chakrashim/src/jsrtutils.h b/deps/chakrashim/src/jsrtutils.h
index dc6702d7aa6..3ffa5fb04bc 100644
--- a/deps/chakrashim/src/jsrtutils.h
+++ b/deps/chakrashim/src/jsrtutils.h
@@ -319,11 +319,6 @@ class JsArguments {
   }
 };
 
-// Check if the errorCode was JsErrorOutOfMemory and if yes,
-// create and set the exception on the context
-void SetOutOfMemoryErrorIfExist(_In_ JsErrorCode errorCode);
-
-
 template <bool LIKELY,
           class JsConvertToValueFunc,
           class JsValueToNativeFunc,
diff --git a/deps/chakrashim/src/v8function.cc b/deps/chakrashim/src/v8function.cc
index e0c4fe1c399..64d9d386c40 100644
--- a/deps/chakrashim/src/v8function.cc
+++ b/deps/chakrashim/src/v8function.cc
@@ -66,18 +66,12 @@ Local<Value> Function::Call(
   JsValueRef result;
   {
     TryCatch tryCatch;
-    JsErrorCode error =
-      JsCallFunction((JsValueRef)this, args.get(), argc + 1, &result);
-
-    jsrt::SetOutOfMemoryErrorIfExist(error);
-
-    if (error == JsNoError) {
-      return Local<Value>::New(static_cast<Value*>(result));
-    }
-    if (error != JsErrorInvalidArgument) {
+    if (JsCallFunction((JsValueRef)this, args.get(), 
+                       argc + 1, &result) != JsNoError) {
       tryCatch.CheckReportExternalException();
       return Local<Value>();
     }
+    return Local<Value>::New(static_cast<Value*>(result));
   }
 
   // Invalid argument may mean some of the object are from another context
@@ -110,12 +104,8 @@ Local<Value> Function::Call(
 
   {
       TryCatch tryCatch;
-      JsErrorCode error = JsCallFunction((JsValueRef)this,
-          args.get(), argc + 1, &result);
-
-      jsrt::SetOutOfMemoryErrorIfExist(error);
-
-      if (error != JsNoError) {
+      if (JsCallFunction((JsValueRef)this,
+                         args.get(), argc + 1, &result) != JsNoError) {
           tryCatch.CheckReportExternalException();
           return Local<Value>();
       }
diff --git a/deps/chakrashim/src/v8script.cc b/deps/chakrashim/src/v8script.cc
index fb2f08e4943..55ca3874330 100644
--- a/deps/chakrashim/src/v8script.cc
+++ b/deps/chakrashim/src/v8script.cc
@@ -91,9 +91,6 @@ Local<Script> Script::Compile(Handle<String> source, Handle<String> file_name) {
       }
     }
   }
-
-  jsrt::SetOutOfMemoryErrorIfExist(error);
-
   return Local<Script>();
 }
 
@@ -104,11 +101,8 @@ Local<Value> Script::Run() {
   }
 
   JsValueRef result;
-  JsErrorCode errorCode = JsCallFunction(scriptFunction, nullptr, 0, &result);
-
-  jsrt::SetOutOfMemoryErrorIfExist(errorCode);
-
-  if (errorCode != JsNoError) {
+  if (JsCallFunction(scriptFunction, nullptr, 
+                     0, &result) != JsNoError) {
     return Local<Value>();
   }
 

From 7d41f047e07d295710134408f10f1361157f52ee Mon Sep 17 00:00:00 2001
From: Kunal Pathak <Kunal.Pathak@microsoft.com>
Date: Mon, 10 Aug 2015 10:58:45 -0700
Subject: [PATCH 2/4] chakrashim: __keepalive__ changes Changed  __keepalive__ 
 from propertyName to propertySymbol

---
 deps/chakrashim/src/jsrtcachedpropertyidref.inc | 2 +-
 deps/chakrashim/src/jsrtisolateshim.cc          | 4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/deps/chakrashim/src/jsrtcachedpropertyidref.inc b/deps/chakrashim/src/jsrtcachedpropertyidref.inc
index 1661ef66538..bc5f1b0a2de 100644
--- a/deps/chakrashim/src/jsrtcachedpropertyidref.inc
+++ b/deps/chakrashim/src/jsrtcachedpropertyidref.inc
@@ -42,7 +42,6 @@ DEF(ownKeys)
 DEF(preventExtensions)
 DEF(set)
 DEF(setPrototypeOf)
-DEF(__keepalive__)
 DEF(length)
 DEF(writable)
 DEF(configurable)
@@ -77,6 +76,7 @@ DEFSYMBOL(self)
 DEFSYMBOL(__external__)
 DEFSYMBOL(__hiddenvalues__)
 DEFSYMBOL(__isexternal__)
+DEFSYMBOL(__keepalive__)
 
 #undef DEF
 #undef DEFSYMBOL
diff --git a/deps/chakrashim/src/jsrtisolateshim.cc b/deps/chakrashim/src/jsrtisolateshim.cc
index 8a0e1157188..51ba29991ca 100644
--- a/deps/chakrashim/src/jsrtisolateshim.cc
+++ b/deps/chakrashim/src/jsrtisolateshim.cc
@@ -245,9 +245,7 @@ JsPropertyIdRef IsolateShim::GetSelfSymbolPropertyIdRef() {
 }
 
 JsPropertyIdRef IsolateShim::GetKeepAliveObjectSymbolPropertyIdRef() {
-  // CHAKRA-TODO: has a bug with symbols and proxy, just a real property name
-  return GetCachedPropertyIdRef(CachedPropertyIdRef::__keepalive__);
-  // return EnsurePrivateSymbol(&keepAliveObjectSymbolPropertyIdRef);
+  return GetCachedSymbolPropertyIdRef(CachedSymbolPropertyIdRef::__keepalive__);
 }
 
 template <class Index, class CreatePropertyIdFunc>

From 771c016af82efbc3c8579da8aa5f2a43ac92c686 Mon Sep 17 00:00:00 2001
From: Kunal Pathak <Kunal.Pathak@microsoft.com>
Date: Mon, 10 Aug 2015 17:36:05 -0700
Subject: [PATCH 3/4] chakrashim: Removed CHAKRA-TODO for errors

Reviewed CHAKRA-TODO comments for error handling
and removed/updated code to reflect the right behavior
---
 deps/chakrashim/include/v8.h           |  3 --
 deps/chakrashim/src/jsrtisolateshim.cc | 16 ++++++-----
 deps/chakrashim/src/v8function.cc      | 40 +-------------------------
 deps/chakrashim/src/v8handlescope.cc   |  6 ++--
 deps/chakrashim/src/v8isolate.cc       |  6 ----
 deps/chakrashim/src/v8trycatch.cc      | 19 ++++++------
 6 files changed, 23 insertions(+), 67 deletions(-)

diff --git a/deps/chakrashim/include/v8.h b/deps/chakrashim/include/v8.h
index 3c5299b4b4d..40816dddfa3 100644
--- a/deps/chakrashim/include/v8.h
+++ b/deps/chakrashim/include/v8.h
@@ -1448,9 +1448,6 @@ class EXPORT Isolate {
   static Isolate* New();
   static Isolate* GetCurrent();
 
-  typedef bool (*abort_on_uncaught_exception_t)();
-  void SetAbortOnUncaughtException(abort_on_uncaught_exception_t callback);
-
   void Enter();
   void Exit();
   void Dispose();
diff --git a/deps/chakrashim/src/jsrtisolateshim.cc b/deps/chakrashim/src/jsrtisolateshim.cc
index 51ba29991ca..308500f2c22 100644
--- a/deps/chakrashim/src/jsrtisolateshim.cc
+++ b/deps/chakrashim/src/jsrtisolateshim.cc
@@ -113,6 +113,8 @@ bool IsolateShim::Dispose() {
     // Set the current IsolateShim scope
     v8::Isolate::Scope scope(ToIsolate(this));
     if (JsDisposeRuntime(runtime) != JsNoError) {
+      // Can't do much at this point. Assert that this doesn't happen in debug
+      CHAKRA_ASSERT(false);
       return false;
     }
   }
@@ -185,7 +187,6 @@ void IsolateShim::DisposeAll() {
   IsolateShim * curr = s_isolateList;
   s_isolateList = nullptr;
   while (curr) {
-    // CHAKRA-TODO: Handle error?
     curr->Dispose();
     curr = curr->next;
   }
@@ -202,10 +203,10 @@ void IsolateShim::PushScope(
   scope->previous = this->contextScopeStack;
   this->contextScopeStack = scope;
 
-  // CHAKRA-TODO: Error handling?
-  JsSetCurrentContext(contextShim->GetContextRef());
+  // Don't crash even if we fail to set the context
+  JsErrorCode errorCode = JsSetCurrentContext(contextShim->GetContextRef());
+  CHAKRA_ASSERT(errorCode == JsNoError);
 
-  // CHAKRA-TODO: Error handling?
   if (!contextShim->EnsureInitialized()) {
     Fatal("Failed to initialize context");
   }
@@ -215,7 +216,6 @@ void IsolateShim::PopScope(ContextShim::Scope * scope) {
   assert(this->contextScopeStack == scope);
   ContextShim::Scope * prevScope = scope->previous;
   if (prevScope != nullptr) {
-    // Marshal the pending exception
     JsValueRef exception = JS_INVALID_REFERENCE;
     bool hasException;
     if (scope->contextShim != prevScope->contextShim &&
@@ -224,9 +224,11 @@ void IsolateShim::PopScope(ContextShim::Scope * scope) {
         JsGetAndClearException(&exception) == JsNoError) {
     }
 
-    JsSetCurrentContext(prevScope->contextShim->GetContextRef());
+    // Don't crash even if we fail to set the context
+    JsErrorCode errorCode = JsSetCurrentContext(prevScope->contextShim->GetContextRef());
+    CHAKRA_ASSERT(errorCode == JsNoError);
 
-    // CHAKRA-TODO: Error handling?
+    // Propagate the exception to parent scope
     if (exception != JS_INVALID_REFERENCE) {
       JsSetException(exception);
     }
diff --git a/deps/chakrashim/src/v8function.cc b/deps/chakrashim/src/v8function.cc
index 64d9d386c40..d707eaa2af7 100644
--- a/deps/chakrashim/src/v8function.cc
+++ b/deps/chakrashim/src/v8function.cc
@@ -73,44 +73,6 @@ Local<Value> Function::Call(
     }
     return Local<Value>::New(static_cast<Value*>(result));
   }
-
-  // Invalid argument may mean some of the object are from another context
-  // Check and marshal and call again.
-
-  // NOTE: Ideally, we will never run into this situation where we will
-  // also marshal correctly and use object from the same context.
-  // But CopyProperty in node_contextify.cc violate that so, we have this
-  // to paper over it.
-  IsolateShim * isolateShim = IsolateShim::GetCurrent();
-  ContextShim * currentContextShim = isolateShim->GetCurrentContextShim();
-  if (currentContextShim == nullptr) {
-    return Local<Value>();
-  }
-
-  for (int i = 0; i < argc + 1; i++) {
-    JsValueRef valueRef = args.get()[i];
-    ContextShim * objectContextShim = isolateShim->GetContextShimOfObject(valueRef);
-    if (currentContextShim == objectContextShim) {
-      continue;
-    }
-    if (objectContextShim != nullptr) {
-      ContextShim::Scope scope(objectContextShim);
-      args.get()[i] = valueRef;
-    } else {
-      // Can't find a context
-      return Local<Value>();
-    }
-  }
-
-  {
-      TryCatch tryCatch;
-      if (JsCallFunction((JsValueRef)this,
-                         args.get(), argc + 1, &result) != JsNoError) {
-          tryCatch.CheckReportExternalException();
-          return Local<Value>();
-      }
-      return Local<Value>::New(static_cast<Value*>(result));
-  }
 }
 
 void Function::SetName(Handle<String> name) {
@@ -121,7 +83,7 @@ void Function::SetName(Handle<String> name) {
                                      (JsValueRef)*name,
                                      JS_INVALID_REFERENCE,
                                      JS_INVALID_REFERENCE);
-  // CHAKRA-TODO: Check error?
+  CHAKRA_ASSERT(error == JsNoError);
 }
 
 Function *Function::Cast(Value *obj) {
diff --git a/deps/chakrashim/src/v8handlescope.cc b/deps/chakrashim/src/v8handlescope.cc
index 78aa65fdd99..093364bbf0a 100644
--- a/deps/chakrashim/src/v8handlescope.cc
+++ b/deps/chakrashim/src/v8handlescope.cc
@@ -20,6 +20,7 @@
 
 #include "v8.h"
 #include "jsrt.h"
+#include "jsrtutils.h"
 
 namespace v8 {
 
@@ -41,8 +42,9 @@ HandleScope::~HandleScope() {
   AddRefRecord * currRecord = this->_addRefRecordHead;
   while (currRecord != nullptr) {
     AddRefRecord * nextRecord = currRecord->_next;
-    // CHAKRA-TODO: Report error?
-    JsRelease(currRecord->_ref, nullptr);
+    // Don't crash even if we fail to release the scope
+    JsErrorCode errorCode = JsRelease(currRecord->_ref, nullptr);
+    CHAKRA_ASSERT(errorCode == JsNoError);
     delete currRecord;
     currRecord = nextRecord;
   }
diff --git a/deps/chakrashim/src/v8isolate.cc b/deps/chakrashim/src/v8isolate.cc
index 8ba48091b8b..19f54c25d9c 100644
--- a/deps/chakrashim/src/v8isolate.cc
+++ b/deps/chakrashim/src/v8isolate.cc
@@ -35,11 +35,6 @@ Isolate *Isolate::GetCurrent() {
   return jsrt::IsolateShim::GetCurrentAsIsolate();
 }
 
-void Isolate::SetAbortOnUncaughtException(
-    abort_on_uncaught_exception_t callback) {
-  // CHAKRA-TODO
-}
-
 void Isolate::Enter() {
   return jsrt::IsolateShim::FromIsolate(this)->Enter();
 }
@@ -50,7 +45,6 @@ void Isolate::Exit() {
 }
 
 void Isolate::Dispose() {
-  // CHAKRA-TODO: Handle Error?
   jsrt::IsolateShim::FromIsolate(this)->Dispose();
 }
 
diff --git a/deps/chakrashim/src/v8trycatch.cc b/deps/chakrashim/src/v8trycatch.cc
index e9b6d1bd5df..8471a123061 100644
--- a/deps/chakrashim/src/v8trycatch.cc
+++ b/deps/chakrashim/src/v8trycatch.cc
@@ -50,15 +50,14 @@ bool TryCatch::HasCaught() const {
   if (error != JS_INVALID_REFERENCE) {
     return true;
   }
-
   bool hasException;
   JsErrorCode errorCode = JsHasException(&hasException);
   if (errorCode != JsNoError) {
     if (errorCode == JsErrorInDisabledState) {
       return true;
     }
-    // CHAKRA-TODO: report error
-    assert(false);
+    // Should never get errorCode other than JsNoError/JsErrorInDisabledState
+    CHAKRA_ASSERT(false);
     return false;
   }
 
@@ -73,20 +72,20 @@ void TryCatch::GetAndClearException() {
   bool hasException;
   JsErrorCode errorCode = JsHasException(&hasException);
   if (errorCode != JsNoError) {
-    // CHAKRA-TODO: report error
-    assert(errorCode == JsErrorInDisabledState);
+    // If script is in disabled state, no need to fail here.
+    // We will fail subsequent calls to Jsrt anyway.
+    CHAKRA_ASSERT(errorCode == JsErrorInDisabledState);
     return;
   }
 
   if (hasException) {
     JsValueRef exceptionRef;
     errorCode = JsGetAndClearException(&exceptionRef);
-    if (errorCode != JsNoError) {
-      // CHAKRA-TODO: report error
-      assert(errorCode == JsErrorInDisabledState);
-      return;
+    // We came here through JsHasException, so script shouldn't be in disabled state.
+    CHAKRA_ASSERT(errorCode != JsErrorInDisabledState);
+    if (errorCode == JsNoError) {
+      error = exceptionRef;
     }
-    error = exceptionRef;
   }
 }
 

From 028fa17cd95e6d94acf967515efb44f01390e955 Mon Sep 17 00:00:00 2001
From: Kunal Pathak <Kunal.Pathak@microsoft.com>
Date: Tue, 11 Aug 2015 10:30:30 -0700
Subject: [PATCH 4/4] chakrashim: cpplint fixes

---
 deps/chakrashim/src/jsrtisolateshim.cc | 12 ++++++------
 deps/chakrashim/src/v8function.cc      |  2 +-
 deps/chakrashim/src/v8trycatch.cc      |  3 ++-
 3 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/deps/chakrashim/src/jsrtisolateshim.cc b/deps/chakrashim/src/jsrtisolateshim.cc
index 308500f2c22..df8633c6581 100644
--- a/deps/chakrashim/src/jsrtisolateshim.cc
+++ b/deps/chakrashim/src/jsrtisolateshim.cc
@@ -137,8 +137,8 @@ bool IsolateShim::IsDisposing() {
   return isDisposing;
 }
 
-//TODO: Chakra: This is not called after cross context work in chakra. Fix this else we will 
-// leak chakrashim object.
+// CHAKRA-TODO: This is not called after cross context work in chakra. Fix this
+// else we will leak chakrashim object.
 void CALLBACK IsolateShim::JsContextBeforeCollectCallback(
     _In_ JsRef contextRef, _In_opt_ void *data) {
   IsolateShim * isolateShim = reinterpret_cast<IsolateShim *>(data);
@@ -225,7 +225,8 @@ void IsolateShim::PopScope(ContextShim::Scope * scope) {
     }
 
     // Don't crash even if we fail to set the context
-    JsErrorCode errorCode = JsSetCurrentContext(prevScope->contextShim->GetContextRef());
+    JsErrorCode errorCode = JsSetCurrentContext(
+      prevScope->contextShim->GetContextRef());
     CHAKRA_ASSERT(errorCode == JsNoError);
 
     // Propagate the exception to parent scope
@@ -296,8 +297,7 @@ JsPropertyIdRef IsolateShim::GetProxyTrapPropertyIdRef(ProxyTraps trap) {
 
 ContextShim * IsolateShim::GetContextShimOfObject(JsValueRef valueRef) {
   JsContextRef contextRef;
-  if (JsGetContextOfObject(valueRef, &contextRef) != JsNoError) 
-  {
+  if (JsGetContextOfObject(valueRef, &contextRef) != JsNoError) {
     return nullptr;
   }
   assert(contextRef != nullptr);
@@ -327,7 +327,7 @@ bool IsolateShim::AddMessageListener(void * that) {
   try {
     messageListeners.push_back(that);
     return true;
-  } catch (...) {
+  } catch(...) {
     return false;
   }
 }
diff --git a/deps/chakrashim/src/v8function.cc b/deps/chakrashim/src/v8function.cc
index d707eaa2af7..a035ec288f2 100644
--- a/deps/chakrashim/src/v8function.cc
+++ b/deps/chakrashim/src/v8function.cc
@@ -66,7 +66,7 @@ Local<Value> Function::Call(
   JsValueRef result;
   {
     TryCatch tryCatch;
-    if (JsCallFunction((JsValueRef)this, args.get(), 
+    if (JsCallFunction((JsValueRef)this, args.get(),
                        argc + 1, &result) != JsNoError) {
       tryCatch.CheckReportExternalException();
       return Local<Value>();
diff --git a/deps/chakrashim/src/v8trycatch.cc b/deps/chakrashim/src/v8trycatch.cc
index 8471a123061..0bcbf14570f 100644
--- a/deps/chakrashim/src/v8trycatch.cc
+++ b/deps/chakrashim/src/v8trycatch.cc
@@ -81,7 +81,8 @@ void TryCatch::GetAndClearException() {
   if (hasException) {
     JsValueRef exceptionRef;
     errorCode = JsGetAndClearException(&exceptionRef);
-    // We came here through JsHasException, so script shouldn't be in disabled state.
+    // We came here through JsHasException, so script shouldn't be in disabled 
+    // state.
     CHAKRA_ASSERT(errorCode != JsErrorInDisabledState);
     if (errorCode == JsNoError) {
       error = exceptionRef;