[Extensions Bindings] Add JS hooks for the exception handler

There are two JS hooks we used in handling exceptions in extensions
bindings:
- There is an exposed "handleException" method, which handles exceptions
  encountered in running untrusted code.
- The test API allows us to set custom handlers for uncaught exceptions
  (so we can fail a test or verify expected errors).

Implement each of these in the utilities exposed to the JS bindings, and
add unittests for the same.

Bug: 653596
Change-Id: I246308c78b0760f5176949c2b1d88da07dca337a
Reviewed-on: https://chromium-review.googlesource.com/568692Reviewed-by: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#486667}

extensions/renderer/bindings/api_binding_js_util.cc

@@ -4,12 +4,14 @@
 
 #include "extensions/renderer/bindings/api_binding_js_util.h"
 
+#include "base/strings/stringprintf.h"
 #include "base/values.h"
 #include "extensions/renderer/bindings/api_event_handler.h"
 #include "extensions/renderer/bindings/api_request_handler.h"
 #include "extensions/renderer/bindings/api_signature.h"
 #include "extensions/renderer/bindings/api_type_reference_map.h"
 #include "extensions/renderer/bindings/declarative_event.h"
+#include "extensions/renderer/bindings/exception_handler.h"
 #include "gin/converter.h"
 #include "gin/dictionary.h"
 #include "gin/handle.h"
@@ -22,10 +24,12 @@ gin::WrapperInfo APIBindingJSUtil::kWrapperInfo = {gin::kEmbedderNativeGin};
 APIBindingJSUtil::APIBindingJSUtil(APITypeReferenceMap* type_refs,
                                    APIRequestHandler* request_handler,
                                    APIEventHandler* event_handler,
+                                   ExceptionHandler* exception_handler,
                                    const binding::RunJSFunction& run_js)
     : type_refs_(type_refs),
       request_handler_(request_handler),
       event_handler_(event_handler),
+      exception_handler_(exception_handler),
       run_js_(run_js) {}
 
 APIBindingJSUtil::~APIBindingJSUtil() {}
@@ -44,7 +48,9 @@ gin::ObjectTemplateBuilder APIBindingJSUtil::GetObjectTemplateBuilder(
       .SetMethod("clearLastError", &APIBindingJSUtil::ClearLastError)
       .SetMethod("hasLastError", &APIBindingJSUtil::HasLastError)
       .SetMethod("runCallbackWithLastError",
-                 &APIBindingJSUtil::RunCallbackWithLastError);
+                 &APIBindingJSUtil::RunCallbackWithLastError)
+      .SetMethod("handleException", &APIBindingJSUtil::HandleException)
+      .SetMethod("setExceptionHandler", &APIBindingJSUtil::SetExceptionHandler);
 }
 
 void APIBindingJSUtil::SendRequest(
@@ -206,4 +212,35 @@ void APIBindingJSUtil::RunCallbackWithLastError(
   request_handler_->last_error()->ClearError(context, report_if_unchecked);
 }
 
+void APIBindingJSUtil::HandleException(gin::Arguments* arguments,
+                                       const std::string& message,
+                                       v8::Local<v8::Value> exception) {
+  v8::Isolate* isolate = arguments->isolate();
+  v8::HandleScope handle_scope(isolate);
+  v8::Local<v8::Context> context = arguments->GetHolderCreationContext();
+
+  std::string full_message;
+  if (!exception->IsUndefined() && !exception->IsNull()) {
+    v8::TryCatch try_catch(isolate);
+    std::string exception_string;
+    v8::Local<v8::String> v8_exception_string;
+    if (exception->ToString(context).ToLocal(&v8_exception_string))
+      exception_string = gin::V8ToString(v8_exception_string);
+    else
+      exception_string = "(failed to get error message)";
+    full_message =
+        base::StringPrintf("%s: %s", message.c_str(), exception_string.c_str());
+  } else {
+    full_message = message;
+  }
+
+  exception_handler_->HandleException(context, full_message, exception);
+}
+
+void APIBindingJSUtil::SetExceptionHandler(gin::Arguments* arguments,
+                                           v8::Local<v8::Function> handler) {
+  exception_handler_->SetHandlerForContext(
+      arguments->GetHolderCreationContext(), handler);
+}
+
 }  // namespace extensions

extensions/renderer/bindings/api_binding_js_util.h

