Remove port lifetime management from renderer

extensions/renderer/resources/messaging.js

 // Copyright 2014 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.
 
 // chrome.runtime.messaging API implementation.
 // TODO(robwu): Fix this indentation.
 
   // 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 processNatives = requireNative('process');
   var utils = require('utils');
   var messagingUtils = require('messaging_utils');
 
   // The reserved channel name for the sendRequest/send(Native)Message APIs.
   // Note: sendRequest is deprecated.
   var kRequestChannel = "chrome.extension.sendRequest";
   var kMessageChannel = "chrome.runtime.sendMessage";
   var kNativeMessageChannel = "chrome.runtime.sendNativeMessage";
+  var kPortClosedError = 'Attempting to use a disconnected port object';
 
   // Map of port IDs to port object.
   var ports = {__proto__: null};
 
   // Change even to odd and vice versa, to get the other side of a given
   // channel.
   function getOppositePortId(portId) { return portId ^ 1; }
 
   // Port object.  Represents a connection to another script context through
   // which messages can be passed.
@@ skipping 12 matching lines @@
       name: 'message',
       type: 'any',
       optional: true,
     };
     var options = {
       __proto__: null,
       unmanaged: true,
     };
     this.onDisconnect = new Event(null, [portSchema], options);
     this.onMessage = new Event(null, [messageSchema, portSchema], options);
