PeerConnection exception-messages in web-console regressed from useful to cryptic

RESOLVED FIXED in Firefox 25

Status

()

Core
WebRTC
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jib, Assigned: jib)

Tracking

({relnote})

Trunk
mozilla26
relnote
Points:
---

Firefox Tracking Flags

(firefox24 wontfix, firefox25 fixed, firefox26 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment)

Sigh. I'd either forgotten that our errors only show up in the less-used error console (not the web console), or it's a regression. It may have regressed when we switched to webidl.

This affects all exceptions from PeerConnection, and means web developers have to turn on and look in the Error Console, if they want to see informative error messages, beyond the NS_ERROR_Failure they'll see in web console.
Assignee: nobody → jib
I've now verified that this regressed when we switched to webidl. This means errors from RTCPeerConnection constructor and methods (not callbacks) stop being readable with Aurora.
Depends on: 905392
To re-summarize:

When we converted RTCPeerConnection to webidl in FF25, it's web-console exception-messages regressed from useful:

  NS_ERROR_MALFORMED_URI: RTCPeerConnection constructor passed invalid RTCConfig -
  improper scheme: stunn

To cryptic:

   NS_ERROR_UNEXPECTED: Unexpected error

Our calls didn't change:

  throw new Components.Exception(errorMsg + " - improper scheme: " + url.scheme,
                                 Cr.NS_ERROR_MALFORMED_URI);

I've tried other ways with no success:

  throw new Error(msg);
  throw new win.DOMError("NotSupportedError", "Improper scheme");
  throw publify(new Components.Exception()); //where "publify" uses Cu.createObjectIn

I've opened Bug 905392 to request a way to throw messages that make it to content JS.
Summary: PeerConnection friendly exceptions text should show in web console, not error console → PeerConnection exception-messages in web-console regressed from useful to cryptic
Target Milestone: --- → mozilla26
For testing: simple test (that produces an error message):

  var pc = new mozRTCPeerConnection({ iceServers: [{ url:"html:x.net" }]}, {});

I'll upload an automated test soon.
Created attachment 800931 [details] [diff] [review]
Uses new DOMError approach to get error messages out + unittests

Error messages are readable again.

- Puts new code in bug 905392 to use.
- Adds two unit-tests to verify that error messages are readable
  (I have verified that they fail without bug 905392 fixed).

Lets try - https://tbpl.mozilla.org/?tree=Try&rev=153265d80cc1
Attachment #800931 - Flags: review?(rjesup)

Updated

4 years ago
Attachment #800931 - Flags: review?(rjesup) → review+
Last try had an older 905392 in it. New try on inbound https://tbpl.mozilla.org/?tree=Try&rev=ce2933b581eb
Try looks good.
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e839e0432eb
status-firefox24: --- → unaffected
status-firefox25: --- → affected
status-firefox26: --- → affected
Keywords: checkin-needed
Comment on attachment 800931 [details] [diff] [review]
Uses new DOMError approach to get error messages out + unittests

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
Behavior regressed in FF24 when we converted PeerConnection to webidl (Bug 823512)

User impact if declined: 
WebRTC mozPeerConnection parsing errors show as "Unexpected error" in web console with no further details. These are all parsing errors meant to inform web makers about WebRTC-specific mistakes they've made in configuration structures that they've passed in (webidl catches basic structure mistakes only).

For instance, where FF23 web console would say:

  createOffer passed invalid constraints - unknown mandatory constraint:
  OfferToRecieveAudio

FF25 would say:
  Unexpected error

User would need to visit less-known Browser Console to see message instead.

Testing completed (on m-c, etc.): 
Patch includes changes to two existing mochitests that verify that errors are readable. Try shows them passing on all platform.
Separately observed working on OSX.

Risk to taking this patch (and alternatives if risky): 

Extremely low in this patch which only changes how error messages are thrown to use new feature. All risk is in dependent bug 903741 which must be approved before this bug.

No known alternatives.

String or IDL/UUID changes made by this patch: None

*NOTE THAT BUG 903741 MUST BE APPROVED FIRST* - I wrote this request first to aid someone who knows bug 903741 better (peterv or bz?) to write an approval request on that bug, since the benefits are hard to appreciate with this bug here.
Attachment #800931 - Flags: approval-mozilla-aurora?
hard to appreciate withOUT this bug here.
https://hg.mozilla.org/mozilla-central/rev/9e839e0432eb
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
This affects FF24 as well, though beta uplift is perhaps ambitious?
status-firefox24: unaffected → affected
(In reply to Jan-Ivar Bruaroey [:jib] from comment #11)
> This affects FF24 as well, though beta uplift is perhaps ambitious?

Agreed.  We can relnote it for 24
status-firefox24: affected → wontfix
Keywords: relnote
(In reply to Jan-Ivar Bruaroey [:jib] from comment #8)
> Risk to taking this patch (and alternatives if risky): 
> 
> Extremely low in this patch which only changes how error messages are thrown
> to use new feature. All risk is in dependent bug 903741 which must be
> approved before this bug.
> 
> No known alternatives.
> 
> String or IDL/UUID changes made by this patch: None
> 
> *NOTE THAT BUG 903741 MUST BE APPROVED FIRST* - I wrote this request first
> to aid someone who knows bug 903741 better (peterv or bz?) to write an
> approval request on that bug, since the benefits are hard to appreciate with
> this bug here.

903741 is this bug. Meant to bug 905392?
(In reply to Masatoshi Kimura [:emk] from comment #13)
> > *NOTE THAT BUG 903741 MUST BE APPROVED FIRST* - I wrote this request first
> > to aid someone who knows bug 903741 better (peterv or bz?) to write an
> > approval request on that bug, since the benefits are hard to appreciate with
> > this bug here.
> 
> 903741 is this bug. Meant to bug 905392?

Yes, my apologies. Bug 905392 must be approved first. Thanks.
Comment on attachment 800931 [details] [diff] [review]
Uses new DOMError approach to get error messages out + unittests

This is important for devs, and meets the risk bar for Aurora. Also approved bug 905392.
Attachment #800931 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/1d2a918f0f9d
status-firefox25: affected → fixed
status-firefox26: affected → fixed
Assuming no QA needed here. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs QA.
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.