Remove port lifetime management from renderer

extensions/renderer/resources/messaging.js

Index: extensions/renderer/resources/messaging.js
diff --git a/extensions/renderer/resources/messaging.js b/extensions/renderer/resources/messaging.js
index 8ca84ea46559aa91d4688c5a38b0e5f5a162ab27..30ca0e44fc94529966f2056278796b52faaa4bb1 100644
--- a/extensions/renderer/resources/messaging.js
+++ b/extensions/renderer/resources/messaging.js
@@ -21,6 +21,7 @@
   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};
@@ -53,13 +54,15 @@
     };
     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;
@@ -80,28 +83,24 @@
 
   // 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_];
   };
 
@@ -119,7 +118,6 @@
       throw new Error("Port '" + portId + "' already exists.");
     var port = new Port(portId, opt_name);
     ports[portId] = port;
-    messagingNatives.PortAddRef(portId);
     return port;
   };
 
@@ -177,7 +175,11 @@
       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
@@ -198,7 +200,7 @@
       // 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);
@@ -209,15 +211,12 @@
         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";
@@ -314,8 +313,7 @@
   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 {
@@ -398,10 +396,6 @@
       }
     }
 
-    privates(port).impl.onDestroy_ = function() {
-      port.onDisconnect.removeListener(disconnectListener);
-      port.onMessage.removeListener(messageListener);
-    };
     port.onDisconnect.addListener(disconnectListener);
     port.onMessage.addListener(messageListener);
   };