[Extensions Bindings] Add native declarativeContent verification

chrome/renderer/resources/extensions/declarative_content_custom_bindings.js

Index: chrome/renderer/resources/extensions/declarative_content_custom_bindings.js
diff --git a/chrome/renderer/resources/extensions/declarative_content_custom_bindings.js b/chrome/renderer/resources/extensions/declarative_content_custom_bindings.js
index 8f5612432442933d61ef9c68cb87c61f07907acc..06fb1574bcb950461fd6737102ce26893732b11d 100644
--- a/chrome/renderer/resources/extensions/declarative_content_custom_bindings.js
+++ b/chrome/renderer/resources/extensions/declarative_content_custom_bindings.js
@@ -7,15 +7,43 @@
 var binding =
     apiBridge || require('binding').Binding.create('declarativeContent');
 
-var utils = require('utils');
-var validate = require('schemaUtils').validate;
-var canonicalizeCompoundSelector =
-    requireNative('css_natives').CanonicalizeCompoundSelector;
+if (!apiBridge) {
+  var utils = require('utils');
+  var validate = require('schemaUtils').validate;
+  var canonicalizeCompoundSelector =
+      requireNative('css_natives').CanonicalizeCompoundSelector;
+}
+
 var setIcon = require('setIcon').setIcon;
 
 binding.registerCustomHook(function(api) {
   var declarativeContent = api.compiledApi;
 
+  if (apiBridge) {
+    // Validation for most types is done in the native C++ with native bindings,
+    // but setIcon is funny (and sadly broken). Ideally, we can move this
+    // validation entirely into the native code, and this whole file can go
+    // away.
+    var nativeSetIcon = declarativeContent.SetIcon;
+    declarativeContent.SetIcon = function(parameters) {
+      // TODO(devlin): This is very, very wrong. setIcon() is potentially
+      // asynchronous (in the case of a path being specified), which means this
+      // becomes an "asynchronous constructor". Errors can be thrown *after* the
+      // `new declarativeContent.SetIcon(...)` call, and in the async cases,
+      // this wouldn't work when we immediately add the action via an API call
+      // (e.g.,
+      //   chrome.declarativeContent.onPageChange.addRules(
+      //       [{conditions: ..., actions: [ new SetIcon(...) ]}]);
+      // ). Some of this is tracked in http://crbug.com/415315.
+      setIcon(parameters, $Function.bind(function (data) {
+        // Fake calling the original function as a constructor.

[jbroman, 2017/05/02 18:36:33]

Yeah, eww. Can't think of something better, short of the longer-term solution
you talk about here.

Naturally this isn't quite perfect emulation (e.g. new.target or IsConstructCall
will give it away, as with the fact that we're ignoring the return value), but
it's probably good enough in the meantime.

[Devlin, 2017/05/02 20:51:43]

Yep. :/  See also the note in declarative_content_hooks_delegate about wishing
we could use IsConstructCall.

+        this.__proto__ = nativeSetIcon.prototype;

[jbroman, 2017/05/02 18:36:33]

This can run author script, like this:

Object.defineProperty(chrome.declarativeContent.SetIcon.prototype, '__proto__',
                      { set() { console.error('ran author script!'); } });

I think Object.setPrototypeOf (which is in safe builtins already as
$Object.setPrototypeOf) is the proper workaround. Not sure if this issue occurs
elsewhere in the extension JS bindings.

[Devlin, 2017/05/02 20:51:43]

Wild.  Wouldn't have expected that.  Fixed; thanks for catching it.

+        $Function.apply(nativeSetIcon, this, [data]);
+      }, this));
+    };
+    return;
+  }
+
   // Returns the schema definition of type |typeId| defined in |namespace|.
   function getSchema(typeId) {
     return utils.lookup(api.schema.types,
@@ -35,16 +63,8 @@ binding.registerCustomHook(function(api) {
       }
     }
     instance.instanceType = 'declarativeContent.' + typeId;
-    // TODO(devlin): This is wrong. It means we don't validate the construction
-    // of the instance (which really only matters for PageStateMatcher).
-    // Currently, we don't pass the schema to JS with native bindings because
-    // validation should be done natively. We'll need to fix this by either
-    // allowing some validation to occur in JS, or by moving the instantiation
-    // of these types to native code.
-    if (!apiBridge) {
-      var schema = getSchema(typeId);
-      validate([instance], [schema]);
-    }
+    var schema = getSchema(typeId);
+    validate([instance], [schema]);
   }
 
   function canonicalizeCssSelectors(selectors) {
@@ -75,10 +95,11 @@ binding.registerCustomHook(function(api) {
   };
   // TODO(rockot): Do not expose this in M39 stable. Making this restriction
   // possible will take some extra work. See http://crbug.com/415315
+  // Note: See also the SetIcon wrapper above for more issues.
   declarativeContent.SetIcon = function(parameters) {
-    setIcon(parameters, function (data) {
+    setIcon(parameters, $Function.bind(function(data) {

[jbroman, 2017/05/02 18:36:33]

Good catch. :)

       setupInstance(this, data, 'SetIcon');
-    }.bind(this));
+    }, this));
   };
 });