-    this.onDestroy_ = null;
   }
   $Object.setPrototypeOf(PortImpl.prototype, null);
 
   // Sends a message asynchronously to the context on the other end of this
   // port.
   PortImpl.prototype.postMessage = function(msg) {
+    if (!(this.portId_ in ports))

[Devlin, 2016/05/17 19:50:30]

'in' implicitly relies on Object.prototype; use $Object.hasOwnProperty.

[robwu, 2016/05/17 22:47:51]

ports has a null prototype, so it doesn't matter. But I've switched to
hasOwnProperty anyway (and changed all other "in"s for consistency.

+      throw new Error(kPortClosedError);
+
     // 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() {
+    if (!(this.portId_ in ports))

[Devlin, 2016/05/17 19:50:30]

ditto

[robwu, 2016/05/17 22:47:51]

Done.

+      return;  // disconnect() on an already-closed port is a no-op.
     messagingNatives.CloseChannel(this.portId_, true);
     this.destroy_();
   };
 
+  // Close this specific port without forcing the channel to close. The channel
+  // will close if this was the only port at this end of the channel.
+  PortImpl.prototype.disconnectSoftly = function() {
+    if (!(this.portId_ in ports))

[Devlin, 2016/05/17 19:50:30]

ditto, but can this happen?

[robwu, 2016/05/17 22:47:51]

Done. This is not expected to happen, but I didn't bother with a DCHECK/CHECK
for no specific reason.

+      return;
+    messagingNatives.CloseChannel(this.portId_, false);
+    this.destroy_();
+  };
+
   PortImpl.prototype.destroy_ = function() {
-    if (this.onDestroy_) {
-      this.onDestroy_();
-      this.onDestroy_ = null;
-    }
     privates(this.onDisconnect).impl.destroy_();
     privates(this.onMessage).impl.destroy_();
-    // TODO(robwu): Remove port lifetime management because it is completely
-    // handled in the browser. The renderer's only roles are
-    // 1) rejecting ports so that the browser knows that the renderer is not
-    //    interested in the port (this is merely an optimization)
-    // 2) acknowledging port creations, so that the browser knows that the port
-    //    was successfully created (from the perspective of the extension), but
-    //    then closed for some non-fatal reason.
-    // 3) notifying the browser of explicit port closure via .disconnect().
-    // In other cases (navigations), the browser automatically cleans up the
-    //    port.
-    messagingNatives.PortRelease(this.portId_);
     delete ports[this.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.
   function hasPort(portId) {
     return portId in ports;
   };
 
   // Hidden port creation function.  We don't want to expose an API that lets
   // people add arbitrary port IDs to the port list.
   function createPort(portId, opt_name) {
     if (ports[portId])
       throw new Error("Port '" + portId + "' already exists.");
     var port = new Port(portId, opt_name);
     ports[portId] = port;
-    messagingNatives.PortAddRef(portId);
     return port;
   };
 
   // Helper function for dispatchOnRequest.
   function handleSendRequestError(isSendMessage,
                                   responseCallbackPreserved,
                                   sourceExtensionId,
                                   targetExtensionId,
                                   sourceUrl) {
     var errorMsg;
@@ skipping 37 matching lines @@
       return false;
     if (!requestEvent.hasListeners())
       return false;
     var port = createPort(portId, channelName);
 
     function messageListener(request) {
       var responseCallbackPreserved = false;
       var responseCallback = function(response) {
         if (port) {
           port.postMessage(response);
-          privates(port).impl.destroy_();
+          // TODO(robwu): This can be changed to disconnect() because there is

[Devlin, 2016/05/17 19:50:30]

What's the reason to not do this now?

[robwu, 2016/05/17 22:47:51]

Because this would be a potential change in behavior. This CL is refactoring,
the next one is fixing the TODOs, and then finally another one to fix
crbug.com/597698.

If for some reason one of my assumptions in the TODO are incorrect, then
bisecting would point to the (small) follow-up CLs instead of this relatively
big patch.

+          // no point in allowing other receivers at this end of the port to
+          // keep the channel alive because the opener port can only receive one
+          // message.
+          privates(port).impl.disconnectSoftly();
           port = null;
         } else {
           // We nulled out port when sending the response, and now the page
           // is trying to send another response for the same request.
           handleSendRequestError(isSendMessage, responseCallbackPreserved,
                                  sourceExtensionId, targetExtensionId);
         }
       };
       // In case the extension never invokes the responseCallback, and also
       // doesn't keep a reference to it, we need to clean up the port. Do
       // so by attaching to the garbage collection of the responseCallback
       // using some native hackery.
       //
       // If the context is destroyed before this has a chance to execute,
       // BindToGC knows to release |portId| (important for updating C++ state
       // both in this renderer and on the other end). We don't need to clear
       // any JavaScript state, as calling destroy_() would usually do - but
       // the context has been destroyed, so there isn't any JS state to clear.
       messagingNatives.BindToGC(responseCallback, function() {
         if (port) {
-          privates(port).impl.destroy_();
+          privates(port).impl.disconnectSoftly();
           port = null;
         }
       }, portId);
       var rv = requestEvent.dispatch(request, sender, responseCallback);
       if (isSendMessage) {
         responseCallbackPreserved =
             rv && rv.results && $Array.indexOf(rv.results, true) > -1;
         if (!responseCallbackPreserved && port) {
           // If they didn't access the response callback, they're not
           // going to send a response, so clean up the port immediately.
-          privates(port).impl.destroy_();
+          privates(port).impl.disconnectSoftly();
           port = null;
         }
       }
     }
 
-    privates(port).impl.onDestroy_ = function() {
-      port.onMessage.removeListener(messageListener);
-    };
     port.onMessage.addListener(messageListener);
 
     var eventName = isSendMessage ? "runtime.onMessage" : "extension.onRequest";
     if (isExternal)
       eventName += "External";
     logActivity.LogEvent(targetExtensionId,
                          eventName,
                          [sourceExtensionId, sourceUrl]);
     return true;
   }
@@ skipping 76 matching lines @@
     logActivity.LogEvent(targetExtensionId,
                          eventName,
                          [sourceExtensionId]);
     return true;
   };
 
   // Called by native code when a channel has been closed.
   function dispatchOnDisconnect(portId, errorMessage) {
     var port = ports[portId];
     if (port) {
-      // Update the renderer's port bookkeeping, without notifying the browser.
-      messagingNatives.CloseChannel(portId, false);
+      delete ports[portId];
       if (errorMessage)
         lastError.set('Port', errorMessage, null, chrome);
       try {
         port.onDisconnect.dispatch(port);
       } finally {
         privates(port).impl.destroy_();
         lastError.clear(chrome);
       }
     }
   };
@@ skipping 62 matching lines @@
 
     function messageListener(response) {
       try {
         if (responseCallback)
           sendResponseAndClearCallback(response);
       } finally {
         port.disconnect();
       }
     }
 
-    privates(port).impl.onDestroy_ = function() {
-      port.onDisconnect.removeListener(disconnectListener);
-      port.onMessage.removeListener(messageListener);
-    };
     port.onDisconnect.addListener(disconnectListener);
     port.onMessage.addListener(messageListener);
   };
 
   function sendMessageUpdateArguments(functionName, hasOptionsArgument) {
     // skip functionName and hasOptionsArgument
     var args = $Array.slice(arguments, 2);
     var alignedArgs = messagingUtils.alignSendMessageArguments(args,
         hasOptionsArgument);
     if (!alignedArgs)
@@ skipping 22 matching lines @@
 exports.$set('Port', Port);
 exports.$set('createPort', createPort);
 exports.$set('sendMessageImpl', sendMessageImpl);
 exports.$set('sendMessageUpdateArguments', sendMessageUpdateArguments);
 
 // For C++ code to call.
 exports.$set('hasPort', hasPort);
 exports.$set('dispatchOnConnect', dispatchOnConnect);
 exports.$set('dispatchOnDisconnect', dispatchOnDisconnect);
 exports.$set('dispatchOnMessage', dispatchOnMessage);