Skip to content

GitLab

T
tangled
Commit f686a1ff authored by Devlin Cronin's avatar Devlin Cronin Committed by Commit Bot
Browse files
Options

[Extensions Bindings] Force API creation in the owning context 

API bindings are created lazily on first-access. Because of this, the
active context may not be the context to which the API belongs. For
instance, a child frame may instantiate an API on its parent frame, so
that the owning context is the parent frame, but the current context (at
binding instantiation time) is the child frame.

This is problematic if we create objects during the binding
instantiation (such as the APIBindingBridge), because methods called on
these objects will be executed in the context that was active when they
were created. This means that the APIBindingBridge object, for instance,
could belong to a different context than the Chrome API for which it
was instantiated.

In addition to being strange, this can cause a crash in the case that
the child context was not fully registered with the extension system. If
the context is not registered, the C++ code cannot safely run JS code in
that context, which is necessary for API instantiation with JS custom
hooks.  Additionally, the calling context, though not fully registered,
will not be marked as invalid because it isn't scheduled for release.

Contexts can be active, but not registered with the extension system, in
the case when the registration was deferred by the ExtensionFrameHelper
because of a pending browser navigation.

This can eventually lead to a crash when we try to execute JS through
the extension JSRunner for the uninitialized context.

To fix this, force API binding construction to happen with the owning
context as the active context. In this case, the context should always
have been initialized (because otherwise we would not have set up the
hooks for API instantiation).

Bug: 819968

