Suppress the exception thrown when responding to message with no response callback.

Suppress the exception thrown when responding to message with no response callback.

BUG=414483
Committed: https://crrev.com/bd3ad7a6a3a94a532a3ee3e73f3c92c2f1b746e5
Cr-Commit-Position: refs/heads/master@{#297111}

[Anand Mistry (off Chromium), 2014-09-22 05:39:14]

amistry@chromium.org changed reviewers:
+ rlp@chromium.org

[rpetterson, 2014-09-22 23:04:41]

https://codereview.chromium.org/587153002/diff/1/chrome/browser/resources/hot...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/1/chrome/browser/resources/hot...
chrome/browser/resources/hotword_helper/manager.js:79: if (err.message ==
'Attempting to use a disconnected port object')
Weird question, but is this always going to be in english or the same phrasing?
It would seem that an error code would be better if possible.

[Anand Mistry (off Chromium), 2014-09-22 23:50:42]

amistry@chromium.org changed reviewers:
+ dbeam@chromium.org

[Anand Mistry (off Chromium), 2014-09-22 23:50:43]

https://codereview.chromium.org/587153002/diff/1/chrome/browser/resources/hot...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/1/chrome/browser/resources/hot...
chrome/browser/resources/hotword_helper/manager.js:79: if (err.message ==
'Attempting to use a disconnected port object')
On 2014/09/22 23:04:41, rpetterson wrote:
> Weird question, but is this always going to be in english or the same
phrasing?
> It would seem that an error code would be better if possible.

It's an untranslated constant in
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...

I realise this is brittle, but I'm afraid to change this exception to be
something other than a string since it might break existing apps which have
discovered this (undocumented) behaviour.

[rpetterson, 2014-09-22 23:53:03]

lgtm

https://codereview.chromium.org/587153002/diff/1/chrome/browser/resources/hot...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/1/chrome/browser/resources/hot...
chrome/browser/resources/hotword_helper/manager.js:79: if (err.message ==
'Attempting to use a disconnected port object')
On 2014/09/22 23:50:43, Anand Mistry wrote:
> On 2014/09/22 23:04:41, rpetterson wrote:
> > Weird question, but is this always going to be in english or the same
> phrasing?
> > It would seem that an error code would be better if possible.
> 
> It's an untranslated constant in
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
> 
> I realise this is brittle, but I'm afraid to change this exception to be
> something other than a string since it might break existing apps which have
> discovered this (undocumented) behaviour.

Understood. Please add a note to the comment listing where this string is
defined.

[Anand Mistry (off Chromium), 2014-09-23 00:31:30]

https://codereview.chromium.org/587153002/diff/1/chrome/browser/resources/hot...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/1/chrome/browser/resources/hot...
chrome/browser/resources/hotword_helper/manager.js:79: if (err.message ==
'Attempting to use a disconnected port object')
On 2014/09/22 23:53:03, rpetterson wrote:
> On 2014/09/22 23:50:43, Anand Mistry wrote:
> > On 2014/09/22 23:04:41, rpetterson wrote:
> > > Weird question, but is this always going to be in english or the same
> > phrasing?
> > > It would seem that an error code would be better if possible.
> > 
> > It's an untranslated constant in
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/rendere...
> > 
> > I realise this is brittle, but I'm afraid to change this exception to be
> > something other than a string since it might break existing apps which have
> > discovered this (undocumented) behaviour.
> 
> Understood. Please add a note to the comment listing where this string is
> defined.

Done.

[Dan Beam, 2014-09-23 03:35:38]

https://codereview.chromium.org/587153002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/20001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:74:
sendResponse({'doNotShowOptinMessage': true});
^ which of these lines actually throw?

https://codereview.chromium.org/587153002/diff/20001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:81: if (err.message ==
'Attempting to use a disconnected port object')
does sendResponse() need to be invoked here?

[Anand Mistry (off Chromium), 2014-09-23 03:55:07]

https://codereview.chromium.org/587153002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/20001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:74:
sendResponse({'doNotShowOptinMessage': true});
On 2014/09/23 03:35:38, Dan Beam wrote:
> ^ which of these lines actually throw?

In theory, any of these calls to sendResponse() could throw (hence the large try
block), but it's the last one that I've seen in practice.

https://codereview.chromium.org/587153002/diff/20001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:81: if (err.message ==
'Attempting to use a disconnected port object')
On 2014/09/23 03:35:38, Dan Beam wrote:
> does sendResponse() need to be invoked here?

No. It's the sendResponse() that threw the exception. I've clarified the comment
to say that sendResponse throws.

