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

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: 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

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Last try had an older 905392 in it. New try on inbound https://tbpl.mozilla.org/?tree=Try&rev=ce2933b581eb

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Try looks good.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e839e0432eb

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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.

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

hard to appreciate withOUT this bug here.

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/9e839e0432eb

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

This affects FF24 as well, though beta uplift is perhaps ambitious?

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

(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

Comment 13

[Masatoshi Kimura [:emk]]

(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?

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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 15

[Alex Keybl [:akeybl]]

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.

Comment 16

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1d2a918f0f9d

Comment 17

[u279076]

Assuming no QA needed here. Please remove [qa-] from the whiteboard and add the verifyme keyword if this needs QA.