[Extensions Bindings] Add an end-to-end test for the fileSystem API

Add an end-to-end test for the fileSystem API, which exercises a few
different niche properties:
a) It has an unusual isInstanceOf property (Entry)
b) It hacks up the arguments with a setUpdateArgumentsPostValidate hook
c) It uses some craziness to bind file entries to the background page

a) is already implemented, but this adds an end-to-end test for it.
b) required updating logic to no longer parse and validate arguments
after a post-update hook. This is unfortunate (see comments for why),
but a number of APIs do this, so for now, we have to support it. Add
a few comments lamenting the state, and a test to ensure it doesn't
break.
c) required updating the util js module to not require the method from
the API module's custom bindings, since an API instantiated through a
require() won't have the apiBridge passed in. APIs should be instatiated
through the lazy get; utils should be require()d.

BUG=653596

Review-Url: https://codereview.chromium.org/2793033002
Cr-Commit-Position: refs/heads/master@{#461976}

chrome/browser/extensions/native_bindings_apitest.cc

@@ -5,6 +5,7 @@
 #include "base/command_line.h"
 #include "base/run_loop.h"
 #include "chrome/browser/extensions/api/extension_action/extension_action_api.h"
+#include "chrome/browser/extensions/api/file_system/file_system_api.h"
 #include "chrome/browser/extensions/extension_action.h"
 #include "chrome/browser/extensions/extension_action_manager.h"
 #include "chrome/browser/extensions/extension_apitest.h"
@@ -105,4 +106,17 @@ IN_PROC_BROWSER_TEST_F(NativeBindingsApiTest, LazyListeners) {
                                                       "tabs.onCreated"));
 }
 
+// End-to-end test for the fileSystem API, which includes parameters with
+// instance-of requirements and a post-validation argument updater that violates
+// the schema.
+IN_PROC_BROWSER_TEST_F(NativeBindingsApiTest, FileSystemApiGetDisplayPath) {
+  base::FilePath test_dir = test_data_dir_.AppendASCII("native_bindings");
+  FileSystemChooseEntryFunction::RegisterTempExternalFileSystemForTest(
+      "test_root", test_dir);
+  base::FilePath test_file = test_dir.AppendASCII("text.txt");
+  FileSystemChooseEntryFunction::SkipPickerAndAlwaysSelectPathForTest(
+      &test_file);
+  ASSERT_TRUE(RunPlatformAppTest("native_bindings/instance_of")) << message_;
+}
+
 }  // namespace extensions

chrome/renderer/resources/extensions/file_entry_binding_util.js

@@ -109,8 +109,9 @@ function getFileBindingsForApi(apiName) {
     // backgroundPageModuleSystem.require('fileSystem') is insufficient as
     // requireNative is only allowed while lazily loading an API.
     backgroundPage.chrome.fileSystem;
-    var bindFileEntryCallback = backgroundPageModuleSystem.require(
-        apiName).bindFileEntryCallback;
+    var bindFileEntryCallback =
+        backgroundPageModuleSystem.require('fileEntryBindingUtil')
+            .getFileBindingsForApi(apiName).bindFileEntryCallback;
     var entryIdManager = backgroundPageModuleSystem.require('entryIdManager');
   }
   return {bindFileEntryCallback: bindFileEntryCallback,

chrome/renderer/resources/extensions/file_system_custom_bindings.js

@@ -4,9 +4,10 @@
 
 // Custom binding for the fileSystem API.
 
-var binding = require('binding').Binding.create('fileSystem');
-var sendRequest = require('sendRequest');
-
+var binding = apiBridge || require('binding').Binding.create('fileSystem');
+var sendRequest = bindingUtil ?
+    bindingUtil.sendRequest.bind(bindingUtil) :
+    require('sendRequest').sendRequest;
 var getFileBindingsForApi =
     require('fileEntryBindingUtil').getFileBindingsForApi;
 var fileBindings = getFileBindingsForApi('fileSystem');
@@ -42,8 +43,8 @@ binding.registerCustomHook(function(bindingsAPI) {
     var fileSystemName = fileEntry.filesystem.name;
     var relativePath = $String.slice(fileEntry.fullPath, 1);
 
-    sendRequest.sendRequest(this.name, [id, fileSystemName, relativePath],
-      this.definition.parameters);
+    sendRequest('fileSystem.retainEntry', [id, fileSystemName, relativePath],
+                bindingUtil ? undefined : this.definition.parameters);
     return id;
   });
 
@@ -53,8 +54,8 @@ binding.registerCustomHook(function(bindingsAPI) {
     if (savedEntry) {
       safeCallbackApply('fileSystem.isRestorable', {}, callback, [true]);
     } else {
-      sendRequest.sendRequest(
-          this.name, [id, callback], this.definition.parameters);
+      sendRequest('fileSystem.isRestorable', [id, callback],
+                  bindingUtil ? undefined : this.definition.parameters);
     }
   });
 
@@ -104,5 +105,5 @@ binding.registerCustomHook(function(bindingsAPI) {
   };
 });
 
