Issue 4579 - Ignore runtime.lastError caused by wrapper

Issue 4579 - Ignore runtime.lastError caused by wrapper

[Manish Jethani, 2017-10-19 01:59:46]

Patch Set 1

Sometimes we have code like this:

  browser.runtime.sendMessage({type: "composer.ready"});

This causes an error with the new wrapper because we're passing in a callback,
Chrome thinks we're interested in the response, there is no response so it's an
error.

We should check if the caller is actually interested in the response. If not, we
can ignore the error.

https://codereview.adblockplus.org/29582716/diff/29582717/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29582716/diff/29582717/polyfill.js#newcode52
polyfill.js:52: const fulfillmentInterestFlag = Symbol();
We use a symbol here so it becomes like a private property.

https://codereview.adblockplus.org/29582716/diff/29582717/polyfill.js#newcode56
polyfill.js:56: return Reflect.construct(Promise, args, SpecialPromise);
Essentially an extended promise.

[Sebastian Noack, 2017-10-19 03:30:49]

I just tested on Firefox, and it seems in order to achieve consistent behavior,
the solution is much simpler:

  if (error &&
      error != "The message port closed before a response was received.")
    reject(error);
  else
    resolve(result);

On Firefox, even if a callback has been attached to the promise and no response
is sent, the promise is just resolved successfully with undefined. So I guess,
we should do the same, as it is simpler anyway, and gives us the same semantics
across platforms.

[Manish Jethani, 2017-10-19 10:13:03]

On 2017/10/19 03:30:49, Sebastian Noack wrote:
> I just tested on Firefox, and it seems in order to achieve consistent
behavior,
> the solution is much simpler:
>
> [...]

OK, that was some unnecessary second-guessing on my part. I should have checked,
I just assumed Firefox would try to be fully compatible with Chrome.

[Manish Jethani, 2017-10-19 10:13:23]

Patch Set 2: Remove unnecessary code

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js#newcode46
polyfill.js:46: // Errors that occur only when we show an interest in the
response from an
We should keep this array because there are more such errors.

[kzar, 2017-10-19 10:17:49]

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js#newcode46
polyfill.js:46: // Errors that occur only when we show an interest in the
response from an
On 2017/10/19 10:13:23, Manish Jethani wrote:
> We should keep this array because there are more such errors.

Maybe a Set would be a better data structure to use?

I also wonder if we should export this, so it's transparent to the rest of the
code what we're ignoring. But I guess we can do that when it becomes necessary.

[Manish Jethani, 2017-10-19 10:24:46]

Ollie, can you help me figure out if this is appropriate for Edge?

On Chrome if you call chrome.runtime.sendMessage with a callback, and there's no
response from the other side, it's considered to be an error.
chrome.runtime.lastError.message is a specific string in this case. Since we're
wrapping the API to make it return a promise, we must pass a callback, but the
caller is still not interested in the response so we must ignore the error.

What I don't know about Edge:

 1.  Does it also set runtime.lastError when you pass a callback to
runtime.sendMessage and there's no response from the other side?

 2.  What is the error message?

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js#newcode46
polyfill.js:46: // Errors that occur only when we show an interest in the
response from an
Regarding the error message itself, I should go back and check earlier Chrome
versions. I'll also include Ollie here to verify the behavior on Edge.

[Manish Jethani, 2017-10-19 10:30:32]

Patch Set 3: Use Set object for error list

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js#newcode46
polyfill.js:46: // Errors that occur only when we show an interest in the
response from an
On 2017/10/19 10:17:49, kzar wrote:
> On 2017/10/19 10:13:23, Manish Jethani wrote:
> > We should keep this array because there are more such errors.
> 
> Maybe a Set would be a better data structure to use?

Good point, done.

> I also wonder if we should export this, so it's transparent to the rest of the
> code what we're ignoring. But I guess we can do that when it becomes
necessary.

Sounds like it might be a good idea, though I agree let's wait until it's
actually needed.

[Sebastian Noack, 2017-10-19 18:30:19]

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js#newcode46
polyfill.js:46: // Errors that occur only when we show an interest in the
response from an
On 2017/10/19 10:13:23, Manish Jethani wrote:
> We should keep this array because there are more such errors.

We can easily turn it into an array, set or regexp when needed. But as long as
we only checking for a single string, I think we should not over-engineer our
data structures for some potential future use case.

