[Extensions Bindings] Support chrome.extension.lastError

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 fbf516b564331e6eb71cedab574b2c33302a92bb..a553ba5d1e56346a61d44632a5ee564ea260ea50 100644
--- a/extensions/renderer/api_last_error.cc
+++ b/extensions/renderer/api_last_error.cc
@@ -5,6 +5,7 @@
 #include "extensions/renderer/api_last_error.h"
 
 #include "gin/converter.h"
+#include "gin/data_object_builder.h"
 #include "gin/handle.h"
 #include "gin/object_template_builder.h"
 #include "gin/wrappable.h"
@@ -132,47 +133,11 @@ void APILastError::SetError(v8::Local<v8::Context> context,
   v8::TryCatch try_catch(isolate);
   try_catch.SetVerbose(true);
 
-  v8::Local<v8::Object> parent = get_parent_.Run(context);
-  if (parent.IsEmpty())
-    return;
-  v8::Local<v8::String> key = gin::StringToSymbol(isolate, kLastErrorProperty);
-  v8::Local<v8::Value> v8_error;
-  // Two notes: this Get() is visible to external script, and this will actually
-  // mark the lastError as accessed, if one exists. These shouldn't be a
-  // problem (lastError is meant to be helpful, but isn't designed to handle
-  // crazy chaining, etc). However, if we decide we needed to be fancier, we
-  // could detect the presence of a current error through a GetPrivate(), and
-  // optionally throw it if one exists.
-  if (!parent->Get(context, key).ToLocal(&v8_error))
-    return;
+  v8::Local<v8::Object> secondary_parent;
+  v8::Local<v8::Object> parent = get_parent_.Run(context, &secondary_parent);
 
-  if (!v8_error->IsUndefined()) {
-    // There may be an existing last error to overwrite.
-    LastErrorObject* last_error = nullptr;
-    if (!gin::Converter<LastErrorObject*>::FromV8(isolate, v8_error,
-                                                  &last_error)) {
-      // If it's not a real lastError (e.g. if a script manually set it), don't
-      // do anything. We shouldn't mangle a property set by other script.
-      // TODO(devlin): Or should we? If someone sets chrome.runtime.lastError,
-      // it might be the right course of action to overwrite it.
-      return;
-    }
-    last_error->Reset(error);
-  } else {
-    v8::Local<v8::Value> last_error =
-        gin::CreateHandle(isolate, new LastErrorObject(error)).ToV8();
-    v8::Maybe<bool> set_private = parent->SetPrivate(
-        context, v8::Private::ForApi(isolate, key), last_error);
-    if (!set_private.IsJust() || !set_private.FromJust()) {
-      NOTREACHED();
-      return;
-    }
-    DCHECK(!last_error.IsEmpty());
-    // This Set() can fail, but there's nothing to do if it does (the exception
-    // will be caught by the TryCatch above).
-    ignore_result(parent->SetAccessor(context, key, &LastErrorGetter,
-                                      &LastErrorSetter, last_error));
-  }
+  SetErrorOnPrimaryParent(context, parent, error);
+  SetErrorOnSecondaryParent(context, secondary_parent, error);
 }
 
 void APILastError::ClearError(v8::Local<v8::Context> context,
@@ -181,6 +146,7 @@ void APILastError::ClearError(v8::Local<v8::Context> context,
   v8::HandleScope handle_scope(isolate);
 
   v8::Local<v8::Object> parent;
+  v8::Local<v8::Object> secondary_parent;
   LastErrorObject* last_error = nullptr;
   v8::Local<v8::String> key;
   v8::Local<v8::Private> private_key;
@@ -189,16 +155,15 @@ void APILastError::ClearError(v8::Local<v8::Context> context,
     v8::TryCatch try_catch(isolate);
     try_catch.SetVerbose(true);
 
-    parent = get_parent_.Run(context);
+    parent = get_parent_.Run(context, &secondary_parent);

[lazyboy, 2017/06/27 17:58:44]

It feels a bit odd to return two parents differently from GetParent: one through
return value and the other through a pointer arg. Returning both through pointer
arg seems consistent,  up to you.

[Devlin, 2017/06/28 17:52:01]

I have a slight preference for this, since the primary parent should almost
never be null, whereas the secondary parent might be.  If it's alright, I'd like
to keep it as-is.

     if (parent.IsEmpty())
       return;
     key = gin::StringToSymbol(isolate, kLastErrorProperty);
     private_key = v8::Private::ForApi(isolate, key);
     v8::Local<v8::Value> error;
     // Access through GetPrivate() so that we don't trigger accessed().
-    if (!parent->GetPrivate(context, private_key).ToLocal(&error))
-      return;
-    if (!gin::Converter<LastErrorObject*>::FromV8(context->GetIsolate(), error,
+    if (!parent->GetPrivate(context, private_key).ToLocal(&error) ||
+        !gin::Converter<LastErrorObject*>::FromV8(context->GetIsolate(), error,
                                                   &last_error)) {
       return;
     }
@@ -218,9 +183,11 @@ void APILastError::ClearError(v8::Local<v8::Context> context,
     NOTREACHED();
     return;
   }
-  // This Delete() can fail, but there's nothing to do if it does (the exception
-  // will be caught by the TryCatch above).
+  // These Delete()s can fail, but there's nothing to do if it does (the
+  // exception will be caught by the TryCatch above).
   ignore_result(parent->Delete(context, key));
+  if (!secondary_parent.IsEmpty())
+    ignore_result(secondary_parent->Delete(context, key));
 }
 
 bool APILastError::HasError(v8::Local<v8::Context> context) {
@@ -231,7 +198,7 @@ bool APILastError::HasError(v8::Local<v8::Context> context) {
   v8::TryCatch try_catch(isolate);
   try_catch.SetVerbose(true);
 
-  v8::Local<v8::Object> parent = get_parent_.Run(context);
+  v8::Local<v8::Object> parent = get_parent_.Run(context, nullptr);
   if (parent.IsEmpty())
     return false;
   v8::Local<v8::Value> error;
@@ -246,4 +213,70 @@ bool APILastError::HasError(v8::Local<v8::Context> context) {
                                                   &last_error);
 }
 
+void APILastError::SetErrorOnPrimaryParent(v8::Local<v8::Context> context,
+                                           v8::Local<v8::Object> parent,
+                                           const std::string& error) {
+  if (parent.IsEmpty())

[Devlin, 2017/06/24 01:24:17]

Note: copy-paste from above.

+    return;
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::Local<v8::String> key = gin::StringToSymbol(isolate, kLastErrorProperty);
+  v8::Local<v8::Value> v8_error;
+  // Two notes: this Get() is visible to external script, and this will actually
+  // mark the lastError as accessed, if one exists. These shouldn't be a
+  // problem (lastError is meant to be helpful, but isn't designed to handle
+  // crazy chaining, etc). However, if we decide we needed to be fancier, we
+  // could detect the presence of a current error through a GetPrivate(), and
+  // optionally throw it if one exists.
+  if (!parent->Get(context, key).ToLocal(&v8_error))
+    return;
+
+  if (!v8_error->IsUndefined()) {
+    // There may be an existing last error to overwrite.
+    LastErrorObject* last_error = nullptr;
+    if (!gin::Converter<LastErrorObject*>::FromV8(isolate, v8_error,
+                                                  &last_error)) {
+      // If it's not a real lastError (e.g. if a script manually set it), don't
+      // do anything. We shouldn't mangle a property set by other script.
+      // TODO(devlin): Or should we? If someone sets chrome.runtime.lastError,
+      // it might be the right course of action to overwrite it.
+      return;
+    }
+    last_error->Reset(error);
+  } else {
+    v8::Local<v8::Value> last_error =
+        gin::CreateHandle(isolate, new LastErrorObject(error)).ToV8();
+    v8::Maybe<bool> set_private = parent->SetPrivate(
+        context, v8::Private::ForApi(isolate, key), last_error);
+    if (!set_private.IsJust() || !set_private.FromJust()) {
+      NOTREACHED();
+      return;
+    }
+    DCHECK(!last_error.IsEmpty());
+    // This Set() can fail, but there's nothing to do if it does (the exception

[jbroman, 2017/06/25 00:12:49]

nit: this is SetAccessor, not Set.

[Devlin, 2017/06/28 17:52:01]

Done.

+    // will be caught by the TryCatch above).

[jbroman, 2017/06/25 00:12:49]

Here and below: is "the TryCatch above" the one in APILastError::SetError?

[Devlin, 2017/06/28 17:52:01]

Yep, clarified in the comments.

+    ignore_result(parent->SetAccessor(context, key, &LastErrorGetter,
+                                      &LastErrorSetter, last_error));
+  }
+}
+
+void APILastError::SetErrorOnSecondaryParent(
+    v8::Local<v8::Context> context,
+    v8::Local<v8::Object> secondary_parent,
+    const std::string& error) {
+  if (secondary_parent.IsEmpty())
+    return;
+
+  // For the secondary parent, simply set chrome.extension.lastError to
+  // {message: <error>}.
+  // TODO(devlin): Gather metrics on how frequently this is checked. It'd be
+  // nice to get rid of it.
+  v8::Isolate* isolate = context->GetIsolate();
+  v8::Local<v8::String> key = gin::StringToSymbol(isolate, kLastErrorProperty);
+  // This Set() can fail, but there's nothing to do if it does (the exception
+  // will be caught by the TryCatch above).
+  ignore_result(secondary_parent->CreateDataProperty(

[jbroman, 2017/06/25 00:12:49]

nit: this is CreateDataProperty, not Set.

[Devlin, 2017/06/28 17:52:01]

Done.

+      context, key,
+      gin::DataObjectBuilder(isolate).Set("message", error).Build()));
+}
+
 }  // namespace extensions