[Dan Beam, 2014-09-23 04:14:01]

https://codereview.chromium.org/587153002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:61: try {
nit: if sendReponse() is what throws, i'd think it's best to do try/catch only
around that call, e.g.

  var response = ...;
  if (...)
    response = ...;
  else if (...)
    response = ...;

  try {
    sendResponse(response);
  } catch(e) {
    ...
  }

https://codereview.chromium.org/587153002/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:73:
chrome.tabs.executeScript(tab.id, {'file': 'audio_client.js'});
nit: \n

https://codereview.chromium.org/587153002/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:81: if (err.message ==
'Attempting to use a disconnected port object')
can you write a test that exercises this case? (so there's a shred of hope of it
breaking if the error message changes)

[Anand Mistry (off Chromium), 2014-09-24 01:11:40]

amistry@chromium.org changed reviewers:
+ asargent@chromium.org

[Anand Mistry (off Chromium), 2014-09-24 01:11:41]

asargent@chromium.org: Please review changes in hotword_browsertest.cc

https://codereview.chromium.org/587153002/diff/40001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:61: try {
On 2014/09/23 04:14:00, Dan Beam wrote:
> nit: if sendReponse() is what throws, i'd think it's best to do try/catch only
> around that call, e.g.
> 
>   var response = ...;
>   if (...)
>     response = ...;
>   else if (...)
>     response = ...;
> 
>   try {
>     sendResponse(response);
>   } catch(e) {
>     ...
>   }

Done.

https://codereview.chromium.org/587153002/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:73:
chrome.tabs.executeScript(tab.id, {'file': 'audio_client.js'});
On 2014/09/23 04:14:00, Dan Beam wrote:
> nit: \n

Done.

https://codereview.chromium.org/587153002/diff/40001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:81: if (err.message ==
'Attempting to use a disconnected port object')
On 2014/09/23 04:14:00, Dan Beam wrote:
> can you write a test that exercises this case? (so there's a shred of hope of
it
> breaking if the error message changes)

I've given it a shot.

[Dan Beam, 2014-09-24 01:37:35]

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/hotword_browsertest.cc (right):

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/hotword_browsertest.cc:26: base::FilePath
component_extension_dir_;
nit: make this member private and expose via

  const base::FilePath& component_extension_dir() const {
    return component_extension_dir_;
  }

if you can

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/hotword_browsertest.cc:45: CHECK(error_console_);
nit: CHECK() -> ASSERT_TRUE() if you can

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:72: response =
{'doNotShowOptinMessage': true};
how's this different from what |response| already is?

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:113: sendResponse));
nit:

  chrome.hotwordPrivate.getStatus(this.injectTab_.bind(
      this,
      request.tab || sender.tab || {incognito: true},
      sendResponse));

[Anand Mistry (off Chromium), 2014-09-24 03:44:12]

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/hotword_browsertest.cc (right):

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/hotword_browsertest.cc:26: base::FilePath
component_extension_dir_;
On 2014/09/24 01:37:35, Dan Beam wrote:
> nit: make this member private and expose via
> 
>   const base::FilePath& component_extension_dir() const {
>     return component_extension_dir_;
>   }
> 
> if you can

Done.

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/extension...
chrome/browser/extensions/hotword_browsertest.cc:45: CHECK(error_console_);
On 2014/09/24 01:37:35, Dan Beam wrote:
> nit: CHECK() -> ASSERT_TRUE() if you can

Done.

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:72: response =
{'doNotShowOptinMessage': true};
On 2014/09/24 01:37:35, Dan Beam wrote:
> how's this different from what |response| already is?

Oops. Just translated that part blindly.

https://codereview.chromium.org/587153002/diff/60001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:113: sendResponse));
On 2014/09/24 01:37:35, Dan Beam wrote:
> nit:
> 
>   chrome.hotwordPrivate.getStatus(this.injectTab_.bind(
>       this,
>       request.tab || sender.tab || {incognito: true},
>       sendResponse));

Done.

[Dan Beam, 2014-09-24 04:58:27]

lgtm

https://codereview.chromium.org/587153002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/hotword_browsertest.cc (right):

https://codereview.chromium.org/587153002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/hotword_browsertest.cc:70: base::FilePath
component_extension_dir_;
nit: DISALLOW_COPY_AND_ASSIGN()

