Improve error reporting of unrecognized packets

RESOLVED FIXED in Firefox 35

Status

()

Firefox
Developer Tools: Framework
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: ochameau, Assigned: ochameau)

Tracking

unspecified
Firefox 35
x86_64
Windows 7
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

3 years ago
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...
(Assignee)

Comment 1

3 years ago
Created attachment 8482725 [details] [diff] [review]
patch

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

Updated

3 years ago
Attachment #8482725 - Flags: review?(jryans)
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
Attachment #8482725 - Flags: review?(jryans) → review-
(Assignee)

Comment 3

3 years ago
Created attachment 8484076 [details] [diff] [review]
patch

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

https://tbpl.mozilla.org/?tree=Try&rev=e76ab5a0d05a
Attachment #8482725 - Attachment is obsolete: true
(Assignee)

Comment 4

3 years ago
Created attachment 8484269 [details] [diff] [review]
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
Attachment #8484076 - Attachment is obsolete: true
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.
(Assignee)

Comment 6

3 years ago
Created attachment 8485671 [details] [diff] [review]
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
Attachment #8484269 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8485671 - Flags: review?(jryans)
Comment on attachment 8485671 [details] [diff] [review]
patch

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

Okay, seems reasonable enough to me.
Attachment #8485671 - Flags: review?(jryans) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/de6301ff59d2
Assignee: nobody → poirot.alex
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/de6301ff59d2
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 35
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.