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

extensions/renderer/api_last_error.cc

 // 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/api_last_error.h"
 
 #include "gin/converter.h"
 #include "gin/handle.h"
 #include "gin/object_template_builder.h"
 #include "gin/wrappable.h"
@@ skipping 35 matching lines @@
   std::string error_;
   bool accessed_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(LastErrorObject);
 };
 
 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;
 
 void APILastError::SetError(v8::Local<v8::Context> context,
                             const std::string& error) {
   v8::Isolate* isolate = context->GetIsolate();
   DCHECK(isolate);
   v8::HandleScope handle_scope(isolate);
 
   // The various accesses/sets on an object could potentially fail if script has
@@ skipping 58 matching lines @@
     v8::Local<v8::Value> error;
     if (!parent->Get(context, key).ToLocal(&error))
       return;
     if (!gin::Converter<LastErrorObject*>::FromV8(context->GetIsolate(), error,
                                                   &last_error)) {
       return;
     }
   }
 
   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().
   v8::TryCatch try_catch(isolate);
   try_catch.SetVerbose(true);
 
   // This Delete() can fail, but there's nothing to do if it does (the exception
   // will be caught by the TryCatch above).
   parent->Delete(context, key).ToChecked();
 }
@@ skipping 14 matching lines @@
            .ToLocal(&error)) {
     return false;
   }
 
   LastErrorObject* last_error = nullptr;
   return gin::Converter<LastErrorObject*>::FromV8(context->GetIsolate(), error,
                                                   &last_error);
 }
 
 }  // namespace extensions