[Extensions Bindings] Add an ExceptionHandler class

extensions/renderer/bindings/exception_handler.cc

Index: extensions/renderer/bindings/exception_handler.cc
diff --git a/extensions/renderer/bindings/exception_handler.cc b/extensions/renderer/bindings/exception_handler.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f356d8474038582c5a0f633cd497b3f87aa0642a
--- /dev/null
+++ b/extensions/renderer/bindings/exception_handler.cc
@@ -0,0 +1,81 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "extensions/renderer/bindings/exception_handler.h"
+
+#include "base/logging.h"
+#include "base/strings/stringprintf.h"
+#include "gin/converter.h"
+
+namespace extensions {
+
+ExceptionHandler::ExceptionHandler(
+    const binding::AddConsoleError& add_console_error,
+    const binding::RunJSFunction& run_js)
+    : add_console_error_(add_console_error), run_js_(run_js) {}
+ExceptionHandler::~ExceptionHandler() {}
+
+void ExceptionHandler::HandleException(v8::Local<v8::Context> context,
+                                       const std::string& message,
+                                       const v8::TryCatch& try_catch) {

[jbroman, 2017/06/30 18:35:31]

Hmm. Minor question here: do we want to reset the TryCatch once we've "handled"
it, to reduce the chance of handling it more than once?

[Devlin, 2017/07/06 16:49:22]

Good idea; done.

+  DCHECK(try_catch.HasCaught());
+
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::HandleScope handle_scope(isolate);
+
+  v8::Local<v8::Message> v8_message = try_catch.Message();
+  std::string full_message =
+      !v8_message.IsEmpty()
+          ? base::StringPrintf("%s: %s", message.c_str(),
+                               gin::V8ToString(v8_message->Get()).c_str())
+          : message;
+
+  v8::Local<v8::Function> handler = GetCustomHandler(context);
+  if (!handler.IsEmpty()) {
+    v8::Local<v8::Value> arguments[] = {
+        gin::StringToV8(isolate, full_message), try_catch.Exception(),
+    };
+    v8::TryCatch try_catch(isolate);

[jbroman, 2017/06/30 18:35:31]

Is it desirable to have exceptions here be verbose, or not?

Also, I thought the signature of the RunJSFunction already involved catching the
exceptions (and possibly also running asynchronously). Is that so, or am I
misremembering?

[Devlin, 2017/07/06 16:49:22]

Done.
AFAIK, it doesn't (in production).  RunJSFunction is CallJsFunction() from
native_extension_bindings_system.cc
(https://chromium.googlesource.com/chromium/src/+/5bf60281e3ba01737805ba8b505a...),
which calls ScriptContext::SafeCallFunction().  I don't think there's any
TryCatch-ing there.

The asynchronous aspect is a good and interesting point.  I suppose that if JS
were disabled when we caught an exception to handle it (can that happen?), then
this try catch would go out of scope before the script was executed, and we'd
then throw an error.  A few thoughts here:
- This is only relevant with custom handling, which should only be relevant in
tests.
- Is it possible to have JS disabled immediately after throwing an error?
- What's the worst that could happen?  This doesn't strike me as a security
concern, even if it's a bit weird.

Overall, I'm inclined to say this is okay, but if this is worse than I
understand, lemme know. :)

[jbroman, 2017/07/06 21:09:18]

Ah, you're quite correct; I misremembered.
Yeah, I think that's fine. I am generally suspect that we do something
reasonable when RequestExecuteV8Function goes async and then throws an
exception, but I suppose that's separate from this.

+    run_js_.Run(handler, context, arraysize(arguments), arguments);
+  } else {
+    add_console_error_.Run(context, full_message);
+  }
+}
+
+void ExceptionHandler::SetHandlerForContext(v8::Local<v8::Context> context,
+                                            v8::Local<v8::Function> handler) {
+  v8::Isolate* isolate = context->GetIsolate();
+  for (auto& pair : custom_handlers_) {
+    if (pair.first == context) {
+      pair.second = v8::Global<v8::Function>(isolate, handler);
+      return;
+    }
+  }
+  custom_handlers_.emplace_back(v8::Global<v8::Context>(isolate, context),
+                                v8::Global<v8::Function>(isolate, handler));
+}
+
+void ExceptionHandler::InvalidateContext(v8::Local<v8::Context> context) {
+  for (auto iter = custom_handlers_.begin(); iter != custom_handlers_.end();
+       ++iter) {
+    if (iter->first == context) {
+      custom_handlers_.erase(iter);
+      break;
+    }
+  }
+  // We erase only the first match we see in the list because we should have at
+  // most one handler per context; assert that there aren't any others.
+  DCHECK(GetCustomHandler(context).IsEmpty());
+}
+
+v8::Local<v8::Function> ExceptionHandler::GetCustomHandler(
+    v8::Local<v8::Context> context) {
+  for (auto& pair : custom_handlers_) {
+    if (pair.first == context)
+      return pair.second.Get(context->GetIsolate());
+  }
+  return v8::Local<v8::Function>();
+}
+
+}  // namespace extensions