Open Bug 978617 Opened 10 years ago Updated 2 years ago

WebRTC: System JS throws several errors

Categories

(Core :: WebRTC, defect, P4)

x86_64
macOS
defect

Tracking

()

People

(Reporter: jruderman, Unassigned)

References

Details

(Keywords: testcase)

Attachments

(1 file)

Attached file testcase
System JS : ERROR (null):0 - Permission denied to access property 'toString'
System JS : ERROR (null):0 - Permission denied to access property 'message'
System JS : ERROR (null):0 - uncaught exception: unknown (can't convert to string)

JavaScript error: file:///Users/jruderman/Desktop/b.html, line 13: NS_ERROR_UNEXPECTED:

(Also, why are the filenames and line numbers missing?)
Filed bug 978618 on the line number issue.
Byron, I think this is coming from your code (as is bug 978618).  If I'm wrong, can you reassign this to jib?  (I'm doing the same for bug 978618.)   I'd love to get this fixed soon (especially if it's a quick fix.)
Assignee: nobody → docfaraday
The errors look similar, and probably has the same underlying cause as bug 962109, but I suspect my code has nothing to do with it. I get the same behavior when running with the patches from 958221. I can try to dig into the underlying cause though.
Now that bug 978618 is fixed, I get a line number for the initial error.

System JS : ERROR resource://gre/components/PeerConnection.js:337 - NS_ERROR_FAILURE:

JavaScript error: file:///Users/jruderman/Desktop/b.html, line 13: NS_ERROR_UNEXPECTED:
My suspicion is that this is because the turn URL is malformed, but most of the validation logic lives in c++ code.
Jesse passes in a TURN url that slips past the JS validation and gets caught by the c++ validation code (it's what he does ;-). - I don't think the webrtc code is doing anything wrong here. We do preliminary validation in JS because it gives nicer errors in web console, but it's not meant to catch everything.

I think we should, however convert Jesse's test into a mochitest to cover our general c++ error recovery here.
It's usually a bug when chrome JS does not try..catch something that can throw.

It's usually a bug when an "NS_" error (rather than an error mentioned in a DOM spec) makes its way into a web page.
It should definitely throw in this case, but "NS_" errors from a spec API is not great, you're right.

But blanket try..catch around NS_ERROR_UNEXPECTED sounds wrong also. Is the answer to replumb our c++ calls to take ErrorResult, so we can ThrowTypeError()? Or DOMError/DOMExceptions or something else?

If there's code we can look at to model after here that would be great.

Our errors are a bit in the air, the spec seems to be settling on DOMError for callbacks but does not mention DOMException for instance.
> It should definitely throw in this case,

Per spec?  What does the spec say we should throw in this case?

The NS_ERROR_UNEXPECTED you're seeing here is for cases when a JS-implemented WebIDL implementation throws an exception that was NOT expected to be thrown.  When you're throwing an exception that is expected to be thrown per spec, you should be doing so explicitly.

I just realized we had no documentation for this; I wrote some up at https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Throwing_exceptions_from_JS-implemented_APIs

> the spec seems to be settling on DOMError for callbacks 

DOMError is slated to be removed from the DOM spec.  See <http://dom.spec.whatwg.org/#interface-domerror>.  So I would really we didn't create specs using it...  Yes, I know the mechanism at the link above uses it, but that's not web-exposed.
(In reply to Boris Zbarsky [:bz] from comment #9)
> > It should definitely throw in this case,
> 
> Per spec?  What does the spec say we should throw in this case?

SyntaxError (yes, we threw darts at the dom errors in November).

http://dev.w3.org/2011/webrtc/editor/webrtc.html#operation then follow link to UpdateIce() in step 1.

> I just realized we had no documentation for this; I wrote some up at
> https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings#Throwing_exceptions_from_JS-implemented_APIs

Thanks!

> DOMError is slated to be removed from the DOM spec.  See
> <http://dom.spec.whatwg.org/#interface-domerror>.  So I would really we
> didn't create specs using it...  Yes, I know the mechanism at the link above
> uses it, but that's not web-exposed.

So the spec should prepare to return DOMException, even as args in error callbacks? - I understand the logic, but the naming seems unfortunate. Generally speaking, an "exception" seems like a mechanism for returning "errors", not the other way around.

Any reason the DOMException name won? Odd, as it seems less awkward to throw DOMError somehow.
> SyntaxError (yes, we threw darts at the dom errors in November).

OK.  So since this is a case you know the spec requires you to throw in, you should do the DOMError stuff from the documentation for now.

> So the spec should prepare to return DOMException, even as args in error callbacks?

That's a question for Anne, not me.
Flags: needinfo?(annevk)
So, looking at that documentation, the current behavior when our c++ code returns an internal error is the same thing that would happen if our chrome JS just threw an exception without using a DOMError from the content window? If so, it seems that things are consistent. We could improve the garbage checking in our JS to improve the quality of feedback to the app, though.
> is the same thing that would happen if our chrome JS just threw an exception without
> using a DOMError from the content window?

Yes.  That is, it's being treated as an unexpected exception and is being sanitized so the web page has no idea why it's being thrown, just which line of the web page's script it was thrown on.
Right. But not all c++ errors are internal errors. Specifically, the RTCConfiguration validation code in c++ here http://mxr.mozilla.org/mozilla-central/source/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#659

          if (!hostLen) {
659         return NS_ERROR_FAILURE;

should arguably aRv.ThrowTypeError(MSG_INVALID_URL, &label) or something similar (guessing, how do I throw a DOMError from c++?), that fails the constructor and makes it back to content. What language the code is in seems irrelevant here.

If the only way to do this is "throw" A from c++, catch A in JS and throw DOMError(B) then fine, but yuck.
> guessing, how do I throw a DOMError from c++?

Pretty painfully.  Especially if you don't have a JSContext to work with.

If the code involved is guaranteed to always be called from JS, then you could AutoJSContext, but then you need to reach the right window...  We really need a better setup here somehow, but the mix of xpidl and webidl is not helping.  :(

I suspect the throw from C++, catch in JS, rethrow in JS setup is much simpler than trying to create a DOMError in the right window scope in C++.
(In reply to Boris Zbarsky [:bz] from comment #15)
> If the code involved is guaranteed to always be called from JS, then you
> could AutoJSContext, but then you need to reach the right window...

PeerConnectionImpl.cpp has an mWindow pointer and is always called from PeerConnection.js if it helps.

> I suspect the throw from C++, catch in JS, rethrow in JS setup is much
> simpler than trying to create a DOMError in the right window scope in C++.

Much less work for sure.
> if it helps.

It does help.  You can "new DOMError(mWindow, name, message)", then JS-wrap it via WrapNewBindingObject, then AutoJSContext and JS_SetPendingException, then return a failure nsresult.  That should throw the DOMError.
So, JavaScript's SyntaxError is out of bounds for DOM APIs as it is strictly meant for syntax errors in JavaScript (per Allen usage in JSON is a mistake).

DOMError is going away, so seems best to use DOMException, and JavaScript's TypeError and RangeError.
Flags: needinfo?(annevk)
Is there a next-step on this bug?  Last comment about DOMError going away was over a year ago.  Has it been overtaken by events?
backlog: --- → webRTC+
Rank: 38
Priority: -- → P3
Mass change P3->P4 to align with new Mozilla triage process.
Priority: P3 → P4
(In reply to Jesse Ruderman from comment #4)
> Now that bug 978618 is fixed, I get a line number for the initial error.
> 
> System JS : ERROR resource://gre/components/PeerConnection.js:337 -
> NS_ERROR_FAILURE:
> 
> JavaScript error: file:///Users/jruderman/Desktop/b.html, line 13:
> NS_ERROR_UNEXPECTED:

Looks like these are still the case on current trunk.
Has Regression Range: --- → no
Assignee: docfaraday → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: