[Extensions Bindings] Add chrome.runtime.lastError support

extensions/renderer/api_last_error_unittest.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 "base/bind.h"
+#include "base/callback_helpers.h"
+#include "extensions/renderer/api_binding_test.h"
+#include "extensions/renderer/api_binding_test_util.h"
+#include "gin/converter.h"
+#include "gin/public/context_holder.h"
+
+namespace extensions {
+
+namespace {
+
+std::string GetLastError(v8::Local<v8::Object> parent,
+                         v8::Local<v8::Context> context) {
+  v8::Local<v8::Value> last_error =
+      GetPropertyFromObject(parent, context, "lastError");
+  if (last_error.IsEmpty() || !last_error->IsObject())
+    return V8ToString(last_error, context);
+  v8::Local<v8::Value> message =
+      GetPropertyFromObject(last_error.As<v8::Object>(), context, "message");
+  return V8ToString(message, context);
+}
+
+using ParentList =
+    std::vector<std::pair<v8::Local<v8::Context>, v8::Local<v8::Object>>>;
+v8::Local<v8::Object> GetParent(
+    const ParentList& parents,
+    v8::Local<v8::Context> context) {
+  // This would be simpler with a map<context, object>, but Local<> doesn't
+  // define an operator<.
+  for (const auto& parent : parents) {
+    if (parent.first == context)
+      return parent.second;
+  }
+  return v8::Local<v8::Object>();
+}
+
+}  // namespace
+
+using APILastErrorTest = APIBindingTest;
+
+// Test basic functionality of the lastError object.
+TEST_F(APILastErrorTest, TestLastError) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = ContextLocal();
+  v8::Local<v8::Object> parent_object = v8::Object::New(isolate());
+
+  ParentList parents = {{context, parent_object}};
+  APILastError last_error(base::Bind(&GetParent, parents));
+
+  EXPECT_EQ("undefined", GetLastError(parent_object, context));
+
+  last_error.SetError(context, "Some last error");
+  EXPECT_EQ("\"Some last error\"", GetLastError(parent_object, context));
+
+  last_error.ClearError(context, false);
+  EXPECT_EQ("undefined", GetLastError(parent_object, context));
+}
+
+// Test throwing an error if the last error wasn't checked.
+TEST_F(APILastErrorTest, ReportIfUnchecked) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = ContextLocal();
+  v8::Local<v8::Object> parent_object = v8::Object::New(isolate());
+
+  ParentList parents = {{context, parent_object}};
+  APILastError last_error(base::Bind(&GetParent, parents));
+
+  bool did_throw = false;
+  auto message_listener = [](v8::Local<v8::Message> message,
+                             v8::Local<v8::Value> data) {
+    ASSERT_FALSE(data.IsEmpty());
+    ASSERT_TRUE(data->IsExternal());
+    bool* did_throw = static_cast<bool*>(data.As<v8::External>()->Value());
+    *did_throw = true;
+    EXPECT_EQ("Uncaught Error: A last error",
+              gin::V8ToString(message->Get()));
+  };
+
+  isolate()->AddMessageListener(message_listener,
+                                v8::External::New(isolate(), &did_throw));
+  base::ScopedClosureRunner remove_message_listener(base::Bind(
+      [](v8::Isolate* isolate, v8::MessageCallback listener) {
+        isolate->RemoveMessageListeners(listener);
+      },
+      isolate(), message_listener));
+
+  last_error.SetError(context, "foo");
+  // GetLastError() will count as accessing the error property, so we shouldn't
+  // throw an exception.
+  EXPECT_EQ("\"foo\"", GetLastError(parent_object, context));
+  last_error.ClearError(context, true);
+  EXPECT_FALSE(did_throw);
+
+  // This time, we should throw an exception.
+  last_error.SetError(context, "A last error");
+  last_error.ClearError(context, true);
+  EXPECT_TRUE(did_throw);

[Devlin, 2017/01/27 21:41:48]

Strangely, this test fails.  I've verified that we throw the exception in
api_last_error.cc as expected, but it doesn't reach the message listener.  Any
idea why?  Should we be throwing in a different way than
Isolate::ThrowException()?

Also interesting, this test fails in the tear down where we verify that the
contexts get properly cleaned up, and it seems the culprit is the call to
ThrowException() (if I comment it out, the context is cleaned up properly).  Any
ideas?

[jbroman, 2017/01/30 22:49:52]

This is a subtlety about how throwing exceptions works. The public method
actually only makes it the "scheduled" exception, which is thrown when the V8
runtime is entered. As a result, the exception is getting thrown in a subsequent
test, when the isolate next checks for a scheduled exception to promote.

I think in this case, it's probably simplest to check for the error by using a
TryCatch and checking HasCaught (you can also get the message from the TryCatch,
if you want to examine it). Nothing strange is going on here that you should
need to use a global message listener (unlike the other case, where you were
checking what happened if an exception was unhandled in an event listener).

[Devlin, 2017/01/31 17:15:00]

Using the TryCatch here worked, thanks!  It's still crashing when trying to
dispose the Isolate, though... any ideas on that front?

[jbroman, 2017/02/06 18:31:26]

That's a red herring (due to a bug in APIBindingTest, that I'll send you a CL
for shortly). The actual problem is that the check that the context can be
collected failed -- which suggests that there's a memory leak around somewhere.

[Devlin, 2017/02/06 20:28:22]

As discussed offline, the real question here is what the leak is... it appears
to only happen when we have the ThrowException() call in ClearError() - without
that, the context is properly destroyed.  But nothing appears to be hanging on
to the error directly, so it seems like something in v8's machinery.

[jbroman, 2017/02/07 18:19:42]

Filed a v8 bug (https://bugs.chromium.org/p/v8/issues/detail?id=5941) and sent
out a CL with a possible fix. Agreed that this is an issue on the v8 side.

+}
+
+// Test behavior when something else sets a lastError property on the parent
+// object.
+TEST_F(APILastErrorTest, NonLastErrorObject) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = ContextLocal();
+  v8::Local<v8::Object> parent_object = v8::Object::New(isolate());
+
+  ParentList parents = {{context, parent_object}};
+  APILastError last_error(base::Bind(&GetParent, parents));
+
+  auto checked_set = [context](v8::Local<v8::Object> object,
+                              base::StringPiece key,
+                              v8::Local<v8::Value> value) {
+    v8::Maybe<bool> success =
+        object->Set(context, gin::StringToSymbol(context->GetIsolate(), key),
+                    value);
+    ASSERT_TRUE(success.IsJust());
+    ASSERT_TRUE(success.FromJust());
+  };
+
+  // Set a "fake" lastError property on the parent.
+  v8::Local<v8::Object> fake_last_error = v8::Object::New(isolate());
+  checked_set(fake_last_error, "message",
+              gin::StringToV8(isolate(), "fake error"));
+  checked_set(parent_object, "lastError", fake_last_error);
+
+  EXPECT_EQ("\"fake error\"", GetLastError(parent_object, context));
+
+  // The bindings shouldn't mangle an existing property.
+  // TODO(devlin): Or should we? If someone sets chrome.runtime.lastError, it
+  // might be the right course of action to overwrite it.
+  last_error.SetError(context, "Real last error");
+  EXPECT_EQ("\"fake error\"", GetLastError(parent_object, context));
+  last_error.ClearError(context, false);
+  EXPECT_EQ("\"fake error\"", GetLastError(parent_object, context));
+}
+
+// Test lastError in multiple different contexts.
+TEST_F(APILastErrorTest, MultipleContexts) {
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context_a = ContextLocal();
+  v8::Local<v8::Context> context_b = v8::Context::New(isolate());
+  gin::ContextHolder holder_b(isolate());
+  holder_b.SetContext(context_b);
+
+  v8::Local<v8::Object> parent_a = v8::Object::New(isolate());
+  v8::Local<v8::Object> parent_b = v8::Object::New(isolate());
+  ParentList parents = {{context_a, parent_a}, {context_b, parent_b}};
+  APILastError last_error(base::Bind(&GetParent, parents));
+
+  last_error.SetError(context_a, "Last error a");
+  EXPECT_EQ("\"Last error a\"", GetLastError(parent_a, context_a));
+  EXPECT_EQ("undefined", GetLastError(parent_b, context_b));
+
+  last_error.SetError(context_b, "Last error b");
+  EXPECT_EQ("\"Last error a\"", GetLastError(parent_a, context_a));
+  EXPECT_EQ("\"Last error b\"", GetLastError(parent_b, context_b));
+
+  last_error.ClearError(context_b, false);
+  EXPECT_EQ("\"Last error a\"", GetLastError(parent_a, context_a));
+  EXPECT_EQ("undefined", GetLastError(parent_b, context_b));
+
+  last_error.ClearError(context_a, false);
+  EXPECT_EQ("undefined", GetLastError(parent_a, context_a));
+  EXPECT_EQ("undefined", GetLastError(parent_b, context_b));
+}
+
+}  // namespace extensions