Revert of Check the tab ID before delivering an extension message. (patchset #1 id:1 of https://codereview.chromium.org/1335743003/ )
Reason for revert:
Broke build.
Original issue's description:
> 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)
TBR=dcheng@chromium.org,rdevlin.cronin@chromium.org
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=520303
Review URL: https://codereview.chromium.org/1333033003
Cr-Commit-Position: refs/branch-heads/2454@{#456}
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 1e4f87f..7f0da642 100644
--- a/chrome/browser/extensions/api/messaging/extension_message_port.cc
+++ b/chrome/browser/extensions/api/messaging/extension_message_port.cc
@@ -27,7 +27,6 @@
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,
@@ -44,7 +43,6 @@
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 79b8003..8445bdac 100644
--- a/chrome/browser/extensions/api/messaging/extension_message_port.h
+++ b/chrome/browser/extensions/api/messaging/extension_message_port.h
@@ -25,7 +25,6 @@
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 357616fe..acef124 100644
--- a/chrome/browser/extensions/api/messaging/message_service.cc
+++ b/chrome/browser/extensions/api/messaging/message_service.cc
@@ -125,7 +125,6 @@
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;
@@ -141,7 +140,6 @@
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,
@@ -153,7 +151,6 @@
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),
@@ -351,8 +348,8 @@
}
scoped_ptr<OpenChannelParams> params(new OpenChannelParams(
- source_process_id, source_tab.Pass(), source_frame_id, -1,
- -1, // no target_tab_id/target_frame_id for connections to extensions
+ source_process_id, source_tab.Pass(), source_frame_id,
+ -1, // no target_frame_id for a channel to an extension/background page.
nullptr, receiver_port_id, source_extension_id, target_extension_id,
source_url, channel_name, include_tls_channel_id,
include_guest_process_info));
@@ -535,11 +532,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.
- tab_id, frame_id, receiver.release(), receiver_port_id, extension_id,
- extension_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 12133819..e68b19e 100644
--- a/chrome/browser/extensions/api/messaging/message_service.h
+++ b/chrome/browser/extensions/api/messaging/message_service.h
@@ -73,7 +73,6 @@
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 484cc37..c3ada31 100644
--- a/chrome/browser/extensions/extension_tabs_apitest.cc
+++ b/chrome/browser/extensions/extension_tabs_apitest.cc
@@ -159,7 +159,8 @@
ASSERT_TRUE(RunExtensionTest("tabs/get_current")) << message_;
}
-IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabConnect) {
+// Flaky on the trybots. See http://crbug.com/96725.
+IN_PROC_BROWSER_TEST_F(ExtensionApiTest, DISABLED_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 c75a4a9..c2a49285 100644
--- a/chrome/test/data/extensions/api_test/tabs/connect/echo.js
+++ b/chrome/test/data/extensions/api_test/tabs/connect/echo.js
@@ -6,9 +6,6 @@
// 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) {
@@ -20,16 +17,6 @@
});
});
-// 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/empty.html b/chrome/test/data/extensions/api_test/tabs/connect/relative.html
similarity index 73%
rename from chrome/test/data/extensions/api_test/tabs/connect/empty.html
rename to chrome/test/data/extensions/api_test/tabs/connect/relative.html
index c5e129ca..8acaac3 100644
--- a/chrome/test/data/extensions/api_test/tabs/connect/empty.html
+++ b/chrome/test/data/extensions/api_test/tabs/connect/relative.html
@@ -3,5 +3,8 @@
* source code is governed by a BSD-style license that can be found in the
* LICENSE file.
-->
-
-<!-- Content script will be injected here. -->
+<html>
+<head>
+<script src="relative.js"></script>
+</head>
+</html>
\ No newline at end of file
diff --git a/chrome/test/data/extensions/api_test/tabs/connect/relative.js b/chrome/test/data/extensions/api_test/tabs/connect/relative.js
new file mode 100644
index 0000000..03e6dd2
--- /dev/null
+++ b/chrome/test/data/extensions/api_test/tabs/connect/relative.js
@@ -0,0 +1,7 @@
+// 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 92369f9..9f13dd9d 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,7 +1,3 @@
-// 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
@@ -43,3 +39,32 @@
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 85f6ea4..7c8ee4b3 100644
--- a/chrome/test/data/extensions/api_test/tabs/connect/test.js
+++ b/chrome/test/data/extensions/api_test/tabs/connect/test.js
@@ -1,173 +1,124 @@
-// 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.
+// tabs api test
+// browser_tests.exe --gtest_filter=ExtensionApiTest.TabConnect
-// browser_tests --gtest_filter=ExtensionApiTest.TabConnect
+// 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;
-// 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 pass = chrome.test.callbackPass;
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;
-function waitForReady(ready) {
- chrome.test.listenOnce(chrome.runtime.onMessage, function(msg, sender) {
- assertEq('ready', msg);
- ready(sender.tab);
- });
-}
+chrome.test.getConfig(function(config) {
-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;
+ 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());
}));
- }));
- },
+ },
- 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');
- }
- 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();
+ 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");
}
- });
- for (var i = 0; i < 100; i++) {
- port.postMessage(i);
- }
- },
+ connect10();
+ },
- 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 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");
+ });
+ 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(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 sendRequest() {
+ var request = "test";
+ chrome.tabs.sendRequest(testTabId, request, pass(function(response) {
+ assertEq(request, response);
}));
- }));
- },
+ },
- 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();
- });
- });
- });
- }));
- },
-]);
+ 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));
+ }));
+ }));
+ }
+ ]);
+});
diff --git a/extensions/common/extension_messages.h b/extensions/common/extension_messages.h
index 5af81f8c..8ae6d19 100644
--- a/extensions/common/extension_messages.h
+++ b/extensions/common/extension_messages.h
@@ -180,12 +180,7 @@
// The URL of the frame that initiated the request.
IPC_STRUCT_MEMBER(GURL, source_url)
- // 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).
+ // The ID of the frame that is the target of the request.
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 23ce40ab..dd8b54d4 100644
--- a/extensions/renderer/messaging_bindings.cc
+++ b/extensions/renderer/messaging_bindings.cc
@@ -25,8 +25,6 @@
#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"
@@ -358,15 +356,6 @@
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);