But yeah, we should probably figure out the behavior (and error message if any)
on Microsoft Edge first.

[kzar, 2017-10-19 18:37:40]

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js#newcode46
polyfill.js:46: // Errors that occur only when we show an interest in the
response from an
On 2017/10/19 18:30:18, Sebastian Noack wrote:
> But as long as we only checking for a single string, I think we should
> not over-engineer our data structures for some potential future use case.

Yea I'm pretty sure we all agree there, if we can just compare one string we
should do that instead.

[Sebastian Noack, 2017-10-19 22:08:31]

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29582716/diff/29583557/polyfill.js#newcode46
polyfill.js:46: // Errors that occur only when we show an interest in the
response from an
On 2017/10/19 18:37:39, kzar wrote:
> On 2017/10/19 18:30:18, Sebastian Noack wrote:
> > But as long as we only checking for a single string, I think we should
> > not over-engineer our data structures for some potential future use case.
> 
> Yea I'm pretty sure we all agree there, if we can just compare one string we
> should do that instead.

Ollie just confirmed, that on Microsoft Edge no error occurs if no response is
sent. So it seems this is the only error message string we have to check for.
Hence a Set or array seems unnecessary.

[Oleksandr, 2017-10-19 22:13:32]

On 2017/10/19 10:24:46, Manish Jethani wrote:
> Ollie, can you help me figure out if this is appropriate for Edge?
> 
> On Chrome if you call chrome.runtime.sendMessage with a callback, and there's
no
> response from the other side, it's considered to be an error.
> chrome.runtime.lastError.message is a specific string in this case. Since
we're
> wrapping the API to make it return a promise, we must pass a callback, but the
> caller is still not interested in the response so we must ignore the error.
> 
> What I don't know about Edge:
> 
>  1.  Does it also set runtime.lastError when you pass a callback to
> runtime.sendMessage and there's no response from the other side?

No. Calling something like:

browser.runtime.sendMessage({
    type: "notavail", what: "notavail"
  }, 
  function(response) {
    console.log(response); 
    console.log(browser.runtime.lastError);
  });

Never calls the callback, doesn't throw and browser.runtime.lastError remained
'undefined'

[Manish Jethani, 2017-10-19 23:52:55]

On 2017/10/19 22:13:32, Oleksandr wrote:
> On 2017/10/19 10:24:46, Manish Jethani wrote:
> > Ollie, can you help me figure out if this is appropriate for Edge?
> > 
> > On Chrome if you call chrome.runtime.sendMessage with a callback, and
there's
> no
> > response from the other side, it's considered to be an error.
> > chrome.runtime.lastError.message is a specific string in this case. Since
> we're
> > wrapping the API to make it return a promise, we must pass a callback, but
the
> > caller is still not interested in the response so we must ignore the error.
> > 
> > What I don't know about Edge:
> > 
> >  1.  Does it also set runtime.lastError when you pass a callback to
> > runtime.sendMessage and there's no response from the other side?
> 
> No. Calling something like:
> 
> browser.runtime.sendMessage({
>     type: "notavail", what: "notavail"
>   }, 
>   function(response) {
>     console.log(response); 
>     console.log(browser.runtime.lastError);
>   });
> 
> Never calls the callback, doesn't throw and browser.runtime.lastError remained
> 'undefined'

What if you modify the following line in composer.postload.js?

  ext.backgroundPage.sendMessage({type: "composer.ready"});

To this:

  ext.backgroundPage.sendMessage({type: "composer.ready"}, response =>
  {
    if (chrome.runtime.lastError)
      console.error(chrome.runtime.lastError);
    else
      console.log("Response:", response);
  });

And then reload the extension and load a page?

The reason I'm asking is that we have to be sure about how it behaves,
especially when there's a value message (like "compose.ready") but the sender
just isn't interested in the response.

[Manish Jethani, 2017-10-20 01:13:07]

Patch Set 4: Add old error message with typo

I tested with Chrome 50, there's a typo in older versions of Chrome (before June
30, 2017).

[Manish Jethani, 2017-10-20 01:19:04]

Here's where this is done:

https://cs.chromium.org/chromium/src/extensions/renderer/resources/messaging....

This might be the only error of this kind, though there are two versions (with
and without typo). I think I'll just use a regular expression here instead of a
set.