@@ -20,6 +20,7 @@ namespace extensions {
 class APIEventHandler;
 class APIRequestHandler;
 class APITypeReferenceMap;
+class ExceptionHandler;
 
 // An object that exposes utility methods to the existing JS bindings, such as
 // sendRequest and registering event argument massagers. If/when we get rid of
@@ -29,6 +30,7 @@ class APIBindingJSUtil final : public gin::Wrappable<APIBindingJSUtil> {
   APIBindingJSUtil(APITypeReferenceMap* type_refs,
                    APIRequestHandler* request_handler,
                    APIEventHandler* event_handler,
+                   ExceptionHandler* exception_handler,
                    const binding::RunJSFunction& run_js);
   ~APIBindingJSUtil() override;
 
@@ -89,14 +91,27 @@ class APIBindingJSUtil final : public gin::Wrappable<APIBindingJSUtil> {
                                 const std::string& error,
                                 v8::Local<v8::Function> callback);
 
+  // Handles an exception with the given |message| and |exception| value.
+  void HandleException(gin::Arguments* arguments,
+                       const std::string& message,
+                       v8::Local<v8::Value> exception);
+
+  // Sets a custom exception handler to be used when an uncaught exception is
+  // found.
+  void SetExceptionHandler(gin::Arguments* arguments,
+                           v8::Local<v8::Function> handler);
+
   // Type references. Guaranteed to outlive this object.
-  APITypeReferenceMap* type_refs_;
+  APITypeReferenceMap* const type_refs_;
 
   // The request handler. Guaranteed to outlive this object.
-  APIRequestHandler* request_handler_;
+  APIRequestHandler* const request_handler_;
 
   // The event handler. Guaranteed to outlive this object.
-  APIEventHandler* event_handler_;
+  APIEventHandler* const event_handler_;
+
+  // The exception handler. Guaranteed to outlive this object.
+  ExceptionHandler* const exception_handler_;
 
   binding::RunJSFunction run_js_;
 

extensions/renderer/bindings/api_binding_js_util_unittest.cc

@@ -8,10 +8,24 @@
 #include "extensions/renderer/bindings/api_binding_test_util.h"
 #include "extensions/renderer/bindings/api_bindings_system.h"
 #include "extensions/renderer/bindings/api_bindings_system_unittest.h"
+#include "gin/arguments.h"
 #include "gin/handle.h"
+#include "testing/gmock/include/gmock/gmock.h"
 
 namespace extensions {
 
+namespace {
+
+// Calls handleException on |obj|, which is presumed to be the JS binding util.
+const char kHandleException[] =
+    "try {\n"
+    "  throw new Error('some error');\n"
+    "} catch (e) {\n"
+    "  obj.handleException('handled', e);\n"
+    "}";
+
+}  // namespace
+
 class APIBindingJSUtilUnittest : public APIBindingsSystemTest {
  protected:
   APIBindingJSUtilUnittest() {}
@@ -23,6 +37,7 @@ class APIBindingJSUtilUnittest : public APIBindingsSystemTest {
         new APIBindingJSUtil(bindings_system()->type_reference_map(),
                              bindings_system()->request_handler(),
                              bindings_system()->event_handler(),
+                             bindings_system()->exception_handler(),
                              base::Bind(&RunFunctionOnGlobalAndIgnoreResult)));
   }
 
@@ -32,6 +47,11 @@ class APIBindingJSUtilUnittest : public APIBindingsSystemTest {
     return context->Global();
   }
 
+  void AddConsoleError(v8::Local<v8::Context> context,
+                       const std::string& error) override {
+    console_errors_.push_back(error);
+  }
+
   std::string GetExposedError(v8::Local<v8::Context> context) {
     v8::Local<v8::Value> last_error =
         GetPropertyFromObject(context->Global(), context, "lastError");
@@ -56,7 +76,13 @@ class APIBindingJSUtilUnittest : public APIBindingsSystemTest {
     return bindings_system()->request_handler()->last_error();
   }
 
+  const std::vector<std::string>& console_errors() const {
+    return console_errors_;
+  }
+
  private:
+  std::vector<std::string> console_errors_;
+
   DISALLOW_COPY_AND_ASSIGN(APIBindingJSUtilUnittest);
 };
 
@@ -191,4 +217,75 @@ TEST_F(APIBindingJSUtilUnittest, TestSendRequestWithOptions) {
                                                 "callbackCalled"));
 }
 
+TEST_F(APIBindingJSUtilUnittest, TestCallHandleException) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  gin::Handle<APIBindingJSUtil> util = CreateUtil();
+  v8::Local<v8::Object> v8_util = util.ToV8().As<v8::Object>();
+
+  ASSERT_TRUE(console_errors().empty());
+  CallFunctionOnObject(context, v8_util, kHandleException);
+  EXPECT_THAT(console_errors(),
+              testing::ElementsAre("handled: Error: some error"));
+
+  const char kHandleTrickyException[] =
+      "try {\n"
+      "  throw { toString: function() { throw new Error('hahaha'); } };\n"
+      "} catch (e) {\n"
+      "  obj.handleException('handled again', e);\n"
+      "}\n";
+  CallFunctionOnObject(context, v8_util, kHandleTrickyException);
+  EXPECT_THAT(
+      console_errors(),
+      testing::ElementsAre("handled: Error: some error",
+                           "handled again: (failed to get error message)"));
+}
+
+TEST_F(APIBindingJSUtilUnittest, TestSetExceptionHandler) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+
+  gin::Handle<APIBindingJSUtil> util = CreateUtil();
+  v8::Local<v8::Object> v8_util = util.ToV8().As<v8::Object>();
+
+  struct ErrorInfo {
+    std::string full_message;
+    std::string exception_message;
+  };
+
+  auto custom_handler = [](const v8::FunctionCallbackInfo<v8::Value>& info) {
+    gin::Arguments arguments(info);
+    std::string full_message;
+    ASSERT_TRUE(arguments.GetNext(&full_message));
+    v8::Local<v8::Object> error_object;
+    ASSERT_TRUE(arguments.GetNext(&error_object));
+
+    ASSERT_TRUE(info.Data()->IsExternal());
+    ErrorInfo* error_out =
+        static_cast<ErrorInfo*>(info.Data().As<v8::External>()->Value());
+    error_out->full_message = full_message;
+    error_out->exception_message = GetStringPropertyFromObject(
+        error_object, arguments.GetHolderCreationContext(), "message");
+  };
+
+  ErrorInfo error_info;
+  v8::Local<v8::Function> v8_handler =
+      v8::Function::New(context, custom_handler,
+                        v8::External::New(isolate(), &error_info))
+          .ToLocalChecked();
+  v8::Local<v8::Function> add_handler = FunctionFromString(
+      context,
+      "(function(util, handler) { util.setExceptionHandler(handler); })");
+  v8::Local<v8::Value> args[] = {v8_util, v8_handler};
+  RunFunction(add_handler, context, arraysize(args), args);
+
+  CallFunctionOnObject(context, v8_util, kHandleException);
+  // The error should not have been reported to the console since we have a
+  // cusotm handler.
+  EXPECT_TRUE(console_errors().empty());
+  EXPECT_EQ("handled: Error: some error", error_info.full_message);
+  EXPECT_EQ("\"some error\"", error_info.exception_message);
+}
+
 }  // namespace extensions

