Ensure that sendMessage's callback is called.

Ensure that sendMessage's callback is called.

Also: unchecked errors are now also reported to the extension's console.
Unchecked errors were silenced before because the internal disconnect
handler accessed chrome.runtime.lastError.

BUG=439780
TEST=browser_tests --gtest_filter=ExtensionApiTest.MessagingCrash

Committed: https://crrev.com/3607f1773518f29bc704daf7de5a89961c2c4730
Cr-Commit-Position: refs/heads/master@{#368469}

[robwu, 2016-01-08 14:42:32]

Patchset #1 (id:1) has been deleted

[robwu, 2016-01-08 15:55:13]

rob@robwu.nl changed reviewers:
+ rdevlin.cronin@chromium.org

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

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:339: var sendResponse =
responseCallback;
Why the copy?

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:358: port.name, 'The message port
closed before a reponse was received.',
I must be missing something - where is this listener removed when a response is
received?

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

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:339: var sendResponse =
responseCallback;
On 2016/01/08 17:42:28, Devlin wrote:
> Why the copy?

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).

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:358: port.name, 'The message port
closed before a reponse was received.',
On 2016/01/08 17:42:28, Devlin wrote:
> I must be missing something - where is this listener removed when a response
is
> received?

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

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

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:339: var sendResponse =
responseCallback;
On 2016/01/08 17:53:20, robwu wrote:
> On 2016/01/08 17:42:28, Devlin wrote:
> > Why the copy?
> 
> 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).

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);
  }
}

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:358: port.name, 'The message port
closed before a reponse was received.',
On 2016/01/08 17:53:20, robwu wrote:
> On 2016/01/08 17:42:28, Devlin wrote:
> > I must be missing something - where is this listener removed when a response
> is
> > received?
> 
> Are you referring to the existing comment above? I guess at line 376-379
below.

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]

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:339: var sendResponse =
responseCallback;
On 2016/01/08 18:04:25, Devlin wrote:
> On 2016/01/08 17:53:20, robwu wrote:
> > On 2016/01/08 17:42:28, Devlin wrote:
> > > Why the copy?
> > 
> > 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).
> 
> 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);
>   }
> }

Added a comment.

https://codereview.chromium.org/1567423002/diff/20001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:358: port.name, 'The message port
closed before a reponse was received.',
On 2016/01/08 18:04:25, Devlin wrote:
> On 2016/01/08 17:53:20, robwu wrote:
> > On 2016/01/08 17:42:28, Devlin wrote:
> > > I must be missing something - where is this listener removed when a
response
> > is
> > > received?
> > 
> > Are you referring to the existing comment above? I guess at line 376-379
> below.
> 
> 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?

Good point. Fixed!

[Devlin, 2016-01-08 18:20:01]

lgtm

https://codereview.chromium.org/1567423002/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/1567423002/diff/40001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:353: if (!responseCallback)
nit: Maybe add a super short comment.
if (!responseCallback)  // Callback was already called; our work is done.

[robwu, 2016-01-08 18:22:44]

If you have no more remarks, just hit CQ.

https://codereview.chromium.org/1567423002/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/1567423002/diff/40001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:353: if (!responseCallback)
On 2016/01/08 18:20:01, Devlin wrote:
> nit: Maybe add a super short comment.
> if (!responseCallback)  // Callback was already called; our work is done.

Or the user did not supply a callback in the first place (the callback is
optional). Anyway, this is so trivial that I think that a comment is not
necessary.

[Devlin, 2016-01-08 21:48:09]

https://codereview.chromium.org/1567423002/diff/40001/extensions/renderer/res...
File extensions/renderer/resources/messaging.js (right):

https://codereview.chromium.org/1567423002/diff/40001/extensions/renderer/res...
extensions/renderer/resources/messaging.js:353: if (!responseCallback)
On 2016/01/08 18:22:44, robwu wrote:
> On 2016/01/08 18:20:01, Devlin wrote:
> > nit: Maybe add a super short comment.
> > if (!responseCallback)  // Callback was already called; our work is done.
> 
> Or the user did not supply a callback in the first place (the callback is
> optional). Anyway, this is so trivial that I think that a comment is not
> necessary.

Fair enough.

[Devlin, 2016-01-08 21:48:11]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2016-01-08 21:50:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1567423002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1567423002/40001

[commit-bot: I haz the power, 2016-01-09 00:07:54]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2016-01-09 00:08:54]

Description was changed from

==========
Ensure that sendMessage's callback is called.

Also: unchecked errors are now also reported to the extension's console.
Unchecked errors were silenced before because the internal disconnect
handler accessed chrome.runtime.lastError.

BUG=439780
TEST=browser_tests --gtest_filter=ExtensionApiTest.MessagingCrash
==========

to

==========
Ensure that sendMessage's callback is called.

Also: unchecked errors are now also reported to the extension's console.
Unchecked errors were silenced before because the internal disconnect
handler accessed chrome.runtime.lastError.

BUG=439780
TEST=browser_tests --gtest_filter=ExtensionApiTest.MessagingCrash

Committed: https://crrev.com/3607f1773518f29bc704daf7de5a89961c2c4730
Cr-Commit-Position: refs/heads/master@{#368469}
==========

[commit-bot: I haz the power, 2016-01-09 00:08:55]

Patchset 2 (id:??) landed as
https://crrev.com/3607f1773518f29bc704daf7de5a89961c2c4730
Cr-Commit-Position: refs/heads/master@{#368469}