[Extenisons Bindings] Don't throw unchecked errors; add console errors

extensions/renderer/api_last_error.cc

Index: extensions/renderer/api_last_error.cc
diff --git a/extensions/renderer/api_last_error.cc b/extensions/renderer/api_last_error.cc
index 4073d276c09a526981cceb2d58fc5737e0e3319d..ece0de11dd2de4d988d0f70654a7eb3658a93094 100644
--- a/extensions/renderer/api_last_error.cc
+++ b/extensions/renderer/api_last_error.cc
@@ -53,8 +53,9 @@ gin::WrapperInfo LastErrorObject::kWrapperInfo = {gin::kEmbedderNativeGin};
 
 }  // namespace
 
-APILastError::APILastError(const GetParent& get_parent)
-    : get_parent_(get_parent) {}
+APILastError::APILastError(const GetParent& get_parent,
+                           const AddConsoleError& add_console_error)
+    : get_parent_(get_parent), add_console_error_(add_console_error) {}
 APILastError::APILastError(APILastError&& other) = default;
 APILastError::~APILastError() = default;
 
@@ -133,8 +134,8 @@ void APILastError::ClearError(v8::Local<v8::Context> context,
   }
 
   if (report_if_unchecked && !last_error->accessed()) {
-    isolate->ThrowException(
-        v8::Exception::Error(gin::StringToV8(isolate, last_error->error())));
+    add_console_error_.Run(

[jbroman, 2017/04/19 17:45:04]

Heh, I'd missed that the original code used console.error rather than throwing
an exception all along. Sorry.

The other way, I suppose, would be to wrap it in a verbose TryCatch and let
whatever handler is registered on the isolate handle that. I think I like the
current approach better. (I'm not entirely sure whether you get a stack trace
this way, though.)

[lazyboy, 2017/04/19 18:45:40]

I might be missing something, but
add_console_error_.is_null() == true for non-test code, right? Shouldn't we be
checking for that before calling Run()?
I've also noticed the same is true for get_parent_.

[Devlin, 2017/04/19 19:45:33]

I don't think it should ever be null for non-test code (the only place we
instantiate it is through NativeExtensionBindingsSystem, passed to
APIBindingsSystem).  So I think unconditionally calling it (and requiring tests
that trigger it to provide something) is reasonable.

Am I missing something?

[Devlin, 2017/04/19 19:45:33]

Clearly, I did too. :)
The way the current code handles stack traces is by recording a stack trace at
the start of the request and currying it along, and then passing it to
lastError.reportIfUnchecked.  I've been torn on whether it's worth that currying
for the benefit it provides (which is also that if there's an error in a
callback, we can use that same trace).  In either case, I think it's best
handled in a follow-up, since it's a more far-reaching change.

[lazyboy, 2017/04/19 20:28:07]

Nope. Looking again, I think I misread api_binding_unittest.cc.

+        context, "Unchecked runtime.lastError: " + last_error->error());
   }
 
   // See comment in SetError().