extensions/renderer/bindings/api_bindings_system_unittest.cc

@@ -115,6 +115,8 @@ void APIBindingsSystemTest::SetUp() {
     api_schemas_[api.name] = std::move(api_schema);
   }
 
+  binding::AddConsoleError add_console_error(base::Bind(
+      &APIBindingsSystemTest::AddConsoleError, base::Unretained(this)));
   bindings_system_ = base::MakeUnique<APIBindingsSystem>(
       base::Bind(&RunFunctionOnGlobalAndIgnoreResult),
       base::Bind(&RunFunctionOnGlobalAndReturnHandle),
@@ -123,10 +125,10 @@ void APIBindingsSystemTest::SetUp() {
       base::Bind(&APIBindingsSystemTest::OnAPIRequest, base::Unretained(this)),
       base::Bind(&APIBindingsSystemTest::OnEventListenersChanged,
                  base::Unretained(this)),
-      base::Bind(&DoNothingWithSilentRequest), binding::AddConsoleError(),
+      base::Bind(&DoNothingWithSilentRequest), add_console_error,
       APILastError(base::Bind(&APIBindingsSystemTest::GetLastErrorParent,
                               base::Unretained(this)),
-                   binding::AddConsoleError()));
+                   add_console_error));
 }
 
 void APIBindingsSystemTest::TearDown() {

extensions/renderer/bindings/api_bindings_system_unittest.h

@@ -53,6 +53,10 @@ class APIBindingsSystemTest : public APIBindingTest {
       v8::Local<v8::Context> context,
       v8::Local<v8::Object>* secondary_parent);
 
+  // Simulates logging an error to the console.
+  virtual void AddConsoleError(v8::Local<v8::Context> context,
+                               const std::string& error) {}
+
   // Returns the DictionaryValue representing the schema with the given API
   // name.
   const base::DictionaryValue& GetAPISchema(const std::string& api_name);

extensions/renderer/bindings/exception_handler.cc

@@ -63,19 +63,29 @@ void ExceptionHandler::HandleException(v8::Local<v8::Context> context,
           ? base::StringPrintf("%s: %s", message.c_str(),
                                gin::V8ToString(v8_message->Get()).c_str())
           : message;
+  HandleException(context, full_message, try_catch->Exception());
+  try_catch->Reset();  // Reset() to avoid handling the error more than once.
+}
+
+void ExceptionHandler::HandleException(v8::Local<v8::Context> context,
+                                       const std::string& full_message,
+                                       v8::Local<v8::Value> exception_value) {
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::HandleScope handle_scope(isolate);
 
   v8::Local<v8::Function> handler = GetCustomHandler(context);
   if (!handler.IsEmpty()) {
     v8::Local<v8::Value> arguments[] = {
-        gin::StringToV8(isolate, full_message), try_catch->Exception(),
+        gin::StringToV8(isolate, full_message), exception_value,
     };
+    // Hopefully, handling an exception doesn't throw an exception - but it's
+    // possible. Handle this gracefully, and log errors normally.
     v8::TryCatch handler_try_catch(isolate);
+    handler_try_catch.SetVerbose(true);
     run_js_.Run(handler, context, arraysize(arguments), arguments);
   } else {
     add_console_error_.Run(context, full_message);
   }
-
-  try_catch->Reset();  // Reset() to avoid handling the error more than once.
 }
 
 void ExceptionHandler::SetHandlerForContext(v8::Local<v8::Context> context,

extensions/renderer/bindings/exception_handler.h

@@ -29,6 +29,11 @@ class ExceptionHandler {
   void HandleException(v8::Local<v8::Context> context,
                        const std::string& message,
                        v8::TryCatch* try_catch);
+  // Same as above, but accepts a v8::Value for the exception rather than
+  // retrieving it from a TryCatch.
+  void HandleException(v8::Local<v8::Context> context,
+                       const std::string& full_message,
+                       v8::Local<v8::Value> exception_value);
 
   // Sets a custom handler for the given context, which will be notified of
   // exceptions thrown. This is only allowed to be called while the context is

extensions/renderer/native_extension_bindings_system.cc

@@ -707,7 +707,8 @@ void NativeExtensionBindingsSystem::GetJSBindingUtil(
       context->GetIsolate(),
       new APIBindingJSUtil(
           api_system_.type_reference_map(), api_system_.request_handler(),
-          api_system_.event_handler(), base::Bind(&CallJsFunction)));
+          api_system_.event_handler(), api_system_.exception_handler(),
+          base::Bind(&CallJsFunction)));
   *binding_util_out = handle.ToV8();
 }
 

