chakrashim: fix inspector evaluate

* Fixed the way that Runtime::evaluate handled and returned exceptions
* Fixed Runtime::evaluate to use the global stack frame
* Added support for console command line during evaluate calls
* Fixed various incorrect function calls in chakrashim

PR-URL: #442
Reviewed-By: Jimmy Thomson <jithomso@microsoft.com>

deps/chakrashim/include/v8.h

@@ -351,6 +351,7 @@ class Local {
   friend class PropertyDescriptor;
   friend class Proxy;
   friend class RegExp;
+  friend class Set;
   friend class Signature;
   friend class Script;
   friend class StackFrame;

deps/chakrashim/src/inspector/v8-console.cc

@@ -839,11 +839,13 @@ V8Console::CommandLineAPIScope::~CommandLineAPIScope() {
     v8::Local<v8::Value> name;
     if (!names->Get(m_context, i).ToLocal(&name) || !name->IsName()) continue;
     if (name->IsString()) {
-      v8::Local<v8::Value> descriptor;
-      bool success = m_global
-                         ->GetOwnPropertyDescriptor(
-                             m_context, v8::Local<v8::Name>::Cast(name))
-                         .ToLocal(&descriptor);
+      // Trigger the accessorGetterCallback to do the delete. This ensures that
+      // the correct property is being deleted. It's possible (though unlikely)
+      // that some other code (or the user) has replaced these properties before
+      // they were able to be cleaned up.
+      v8::Local<v8::Value> value;
+      bool success = m_global->Get(m_context, v8::Local<v8::Name>::Cast(name))
+          .ToLocal(&value);
       DCHECK(success);
       USE(success);
     }

deps/chakrashim/src/inspector/v8-debugger-agent-impl.cc

@@ -6,11 +6,13 @@
 
 #include <algorithm>
 
+#include "src/inspector/inspected-context.h"
 #include "src/inspector/java-script-call-frame.h"
 #include "src/inspector/protocol/Protocol.h"
 #include "src/inspector/script-breakpoint.h"
 #include "src/inspector/search-util.h"
 #include "src/inspector/string-util.h"
+#include "src/inspector/v8-console.h"
 #include "src/inspector/v8-debugger-script.h"
 #include "src/inspector/v8-debugger.h"
 #include "src/inspector/v8-inspector-impl.h"
@@ -713,6 +715,20 @@ void V8DebuggerAgentImpl::evaluateOnCallFrame(
     return;
   }
 
+  std::unique_ptr<V8Console::CommandLineAPIScope> claScope;
+
+  if (includeCommandLineAPI.fromMaybe(false)) {
+    InspectedContext* inspectedContext =
+        m_inspector->getContext(
+            m_session->contextGroupId(),
+            V8Debugger::contextId(
+                v8::Local<v8::Context>::New(m_isolate, m_pausedContext)));
+    claScope.reset(new V8Console::CommandLineAPIScope(
+        inspectedContext->context(),
+        V8Console::createCommandLineAPI(inspectedContext),
+        inspectedContext->context()->Global()));
+  }
+
   bool isError = false;
   v8::MaybeLocal<v8::Value> maybeResultValue =
       m_pausedCallFrames[ordinal]->evaluate(

deps/chakrashim/src/inspector/v8-debugger.cc

@@ -278,7 +278,7 @@ JavaScriptCallFrames V8Debugger::currentCallFrames(int limit) {
   }
 
   JsValueRef stackTrace = JS_INVALID_REFERENCE;
-  JsDiagGetStackTrace(&stackTrace);
+  CHAKRA_VERIFY_NOERROR(JsDiagGetStackTrace(&stackTrace));
 
   unsigned int length = 0;
   CHAKRA_VERIFY_NOERROR(jsrt::GetArrayLength(stackTrace, &length));

deps/chakrashim/src/inspector/v8-runtime-agent-impl.cc

@@ -33,6 +33,7 @@
 #include "src/inspector/inspected-context.h"
 #include "src/inspector/protocol/Protocol.h"
 #include "src/inspector/string-util.h"
+#include "src/inspector/v8-console.h"
 #include "src/inspector/v8-console-message.h"
 #include "src/inspector/v8-debugger-agent-impl.h"
 #include "src/inspector/v8-debugger.h"
@@ -69,6 +70,27 @@ std::unique_ptr<protocol::DictionaryValue> ParseObjectId(
   return wrapUnique(protocol::DictionaryValue::cast(parsedValue.release()));
 }
 
+bool ensureContext(ErrorString* errorString, V8InspectorImpl* inspector,
+                   int contextGroupId, Maybe<int> executionContextId,
+                   int* contextId) {
+  if (executionContextId.isJust()) {
+    *contextId = executionContextId.fromJust();
+  } else {
+    v8::HandleScope handles(inspector->isolate());
+    v8::Local<v8::Context> defaultContext =
+        inspector->client()->ensureDefaultContextInGroup(contextGroupId);
+
+    if (defaultContext.IsEmpty()) {
+      *errorString = "Cannot find default execution context";
+      return false;
+    }
+
+    *contextId = V8Debugger::contextId(defaultContext);
+  }
+
+  return true;
+}
+
 }  // namespace
 
 V8RuntimeAgentImpl::V8RuntimeAgentImpl(
@@ -91,6 +113,24 @@ void V8RuntimeAgentImpl::evaluate(
     std::unique_ptr<EvaluateCallback> callback) {
   ErrorString errorString;
 
+  int contextId = 0;
+  if (!ensureContext(&errorString, m_inspector, m_session->contextGroupId(),
+                     std::move(executionContextId), &contextId)) {
+    callback->sendFailure(errorString);
+    return;
+  }
+
+  std::unique_ptr<V8Console::CommandLineAPIScope> claScope;
+
+  if (includeCommandLineAPI.fromMaybe(false)) {
+    InspectedContext* inspectedContext =
+        m_inspector->getContext(m_session->contextGroupId(), contextId);
+    claScope.reset(new V8Console::CommandLineAPIScope(
+        inspectedContext->context(),
+        V8Console::createCommandLineAPI(inspectedContext),
+        inspectedContext->context()->Global()));
+  }
+
   JsValueRef expStr;
   if (JsCreateStringUtf16(expression.characters16(), expression.length(),
                           &expStr) != JsNoError) {
@@ -99,9 +139,10 @@ void V8RuntimeAgentImpl::evaluate(
     return;
   }
 
+  bool isError = false;
   v8::Local<v8::Value> evalResult =
-      jsrt::InspectorHelpers::EvaluateOnCallFrame(
-          /* ordinal */ 0, expStr, returnByValue.fromMaybe(false));
+      jsrt::InspectorHelpers::EvaluateOnGlobalCallFrame(
+          expStr, returnByValue.fromMaybe(false), &isError);
 
   if (evalResult.IsEmpty()) {
     errorString = "Failed to evaluate expression";
@@ -114,7 +155,7 @@ void V8RuntimeAgentImpl::evaluate(
   std::unique_ptr<protocol::Value> protocolValue =
       toProtocolValue(&errorString, v8::Context::GetCurrent(), evalResult);
   if (!protocolValue) {
-    callback->sendSuccess(nullptr, exceptionDetails);
+    callback->sendFailure(errorString);
     return;
   }
   std::unique_ptr<protocol::Runtime::RemoteObject> remoteObject =
@@ -125,6 +166,29 @@ void V8RuntimeAgentImpl::evaluate(
     return;
   }
 
+  if (isError) {
+    protocol::ErrorSupport errors;
+    std::unique_ptr<protocol::Runtime::RemoteObject> exceptionObject =
+        protocol::Runtime::RemoteObject::parse(protocolValue.get(), &errors);
+    if (!exceptionObject) {
+      errorString = errors.errors();
+      callback->sendFailure(errorString);
+      return;
+    }
+
+    std::unique_ptr<protocol::Runtime::ExceptionDetails> exDetails =
+        protocol::Runtime::ExceptionDetails::create()
+          .setExceptionId(m_session->inspector()->nextExceptionId())
+          .setText(exceptionObject->getDescription("Uncaught"))
+          .setLineNumber(0)
+          .setColumnNumber(0)
+          .build();
+
+    exDetails->setException(std::move(exceptionObject));
+
+    exceptionDetails = std::move(exDetails);
+  }
+
   callback->sendSuccess(std::move(remoteObject), exceptionDetails);
 }
 

deps/chakrashim/src/jsrtcachedpropertyidref.inc

@@ -125,6 +125,9 @@ DEF(url)
 DEF(toStringTag)
 DEF(Symbol_toStringTag)
 
+DEF(add)
+DEF(from)
+
 DEFSYMBOL(self)
 DEFSYMBOL(__external__)
 DEFSYMBOL(__hiddenvalues__)

deps/chakrashim/src/jsrtcontextcachedobj.inc

@@ -46,16 +46,19 @@ DEFTYPE(Float64Array)
 DEFTYPE(RegExp)
 DEFTYPE(Map)
 DEFTYPE(Symbol)
+DEFTYPE(Set)
 
 
 // These prototype functions will be cached/shimmed
 DEFMETHOD(Object,         toString)
 DEFMETHOD(Object,         valueOf)
 DEFMETHOD(String,         concat)
+DEFMETHOD(Array,          from)
 DEFMETHOD(Array,          push)
 DEFMETHOD(Map,            get)
 DEFMETHOD(Map,            set)
 DEFMETHOD(Map,            has)
+DEFMETHOD(Set,            add)
 
 
 #undef DEFTYPE

deps/chakrashim/src/jsrtcontextshim.cc

@@ -532,6 +532,10 @@ DECLARE_GETOBJECT(ProxyConstructor,
                   globalConstructor[GlobalType::Proxy])
 DECLARE_GETOBJECT(MapConstructor,
                   globalConstructor[GlobalType::Map])
+DECLARE_GETOBJECT(SetConstructor,
+                  globalConstructor[GlobalType::Set])
+DECLARE_GETOBJECT(ArrayConstructor,
+                  globalConstructor[GlobalType::Array])
 DECLARE_GETOBJECT(ToStringFunction,
                   globalPrototypeFunction[GlobalPrototypeFunction
                     ::Object_toString])
@@ -541,6 +545,9 @@ DECLARE_GETOBJECT(ValueOfFunction,
 DECLARE_GETOBJECT(StringConcatFunction,
                   globalPrototypeFunction[GlobalPrototypeFunction
                     ::String_concat])
+DECLARE_GETOBJECT(ArrayFromFunction,
+                  globalPrototypeFunction[GlobalPrototypeFunction
+                    ::Array_from])
 DECLARE_GETOBJECT(ArrayPushFunction,
                   globalPrototypeFunction[GlobalPrototypeFunction
                     ::Array_push])
@@ -553,6 +560,9 @@ DECLARE_GETOBJECT(MapSetFunction,
 DECLARE_GETOBJECT(MapHasFunction,
                   globalPrototypeFunction[GlobalPrototypeFunction
                     ::Map_has])
+DECLARE_GETOBJECT(SetAddFunction,
+                  globalPrototypeFunction[GlobalPrototypeFunction
+                    ::Set_add])
 
 
 JsValueRef ContextShim::GetProxyOfGlobal() {

deps/chakrashim/src/jsrtcontextshim.h

@@ -79,17 +79,21 @@ class ContextShim {
   JsValueRef GetRegExpConstructor();
   JsValueRef GetProxyConstructor();
   JsValueRef GetMapConstructor();
+  JsValueRef GetSetConstructor();
+  JsValueRef GetArrayConstructor();
   JsValueRef GetGlobalType(GlobalType index);
 
   JsValueRef GetToStringFunction();
   JsValueRef GetValueOfFunction();
   JsValueRef GetStringConcatFunction();
+  JsValueRef GetArrayFromFunction();
   JsValueRef GetArrayPushFunction();
   JsValueRef GetGlobalPrototypeFunction(GlobalPrototypeFunction index);
   JsValueRef GetProxyOfGlobal();
   JsValueRef GetMapGetFunction();
   JsValueRef GetMapSetFunction();
   JsValueRef GetMapHasFunction();
+  JsValueRef GetSetAddFunction();
 
   void * GetAlignedPointerFromEmbedderData(int index);
   void SetAlignedPointerInEmbedderData(int index, void * value);

deps/chakrashim/src/jsrtinspectorhelpers.cc

@@ -630,6 +630,22 @@ namespace jsrt {
     return EvaluateOnCallFrame(ordinal, expression, returnByValue, isError);
   }
 
+  v8::Local<v8::Value> InspectorHelpers::EvaluateOnGlobalCallFrame(
+      JsValueRef expression, bool returnByValue, bool* isError) {
+    JsValueRef stackTrace = JS_INVALID_REFERENCE;
+    JsErrorCode err = JsDiagGetStackTrace(&stackTrace);
+    if (err == JsErrorDiagNotAtBreak) {
+      return v8::Local<v8::Value>();
+    }
+
+    CHAKRA_VERIFY_NOERROR(err);
+
+    unsigned int length = 0;
+    CHAKRA_VERIFY_NOERROR(jsrt::GetArrayLength(stackTrace, &length));
+
+    return EvaluateOnCallFrame(length - 1, expression, returnByValue, isError);
+  }
+
   v8::Local<v8::Value> InspectorHelpers::GetScriptSource(
       unsigned int scriptId) {
     JsValueRef scriptSource = JS_INVALID_REFERENCE;

deps/chakrashim/src/jsrtinspectorhelpers.h

@@ -53,6 +53,8 @@ class InspectorHelpers {
                                                   JsValueRef expression,
                                                   bool returnByValue,
                                                   bool* isError = nullptr);
+  static v8::Local<v8::Value> EvaluateOnGlobalCallFrame(
+      JsValueRef expression, bool returnByValue, bool* isError = nullptr);
 
   static v8::Local<v8::Value> GetScriptSource(unsigned int scriptId);
 

deps/chakrashim/src/v8map.cc

@@ -28,6 +28,7 @@ Local<Map> Map::New(Isolate* isolate) {
 
   JsValueRef newMapRef;
   if (jsrt::ConstructObject(mapConstructor, &newMapRef) != JsNoError) {
+    CHAKRA_ASSERT(false);
     return Local<Map>();
   }
 
@@ -39,8 +40,10 @@ MaybeLocal<Value> Map::Get(Local<Context> context, Local<Value> key) {
     ContextShim::GetCurrent()->GetMapGetFunction();
 
   JsValueRef mapGetResult;
-  if (jsrt::CallFunction(mapGetFunction, (JsValueRef)this, *key,
-                         &mapGetResult) != JsNoError) {
+  JsValueRef args[] = { (JsValueRef)this, *key };
+  if (JsCallFunction(mapGetFunction, args, _countof(args),
+                     &mapGetResult) != JsNoError) {
+    CHAKRA_ASSERT(false);
     return MaybeLocal<Value>();
   }
 
@@ -53,8 +56,10 @@ MaybeLocal<Map> Map::Set(Local<Context> context, Local<Value> key,
     ContextShim::GetCurrent()->GetMapSetFunction();
 
   JsValueRef mapSetResult;
-  if (jsrt::CallFunction(mapSetFunction, (JsValueRef)this, *key, *value,
-                         &mapSetResult) != JsNoError) {
+  JsValueRef args[] = { (JsValueRef)this, *key, *value };
+  if (JsCallFunction(mapSetFunction, args, _countof(args),
+                     &mapSetResult) != JsNoError) {
+    CHAKRA_ASSERT(false);
     return MaybeLocal<Map>();
   }
 
@@ -66,8 +71,10 @@ Maybe<bool> Map::Has(Local<Context> context, Local<Value> key) {
     ContextShim::GetCurrent()->GetMapHasFunction();
 
   JsValueRef mapHasResult;
-  if (jsrt::CallFunction(mapHasFunction, (JsValueRef)this, *key,
-                         &mapHasResult) != JsNoError) {
+  JsValueRef args[] = { (JsValueRef)this, *key };
+  if (JsCallFunction(mapHasFunction, args, _countof(args),
+                     &mapHasResult) != JsNoError) {
+    CHAKRA_ASSERT(false);
     return Nothing<bool>();
   }
 
@@ -76,6 +83,7 @@ Maybe<bool> Map::Has(Local<Context> context, Local<Value> key) {
                                           JsBooleanToBool,
                                           mapHasResult,
                                           &hasResult) != JsNoError) {
+    CHAKRA_ASSERT(false);
     return Nothing<bool>();
   }