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}

12 files changed, 157 insertions, 206 deletions

diff --git a/chrome/browser/extensions/api/messaging/extension_message_port.cc b/chrome/browser/extensions/api/messaging/extension_message_port.cc
index 1e4f87f..7f0da64 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 @@ void ExtensionMessagePort::DispatchOnConnect(
     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 @@ void ExtensionMessagePort::DispatchOnConnect(
   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..8445bda 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 @@ class ExtensionMessagePort : public MessageService::MessagePort {
                          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 357616f..acef124 100644
--- a/chrome/browser/extensions/api/messaging/message_service.cc
+++ b/chrome/browser/extensions/api/messaging/message_service.cc
@@ -125,7 +125,6 @@ struct MessageService::OpenChannelParams {
   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 @@ struct MessageService::OpenChannelParams {
   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 @@ struct MessageService::OpenChannelParams {
                     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 @@ void MessageService::OpenChannelToExtension(
   }
 
   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 @@ void MessageService::OpenChannelToTab(int source_process_id,
       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 1213381..e68b19e 100644
--- a/chrome/browser/extensions/api/messaging/message_service.h
+++ b/chrome/browser/extensions/api/messaging/message_service.h
@@ -73,7 +73,6 @@ class MessageService : public BrowserContextKeyedAPI,
                                    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 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, DISABLED_TabGetCurrent) {
   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..c2a4928 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 @@ chrome.runtime.onConnect.addListener(function onConnect(port) {
   });
 });
 
-// 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
index c5e129c..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.onload = function() {
+  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..9f13dd9 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 @@ function createWindow(tabUrls, winOptions, callback) {
     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..7c8ee4b 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
-
-// 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.';
+// 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;
 
+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('');
 
-  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();
+      setupWindow([testUrl], pass(function(winId, tabIds) {
+        testTabId = tabIds[0];
+        waitForAllTabs(pass());
+      }));
+    },
+
+    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");
       });
-      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");
+    },
+
+    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('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();
+      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);
       }
-    });
-    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 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 5af81f8..8ae6d19 100644
--- a/extensions/common/extension_messages.h
+++ b/extensions/common/extension_messages.h
@@ -180,12 +180,7 @@ IPC_STRUCT_BEGIN(ExtensionMsg_ExternalConnectionInfo)
   // 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 23ce40a..dd8b54d 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 @@ void DispatchOnConnectToScriptContext(
   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);