extensions/renderer/resources/test_custom_bindings.js

@@ -12,11 +12,26 @@ var GetExtensionAPIDefinitionsForTest =
     requireNative('apiDefinitions').GetExtensionAPIDefinitionsForTest;
 var GetAPIFeatures = requireNative('test_features').GetAPIFeatures;
 var natives = requireNative('test_native_handler');
-var uncaughtExceptionHandler = require('uncaught_exception_handler');
 var userGestures = requireNative('user_gestures');
 
 var GetModuleSystem = requireNative('v8_context').GetModuleSystem;
 
+var jsExceptionHandler =
+    bindingUtil ? undefined : require('uncaught_exception_handler');
+function setExceptionHandler(handler) {
+  if (bindingUtil)
+    bindingUtil.setExceptionHandler(handler);
+  else
+    jsExceptionHandler.setHandler(handler);
+}
+
+function handleException(message, error) {
+  if (bindingUtil)
+    bindingUtil.handleException(message, error);
+  else
+    jsExceptionHandler.handle(message, error);
+}
+
 binding.registerCustomHook(function(api) {
   var chromeTest = api.compiledApi;
   var apiFunctions = api.apiFunctions;
@@ -88,13 +103,13 @@ binding.registerCustomHook(function(api) {
 
     try {
       chromeTest.log("( RUN      ) " + testName(currentTest));
-      uncaughtExceptionHandler.setHandler(function(message, e) {
+      setExceptionHandler(function(message, e) {
         if (e !== failureException)
           chromeTest.fail('uncaught exception: ' + message);
       });
       currentTest.call();
     } catch (e) {
-      uncaughtExceptionHandler.handle(e.message, e);
+      handleException(e.message, e);
     }
   });
 
@@ -261,7 +276,7 @@ binding.registerCustomHook(function(api) {
     } catch (e) {
       if (e === failureException)
         throw e;
-      uncaughtExceptionHandler.handle(e.message, e);
+      handleException(e.message, e);
     }
   };
 
@@ -354,7 +369,7 @@ binding.registerCustomHook(function(api) {
 
   apiFunctions.setHandleRequest('setExceptionHandler', function(callback) {
     chromeTest.assertEq(typeof(callback), 'function');
-    uncaughtExceptionHandler.setHandler(callback);
+    setExceptionHandler(callback);
   });
 
   apiFunctions.setHandleRequest('getWakeEventPage', function() {