-exports.$set('bindFileEntryCallback', bindFileEntryCallback);
-exports.$set('binding', binding.generate());
+if (!apiBridge)
+  exports.$set('binding', binding.generate());

chrome/test/data/extensions/api_test/native_bindings/instance_of/background.js

@@ -0,0 +1,7 @@
+// 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.
+
+chrome.app.runtime.onLaunched.addListener(function () {
+  chrome.app.window.create('test.html');
+});

chrome/test/data/extensions/api_test/native_bindings/instance_of/manifest.json

@@ -0,0 +1,11 @@
+{
+  "name": "instanceof test",
+  "version": "1",
+  "description": "instanceof test",
+  "permissions": ["fileSystem"],
+  "app": {
+    "background": {
+      "scripts": ["background.js"]
+    }
+  }
+}

chrome/test/data/extensions/api_test/native_bindings/instance_of/test.html

@@ -0,0 +1,3 @@
+<html>
+<script src="test.js"></script>
+</html>

chrome/test/data/extensions/api_test/native_bindings/instance_of/test.js

@@ -0,0 +1,17 @@
+// 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.
+
+chrome.test.runTests([
+  function getDisplayPath() {
+    chrome.fileSystem.chooseEntry(function(entry) {
+      chrome.test.assertEq('text.txt', entry.name);
+      // Test that we can get the display path of the file.
+      chrome.fileSystem.getDisplayPath(entry, function(path) {
+        chrome.test.assertTrue(path.indexOf('native_bindings') >= 0);
+        chrome.test.assertTrue(path.indexOf('text.txt') >= 0);
+        chrome.test.succeed();
+      });
+    });
+  },
+]);

chrome/test/data/extensions/api_test/native_bindings/text.txt

@@ -0,0 +1 @@
+Text

extensions/renderer/api_binding.cc

@@ -484,6 +484,7 @@ void APIBinding::HandleCall(const std::string& name,
 
   bool invalid_invocation = false;
   v8::Local<v8::Function> custom_callback;
+  bool updated_args = false;
   {
     v8::TryCatch try_catch(isolate);
     APIBindingHooks::RequestResult hooks_result = binding_hooks_->RunHooks(
@@ -502,6 +503,8 @@ void APIBinding::HandleCall(const std::string& name,
         if (!hooks_result.return_value.IsEmpty())
           arguments->Return(hooks_result.return_value);
         return;  // Our work here is done.
+      case APIBindingHooks::RequestResult::ARGUMENTS_UPDATED:
+        updated_args = true;  // Intentional fall-through.
       case APIBindingHooks::RequestResult::NOT_HANDLED:
         break;  // Handle in the default manner.
     }
@@ -517,9 +520,29 @@ void APIBinding::HandleCall(const std::string& name,
   v8::Local<v8::Function> callback;
   {
     v8::TryCatch try_catch(isolate);
-    invalid_invocation = !signature->ParseArgumentsToJSON(
-        context, argument_list, *type_refs_,
-        &converted_arguments, &callback, &error);
+
+    // If custom hooks updated the arguments post-validation, we just trust the
+    // values the hooks provide and convert them directly. This is because some
+    // APIs have one set of values they use for validation, and a second they
+    // use in the implementation of the function (see, e.g.
+    // fileSystem.getDisplayPath).
+    // TODO(devlin): That's unfortunate. Not only does it require special casing
+    // here, but it also means we can't auto-generate the params for the
+    // function on the browser side.
+    if (updated_args) {
+      bool success = signature->ConvertArgumentsIgnoringSchema(
+          context, argument_list, &converted_arguments, &callback);
+      if (!success) {
+        // Converted arguments passed to us by our bindings should never fail.
+        NOTREACHED();
+        return;
+      }
+    } else {
+      invalid_invocation = !signature->ParseArgumentsToJSON(
+          context, argument_list, *type_refs_, &converted_arguments, &callback,
+          &error);
+    }
+
     if (try_catch.HasCaught()) {
       DCHECK(!converted_arguments);
       try_catch.ReThrow();

extensions/renderer/api_binding_hooks.cc

@@ -291,16 +291,22 @@ APIBindingHooks::RequestResult APIBindingHooks::RunHooks(
     arguments->swap(parsed_v8_args);
   }
 
+  bool updated_args = false;
   if (!post_validate_hook.IsEmpty()) {
+    updated_args = true;
     UpdateArguments(post_validate_hook, context, arguments);
     if (try_catch.HasCaught()) {
       try_catch.ReThrow();
       return RequestResult(RequestResult::THROWN);
     }
   }
 
-  if (handle_request.IsEmpty())
-    return RequestResult(RequestResult::NOT_HANDLED, custom_callback);
+  if (handle_request.IsEmpty()) {
+    RequestResult::ResultCode result = updated_args
+                                           ? RequestResult::ARGUMENTS_UPDATED
+                                           : RequestResult::NOT_HANDLED;
+    return RequestResult(result, custom_callback);
+  }
 
   v8::Global<v8::Value> global_result =
       run_js_.Run(handle_request, context, arguments->size(),

extensions/renderer/api_binding_hooks.h

@@ -33,6 +33,7 @@ class APIBindingHooks {
   struct RequestResult {
     enum ResultCode {
       HANDLED,             // A custom hook handled the request.
+      ARGUMENTS_UPDATED,   // The arguments were updated post-validation.
       THROWN,              // An exception was thrown during parsing or
                            // handling.
       INVALID_INVOCATION,  // The request was called with invalid arguments.

extensions/renderer/api_binding_unittest.cc

@@ -1077,6 +1077,40 @@ TEST_F(APIBindingUnittest, TestUpdateArgumentsPostValidate) {
              "['foo',42]", false);
 }
 
+// Tests using setUpdateArgumentsPostValidate to return a list of arguments
+// that violates the function schema. Sadly, this should succeed. :(
+// See comment in api_binding.cc.
+TEST_F(APIBindingUnittest, TestUpdateArgumentsPostValidateViolatingSchema) {
+  // Register a hook for the test.oneString method.
+  auto hooks = base::MakeUnique<APIBindingHooks>(
+      kBindingName, base::Bind(&RunFunctionOnGlobalAndReturnHandle));
+
+  v8::HandleScope handle_scope(isolate());
+  v8::Local<v8::Context> context = MainContext();
+  const char kRegisterHook[] =
+      "(function(hooks) {\n"
+      "  hooks.setUpdateArgumentsPostValidate('oneString', function() {\n"
+      "    return [{}];\n"
+      "  });\n"
+      "})";
+  v8::Local<v8::String> source_string =
+      gin::StringToV8(isolate(), kRegisterHook);
+  v8::Local<v8::String> source_name = gin::StringToV8(isolate(), "custom_hook");
+  hooks->RegisterJsSource(v8::Global<v8::String>(isolate(), source_string),
+                          v8::Global<v8::String>(isolate(), source_name));
+
+  SetHooks(std::move(hooks));
+  SetFunctions(kFunctions);
+  InitializeBinding();
+
+  v8::Local<v8::Object> binding_object =
+      binding()->CreateInstance(context, isolate(), base::Bind(&AllowAllAPIs));
+
+  // Call the method with a valid signature. The hook should be entered and
+  // manipulate the arguments.
+  ExpectPass(binding_object, "obj.oneString('ping');", "[{}]", false);
+}
+
 // Test that user gestures are properly recorded when calling APIs.
 TEST_F(APIBindingUnittest, TestUserGestures) {
   SetFunctions(kFunctions);

extensions/renderer/api_signature.cc

@@ -8,13 +8,23 @@
 
 #include "base/memory/ptr_util.h"
 #include "base/values.h"
+#include "content/public/child/v8_value_converter.h"
 #include "extensions/renderer/argument_spec.h"
 #include "gin/arguments.h"
 
 namespace extensions {
 
 namespace {
 
+bool HasCallback(const std::vector<std::unique_ptr<ArgumentSpec>>& signature) {
+  // TODO(devlin): This is how extension APIs have always determined if a
+  // function has a callback, but it seems a little silly. In the long run (once
+  // signatures are generated), it probably makes sense to indicate this
+  // differently.
+  return !signature.empty() &&
+         signature.back()->type() == ArgumentType::FUNCTION;
+}
+
 // A class to help with argument parsing. Note that this uses v8::Locals and
 // const&s because it's an implementation detail of the APISignature; this
 // should *only* be used directly on the stack!
@@ -59,6 +69,7 @@ class ArgumentParser {
 
   // Adds a null value to the parsed arguments.
   virtual void AddNull() = 0;
+  virtual void AddNullCallback() = 0;
   // Returns a base::Value to be populated during argument matching.
   virtual std::unique_ptr<base::Value>* GetBuffer() = 0;
   // Adds a new parsed argument.
@@ -89,6 +100,9 @@ class V8ArgumentParser : public ArgumentParser {
 
  private:
   void AddNull() override { values_->push_back(v8::Null(GetIsolate())); }
+  void AddNullCallback() override {
+    values_->push_back(v8::Null(GetIsolate()));
+  }
   std::unique_ptr<base::Value>* GetBuffer() override { return nullptr; }
   void AddParsedArgument(v8::Local<v8::Value> value) override {
     values_->push_back(value);
@@ -120,6 +134,10 @@ class BaseValueArgumentParser : public ArgumentParser {
   void AddNull() override {
     list_value_->Append(base::Value::CreateNullValue());
   }
+  void AddNullCallback() override {
+    // The base::Value conversion doesn't include the callback directly, so we
+    // don't add a null parameter here.
+  }
   std::unique_ptr<base::Value>* GetBuffer() override { return &last_arg_; }
   void AddParsedArgument(v8::Local<v8::Value> value) override {
     // The corresponding base::Value is expected to have been stored in
@@ -140,13 +158,7 @@ class BaseValueArgumentParser : public ArgumentParser {
 };
 
 bool ArgumentParser::ParseArguments() {
-  // TODO(devlin): This is how extension APIs have always determined if a
-  // function has a callback, but it seems a little silly. In the long run (once
-  // signatures are generated), it probably makes sense to indicate this
-  // differently.
-  bool signature_has_callback =
-      !signature_.empty() &&
-      signature_.back()->type() == ArgumentType::FUNCTION;
+  bool signature_has_callback = HasCallback(signature_);
 
   size_t end_size =
       signature_has_callback ? signature_.size() - 1 : signature_.size();
@@ -202,6 +214,7 @@ bool ArgumentParser::ParseCallback(const ArgumentSpec& spec) {
       return false;
     }
     ConsumeArgument();
+    AddNullCallback();
     return true;
   }
 
@@ -266,4 +279,43 @@ bool APISignature::ParseArgumentsToJSON(
   return true;
 }
 
+bool APISignature::ConvertArgumentsIgnoringSchema(
+    v8::Local<v8::Context> context,
+    const std::vector<v8::Local<v8::Value>>& arguments,
+    std::unique_ptr<base::ListValue>* json_out,
+    v8::Local<v8::Function>* callback_out) const {
+  size_t size = arguments.size();
+  v8::Local<v8::Function> callback;
+  // TODO(devlin): This is what the current bindings do, but it's quite terribly
+  // incorrect. We only hit this flow when an API method has a hook to update
+  // the arguments post-validation, and in some cases, the arguments returned by
+  // that hook do *not* match the signature of the API method (e.g.
+  // fileSystem.getDisplayPath); see also note in api_bindings.cc for why this
+  // is bad. But then here, we *rely* on the signature to determine whether or
+  // not the last parameter is a callback, even though the hooks may not return
+  // the arguments in the signature. This is very broken.
+  if (HasCallback(signature_)) {
+    CHECK(!arguments.empty());
+    if (arguments[size - 1]->IsFunction()) {
+      callback = arguments[size - 1].As<v8::Function>();
+      --size;
+    }
+  }
+
+  auto json = base::MakeUnique<base::ListValue>();
+  std::unique_ptr<content::V8ValueConverter> converter(
+      content::V8ValueConverter::create());
+  for (size_t i = 0; i < size; ++i) {
+    std::unique_ptr<base::Value> converted =
+        converter->FromV8Value(arguments[i], context);
+    if (!converted)
+      return false;
+    json->Append(std::move(converted));
+  }
+
+  *json_out = std::move(json);
+  *callback_out = callback;
+  return true;
+}
+
 }  // namespace extensions

extensions/renderer/api_signature.h

@@ -50,6 +50,15 @@ class APISignature {
                             v8::Local<v8::Function>* callback_out,
                             std::string* error) const;
 
+  // Converts |arguments| to a base::ListValue, ignoring the defined signature.
+  // This is used when custom bindings modify the passed arguments to a form
+  // that doesn't match the documented signature.
+  bool ConvertArgumentsIgnoringSchema(
+      v8::Local<v8::Context> context,
+      const std::vector<v8::Local<v8::Value>>& arguments,
+      std::unique_ptr<base::ListValue>* json_out,
+      v8::Local<v8::Function>* callback_out) const;
+
  private:
   // The list of expected arguments.
   std::vector<std::unique_ptr<ArgumentSpec>> signature_;