[Sebastian Noack, 2017-10-20 01:26:31]

https://codereview.adblockplus.org/29582716/diff/29583638/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29582716/diff/29583638/polyfill.js#newcode53
polyfill.js:53: ]);
Perhaps, we can just use a regexp to account for this typo? Also, do you happen
to know which Chrome versions had that typo? So that we can document these
versions here, and simplify the logic once we drop support for these versions.

[Manish Jethani, 2017-10-20 01:45:33]

Patch Set 5: Rebase, use regex

https://codereview.adblockplus.org/29582716/diff/29583638/polyfill.js
File polyfill.js (right):

https://codereview.adblockplus.org/29582716/diff/29583638/polyfill.js#newcode53
polyfill.js:53: ]);
On 2017/10/20 01:26:30, Sebastian Noack wrote:
> Perhaps, we can just use a regexp to account for this typo? Also, do you
happen
> to know which Chrome versions had that typo? So that we can document these
> versions here, and simplify the logic once we drop support for these versions.

Yes, using a regex now.

The commit was on June 30, 2017, this means it was probably fixed in Chrome 60.
I'll just link to the commit so that way when the lowest supported version is
closer to that date we can figure it out.

[Sebastian Noack, 2017-10-20 01:47:31]

LGTM

[Oleksandr, 2017-10-20 08:43:37]

On 2017/10/19 23:52:55, Manish Jethani wrote:
> On 2017/10/19 22:13:32, Oleksandr wrote:
> > On 2017/10/19 10:24:46, Manish Jethani wrote:
> > > Ollie, can you help me figure out if this is appropriate for Edge?
> > > 
> > > On Chrome if you call chrome.runtime.sendMessage with a callback, and
> there's
> > no
> > > response from the other side, it's considered to be an error.
> > > chrome.runtime.lastError.message is a specific string in this case. Since
> > we're
> > > wrapping the API to make it return a promise, we must pass a callback, but
> the
> > > caller is still not interested in the response so we must ignore the
error.
> > > 
> > > What I don't know about Edge:
> > > 
> > >  1.  Does it also set runtime.lastError when you pass a callback to
> > > runtime.sendMessage and there's no response from the other side?
> > 
> > No. Calling something like:
> > 
> > browser.runtime.sendMessage({
> >     type: "notavail", what: "notavail"
> >   }, 
> >   function(response) {
> >     console.log(response); 
> >     console.log(browser.runtime.lastError);
> >   });
> > 
> > Never calls the callback, doesn't throw and browser.runtime.lastError
remained
> > 'undefined'
> 
> What if you modify the following line in composer.postload.js?
> 
>   ext.backgroundPage.sendMessage({type: "composer.ready"});
> 
> To this:
> 
>   ext.backgroundPage.sendMessage({type: "composer.ready"}, response =>
>   {
>     if (chrome.runtime.lastError)
>       console.error(chrome.runtime.lastError);
>     else
>       console.log("Response:", response);
>   });
> 
> And then reload the extension and load a page?
> 
> The reason I'm asking is that we have to be sure about how it behaves,
> especially when there's a value message (like "compose.ready") but the sender
> just isn't interested in the response.

I think what I have posted above did just that - sent a message where there is
no response. But to be sure, I have tried your suggestion and the callback just
isn't called, so nothing in console.

[Manish Jethani, 2017-10-20 14:11:49]

On 2017/10/20 08:43:37, Oleksandr wrote:
> [...]
>
> >{...]
> >
> > The reason I'm asking is that we have to be sure about how it behaves,
> > especially when there's a value message (like "compose.ready") but the
sender
> > just isn't interested in the response.

Sorry, that was a typo, I meant "valid" message. e.g. a message the background
page does listen for and returns true (asynchronous) in its runtime.onMessage
handler, but never sends any response for whatever reason. We may still see an
error like this in that case. But I realize that "composer.ready" is not such a
message.

> I think what I have posted above did just that - sent a message where there is
> no response. But to be sure, I have tried your suggestion and the callback
just
> isn't called, so nothing in console.

Thanks a lot, this really helps.

Let's assume this is how Edge behaves in all cases, and if there are any issues
(there probably will be one or two since this API wrapping is essentially a
hack) we can address them as we discover them.

I have closed this.