[Extensions Bindings] Add lastError utilities to APIBindingJSUtil

extensions/renderer/api_binding_js_util_unittest.cc

Index: extensions/renderer/api_binding_js_util_unittest.cc
diff --git a/extensions/renderer/api_binding_js_util_unittest.cc b/extensions/renderer/api_binding_js_util_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..3c708b57c6a3e3b18b334afd741dd1dd8f10ba84
--- /dev/null
+++ b/extensions/renderer/api_binding_js_util_unittest.cc
@@ -0,0 +1,136 @@
+// 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_binding_js_util.h"
+
+#include "base/bind.h"
+#include "extensions/renderer/api_binding_test_util.h"
+#include "extensions/renderer/api_bindings_system.h"
+#include "extensions/renderer/api_bindings_system_unittest.h"
+#include "gin/handle.h"
+
+namespace extensions {
+
+class APIBindingJSUtilUnittest : public APIBindingsSystemTest {
+ protected:
+  APIBindingJSUtilUnittest() {}
+  ~APIBindingJSUtilUnittest() override {}
+
+  gin::Handle<APIBindingJSUtil> CreateUtil() {
+    return gin::CreateHandle(
+        isolate(),
+        new APIBindingJSUtil(bindings_system()->type_reference_map(),
+                             bindings_system()->request_handler(),
+                             bindings_system()->event_handler(),
+                             base::Bind(&RunFunctionOnGlobalAndIgnoreResult)));
+  }
+
+  v8::Local<v8::Object> GetLastErrorParent(
+      v8::Local<v8::Context> context) override {
+    return context->Global();
+  }
+
+  std::string GetExposedError(v8::Local<v8::Context> context) {
+    v8::Local<v8::Value> last_error =
+        GetPropertyFromObject(context->Global(), context, "lastError");
+
+    // Use ADD_FAILURE() to avoid messing up the return type with ASSERT.
+    if (last_error.IsEmpty()) {
+      ADD_FAILURE();
+      return std::string();
+    }
+    if (!last_error->IsObject() && !last_error->IsUndefined()) {
+      ADD_FAILURE();
+      return std::string();
+    }
+
+    if (last_error->IsUndefined())
+      return "[empty]";

[jbroman, 2017/03/20 20:14:49]

I find it slightly confusing that "[empty]" is returned in the case of
undefined, not an empty handle (which we'd ordinarily crash or branch on, but I
see here that seems to be how you're handling exceptions. The more usual pattern
would be something like:

v8::Isolate* isolate = context->GetIsolate();
v8::Local<v8::Value> last_error;
if (!context->Global()->Get(context, gin::StringToSymbol(isolate,
"lastError")).ToLocal(&last_error) {
  ADD_FAILURE() << "getting lastError threw an exception";
  return std::string();
}
// etc

[Devlin, 2017/03/20 23:10:40]

Not so much how we're handling exceptions as how we're handling an error not
existing.  The lastError property in extension APIs (an old concept, but, well,
we're kind of stuck with it for the moment) transitively exists and doesn't
exist on its parent object (in practice, the chrome.runtime object).  So when
[parent].lastError is undefined (as here), it's not an exception; it just means
no last error was set (which is perfectly valid state for a few of these
places).  Invalid state would be if we get an empty handle, or lastError is
something unexpected - each of those are checked above and ADD_FAILURE()'d.

[jbroman, 2017/03/21 22:02:46]

Getting a missing property yields undefined, not an empty handle. Unless an
exception is thrown, you should _not_ get an empty handle here. (As it happens,
if you do get an empty handle here, GetPropertyFromObject has already added a
failure due to EXPECT_TRUE.)

[Devlin, 2017/03/21 22:51:19]

Right; that's what I was saying...  I think maybe I misunderstood your original
comment.  I thought you meant you found it confusing that we were handling
exceptions by returning "[empty]" (which we're not doing - we handle exceptions
above on line 40).  Did you just mean you found the string "[empty]" strange, as
opposed to, say, "[undefined]"?

(changed to "[undefined]")

+    return GetStringPropertyFromObject(last_error.As<v8::Object>(), context,
+                                       "message");
+  }
+
+  APILastError* last_error() {
+    return bindings_system()->request_handler()->last_error();
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(APIBindingJSUtilUnittest);
+};
+
+TEST_F(APIBindingJSUtilUnittest, TestSetLastError) {
+  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>();
+
+  EXPECT_FALSE(last_error()->HasError(context));
+  EXPECT_EQ("[empty]", GetExposedError(context));
+  const char kSetLastError[] = "obj.setLastError('a last error');";
+  CallFunctionOnObject(context, v8_util, kSetLastError);
+  EXPECT_TRUE(last_error()->HasError(context));
+  EXPECT_EQ("\"a last error\"", GetExposedError(context));
+
+  CallFunctionOnObject(context, v8_util,
+                       "obj.setLastError('a new last error')");
+  EXPECT_TRUE(last_error()->HasError(context));
+  EXPECT_EQ("\"a new last error\"", GetExposedError(context));
+}
+
+TEST_F(APIBindingJSUtilUnittest, TestHasLastError) {
+  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>();
+
+  EXPECT_FALSE(last_error()->HasError(context));
+  EXPECT_EQ("[empty]", GetExposedError(context));
+  const char kHasLastError[] = "return obj.hasLastError();";
+  v8::Local<v8::Value> has_error =
+      CallFunctionOnObject(context, v8_util, kHasLastError);
+  EXPECT_EQ("false", V8ToString(has_error, context));
+
+  last_error()->SetError(context, "an error");
+  EXPECT_TRUE(last_error()->HasError(context));
+  EXPECT_EQ("\"an error\"", GetExposedError(context));
+  has_error = CallFunctionOnObject(context, v8_util, kHasLastError);
+  EXPECT_EQ("true", V8ToString(has_error, context));
+
+  last_error()->ClearError(context, false);
+  EXPECT_FALSE(last_error()->HasError(context));
+  EXPECT_EQ("[empty]", GetExposedError(context));
+  has_error = CallFunctionOnObject(context, v8_util, kHasLastError);
+  EXPECT_EQ("false", V8ToString(has_error, context));
+}
+
+TEST_F(APIBindingJSUtilUnittest, TestRunWithLastError) {
+  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>();
+
+  EXPECT_FALSE(last_error()->HasError(context));
+  EXPECT_EQ("[empty]", GetExposedError(context));
+
+  const char kRunWithLastError[] =
+      "obj.runCallbackWithLastError('last error', function() {\n"
+      "  this.exposedLastError =\n"
+      "      this.lastError ? this.lastError.message : 'undefined';\n"
+      "  this.callbackArgs = Array.from(arguments);\n"
+      "}, [1, 'foo']);";
+  CallFunctionOnObject(context, v8_util, kRunWithLastError);
+
+  EXPECT_FALSE(last_error()->HasError(context));
+  EXPECT_EQ("[empty]", GetExposedError(context));
+  EXPECT_EQ("\"last error\"",
+            GetStringPropertyFromObject(context->Global(), context,
+                                        "exposedLastError"));
+  EXPECT_EQ("[1,\"foo\"]", GetStringPropertyFromObject(
+                               context->Global(), context, "callbackArgs"));
+}
+
+}  // namespace extensions