Ensure that sendMessage's callback is called.

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 a15497d23efac334c9073830fcdd47d3237ac543..c1e93eed2101889ee5cc1e9e84c75af0fc77cdff 100644
--- a/extensions/renderer/resources/messaging.js
+++ b/extensions/renderer/resources/messaging.js
@@ -335,9 +335,14 @@
       return;
     }
 
-    // Ensure the callback exists for the older sendRequest API.
-    if (!responseCallback)
-      responseCallback = function() {};
+    function sendResponseIfNeeded(response) {
+      var sendResponse = responseCallback;

[Devlin, 2016/01/08 17:42:28]

Why the copy?

[robwu, 2016/01/08 17:53:20]

This is a bit defensive, to make sure that the callback is called once, if for
whatever reason the destroy/message event is triggered while handling this
message (e.g. due to a model dialog (JS alert) and two recipients who both reply
to the sendMessage call).

[Devlin, 2016/01/08 18:04:25]

That makes sense.  I think if that's the intention, it's more clear to put it
entirely within the if-block.
function() {
  if (responseCallback) {
    // Create a copy so that we don't re-entrantly call responseCallback.
    responseCallbackCopy = responseCallback;
    responseCallback = null;
    responseCallbackCopy(response);
  }
}

[robwu, 2016/01/08 18:14:24]

Added a comment.

+      if (sendResponse) {
+        responseCallback = null;
+        sendResponse(response);
+      }
+    }
+
 
     // Note: make sure to manually remove the onMessage/onDisconnect listeners
     // that we added before destroying the Port, a workaround to a bug in Port
@@ -346,14 +351,23 @@
     // http://crbug.com/320723 tracks a sustainable fix.
 
     function disconnectListener() {
-      // For onDisconnects, we only notify the callback if there was an error.
-      if (chrome.runtime && chrome.runtime.lastError)
-        responseCallback();
+      if (lastError.hasError(chrome)) {
+        sendResponseIfNeeded();
+      } else {
+        lastError.set(
+            port.name, 'The message port closed before a reponse was received.',

[Devlin, 2016/01/08 17:42:28]

I must be missing something - where is this listener removed when a response is
received?

[robwu, 2016/01/08 17:53:20]

Are you referring to the existing comment above? I guess at line 376-379 below.

[Devlin, 2016/01/08 18:04:25]

onDisconnect is called when the port is disconnected, but just because the port
is disconnected doesn't mean it didn't receive a response.  What stops this from
sending a lastError when a message was received and then the port is
disconnected?

[robwu, 2016/01/08 18:14:24]

Good point. Fixed!

+            null, chrome);
+        try {
+          sendResponseIfNeeded();
+        } finally {
+          lastError.clear(chrome);
+        }
+      }
     }
 
     function messageListener(response) {
       try {
-        responseCallback(response);
+        sendResponseIfNeeded(response);
       } finally {
         port.disconnect();
       }