Revert 2.5 CLs that relate to pulling Extension port management out of messaging.js.

The first is 1be16298:
"Clear the onMessage and onDisconnect listeners when their Extension Port is destroyed."
which was a fix to the original patch.

The second, original patch is 32b3a84b:
"Move the Extension Port implementation out of messaging.js into its own file port.js."

This is the key patch, which has caused no end of trouble. The goal was noble:
to fix the myriad of bugs which messaging has due to being written in JS not
C++. It appears that the moral of the story is: this is impossible, and the
hole we've dug in over the last 5 years cannot be recovered from (without a
complete rewrite, I suppose).

The last .5 of a patch was one of the tests added in
b9dca054 which will no longer pass.

BUG=486715,477323,475536
R=rockot@chromium.org

Review URL: https://codereview.chromium.org/1131043003

Cr-Commit-Position: refs/heads/master@{#329571}

chrome/test/data/extensions/api_test/messaging/connect/test.js

@@ -2,10 +2,6 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-// Tests for browser_tests --gtest_filter=ExtensionApiTest.Messaging
-
-var assertEq = chrome.test.assertEq;
-var assertTrue = chrome.test.assertTrue;
 var listenOnce = chrome.test.listenOnce;
 var listenForever = chrome.test.listenForever;
 
@@ -64,7 +60,7 @@ chrome.test.getConfig(function(config) {
       var port = chrome.tabs.connect(testTab.id, {name: portName});
       port.postMessage({testPortName: true});
       listenOnce(port.onMessage, function(msg) {
-        assertEq(msg.portName, portName);
+        chrome.test.assertEq(msg.portName, portName);
         port.disconnect();
       });
     },
@@ -72,14 +68,14 @@ chrome.test.getConfig(function(config) {
     // Tests that postMessage from the tab and its response works.
     function postMessageFromTab() {
       listenOnce(chrome.runtime.onConnect, function(port) {
-        assertEq({
+        chrome.test.assertEq({
           tab: testTab,
           frameId: 0, // Main frame
           url: testTab.url,
-          id: chrome.runtime.id
+           id: chrome.runtime.id
         }, port.sender);
         listenOnce(port.onMessage, function(msg) {
-          assertTrue(msg.testPostMessageFromTab);
+          chrome.test.assertTrue(msg.testPostMessageFromTab);
           port.postMessage({success: true, portName: port.name});
           chrome.test.log("postMessageFromTab: got message from tab");
         });
@@ -98,11 +94,11 @@ chrome.test.getConfig(function(config) {
       var doneListening = listenForever(
         chrome.runtime.onMessage,
         function(request, sender, sendResponse) {
-          assertEq({
+          chrome.test.assertEq({
             tab: testTab,
             frameId: 0, // Main frame
             url: testTab.url,
-            id: chrome.runtime.id
+             id: chrome.runtime.id
           }, sender);
           if (request.step == 1) {
             // Step 1: Page should send another request for step 2.
@@ -110,7 +106,7 @@ chrome.test.getConfig(function(config) {
             sendResponse({nextStep: true});
           } else {
             // Step 2.
-            assertEq(2, request.step);
+            chrome.test.assertEq(request.step, 2);
             sendResponse();
             doneListening();
           }
@@ -155,7 +151,7 @@ chrome.test.getConfig(function(config) {
                 };
               }).sort(sortByFrameId);
               senders.sort(sortByFrameId);
-              assertEq(expectedSenders, senders);
+              chrome.test.assertEq(expectedSenders, senders);
               doneListening();
             });
           }
@@ -187,7 +183,7 @@ chrome.test.getConfig(function(config) {
         var frames = details.filter(function(frame) {
           return /\?testSendMessageFromFrame1$/.test(frame.url);
         });
-        assertEq(1, frames.length);
+        chrome.test.assertEq(1, frames.length);
         connectToTabWithFrameId(frames[0].frameId, ['from_1']);
       });
     },
@@ -237,7 +233,7 @@ chrome.test.getConfig(function(config) {
     // Tests sending a request to a tab and receiving a response.
     function sendMessage() {
       chrome.tabs.sendMessage(testTab.id, {step2: 1}, function(response) {
-        assertTrue(response.success);
+        chrome.test.assertTrue(response.success);
         chrome.test.succeed();
       });
     },
@@ -281,7 +277,7 @@ chrome.test.getConfig(function(config) {
       } catch(e) {
         error = e;
       }
-      assertTrue(error != undefined);
+      chrome.test.assertTrue(error != undefined);
 
       error = undefined;
       try {
@@ -289,7 +285,7 @@ chrome.test.getConfig(function(config) {
       } catch(e) {
         error = e;
       }
-      assertTrue(error != undefined);
+      chrome.test.assertTrue(error != undefined);
 
       chrome.test.succeed();
     },
@@ -313,45 +309,35 @@ chrome.test.getConfig(function(config) {
     // given a callback. This requires a more creative test setup because there
     // is no callback to signal when it's supposed to have been done.
     // Regression test for http://crbug.com/479951.
-    function sendMessageToCurrentTextWithoutCallbackFails() {
-      // Make the iframe - in a different context - watch for the message
-      // event. It *should* get it, while the current context's one doesn't.
-      var iframe = document.createElement('iframe');
-      iframe.src = chrome.runtime.getURL('blank_iframe.html');
-      document.body.appendChild(iframe);
+    //
+    // NOTE(kalman): This test is correct. However, the patch which fixes it
+    // (see bug) was reverted, and I don't plan on resubmitting, so instead
+    // I'll comment out this test, and leave it here for the record.
+    //
+    // function sendMessageToCurrentTextWithoutCallbackFails() {
+    //   // Make the iframe - in a different context - watch for the message
+    //   // event. It *should* get it, while the current context's one doesn't.
+    //   var iframe = document.createElement('iframe');
+    //   iframe.src = chrome.runtime.getURL('blank_iframe.html');
+    //   document.body.appendChild(iframe);
+
+    //   var stopFailing = failWhileListening(chrome.runtime.onMessage);
+    //   chrome.test.listenOnce(
+    //     iframe.contentWindow.chrome.runtime.onMessage,
+    //     function(msg, sender) {
+    //       chrome.test.assertEq('ping', msg);
+    //       chrome.test.assertEq(chrome.runtime.id, sender.id);
+    //       chrome.test.assertEq(location.href, sender.url);
+    //       setTimeout(function() {
+    //         stopFailing();
+    //         chrome.test.succeed();
+    //       }, 0);
+    //     }
+    //   );
+    //
+    //   chrome.runtime.sendMessage('ping');
+    // },
 
-      var stopFailing = failWhileListening(chrome.runtime.onMessage);
-      listenOnce(
-        iframe.contentWindow.chrome.runtime.onMessage,
-        function(msg, sender) {
-          assertEq('ping', msg);
-          assertEq(chrome.runtime.id, sender.id);
-          assertEq(location.href, sender.url);
-          setTimeout(function() {
-            stopFailing();
-            chrome.test.succeed();
-          }, 0);
-        }
-      );
-
-      chrome.runtime.sendMessage('ping');
-    },
-
-    // Tests that destroying a port clears its onMessage and onDisconnect
-    // listeners.
-    function disconnectingPortRemovesEventListeners() {
-      var port = chrome.runtime.connect(chrome.runtime.id);
-      assertTrue(!port.onMessage.hasListeners() &&
-                 !port.onDisconnect.hasListeners());
-      port.onMessage.addListener(function() {});
-      port.onDisconnect.addListener(function() {});
-      assertTrue(port.onMessage.hasListeners() &&
-                 port.onDisconnect.hasListeners());
-      port.disconnect();
-      assertTrue(!port.onMessage.hasListeners() &&
-                 !port.onDisconnect.hasListeners());
-      chrome.test.succeed();
-    }
   ]);
 });
 
@@ -369,7 +355,7 @@ function connectToTabWithFrameId(frameId, expectedMessages) {
     messages.push(message);
     isDone = messages.length == expectedMessages.length;
     if (isDone) {
-      assertEq(expectedMessages.sort(), messages.sort());
+      chrome.test.assertEq(expectedMessages.sort(), messages.sort());
       chrome.test.succeed();
     }
   });

extensions/extensions.gypi

@@ -922,7 +922,6 @@
       'renderer/resources/permissions_custom_bindings.js',
       'renderer/resources/platform_app.css',
       'renderer/resources/platform_app.js',
-      'renderer/resources/port.js',
       'renderer/resources/runtime_custom_bindings.js',
       'renderer/resources/schema_utils.js',
       'renderer/resources/send_request.js',

extensions/renderer/dispatcher.cc

@@ -479,7 +479,6 @@ std::vector<std::pair<std::string, int> > Dispatcher::GetJsResources() {
   resources.push_back(std::make_pair("messaging", IDR_MESSAGING_JS));
   resources.push_back(std::make_pair("messaging_utils",
                                      IDR_MESSAGING_UTILS_JS));
-  resources.push_back(std::make_pair("port", IDR_PORT_JS));
   resources.push_back(std::make_pair(kSchemaUtils, IDR_SCHEMA_UTILS_JS));
   resources.push_back(std::make_pair("sendRequest", IDR_SEND_REQUEST_JS));
   resources.push_back(std::make_pair("setIcon", IDR_SET_ICON_JS));

extensions/renderer/resources/extensions_renderer_resources.grd

@@ -42,7 +42,6 @@
       <include name="IDR_MESSAGING_UTILS_JS" file="messaging_utils.js" type="BINDATA" />
       <include name="IDR_MIME_HANDLER_PRIVATE_CUSTOM_BINDINGS_JS" file="mime_handler_private_custom_bindings.js" type="BINDATA" />
       <include name="IDR_MIME_HANDLER_MOJOM_JS" file="${mojom_root}\extensions\common\api\mime_handler.mojom.js" use_base_dir="false" type="BINDATA" />
-      <include name="IDR_PORT_JS" file="port.js" type="BINDATA" />
       <include name="IDR_SCHEMA_UTILS_JS" file="schema_utils.js" type="BINDATA" />
       <include name="IDR_SEND_REQUEST_JS" file="send_request.js" type="BINDATA" />
       <include name="IDR_SERIAL_CUSTOM_BINDINGS_JS" file="serial_custom_bindings.js" type="BINDATA" />

extensions/renderer/resources/messaging.js

@@ -6,13 +6,14 @@
 
   // TODO(kalman): factor requiring chrome out of here.
   var chrome = requireNative('chrome').GetChrome();
+  var Event = require('event_bindings').Event;
   var lastError = require('lastError');
   var logActivity = requireNative('activityLogger');
   var logging = requireNative('logging');
   var messagingNatives = requireNative('messaging_natives');
-  var Port = require('port').Port;
   var processNatives = requireNative('process');
   var unloadEvent = require('unload_event');
+  var utils = require('utils');
   var messagingUtils = require('messaging_utils');
 
   // The reserved channel name for the sendRequest/send(Native)Message APIs.
@@ -32,6 +33,64 @@
   // channel.
   function getOppositePortId(portId) { return portId ^ 1; }
 
+  // Port object.  Represents a connection to another script context through
+  // which messages can be passed.
+  function PortImpl(portId, opt_name) {
+    this.portId_ = portId;
+    this.name = opt_name;
+
+    var portSchema = {name: 'port', $ref: 'runtime.Port'};
+    var options = {unmanaged: true};
+    this.onDisconnect = new Event(null, [portSchema], options);
+    this.onMessage = new Event(
+        null,
+        [{name: 'message', type: 'any', optional: true}, portSchema],
+        options);
+    this.onDestroy_ = null;
+  }
+
+  // Sends a message asynchronously to the context on the other end of this
+  // port.
+  PortImpl.prototype.postMessage = function(msg) {
+    // JSON.stringify doesn't support a root object which is undefined.
+    if (msg === undefined)
+      msg = null;
+    msg = $JSON.stringify(msg);
+    if (msg === undefined) {
+      // JSON.stringify can fail with unserializable objects. Log an error and
+      // drop the message.
+      //
+      // TODO(kalman/mpcomplete): it would be better to do the same validation
+      // here that we do for runtime.sendMessage (and variants), i.e. throw an
+      // schema validation Error, but just maintain the old behaviour until
+      // there's a good reason not to (http://crbug.com/263077).
+      console.error('Illegal argument to Port.postMessage');
+      return;
+    }
+    messagingNatives.PostMessage(this.portId_, msg);
+  };
+
+  // Disconnects the port from the other end.
+  PortImpl.prototype.disconnect = function() {
+    messagingNatives.CloseChannel(this.portId_, true);
+    this.destroy_();
+  };
+
+  PortImpl.prototype.destroy_ = function() {
+    var portId = this.portId_;
+
+    if (this.onDestroy_)
+      this.onDestroy_();
+    privates(this.onDisconnect).impl.destroy_();
+    privates(this.onMessage).impl.destroy_();
+
+    messagingNatives.PortRelease(portId);
+    unloadEvent.removeListener(portReleasers[portId]);
+
+    delete ports[portId];
+    delete portReleasers[portId];
+  };
+
   // Returns true if the specified port id is in this context. This is used by
   // the C++ to avoid creating the javascript message for all the contexts that
   // don't care about a particular message.
@@ -54,14 +113,6 @@
     return port;
   };
 
-  // Called when a Port is destroyed. Does general accounting cleanup.
-  function onPortDestroyed(port) {
-    var portId = privates(port).impl.portId_;
-    unloadEvent.removeListener(portReleasers[portId]);
-    delete ports[portId];
-    delete portReleasers[portId];
-  }
-
   // Helper function for dispatchOnRequest.
   function handleSendRequestError(isSendMessage,
                                   responseCallbackPreserved,
@@ -149,7 +200,6 @@
 
     privates(port).impl.onDestroy_ = function() {
       port.onMessage.removeListener(messageListener);
-      onPortDestroyed(port);
     };
     port.onMessage.addListener(messageListener);
 
@@ -308,7 +358,6 @@
     privates(port).impl.onDestroy_ = function() {
       port.onDisconnect.removeListener(disconnectListener);
       port.onMessage.removeListener(messageListener);
-      onPortDestroyed(port);
     };
     port.onDisconnect.addListener(disconnectListener);
     port.onMessage.addListener(messageListener);
@@ -324,9 +373,20 @@
     return alignedArgs;
   }
 
+var Port = utils.expose('Port', PortImpl, { functions: [
+    'disconnect',
+    'postMessage'
+  ],
+  properties: [
+    'name',
+    'onDisconnect',
+    'onMessage'
+  ] });
+
 exports.kRequestChannel = kRequestChannel;
 exports.kMessageChannel = kMessageChannel;
 exports.kNativeMessageChannel = kNativeMessageChannel;
+exports.Port = Port;
 exports.createPort = createPort;
 exports.sendMessageImpl = sendMessageImpl;
 exports.sendMessageUpdateArguments = sendMessageUpdateArguments;

extensions/renderer/resources/port.js

-// 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.
-
-var Event = require('event_bindings').Event;
-var messagingNatives = requireNative('messaging_natives');
-var utils = require('utils');
-
-// Port object.  Represents a connection to another script context through
-// which messages can be passed.
-function Port(portId, opt_name) {
-  this.portId_ = portId;
-  this.name = opt_name;
-
-  var portSchema = {name: 'port', $ref: 'runtime.Port'};
-  var messageSchema = {name: 'message', type: 'any', optional: true};
-  var options = {unmanaged: true};
-
-  this.onMessage = new Event(null, [messageSchema, portSchema], options);
-  this.onDisconnect = new Event(null, [portSchema], options);
-  this.onDestroy_ = null;
-}
-
-// Sends a message asynchronously to the context on the other end of this
-// port.
-Port.prototype.postMessage = function(msg) {
-  // JSON.stringify doesn't support a root object which is undefined.
-  if (msg === undefined)
-    msg = null;
-  msg = $JSON.stringify(msg);
-  if (msg === undefined) {
-    // JSON.stringify can fail with unserializable objects. Log an error and
-    // drop the message.
-    //
-    // TODO(kalman/mpcomplete): it would be better to do the same validation
-    // here that we do for runtime.sendMessage (and variants), i.e. throw an
-    // schema validation Error, but just maintain the old behaviour until
-    // there's a good reason not to (http://crbug.com/263077).
-    console.error('Illegal argument to Port.postMessage');
-    return;
-  }
-  messagingNatives.PostMessage(this.portId_, msg);
-};
-
-// Disconnects the port from the other end.
-Port.prototype.disconnect = function() {
-  messagingNatives.CloseChannel(this.portId_, true);
-  this.destroy_();
-};
-
-Port.prototype.destroy_ = function() {
-  if (this.onDestroy_)
-    this.onDestroy_();
-  // Destroy the onMessage/onDisconnect events in case the extension added
-  // listeners, but didn't remove them, when the port closed.
-  privates(this.onMessage).impl.destroy_();
-  privates(this.onDisconnect).impl.destroy_();
-  messagingNatives.PortRelease(this.portId_);
-};
-
-exports.Port = utils.expose('Port', Port, {
-  functions: ['disconnect', 'postMessage'],
-  properties: ['name', 'onMessage', 'onDisconnect']
-});