Change-Id: I09890560ca92743ebc855561c9119e625e61e410
Reviewed-on: https://chromium-review.googlesource.com/978791
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: default avatarIstiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546043}
parent 6e4e4a05
Hide whitespace changes
Inline Side-by-side
Showing with 106 additions and 1 deletion
chrome/browser/extensions/native_bindings_apitest.cc
View file @ f686a1ff
...@@ -283,4 +283,13 @@ IN_PROC_BROWSER_TEST_F(NativeBindingsApiTest, WebUIBindings) { ...@@ -283,4 +283,13 @@ IN_PROC_BROWSER_TEST_F(NativeBindingsApiTest, WebUIBindings) {
EXPECT_FALSE(api_exists("chrome.browserAction")); EXPECT_FALSE(api_exists("chrome.browserAction"));
} }
// Tests creating an API from a context that hasn't been initialized yet
// by doing so in a parent frame. Regression test for https://crbug.com/819968.
IN_PROC_BROWSER_TEST_F(NativeBindingsApiTest, APICreationFromNewContext) {
embedded_test_server()->ServeFilesFromDirectory(test_data_dir_);
ASSERT_TRUE(StartEmbeddedTestServer());
ASSERT_TRUE(RunExtensionTest("native_bindings/context_initialization"))
<< message_;
}
} // namespace extensions } // namespace extensions
chrome/test/data/extensions/api_test/native_bindings/context_initialization/background.js 0 → 100644
View file @ f686a1ff
// Copyright 2018 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.
let i = document.createElement('iframe');
i.src = chrome.runtime.getURL('iframe.html');
document.body.appendChild(i);
// Before the child frame has a chance to load (and thus before the
// ScriptContext is initialized), execute code in the child frame that
// initializes an API in the main frame (this frame). The contextMenus API
// requires JS initialization, which requires a JSRunner be instantiated for
// the context. If the new context (which is uninitialized and has no JSRunner)
// is used, this will fail. Instead, it should execute in this frame's context.
// See https://crbug.com/819968 for more details.
// IMPORTANT: For this test to be valid, the API being initialized (here,
// contextMenus), must have custom JS bindings.
i.contentWindow.eval('parent.chrome.contextMenus;');
chrome.test.notifyPass();
chrome/test/data/extensions/api_test/native_bindings/context_initialization/manifest.json 0 → 100644
View file @ f686a1ff
{
"name": "Extension test",
"manifest_version": 2,
"background": {"scripts": ["background.js"]},
"version": "0.1",
"permissions": ["storage", "contextMenus"],
// Note: we have to allow unsafe-eval here for the test.
"content_security_policy": "script-src 'self' 'unsafe-eval'; object-src 'self';"
}
extensions/renderer/native_extension_bindings_system.cc
View file @ f686a1ff
...@@ -575,6 +575,12 @@ void NativeExtensionBindingsSystem::BindingAccessor( ...@@ -575,6 +575,12 @@ void NativeExtensionBindingsSystem::BindingAccessor(
v8::HandleScope handle_scope(isolate); v8::HandleScope handle_scope(isolate);
v8::Local<v8::Context> context = info.Holder()->CreationContext(); v8::Local<v8::Context> context = info.Holder()->CreationContext();
// Force binding creation in the owning context (even if another context is
// calling in). This is also important to ensure that objects created through
// the initialization process are all instantiated for the owning context.
// See https://crbug.com/819968.
v8::Context::Scope context_scope(context);
// We use info.Data() to store a real name here instead of using the provided // We use info.Data() to store a real name here instead of using the provided
// one to handle any weirdness from the caller (non-existent strings, etc). // one to handle any weirdness from the caller (non-existent strings, etc).
v8::Local<v8::String> api_name = info.Data().As<v8::String>(); v8::Local<v8::String> api_name = info.Data().As<v8::String>();
......
extensions/renderer/native_extension_bindings_system_test_base.cc
View file @ f686a1ff
...@@ -93,7 +93,10 @@ void NativeExtensionBindingsSystemUnittest::OnWillDisposeContext( ...@@ -93,7 +93,10 @@ void NativeExtensionBindingsSystemUnittest::OnWillDisposeContext(
[context](ScriptContext* script_context) { [context](ScriptContext* script_context) {
return script_context->v8_context() == context; return script_context->v8_context() == context;
}); });
ASSERT_TRUE(iter != raw_script_contexts_.end()); if (iter == raw_script_contexts_.end()) {
ASSERT_TRUE(allow_unregistered_contexts_);
return;
}
bindings_system_->WillReleaseScriptContext(*iter); bindings_system_->WillReleaseScriptContext(*iter);
script_context_set_->Remove(*iter); script_context_set_->Remove(*iter);
raw_script_contexts_.erase(iter); raw_script_contexts_.erase(iter);
......
extensions/renderer/native_extension_bindings_system_test_base.h
View file @ f686a1ff
...@@ -137,6 +137,9 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest { ...@@ -137,6 +137,9 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
StringSourceMap* source_map() { return &source_map_; } StringSourceMap* source_map() { return &source_map_; }
TestIPCMessageSender* ipc_message_sender() { return ipc_message_sender_; } TestIPCMessageSender* ipc_message_sender() { return ipc_message_sender_; }
ScriptContextSet* script_context_set() { return script_context_set_.get(); } ScriptContextSet* script_context_set() { return script_context_set_.get(); }
void set_allow_unregistered_contexts(bool allow_unregistered_contexts) {
allow_unregistered_contexts_ = allow_unregistered_contexts;
}
private: private:
ExtensionIdSet extension_ids_; ExtensionIdSet extension_ids_;
...@@ -152,6 +155,10 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest { ...@@ -152,6 +155,10 @@ class NativeExtensionBindingsSystemUnittest : public APIBindingTest {
StringSourceMap source_map_; StringSourceMap source_map_;
TestExtensionsRendererClient renderer_client_; TestExtensionsRendererClient renderer_client_;
// True if we allow some v8::Contexts to avoid registration as a
// ScriptContext.
bool allow_unregistered_contexts_ = false;
DISALLOW_COPY_AND_ASSIGN(NativeExtensionBindingsSystemUnittest); DISALLOW_COPY_AND_ASSIGN(NativeExtensionBindingsSystemUnittest);
}; };
......
extensions/renderer/native_extension_bindings_system_unittest.cc
View file @ f686a1ff
...@@ -1068,4 +1068,54 @@ TEST_F(NativeExtensionBindingsSystemUnittest, CanDeleteAPIs) { ...@@ -1068,4 +1068,54 @@ TEST_F(NativeExtensionBindingsSystemUnittest, CanDeleteAPIs) {
EXPECT_TRUE(property->IsUndefined()); EXPECT_TRUE(property->IsUndefined());
} }
// Test that API initialization happens in the owning context.
TEST_F(NativeExtensionBindingsSystemUnittest, APIIsInitializedByOwningContext) {
// Attach custom JS hooks.
const char kCustomBinding[] =
R"(this.apiBridge = apiBridge;
apiBridge.registerCustomHook(() => {});)";
source_map()->RegisterModule("idle", kCustomBinding);
scoped_refptr<Extension> extension =
ExtensionBuilder("foo").AddPermission("idle").Build();
RegisterExtension(extension);
v8::HandleScope handle_scope(isolate());
v8::Local<v8::Context> context = MainContext();
ScriptContext* script_context = CreateScriptContext(
context, extension.get(), Feature::BLESSED_EXTENSION_CONTEXT);
script_context->set_url(extension->url());
bindings_system()->UpdateBindingsForContext(script_context);
{
// Create a second, uninitialized context, which will trigger the
// construction of chrome.idle in the first context.
set_allow_unregistered_contexts(true);
v8::Local<v8::Context> second_context = AddContext();
v8::Local<v8::Function> get_idle = FunctionFromString(
second_context, "(function(chrome) { chrome.idle; })");
v8::Local<v8::Value> chrome =
context->Global()
->Get(context, gin::StringToV8(isolate(), "chrome"))
.ToLocalChecked();
ASSERT_TRUE(chrome->IsObject());
v8::Context::Scope context_scope(second_context);
v8::Local<v8::Value> args[] = {chrome};
RunFunction(get_idle, second_context, arraysize(args), args);
}
// The apiBridge should have been created in the owning (original) context,
// even though the initialization was triggered by the second context.
v8::Local<v8::Value> api_bridge =
context->Global()
->Get(context, gin::StringToV8(isolate(), "apiBridge"))
.ToLocalChecked();
ASSERT_TRUE(api_bridge->IsObject());
EXPECT_EQ(context, api_bridge.As<v8::Object>()->CreationContext());
}
} // namespace extensions } // namespace extensions
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment