Check the tab ID before delivering an extension message.
r335753 changed extension messaging to use RenderFrames, which had the
side-effect of delivering messages to every RenderFrame in a process,
which may include other tabs.
This patch fixes that by sending the target tab ID along with the message. A
more principled fix would have been to track RenderFrames on the browser and
send to precisely the right ones, but this would need to be part of a more
comprehensive refactor.
I also fixed up ExtensionApiTest.Connect and re-enabled, which was disabled
years ago due to flakiness. Hopefully my test JS changes will fix that.
BUG=520303
TBR=rdevlin.cronin@chromium.org, dcheng@chromium.org
NOPRESUBMIT=true
NOTRY=true
Review URL: https://codereview.chromium.org/1318153002
Cr-Commit-Position: refs/heads/master@{#346176}
(cherry picked from commit 1344c7dfde448d4b80693f597b02993b7f6bbd5b)
Review URL: https://codereview.chromium.org/1335743003
Cr-Commit-Position: refs/branch-heads/2454@{#455}
Cr-Branched-From: 12bfc3360892ec53cd00fc239a47e5298beb063b-refs/heads/master@{#338390}
diff --git a/chrome/browser/extensions/api/messaging/extension_message_port.cc b/chrome/browser/extensions/api/messaging/extension_message_port.cc
index 7f0da642..1e4f87f 100644
--- a/chrome/browser/extensions/api/messaging/extension_message_port.cc
+++ b/chrome/browser/extensions/api/messaging/extension_message_port.cc
@@ -27,6 +27,7 @@
const std::string& channel_name,
scoped_ptr<base::DictionaryValue> source_tab,
int source_frame_id,
+ int target_tab_id,
int target_frame_id,
int guest_process_id,
int guest_render_frame_routing_id,
@@ -43,6 +44,7 @@
info.target_id = target_extension_id;
info.source_id = source_extension_id;
info.source_url = source_url;
+ info.target_tab_id = target_tab_id;
info.target_frame_id = target_frame_id;
info.guest_process_id = guest_process_id;
info.guest_render_frame_routing_id = guest_render_frame_routing_id;
diff --git a/chrome/browser/extensions/api/messaging/extension_message_port.h b/chrome/browser/extensions/api/messaging/extension_message_port.h
index 8445bdac..79b8003 100644
--- a/chrome/browser/extensions/api/messaging/extension_message_port.h
+++ b/chrome/browser/extensions/api/messaging/extension_message_port.h
@@ -25,6 +25,7 @@
const std::string& channel_name,
scoped_ptr<base::DictionaryValue> source_tab,
int source_frame_id,
+ int target_tab_id,
int target_frame_id,
int guest_process_id,
int guest_render_frame_routing_id,
diff --git a/chrome/browser/extensions/api/messaging/message_service.cc b/chrome/browser/extensions/api/messaging/message_service.cc
index acef124..357616fe 100644
--- a/chrome/browser/extensions/api/messaging/message_service.cc
+++ b/chrome/browser/extensions/api/messaging/message_service.cc
@@ -125,6 +125,7 @@
int source_process_id;
scoped_ptr<base::DictionaryValue> source_tab;
int source_frame_id;
+ int target_tab_id;
int target_frame_id;
scoped_ptr<MessagePort> receiver;
int receiver_port_id;
@@ -140,6 +141,7 @@
OpenChannelParams(int source_process_id,
scoped_ptr<base::DictionaryValue> source_tab,
int source_frame_id,
+ int target_tab_id,
int target_frame_id,
MessagePort* receiver,
int receiver_port_id,
@@ -151,6 +153,7 @@
bool include_guest_process_info)
: source_process_id(source_process_id),
source_frame_id(source_frame_id),
+ target_tab_id(target_tab_id),
target_frame_id(target_frame_id),
receiver(receiver),
receiver_port_id(receiver_port_id),
@@ -348,8 +351,8 @@
}
scoped_ptr<OpenChannelParams> params(new OpenChannelParams(
- source_process_id, source_tab.Pass(), source_frame_id,
- -1, // no target_frame_id for a channel to an extension/background page.
+ source_process_id, source_tab.Pass(), source_frame_id, -1,
+ -1, // no target_tab_id/target_frame_id for connections to extensions
nullptr, receiver_port_id, source_extension_id, target_extension_id,
source_url, channel_name, include_tls_channel_id,
include_guest_process_info));
@@ -532,11 +535,11 @@
scoped_ptr<base::DictionaryValue>(), // Source tab doesn't make sense
// for opening to tabs.
-1, // If there is no tab, then there is no frame either.
- frame_id,
- receiver.release(), receiver_port_id, extension_id, extension_id,
+ tab_id, frame_id, receiver.release(), receiver_port_id, extension_id,
+ extension_id,
GURL(), // Source URL doesn't make sense for opening to tabs.
channel_name,
- false, // Connections to tabs don't get TLS channel IDs.
+ false, // Connections to tabs don't get TLS channel IDs.
false)); // Connections to tabs aren't webview guests.
OpenChannelImpl(params.Pass());
}
diff --git a/chrome/browser/extensions/api/messaging/message_service.h b/chrome/browser/extensions/api/messaging/message_service.h
index e68b19e..12133819 100644
--- a/chrome/browser/extensions/api/messaging/message_service.h
+++ b/chrome/browser/extensions/api/messaging/message_service.h
@@ -73,6 +73,7 @@
const std::string& channel_name,
scoped_ptr<base::DictionaryValue> source_tab,
int source_frame_id,
+ int target_tab_id,
int target_frame_id,
int guest_process_id,
int guest_render_frame_routing_id,
diff --git a/chrome/browser/extensions/extension_tabs_apitest.cc b/chrome/browser/extensions/extension_tabs_apitest.cc
index c3ada31..484cc37 100644
--- a/chrome/browser/extensions/extension_tabs_apitest.cc
+++ b/chrome/browser/extensions/extension_tabs_apitest.cc
@@ -159,8 +159,7 @@
ASSERT_TRUE(RunExtensionTest("tabs/get_current")) << message_;
}
-// Flaky on the trybots. See http://crbug.com/96725.
-IN_PROC_BROWSER_TEST_F(ExtensionApiTest, DISABLED_TabConnect) {
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabConnect) {
ASSERT_TRUE(StartEmbeddedTestServer());
ASSERT_TRUE(RunExtensionTest("tabs/connect")) << message_;
}
diff --git a/chrome/test/data/extensions/api_test/tabs/connect/echo.js b/chrome/test/data/extensions/api_test/tabs/connect/echo.js
index c2a49285..c75a4a9 100644
--- a/chrome/test/data/extensions/api_test/tabs/connect/echo.js
+++ b/chrome/test/data/extensions/api_test/tabs/connect/echo.js
@@ -6,6 +6,9 @@
// Posting a message with "GET" returns the name and # of connections opened.
var connections = 0;
+// Notify the test that the content script is ready to go.
+chrome.runtime.sendMessage('ready');
+
chrome.runtime.onConnect.addListener(function onConnect(port) {
connections++;
port.onMessage.addListener(function onMessage(msg) {
@@ -17,6 +20,16 @@
});
});
+// onRequest simply echoes everything.
chrome.extension.onRequest.addListener(function(request, sender, respond) {
respond(request);
});
+
+// onMessage accepts commands (not all of which relate to echoing).
+chrome.runtime.onMessage.addListener(function(request, sender, respond) {
+ if (request.open)
+ open(request.open);
+ if (request.send)
+ chrome.runtime.sendMessage(request.send);
+ respond();
+});
diff --git a/chrome/test/data/extensions/api_test/tabs/connect/relative.html b/chrome/test/data/extensions/api_test/tabs/connect/empty.html
similarity index 73%
rename from chrome/test/data/extensions/api_test/tabs/connect/relative.html
rename to chrome/test/data/extensions/api_test/tabs/connect/empty.html
index 8acaac3..c5e129ca 100644
--- a/chrome/test/data/extensions/api_test/tabs/connect/relative.html
+++ b/chrome/test/data/extensions/api_test/tabs/connect/empty.html
@@ -3,8 +3,5 @@
* source code is governed by a BSD-style license that can be found in the
* LICENSE file.
-->
-<html>
-<head>
-<script src="relative.js"></script>
-</head>
-</html>
\ No newline at end of file
+
+<!-- Content script will be injected here. -->
diff --git a/chrome/test/data/extensions/api_test/tabs/connect/relative.js b/chrome/test/data/extensions/api_test/tabs/connect/relative.js
deleted file mode 100644
index 03e6dd2..0000000
--- a/chrome/test/data/extensions/api_test/tabs/connect/relative.js
+++ /dev/null
@@ -1,7 +0,0 @@
-// Copyright (c) 2011 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.
-
-window. {
- chrome.extension.getBackgroundPage().relativePageLoaded();
-}
diff --git a/chrome/test/data/extensions/api_test/tabs/connect/tabs_util.js b/chrome/test/data/extensions/api_test/tabs/connect/tabs_util.js
index 9f13dd9d..92369f9 100644
--- a/chrome/test/data/extensions/api_test/tabs/connect/tabs_util.js
+++ b/chrome/test/data/extensions/api_test/tabs/connect/tabs_util.js
@@ -1,3 +1,7 @@
+// Copyright 2015 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.
+
// Utility functions to help with tabs/windows testing.
// Removes current windows and creates one window with tabs set to
@@ -39,32 +43,3 @@
callback(win.id, newTabIds);
});
}
-
-// Waits until all tabs (yes, in every window) have status "complete".
-// This is useful to prevent test overlap when testing tab events.
-// |callback| should look like function() {...}. Note that |callback| expects
-// zero arguments.
-function waitForAllTabs(callback) {
- // Wait for all tabs to load.
- function waitForTabs(){
- chrome.windows.getAll({"populate": true}, function(windows) {
- var ready = true;
- for (var i in windows){
- for (var j in windows[i].tabs) {
- if (windows[i].tabs[j].status != "complete") {
- ready = false;
- break;
- }
- }
- if (!ready)
- break;
- }
- if (ready)
- callback();
- else
- window.setTimeout(waitForTabs, 30);
- });
- }
- waitForTabs();
-}
-
diff --git a/chrome/test/data/extensions/api_test/tabs/connect/test.js b/chrome/test/data/extensions/api_test/tabs/connect/test.js
index 7c8ee4b3..85f6ea4 100644
--- a/chrome/test/data/extensions/api_test/tabs/connect/test.js
+++ b/chrome/test/data/extensions/api_test/tabs/connect/test.js
@@ -1,124 +1,173 @@
-// tabs api test
-// browser_tests.exe --gtest_filter=ExtensionApiTest.TabConnect
+// Copyright 2015 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.
-// We have a bunch of places where we need to remember some state from one
-// test (or setup code) to subsequent tests.
-var testTabId = null;
+// browser_tests --gtest_filter=ExtensionApiTest.TabConnect
-var pass = chrome.test.callbackPass;
+// The test tab used for all tests.
+var testTab = null;
+
+// Path to the empty HTML document, fetched from the test configuration.
+var emptyDocumentURL = null;
+
+// The could-not-establish-connection error message. Used in a number of tests.
+var couldNotEstablishError =
+ 'Could not establish connection. Receiving end does not exist.';
+
var assertEq = chrome.test.assertEq;
+var assertFalse = chrome.test.assertFalse;
var assertTrue = chrome.test.assertTrue;
var callbackFail = chrome.test.callbackFail;
var fail = chrome.test.fail;
+var listenForever = chrome.test.listenForever;
+var listenOnce = chrome.test.listenOnce;
+var pass = chrome.test.callbackPass;
-chrome.test.getConfig(function(config) {
+function waitForReady(ready) {
+ chrome.test.listenOnce(chrome.runtime.onMessage, function(msg, sender) {
+ assertEq('ready', msg);
+ ready(sender.tab);
+ });
+}
- chrome.test.runTests([
- function setupConnect() {
- // The web page that our content script will be injected into.
- var relativePath = '/extensions/api_test/tabs/basics/relative.html';
- var testUrl =
- ['http://localhost:PORT'.replace(/PORT/, config.testServer.port),
- relativePath].join('');
-
- setupWindow([testUrl], pass(function(winId, tabIds) {
- testTabId = tabIds[0];
- waitForAllTabs(pass());
+chrome.test.runTests([
+ function setup() {
+ chrome.test.getConfig(pass(function(config) {
+ emptyDocumentURL = 'http://localhost:' + config.testServer.port +
+ '/extensions/api_test/tabs/connect/empty.html';
+ setupWindow([emptyDocumentURL], pass());
+ waitForReady(pass(function(tab) {
+ testTab = tab;
}));
- },
+ }));
+ },
- function connectMultipleConnects() {
- var connectCount = 0;
- function connect10() {
- var port = chrome.tabs.connect(testTabId);
- chrome.test.listenOnce(port.onMessage, function(msg) {
- assertEq(++connectCount, msg.connections);
- if (connectCount < 10)
- connect10();
- });
- port.postMessage("GET");
- }
- connect10();
- },
-
- function connectName() {
- var name = "akln3901n12la";
- var port = chrome.tabs.connect(testTabId, {"name": name});
- chrome.test.listenOnce(port.onMessage, function(msg) {
- assertEq(name, msg.name);
-
- var port = chrome.tabs.connect(testTabId);
- chrome.test.listenOnce(port.onMessage, function(msg) {
- assertEq('', msg.name);
- });
- port.postMessage("GET");
+ function connectMultipleConnects() {
+ var connectCount = 0;
+ function connect10() {
+ var port = chrome.tabs.connect(testTab.id);
+ listenOnce(port.onMessage, function(msg) {
+ assertEq(++connectCount, msg.connections);
+ if (connectCount < 10)
+ connect10();
});
- port.postMessage("GET");
- },
-
- function connectPostMessageTypes() {
- var port = chrome.tabs.connect(testTabId);
- // Test the content script echoes the message back.
- var echoMsg = {"num": 10, "string": "hi", "array": [1,2,3,4,5],
- "obj":{"dec": 1.0}};
- chrome.test.listenOnce(port.onMessage, function(msg) {
- assertEq(echoMsg.num, msg.num);
- assertEq(echoMsg.string, msg.string);
- assertEq(echoMsg.array[4], msg.array[4]);
- assertEq(echoMsg.obj.dec, msg.obj.dec);
- });
- port.postMessage(echoMsg);
- },
-
- function connectPostManyMessages() {
- var port = chrome.tabs.connect(testTabId);
- var count = 0;
- var done = chrome.test.listenForever(port.onMessage, function(msg) {
- assertEq(count++, msg);
- if (count == 999) {
- done();
- }
- });
- for (var i = 0; i < 1000; i++) {
- port.postMessage(i);
- }
- },
-
- function connectNoTab() {
- // Expect a disconnect event when you connect to a non-existant tab, and
- // once disconnected, expect an error while trying to post messages.
- var errorMsg =
- "Could not establish connection. Receiving end does not exist.";
- chrome.tabs.create({}, pass(function(tab) {
- chrome.tabs.remove(tab.id, pass(function() {
- var p = chrome.tabs.connect(tab.id);
- p.onDisconnect.addListener(callbackFail(errorMsg, function() {
- try {
- p.postMessage();
- fail("Error should have been thrown.");
- } catch (e) {
- // Do nothing- an exception should be thrown.
- }
- }));
- }));
- }));
- },
-
- function sendRequest() {
- var request = "test";
- chrome.tabs.sendRequest(testTabId, request, pass(function(response) {
- assertEq(request, response);
- }));
- },
-
- function sendRequestNoTab() {
- var errorMsg =
- "Could not establish connection. Receiving end does not exist.";
- chrome.tabs.create({}, pass(function(tab) {
- chrome.tabs.remove(tab.id, pass(function() {
- chrome.tabs.sendRequest(tab.id, "test", callbackFail(errorMsg));
- }));
- }));
+ port.postMessage('GET');
}
- ]);
-});
+ connect10();
+ },
+
+ function connectName() {
+ var name = 'akln3901n12la';
+ var port = chrome.tabs.connect(testTab.id, {'name': name});
+ listenOnce(port.onMessage, function(msg) {
+ assertEq(name, msg.name);
+
+ var port = chrome.tabs.connect(testTab.id);
+ listenOnce(port.onMessage, function(msg) {
+ assertEq('', msg.name);
+ });
+ port.postMessage('GET');
+ });
+ port.postMessage('GET');
+ },
+
+ function connectPostMessageTypes() {
+ var port = chrome.tabs.connect(testTab.id);
+ // Test the content script echoes the message back.
+ var echoMsg = {'num': 10, 'string': 'hi', 'array': [1,2,3,4,5],
+ 'obj':{'dec': 1.0}};
+ listenOnce(port.onMessage, function(msg) {
+ assertEq(echoMsg.num, msg.num);
+ assertEq(echoMsg.string, msg.string);
+ assertEq(echoMsg.array[4], msg.array[4]);
+ assertEq(echoMsg.obj.dec, msg.obj.dec);
+ });
+ port.postMessage(echoMsg);
+ },
+
+ function connectPostManyMessages() {
+ var port = chrome.tabs.connect(testTab.id);
+ var count = 0;
+ var done = listenForever(port.onMessage, function(msg) {
+ assertEq(count++, msg);
+ if (count == 100) {
+ done();
+ }
+ });
+ for (var i = 0; i < 100; i++) {
+ port.postMessage(i);
+ }
+ },
+
+ function connectToRemovedTab() {
+ // Expect a disconnect event when you connect to a non-existant tab, and
+ // once disconnected, expect an error while trying to post messages.
+ chrome.tabs.create({}, pass(function(tab) {
+ chrome.tabs.remove(tab.id, pass(function() {
+ var p = chrome.tabs.connect(tab.id);
+ p.onDisconnect.addListener(callbackFail(couldNotEstablishError,
+ function() {
+ try {
+ p.postMessage();
+ fail('Error should have been thrown.');
+ } catch (e) {
+ // Do nothing- an exception should be thrown.
+ }
+ }));
+ }));
+ }));
+ },
+
+ function sendRequest() {
+ var request = 'test';
+ chrome.tabs.sendRequest(testTab.id, request, pass(function(response) {
+ assertEq(request, response);
+ }));
+ },
+
+ function sendRequestToImpossibleTab() {
+ chrome.tabs.sendRequest(9999, 'test', callbackFail(couldNotEstablishError));
+ },
+
+ function sendRequestToRemovedTab() {
+ chrome.tabs.create({}, pass(function(tab) {
+ chrome.tabs.remove(tab.id, pass(function() {
+ chrome.tabs.sendRequest(tab.id, 'test',
+ callbackFail(couldNotEstablishError));
+ }));
+ }));
+ },
+
+ function sendRequestMultipleTabs() {
+ // Regression test for crbug.com/520303. Instruct the test tab to create
+ // another tab, then send a message to each. The bug was that the message
+ // is sent to both, if they're in the same process.
+ //
+ // The tab itself must do the open so that they share a process,
+ // chrome.tabs.create doesn't guarantee that.
+ chrome.tabs.sendMessage(testTab.id, {open: emptyDocumentURL});
+ waitForReady(pass(function(secondTab) {
+ var gotDuplicates = false;
+ var messages = new Set();
+ var done = listenForever(chrome.runtime.onMessage, function(msg) {
+ if (messages.has(msg))
+ gotDuplicates = true;
+ else
+ messages.add(msg);
+ });
+ chrome.tabs.sendMessage(testTab.id, {send: 'msg1'}, function() {
+ chrome.tabs.sendMessage(secondTab.id, {send: 'msg2'}, function() {
+ // Send an empty final message to hopefully ensure that the events
+ // for msg1 and msg2 have been fired.
+ chrome.tabs.sendMessage(testTab.id, {}, function() {
+ assertEq(2, messages.size);
+ assertTrue(messages.has('msg1'));
+ assertTrue(messages.has('msg2'));
+ assertFalse(gotDuplicates);
+ done();
+ });
+ });
+ });
+ }));
+ },
+]);
diff --git a/extensions/common/extension_messages.h b/extensions/common/extension_messages.h
index 8ae6d19..5af81f8c 100644
--- a/extensions/common/extension_messages.h
+++ b/extensions/common/extension_messages.h
@@ -180,7 +180,12 @@
// The URL of the frame that initiated the request.
IPC_STRUCT_MEMBER(GURL, source_url)
- // The ID of the frame that is the target of the request.
+ // The ID of the tab that is the target of the request, or -1 if there is no
+ // target tab.
+ IPC_STRUCT_MEMBER(int, target_tab_id)
+
+ // The ID of the frame that is the target of the request, or -1 if there is
+ // no target frame (implying the message is for all frames).
IPC_STRUCT_MEMBER(int, target_frame_id)
// The process ID of the webview that initiated the request.
diff --git a/extensions/renderer/messaging_bindings.cc b/extensions/renderer/messaging_bindings.cc
index dd8b54d4..23ce40ab 100644
--- a/extensions/renderer/messaging_bindings.cc
+++ b/extensions/renderer/messaging_bindings.cc
@@ -25,6 +25,8 @@
#include "extensions/common/manifest_handlers/externally_connectable.h"
#include "extensions/renderer/dispatcher.h"
#include "extensions/renderer/event_bindings.h"
+#include "extensions/renderer/extension_frame_helper.h"
+#include "extensions/renderer/gc_callback.h"
#include "extensions/renderer/object_backed_native_handler.h"
#include "extensions/renderer/script_context.h"
#include "extensions/renderer/script_context_set.h"
@@ -356,6 +358,15 @@
if (info.target_frame_id > 0 &&
renderframe->GetRoutingID() != info.target_frame_id)
return;
+
+ // Bandaid fix for crbug.com/520303.
+ // TODO(rdevlin.cronin): Fix this properly by routing messages to the correct
+ // RenderFrame from the browser (same with |target_frame_id| in fact).
+ if (info.target_tab_id != -1 &&
+ info.target_tab_id != ExtensionFrameHelper::Get(renderframe)->tab_id()) {
+ return;
+ }
+
v8::Isolate* isolate = script_context->isolate();
v8::HandleScope handle_scope(isolate);