[Extensions Bindings] Add native declarativeContent verification

chrome/renderer/resources/extensions/declarative_content_custom_bindings.js

 // Copyright (c) 2012 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.
 
 // Custom binding for the declarativeContent API.
 
 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,
                         'id',
                         'declarativeContent.' + typeId);
   }
 
   // Helper function for the constructor of concrete datatypes of the
   // declarative content API.
   // Makes sure that |this| contains the union of parameters and
   // {'instanceType': 'declarativeContent.' + typeId} and validates the
   // generated union dictionary against the schema for |typeId|.
   function setupInstance(instance, parameters, typeId) {
     for (var key in parameters) {
       if ($Object.hasOwnProperty(parameters, key)) {
         instance[key] = parameters[key];
       }
     }
     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) {
     for (var i = 0; i < selectors.length; i++) {
       var canonicalizedSelector = canonicalizeCompoundSelector(selectors[i]);
       if (canonicalizedSelector == '') {
         throw new Error(
             'Element of \'css\' array must be a ' +
             'list of valid compound selectors: ' +
             selectors[i]);
@@ skipping 10 matching lines @@
     }
   };
   declarativeContent.ShowPageAction = function(parameters) {
     setupInstance(this, parameters, 'ShowPageAction');
   };
   declarativeContent.RequestContentScript = function(parameters) {
     setupInstance(this, parameters, 'RequestContentScript');
   };
   // 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));
   };
 });
 
 if (!apiBridge)
   exports.$set('binding', binding.generate());