https://codereview.chromium.org/587153002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/80001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:71: if (!(tab.incognito ||
!hotwordStatus.available)) {
nit: if (!tab.incognito && hotwordState.available) {

[Anand Mistry (off Chromium), 2014-09-24 06:32:40]

https://codereview.chromium.org/587153002/diff/80001/chrome/browser/extension...
File chrome/browser/extensions/hotword_browsertest.cc (right):

https://codereview.chromium.org/587153002/diff/80001/chrome/browser/extension...
chrome/browser/extensions/hotword_browsertest.cc:70: base::FilePath
component_extension_dir_;
On 2014/09/24 04:58:27, Dan Beam wrote:
> nit: DISALLOW_COPY_AND_ASSIGN()

Done.

https://codereview.chromium.org/587153002/diff/80001/chrome/browser/resources...
File chrome/browser/resources/hotword_helper/manager.js (right):

https://codereview.chromium.org/587153002/diff/80001/chrome/browser/resources...
chrome/browser/resources/hotword_helper/manager.js:71: if (!(tab.incognito ||
!hotwordStatus.available)) {
On 2014/09/24 04:58:27, Dan Beam wrote:
> nit: if (!tab.incognito && hotwordState.available) {

Done.

[asargent_no_longer_on_chrome, 2014-09-24 19:20:00]

https://codereview.chromium.org/587153002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/hotword_browsertest.cc (right):

https://codereview.chromium.org/587153002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/hotword_browsertest.cc:32: "force-fieldtrials",
"VoiceTrigger/Install/");
nit: Are there constants either of these strings that you can use instead of
duplicating the literals?

https://codereview.chromium.org/587153002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/hotword_browsertest.cc:79:
"dnhpdliibojhegemfjheidglijccjfmc", false);
nit: use a constant to avoid the id string duplication here

https://codereview.chromium.org/587153002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/hotword_browsertest.cc:87: RunMessageLoopForMs(200);
In the past doing this sort of "sleep for a fixed amount of time" approach has
not worked out well. One problem is that the production buildbots run in virtual
machines on nearly oversubscribed hardware, and things that take only a few
hundred MS on our fast z620's take substantially longer. 

A much better approach is to add some sort of event that gets fired from within
the extension once you're past the place where the error could happen, and
listen for it here. I know that's more work but I think it is probably worth it
to end up with a less flaky test.

[Anand Mistry (off Chromium), 2014-09-25 00:53:23]

https://codereview.chromium.org/587153002/diff/140001/chrome/browser/extensio...
File chrome/browser/extensions/hotword_browsertest.cc (right):

https://codereview.chromium.org/587153002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/hotword_browsertest.cc:32: "force-fieldtrials",
"VoiceTrigger/Install/");
On 2014/09/24 19:19:59, asargent wrote:
> nit: Are there constants either of these strings that you can use instead of
> duplicating the literals?

Yes for the first, no for the second.

https://codereview.chromium.org/587153002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/hotword_browsertest.cc:79:
"dnhpdliibojhegemfjheidglijccjfmc", false);
On 2014/09/24 19:19:59, asargent wrote:
> nit: use a constant to avoid the id string duplication here

Done.

https://codereview.chromium.org/587153002/diff/140001/chrome/browser/extensio...
chrome/browser/extensions/hotword_browsertest.cc:87: RunMessageLoopForMs(200);
On 2014/09/24 19:19:59, asargent wrote:
> In the past doing this sort of "sleep for a fixed amount of time" approach has
> not worked out well. One problem is that the production buildbots run in
virtual
> machines on nearly oversubscribed hardware, and things that take only a few
> hundred MS on our fast z620's take substantially longer. 
> 
> A much better approach is to add some sort of event that gets fired from
within
> the extension once you're past the place where the error could happen, and
> listen for it here. I know that's more work but I think it is probably worth
it
> to end up with a less flaky test. 

Done.

[asargent_no_longer_on_chrome, 2014-09-26 20:17:32]

lgtm, sorry for delay

[Anand Mistry (off Chromium), 2014-09-27 00:15:47]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-09-27 00:16:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/587153002/180001

[commit-bot: I haz the power, 2014-09-27 06:17:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-27 06:17:35]

Exceeded time limit waiting for builds to trigger.

[Anand Mistry (off Chromium), 2014-09-27 08:13:52]

The CQ bit was checked by amistry@chromium.org

[commit-bot: I haz the power, 2014-09-27 08:14:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/587153002/180001

[commit-bot: I haz the power, 2014-09-27 09:12:48]

Committed patchset #10 (id:180001) as 240370d5b7b6f59bd14eec62a4c9a24a5ca22068

[commit-bot: I haz the power, 2014-09-27 09:13:29]

Patchset 10 (id:??) landed as
https://crrev.com/bd3ad7a6a3a94a532a3ee3e73f3c92c2f1b746e5
Cr-Commit-Position: refs/heads/master@{#297111}