From 577b8f369864082950e5922472780e54317ea2ac Mon Sep 17 00:00:00 2001 From: "rdevlin.cronin" Date: Tue, 4 Apr 2017 21:20:44 -0700 Subject: [PATCH] [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} --- .../extensions/native_bindings_apitest.cc | 14 ++++ .../extensions/file_entry_binding_util.js | 5 +- .../extensions/file_system_custom_bindings.js | 19 +++--- .../native_bindings/instance_of/background.js | 7 ++ .../native_bindings/instance_of/manifest.json | 11 ++++ .../native_bindings/instance_of/test.html | 3 + .../native_bindings/instance_of/test.js | 17 +++++ .../api_test/native_bindings/text.txt | 1 + extensions/renderer/api_binding.cc | 29 +++++++- extensions/renderer/api_binding_hooks.cc | 10 ++- extensions/renderer/api_binding_hooks.h | 1 + extensions/renderer/api_binding_unittest.cc | 34 ++++++++++ extensions/renderer/api_signature.cc | 66 +++++++++++++++++-- extensions/renderer/api_signature.h | 9 +++ 14 files changed, 203 insertions(+), 23 deletions(-) create mode 100644 chrome/test/data/extensions/api_test/native_bindings/instance_of/background.js create mode 100644 chrome/test/data/extensions/api_test/native_bindings/instance_of/manifest.json create mode 100644 chrome/test/data/extensions/api_test/native_bindings/instance_of/test.html create mode 100644 chrome/test/data/extensions/api_test/native_bindings/instance_of/test.js create mode 100644 chrome/test/data/extensions/api_test/native_bindings/text.txt diff --git a/chrome/browser/extensions/native_bindings_apitest.cc b/chrome/browser/extensions/native_bindings_apitest.cc index 98aa030ec2600d..285a811ea40092 100644 --- a/chrome/browser/extensions/native_bindings_apitest.cc +++ b/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 diff --git a/chrome/renderer/resources/extensions/file_entry_binding_util.js b/chrome/renderer/resources/extensions/file_entry_binding_util.js index ddb1828cfb7cf9..e6c477235598a6 100644 --- a/chrome/renderer/resources/extensions/file_entry_binding_util.js +++ b/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, diff --git a/chrome/renderer/resources/extensions/file_system_custom_bindings.js b/chrome/renderer/resources/extensions/file_system_custom_bindings.js index 431ffd21e12991..e8ed00cde0657a 100644 --- a/chrome/renderer/resources/extensions/file_system_custom_bindings.js +++ b/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()); diff --git a/chrome/test/data/extensions/api_test/native_bindings/instance_of/background.js b/chrome/test/data/extensions/api_test/native_bindings/instance_of/background.js new file mode 100644 index 00000000000000..ac575df3480401 --- /dev/null +++ b/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'); +}); diff --git a/chrome/test/data/extensions/api_test/native_bindings/instance_of/manifest.json b/chrome/test/data/extensions/api_test/native_bindings/instance_of/manifest.json new file mode 100644 index 00000000000000..ddbcec2d41b3b9 --- /dev/null +++ b/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"] + } + } +} diff --git a/chrome/test/data/extensions/api_test/native_bindings/instance_of/test.html b/chrome/test/data/extensions/api_test/native_bindings/instance_of/test.html new file mode 100644 index 00000000000000..8d7d1db7db260d --- /dev/null +++ b/chrome/test/data/extensions/api_test/native_bindings/instance_of/test.html @@ -0,0 +1,3 @@ + + + diff --git a/chrome/test/data/extensions/api_test/native_bindings/instance_of/test.js b/chrome/test/data/extensions/api_test/native_bindings/instance_of/test.js new file mode 100644 index 00000000000000..39587d334e1186 --- /dev/null +++ b/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(); + }); + }); + }, +]); diff --git a/chrome/test/data/extensions/api_test/native_bindings/text.txt b/chrome/test/data/extensions/api_test/native_bindings/text.txt new file mode 100644 index 00000000000000..3de705a41ff24b --- /dev/null +++ b/chrome/test/data/extensions/api_test/native_bindings/text.txt @@ -0,0 +1 @@ +Text diff --git a/extensions/renderer/api_binding.cc b/extensions/renderer/api_binding.cc index 5ae6d9aa547c1b..933c87b0ccca9b 100644 --- a/extensions/renderer/api_binding.cc +++ b/extensions/renderer/api_binding.cc @@ -484,6 +484,7 @@ void APIBinding::HandleCall(const std::string& name, bool invalid_invocation = false; v8::Local 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 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(); diff --git a/extensions/renderer/api_binding_hooks.cc b/extensions/renderer/api_binding_hooks.cc index b284071f775299..260c59d1d9740b 100644 --- a/extensions/renderer/api_binding_hooks.cc +++ b/extensions/renderer/api_binding_hooks.cc @@ -291,7 +291,9 @@ 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(); @@ -299,8 +301,12 @@ APIBindingHooks::RequestResult APIBindingHooks::RunHooks( } } - 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 global_result = run_js_.Run(handle_request, context, arguments->size(), diff --git a/extensions/renderer/api_binding_hooks.h b/extensions/renderer/api_binding_hooks.h index 80c7eb2a4a2d5e..a9a3d29a445448 100644 --- a/extensions/renderer/api_binding_hooks.h +++ b/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. diff --git a/extensions/renderer/api_binding_unittest.cc b/extensions/renderer/api_binding_unittest.cc index a595717cada6d0..d3d4675e48a28a 100644 --- a/extensions/renderer/api_binding_unittest.cc +++ b/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( + kBindingName, base::Bind(&RunFunctionOnGlobalAndReturnHandle)); + + v8::HandleScope handle_scope(isolate()); + v8::Local context = MainContext(); + const char kRegisterHook[] = + "(function(hooks) {\n" + " hooks.setUpdateArgumentsPostValidate('oneString', function() {\n" + " return [{}];\n" + " });\n" + "})"; + v8::Local source_string = + gin::StringToV8(isolate(), kRegisterHook); + v8::Local source_name = gin::StringToV8(isolate(), "custom_hook"); + hooks->RegisterJsSource(v8::Global(isolate(), source_string), + v8::Global(isolate(), source_name)); + + SetHooks(std::move(hooks)); + SetFunctions(kFunctions); + InitializeBinding(); + + v8::Local 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); diff --git a/extensions/renderer/api_signature.cc b/extensions/renderer/api_signature.cc index a76d51bf9f3a56..733ec8ed491c9c 100644 --- a/extensions/renderer/api_signature.cc +++ b/extensions/renderer/api_signature.cc @@ -8,6 +8,7 @@ #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" @@ -15,6 +16,15 @@ namespace extensions { namespace { +bool HasCallback(const std::vector>& 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* 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* GetBuffer() override { return nullptr; } void AddParsedArgument(v8::Local 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* GetBuffer() override { return &last_arg_; } void AddParsedArgument(v8::Local 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 context, + const std::vector>& arguments, + std::unique_ptr* json_out, + v8::Local* callback_out) const { + size_t size = arguments.size(); + v8::Local 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(); + --size; + } + } + + auto json = base::MakeUnique(); + std::unique_ptr converter( + content::V8ValueConverter::create()); + for (size_t i = 0; i < size; ++i) { + std::unique_ptr 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 diff --git a/extensions/renderer/api_signature.h b/extensions/renderer/api_signature.h index bbcb81e7e5654e..e89291df2072f1 100644 --- a/extensions/renderer/api_signature.h +++ b/extensions/renderer/api_signature.h @@ -50,6 +50,15 @@ class APISignature { v8::Local* 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 context, + const std::vector>& arguments, + std::unique_ptr* json_out, + v8::Local* callback_out) const; + private: // The list of expected arguments. std::vector> signature_;