834933 - PeerConnection instances throw errors without any details

Status: VERIFIED FIXED

(Core :: WebRTC, defect, P2)

Description

[Jason Smith [:jsmith]]

Steps:

1. Load https://bug829856.bugzilla.mozilla.org/attachment.cgi?id=701383
2. Click crash

Expected:

If we're getting an error in the error console, I'd expect we would see what the error is.

Actual:

We get an error, but there's no output. Are we throwing an exception without any error info here?

Comment 1

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

The fact that errors from PeerConnection.js show as blank in Web Console is a long-standing bug I noticed as well (the easiest way to reproduce is to disable peerconnection in about:config and create mozPeerConnection). I wrapped your call in try{ ...} catch(e) { console.log(e);} to see the error.

I suspect the problem strikes peerconnection.js because it is an xpcom js module or because it uses old XP IDL, or both, or we're throwing the wrong Error class.

We could try lobbing this bug to the team doing Web console and see if it comes back with instructions. :-)

Comment 2

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

After all, this may be a bug in web console.

Comment 3

[Jason Smith [:jsmith]]

Let's send this bug over to dev tools then and see what those guys think.

Comment 4

[Jason Smith [:jsmith]]

Tracking nom. A big point of the building WebRTC-based apps on desktop requires that developers actually understand the errors they are hitting. A non-functional error console for WebRTC-based errors sounds quite problematic against that objective.

Comment 5

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

Attachment: Simpler testcase with workaround

Here's a simpler test-case contrasted with a workaround.

Comment 6

[Mihai Sucan [:msucan]]

Thank you for looping us in. I'm going to look into this bug next week.

Comment 7

[bhavana bajaj [:bajaj]]

(In reply to Mihai Sucan [:msucan] from comment #6)
> Thank you for looping us in. I'm going to look into this bug next week.

Tracking and assigning this to you based on your comment. Please feel free to reassign if someone else would be working on this.Thanks !

Comment 8

[Mihai Sucan [:msucan]]

Investigation shows that the nsIScriptErrors we receive in the Web Console are without any message. They are empty.

This is what also affects the Error Console.

There's not much we can do in the Web Console to fix this issue. I don't see why exceptions/errors thrown in PeerConnection.js lack messages. Might be a problem related to XPCOM/XP IDL stuff which is beyond me.

Taking myself off the assignment because there's not much to do for me at this point. I also suggest the bug is moved into a different component (related to webrtc?).

Please let me know if there's anything I can help with. Feel free to ping me.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I see the same when running the WebRTC crashtests in the terminal. The exception thrown when calling addStream() on a closed PeerConnection instance doens't show any details.

I haven't seen this before with other components so as Mihai said it's not a web console problem. Moving back to WebRTC.

Comment 10

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> I see the same when running the WebRTC crashtests in the terminal. The
> exception thrown when calling addStream() on a closed PeerConnection
> instance doens't show any details.
> 
> I haven't seen this before with other components so as Mihai said it's not a
> web console problem. Moving back to WebRTC.

This happens in multiple cases and we've specified in above comments we wanted in that group to investigate to confirm where the problem is, as that doesn't provide sufficient info. Moving back over to dev tools again.

Comment 11

[Mihai Sucan [:msucan]]

(In reply to Jason Smith [:jsmith] from comment #10)
> (In reply to Henrik Skupin (:whimboo) from comment #9)
> > I see the same when running the WebRTC crashtests in the terminal. The
> > exception thrown when calling addStream() on a closed PeerConnection
> > instance doens't show any details.
> > 
> > I haven't seen this before with other components so as Mihai said it's not a
> > web console problem. Moving back to WebRTC.
> 
> This happens in multiple cases and we've specified in above comments we
> wanted in that group to investigate to confirm where the problem is, as that
> doesn't provide sufficient info. Moving back over to dev tools again.

Please let me know your questions and I'll try to answer them.

Comment 12

[Jason Smith [:jsmith]]

Ah, I didn't see comment 8. So this could be an IDL issue.

Comment 13

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

There ARE details in the exception, though probably not in the expected format (the attached workaround catches the exception in JS and outputs it to the console which shows the message).

But C++ Sub-Systems throw correctly:

Notably, errors from c++ sub-systems called BY PeerConnection.js DO get through, which narrows the problem to exceptions originating in PeerConnection.js itself.

To see this, search for NS_ERROR_MALFORMED_URI in PeerConnection.js, modify it to let this exception through unadulterated and pass in an invalid stun-url: new mozRTCPeerConnection({ "iceServers": [{ "url": "@#$%^&*(^)*&stun.example.org" }] });

Not only does this error appear fine in web console, but interestingly, clicking the "Workaround" button (console.log(e)), shows you this KNOWN-GOOD format that does get through, and it is vastly different from what PeerConnection.js produces:

[12:17:44.375] [Exception... "Component returned failure code: 0x804b000a (NS_ERROR_MALFORMED_URI) [nsIIOService.newURI]"  nsresult: "0x804b000a (NS_ERROR_MALFORMED_URI)"  location: "JS frame :: file:///Users/Jan/moz/mozilla-central/obj-x86_64-apple-darwin12.2.1-debug/dist/NightlyDebug.app/Contents/MacOS/components/PeerConnection.js :: nicerNewURI :: line 351"  data: no]

Comment 14

[Lukas Blakk [:lsblakk] use ?needinfo]

Adam: any progress here?

Comment 15

[Adam Roach [:abr]]

Lukas: I have several higher-priority bugs in my list at the moment. I'll begin investigation on this when those are under control.

Comment 16

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

Attachment: Some more probing

Saw this go by and wanted to write down my analysis from some probing I did earlier in the week when this bugged me. Good luck.

Comment 17

[Alex Keybl [:akeybl]]

Does this continue to block the webrtc feature? If so, this seems as if it should be P1. If the priority was set to P2 to denote being lower priority than other work in your queue Adam, perhaps we should assign elsewhere?

Comment 18

[Eric Rescorla (:ekr)]

There really isn't an elsewhere. Everyone else on WebRTC is already working on other high priority stuff.

Comment 19

[Adam Roach [:abr]]

Alex:

I have several WebRTC blockers in my queue (seven at the moment). They can't all be more important than each other, and slamming them all up to P1 just because they're marked as blockers is just as useless as leaving them all unmarked. I've prioritized the actual crasher bugs -- like "browser fall down go boom" -- as P1. Certainly you understand that a bug that happens to produce weak diagnostics doesn't rise to that level.

If you know of someone else who can work on this immediately and do a reasonable job with it, feel free to reassign elsewhere. WebRTC has far too few engineers working on it, and I doubt we'll turn away competent workers. I'd be happy to skip over this bug and work on other blockers.

Comment 20

[Jason Smith [:jsmith]]

Let's drop this from tracking. I don't think we need to start making use of the tracking flags just yet until we get close to release (< 10 blockers).

Comment 21

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

Attachment: PeerConnection.js throws Components.Exception so errors are readable

Simple fix in the end. Thanks dmose and bent!

Comment 22

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

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

Comment 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 725125 [details] [diff] [review]
PeerConnection.js throws Components.Exception so errors are readable

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

I don't own any of this code but it looks good to me!

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d08461205ce2

Comment 25

[Ryan VanderMeulen [:RyanVM]]

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

Comment 26

[Jason Smith [:jsmith]]

Verified on trunk with attached test case.