[Extensions Bindings] Allow custom hooks to return values, throw

Custom hooks need the ability to return results synchronously and
throw exceptions in the case of bad invocations. Update native
bindings to allow for hooks to do this, and add a number of tests.

BUG=653596

Review-Url: https://codereview.chromium.org/2609553003
Cr-Commit-Position: refs/heads/master@{#442037}

extensions/renderer/api_binding.cc

@@ -200,12 +200,10 @@ void APIBinding::HandleCall(const std::string& name,
   {
     v8::TryCatch try_catch(isolate);
     APIBindingHooks::RequestResult hooks_result =
-        APIBindingHooks::RequestResult::NOT_HANDLED;
-    hooks_result = binding_hooks_->HandleRequest(api_name_, name, context,
-                                                 signature, &argument_list,
-                                                 *type_refs_);
+        binding_hooks_->HandleRequest(api_name_, name, context,
+                                      signature, &argument_list, *type_refs_);
 
-    switch (hooks_result) {
+    switch (hooks_result.code) {
       case APIBindingHooks::RequestResult::INVALID_INVOCATION:
         invalid_invocation = true;
         // Throw a type error below so that it's not caught by our try-catch.
@@ -215,6 +213,8 @@ void APIBinding::HandleCall(const std::string& name,
         try_catch.ReThrow();
         return;
       case APIBindingHooks::RequestResult::HANDLED:
+        if (!hooks_result.return_value.IsEmpty())
+          arguments->Return(hooks_result.return_value);
         return;  // Our work here is done.
       case APIBindingHooks::RequestResult::NOT_HANDLED:
         break;  // Handle in the default manner.

extensions/renderer/api_binding_hooks.cc

@@ -164,6 +164,11 @@ v8::Local<v8::Object> GetJSHookInterfaceObject(
 
 }  // namespace
 
+APIBindingHooks::RequestResult::RequestResult(ResultCode code) : code(code) {}
+APIBindingHooks::RequestResult::~RequestResult() {}
+APIBindingHooks::RequestResult::RequestResult(const RequestResult& other) =
+    default;
+
 APIBindingHooks::APIBindingHooks(const binding::RunJSFunctionSync& run_js)
     : run_js_(run_js) {}
 APIBindingHooks::~APIBindingHooks() {}
@@ -195,7 +200,7 @@ APIBindingHooks::RequestResult APIBindingHooks::HandleRequest(
             signature, context, arguments, type_refs);
     // Right now, it doesn't make sense to register a request handler that
     // doesn't handle the request.
-    DCHECK_NE(RequestResult::NOT_HANDLED, result);
+    DCHECK_NE(RequestResult::NOT_HANDLED, result.code);
     return result;
   }
 
@@ -204,7 +209,7 @@ APIBindingHooks::RequestResult APIBindingHooks::HandleRequest(
   v8::Local<v8::Object> hook_interface_object =
       GetJSHookInterfaceObject(api_name, context, false);
   if (hook_interface_object.IsEmpty())
-    return RequestResult::NOT_HANDLED;
+    return RequestResult(RequestResult::NOT_HANDLED);
 
   v8::Isolate* isolate = context->GetIsolate();
 
@@ -223,7 +228,7 @@ APIBindingHooks::RequestResult APIBindingHooks::HandleRequest(
     UpdateArguments(pre_validate_hook, context, arguments);
     if (try_catch.HasCaught()) {
       try_catch.ReThrow();
-      return RequestResult::THROWN;
+      return RequestResult(RequestResult::THROWN);
     }
   }
 
@@ -237,17 +242,25 @@ APIBindingHooks::RequestResult APIBindingHooks::HandleRequest(
                                                  &parsed_v8_args, &error);
     if (try_catch.HasCaught()) {
       try_catch.ReThrow();
-      return RequestResult::THROWN;
+      return RequestResult(RequestResult::THROWN);
     }
     if (!success)
-      return RequestResult::INVALID_INVOCATION;
+      return RequestResult(RequestResult::INVALID_INVOCATION);
 
+    v8::Global<v8::Value> global_result =
         run_js_.Run(handle_request, context, parsed_v8_args.size(),
                     parsed_v8_args.data());
-    return RequestResult::HANDLED;
+    if (try_catch.HasCaught()) {
+      try_catch.ReThrow();
+      return RequestResult(RequestResult::THROWN);
+    }
+    RequestResult result(RequestResult::HANDLED);
+    if (!global_result.IsEmpty())
+      result.return_value = global_result.Get(isolate);
+    return result;
   }
 
-  return RequestResult::NOT_HANDLED;
+  return RequestResult(RequestResult::NOT_HANDLED);
 }
 
 void APIBindingHooks::InitializeInContext(

extensions/renderer/api_binding_hooks.h

@@ -30,13 +30,23 @@ class APISignature;
 class APIBindingHooks {
  public:
   // The result of checking for hooks to handle a request.
-  enum class RequestResult {
+  struct RequestResult {
+    enum ResultCode {
       HANDLED,             // A custom hook handled the request.
-    THROWN,              // An exception was thrown during parsing or handling.
+      THROWN,              // An exception was thrown during parsing or
+                           // handling.
       INVALID_INVOCATION,  // The request was called with invalid arguments.
       NOT_HANDLED,         // The request was not handled.
     };
 
+    explicit RequestResult(ResultCode code);
+    RequestResult(const RequestResult& other);
+    ~RequestResult();
+
+    ResultCode code;
+    v8::Local<v8::Value> return_value;  // Only valid if code == HANDLED.
+  };
+
   // The callback to handle an API method. We pass in the expected signature
   // (so the caller can verify arguments, optionally after modifying/"massaging"
   // them) and the passed arguments. The handler is responsible for returning,
@@ -68,8 +78,7 @@ class APIBindingHooks {
                            const std::string& api_name);
 
   // Looks for a custom hook to handle the given request and, if one exists,
-  // runs it. Returns the result of trying to run the hook, or NOT_HANDLED if no
-  // hook was found.
+  // runs it. Returns the result of running the hook, if any.
   RequestResult HandleRequest(const std::string& api_name,
                               const std::string& method_name,
                               v8::Local<v8::Context> context,

extensions/renderer/api_binding_unittest.cc

@@ -478,12 +478,14 @@ TEST_F(APIBindingUnittest, TestCustomHooks) {
                  std::vector<v8::Local<v8::Value>>* arguments,
                  const ArgumentSpec::RefMap& ref_map) {
     *did_call = true;
-    if (arguments->size() != 1u) {
+    APIBindingHooks::RequestResult result(
+        APIBindingHooks::RequestResult::HANDLED);
+    if (arguments->size() != 1u) {  // ASSERT* messes with the return type.
       EXPECT_EQ(1u, arguments->size());
-      return APIBindingHooks::RequestResult::HANDLED;
+      return result;
     }
     EXPECT_EQ("foo", gin::V8ToString(arguments->at(0)));
-    return APIBindingHooks::RequestResult::HANDLED;
+    return result;
   };
   hooks->RegisterHandleRequest("test.oneString", base::Bind(hook, &did_call));
 
@@ -691,4 +693,195 @@ TEST_F(APIBindingUnittest, TestThrowInUpdateArgumentsPreValidate) {
   ExpectPass(binding_object, "obj.stringAndInt('foo', 42);", "['foo',42]");
 }
 
+// Tests that custom JS hooks can return results synchronously.
+TEST_F(APIBindingUnittest, TestReturningResultFromCustomJSHook) {
+  // Register a hook for the test.oneString method.
+  auto hooks = base::MakeUnique<APIBindingHooks>(
+      base::Bind(&RunFunctionOnGlobalAndReturnHandle));
+
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = ContextLocal();
+  const char kRegisterHook[] =
+      "(function(hooks) {\n"
+      "  hooks.setHandleRequest('oneString', str => {\n"
+      "    return str + ' pong';\n"
+      "  });\n"
+      "})";
+  v8::Local<v8::String> source_string =
+      gin::StringToV8(isolate(), kRegisterHook);
+  v8::Local<v8::String> source_name =
+      gin::StringToV8(isolate(), "custom_hook");
+  hooks->RegisterJsSource(
+      v8::Global<v8::String>(isolate(), source_string),
+      v8::Global<v8::String>(isolate(), source_name));
+
+  std::unique_ptr<base::ListValue> functions = ListValueFromString(kFunctions);
+  ASSERT_TRUE(functions);
+  ArgumentSpec::RefMap refs;
+
+  APIBinding binding(
+      "test", functions.get(), nullptr, nullptr,
+      base::Bind(&APIBindingUnittest::OnFunctionCall, base::Unretained(this)),
+      std::move(hooks), &refs);
+  EXPECT_TRUE(refs.empty());
+
+  APIEventHandler event_handler(
+      base::Bind(&RunFunctionOnGlobalAndIgnoreResult));
+  v8::Local<v8::Object> binding_object = binding.CreateInstance(
+      context, isolate(), &event_handler, base::Bind(&AllowAllAPIs));
+
+  v8::Local<v8::Function> function =
+      FunctionFromString(context,
+                         "(function(obj) { return obj.oneString('ping'); })");
+  v8::Local<v8::Value> args[] = {binding_object};
+  v8::Local<v8::Value> result =
+      RunFunction(function, context, arraysize(args), args);
+  ASSERT_FALSE(result.IsEmpty());
+  std::unique_ptr<base::Value> json_result = V8ToBaseValue(result, context);
+  ASSERT_TRUE(json_result);
+  EXPECT_EQ("\"ping pong\"", ValueToString(*json_result));
+}
+
+// Tests that JS custom hooks can throw exceptions for bad invocations.
+TEST_F(APIBindingUnittest, TestThrowingFromCustomJSHook) {
+  // Our testing handlers for running functions expect a pre-determined success
+  // or failure. Since we're testing throwing exceptions here, we need a way of
+  // running that allows exceptions to be thrown, but we still expect most JS
+  // calls to succeed.
+  // TODO(devlin): This is a bit clunky. If we need to do this enough, we could
+  // figure out a different solution, like having a stack object for allowing
+  // errors/exceptions. But given this is the only place we need it so far, this
+  // is sufficient.
+  auto run_js_and_expect_error = [](v8::Local<v8::Function> function,
+                                          v8::Local<v8::Context> context,
+                                          int argc,
+                                          v8::Local<v8::Value> argv[]) {
+    v8::MaybeLocal<v8::Value> maybe_result =
+        function->Call(context, context->Global(), argc, argv);
+    v8::Global<v8::Value> result;
+    v8::Local<v8::Value> local;
+    if (maybe_result.ToLocal(&local))
+      result.Reset(context->GetIsolate(), local);
+    return result;
+  };
+  // Register a hook for the test.oneString method.
+  auto hooks = base::MakeUnique<APIBindingHooks>(
+      base::Bind(run_js_and_expect_error));
+
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = ContextLocal();
+  const char kRegisterHook[] =
+      "(function(hooks) {\n"
+      "  hooks.setHandleRequest('oneString', str => {\n"
+      "    throw new Error('Custom Hook Error');\n"
+      "  });\n"
+      "})";
+  v8::Local<v8::String> source_string =
+      gin::StringToV8(isolate(), kRegisterHook);
+  v8::Local<v8::String> source_name =
+      gin::StringToV8(isolate(), "custom_hook");
+  hooks->RegisterJsSource(
+      v8::Global<v8::String>(isolate(), source_string),
+      v8::Global<v8::String>(isolate(), source_name));
+
+  std::unique_ptr<base::ListValue> functions = ListValueFromString(kFunctions);
+  ASSERT_TRUE(functions);
+  ArgumentSpec::RefMap refs;
+
+  APIBinding binding(
+      "test", functions.get(), nullptr, nullptr,
+      base::Bind(&APIBindingUnittest::OnFunctionCall, base::Unretained(this)),
+      std::move(hooks), &refs);
+  EXPECT_TRUE(refs.empty());
+
+  APIEventHandler event_handler(
+      base::Bind(&RunFunctionOnGlobalAndIgnoreResult));
+  v8::Local<v8::Object> binding_object = binding.CreateInstance(
+      context, isolate(), &event_handler, base::Bind(&AllowAllAPIs));
+
+  v8::Local<v8::Function> function =
+      FunctionFromString(context,
+                         "(function(obj) { return obj.oneString('ping'); })");
+  v8::Local<v8::Value> args[] = {binding_object};
+  RunFunctionAndExpectError(function, context, v8::Undefined(isolate()),
+                            arraysize(args), args,
+                            "Uncaught Error: Custom Hook Error");
+}
+
+// Tests that native custom hooks can return results synchronously, or throw
+// exceptions for bad invocations.
+TEST_F(APIBindingUnittest,
+       TestReturningResultAndThrowingExceptionFromCustomNativeHook) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = ContextLocal();
+
+  // Register a hook for the test.oneString method.
+  auto hooks = base::MakeUnique<APIBindingHooks>(
+      base::Bind(&RunFunctionOnGlobalAndReturnHandle));
+  bool did_call = false;
+  auto hook = [](bool* did_call, const APISignature* signature,
+                 v8::Local<v8::Context> context,
+                 std::vector<v8::Local<v8::Value>>* arguments,
+                 const ArgumentSpec::RefMap& ref_map) {
+    APIBindingHooks::RequestResult result(
+        APIBindingHooks::RequestResult::HANDLED);
+    if (arguments->size() != 1u) {  // ASSERT* messes with the return type.
+      EXPECT_EQ(1u, arguments->size());
+      return result;
+    }
+    v8::Isolate* isolate = context->GetIsolate();
+    std::string arg_value = gin::V8ToString(arguments->at(0));
+    if (arg_value == "throw") {
+      isolate->ThrowException(v8::Exception::Error(
+          gin::StringToV8(isolate, "Custom Hook Error")));
+      result.code = APIBindingHooks::RequestResult::THROWN;
+      return result;
+    }
+    result.return_value =
+        gin::StringToV8(context->GetIsolate(), arg_value + " pong");
+    return result;
+  };
+  hooks->RegisterHandleRequest("test.oneString", base::Bind(hook, &did_call));
+
+  std::unique_ptr<base::ListValue> functions = ListValueFromString(kFunctions);
+  ASSERT_TRUE(functions);
+  ArgumentSpec::RefMap refs;
+
+  APIBinding binding(
+      "test", functions.get(), nullptr, nullptr,
+      base::Bind(&APIBindingUnittest::OnFunctionCall, base::Unretained(this)),
+      std::move(hooks), &refs);
+  EXPECT_TRUE(refs.empty());
+
+  APIEventHandler event_handler(
+      base::Bind(&RunFunctionOnGlobalAndIgnoreResult));
+  v8::Local<v8::Object> binding_object = binding.CreateInstance(
+      context, isolate(), &event_handler, base::Bind(&AllowAllAPIs));
+
+  {
+    // Test an invocation that we expect to throw an exception.
+    v8::Local<v8::Function> function =
+        FunctionFromString(
+            context, "(function(obj) { return obj.oneString('throw'); })");
+    v8::Local<v8::Value> args[] = {binding_object};
+    RunFunctionAndExpectError(function, context, v8::Undefined(isolate()),
+                              arraysize(args), args,
+                              "Uncaught Error: Custom Hook Error");
+  }
+
+  {
+    // Test an invocation we expect to succeed.
+    v8::Local<v8::Function> function =
+        FunctionFromString(context,
+                           "(function(obj) { return obj.oneString('ping'); })");
+    v8::Local<v8::Value> args[] = {binding_object};
+    v8::Local<v8::Value> result =
+        RunFunction(function, context, arraysize(args), args);
+    ASSERT_FALSE(result.IsEmpty());
+    std::unique_ptr<base::Value> json_result = V8ToBaseValue(result, context);
+    ASSERT_TRUE(json_result);
+    EXPECT_EQ("\"ping pong\"", ValueToString(*json_result));
+  }
+}
+
 }  // namespace extensions

extensions/renderer/api_bindings_system_unittest.cc

@@ -300,22 +300,24 @@ TEST_F(APIBindingsSystemTest, TestCustomHooks) {
                  std::vector<v8::Local<v8::Value>>* arguments,
                  const ArgumentSpec::RefMap& type_refs) {
     *did_call = true;
+    APIBindingHooks::RequestResult result(
+        APIBindingHooks::RequestResult::HANDLED);
     if (arguments->size() != 2) {  // ASSERT* messes with the return type.
       EXPECT_EQ(2u, arguments->size());
-      return APIBindingHooks::RequestResult::HANDLED;
+      return result;
     }
     std::string argument;
     EXPECT_EQ("foo", gin::V8ToString(arguments->at(0)));
     if (!arguments->at(1)->IsFunction()) {
       EXPECT_TRUE(arguments->at(1)->IsFunction());
-      return APIBindingHooks::RequestResult::HANDLED;
+      return result;
     }
     v8::Local<v8::String> response =
         gin::StringToV8(context->GetIsolate(), "bar");
     v8::Local<v8::Value> response_args[] = {response};
     RunFunctionOnGlobal(arguments->at(1).As<v8::Function>(),
                         context, 1, response_args);
-    return APIBindingHooks::RequestResult::HANDLED;
+    return result;
   };
 
   APIBindingHooks* hooks = bindings_system()->GetHooksForAPI(kAlphaAPIName);