1061673 - Improve error reporting of unrecognized packets

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

Today, if we send an unrecognized packet to a protocol.js actor we get in the console:
console.error: 
  unrecognizedPacketType

That's not really helpful...

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: patch

Hopefully, it doesn't break any test matching for just the error code...
https://tbpl.mozilla.org/?tree=Try&rev=e529bd21ef81

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8482725 [details] [diff] [review]
patch

Review of attachment 8482725 [details] [diff] [review]:
-----------------------------------------------------------------

You've now exceeded the number of "?" and ":" I can parse correctly...  If you keep this level of nesting, please change to regular ifs.

Also, your new format will hit the TBPL error filter we are trying to dodge in the case that there are any packet.error values that end in "Error" and also have a packet.message, but are not packet.error === "unknownError".  There are existing errors that would match this case[2].

I think a simpler approach would be to preface all messages with "Protocol error", so something like this:

let message = "Protocol error (" + packet.error + ")";
if (packet.message) {
  message += ": " + packet.message;
}

[1]: http://hg.mozilla.org/webtools/tbpl/annotate/e3e29619db98/php/inc/GeneralErrorFilter.php#l60
[2]: http://dxr.mozilla.org/mozilla-central/search?q=regexp%3A%22error%3A.*\%22[A-Za-z]%2BError\%22%22+path%3Adevtools+path%3Aactors&case=true&redirect=true

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: patch

I think you now have all the credit for this patch ;)

https://tbpl.mozilla.org/?tree=Try&rev=e76ab5a0d05a

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: patch

At the end, always converting the packet to a string ends up breaking
a webaudio actor test that was expecting an object with type and message attributes.

Here is a new version, similar to the first patch but with if statements.
https://tbpl.mozilla.org/?tree=Try&rev=12356283ce68

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Hmm, maybe we should just tweak the webaudio test then?  Because your most recent patch still has the possibility of triggering the TBPL error filter that we are trying to avoid.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: patch

(In reply to J. Ryan Stinnett [:jryans] from comment #5)
> Hmm, maybe we should just tweak the webaudio test then?  Because your most
> recent patch still has the possibility of triggering the TBPL error filter
> that we are trying to avoid.

I would prefer not to. I don't think it is a good idea to coalesce *all*
exceptions into a string.

In this patch, I'm using if blocks and only convert to TBPL-friendly strings
exceptions that have error and message attributes.

https://tbpl.mozilla.org/?tree=Try&rev=687dcc8b43f6

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8485671 [details] [diff] [review]
patch

Review of attachment 8485671 [details] [diff] [review]:
-----------------------------------------------------------------

Okay, seems reasonable enough to me.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/fx-team/rev/de6301ff59d2

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/de6301ff59d2