826807 - WebRTC constraints implementation abuses JSAPI

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

The structure of this code is somewhat like this:

  if (jsapi_call(...)) {
    // Do things
  }
  if (jsapi_call(...)) {
    // Do things
  }

but a false return value from a JSAPI call usually (but not always!  JS_IsaArrayObject is an exception, for example) means that:

1)  An exception was thrown.
2)  The exception has not been reported yet; it's hanging out on the JSContext.
3)  Making further JSAPI calls without dealing with the exception will 
    typically fatally assert in debug builds.
4)  In opt builds, if the exception is not reported some random later perfectly
    fine call into JS will end up looking like it throws an exception, confusing
    authors.

What's the intended behavior of this function?  Should it be swallowing exceptions, or propagating them?  It needs to pick one or the other...

For what it's worth, I highly recommend getting review from someone familiar with JSAPI on any JSAPI code that goes in the tree.  It's _very_ easy to shoot yourself in the foot with this gun.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Note: not nominating this for tracking because this is all disabled by default so far as far as I know.  But if that's _not_ the case, we need to fix this on 19 as well.

Comment 2

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

I assume this refers to the constraints code in PeerConnectionImpl.cpp.  This doesn't apply to getUserMedia yet, since haven't implemented constraints for gUM yet.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> I assume this refers to the constraints code in PeerConnectionImpl.cpp

Yep.  The code added in bug 802694.

Comment 4

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

Attachment: Clean up JSAPI error handling in PeerConnection constraints

Comment 5

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

Attachment: Clean up JSAPI error handling in PeerConnection constraints

Comment 6

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

Comment on attachment 703331 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints

Fixed per your IRC comments.  I'll add a bug for WebIDL for this interface

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703331 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints

r=me.  Thanks!

Comment 8

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

Attachment: Clean up JSAPI error handling in PeerConnection constraints

Comment 9

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

Attachment: Clean up JSAPI error handling in PeerConnection constraints

Comment 10

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

Comment on attachment 703475 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints

Quick re-review needed - it needs to not error out if the constraints aren't available or are empty, only if they're there and fubarred (tested before pushing, of course!)

Now that we have constraints that actually do something, we need tests.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703475 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints

No, this is wrong.  If JS_GetProperty returns false, you need to error out immediately without doing anything else.

Comment 12

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

Attachment: Clean up JSAPI error handling in PeerConnection constraints

Comment 13

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

Comment on attachment 703558 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints

One last time (I hope).  

I'll note there are a lot of "if (!JS_GetProperty() || !is_a_string()) return NS_OK"'s in the tree or equivalent.

See nsGeolocationService::HandleMozsettingChanged(), etc

Comment 14

[Boris Zbarsky [:bzbarsky]]

> I'll note there are a lot of "if (!JS_GetProperty() || !is_a_string()) return NS_OK"'s in
> the tree or equivalent.

There's a lot of code that people landed without useful review.  :(

> nsGeolocationService::HandleMozsettingChanged

Yep.  Totally busted code there, that as failure mode leaves a time-bomb for the next piece of code to run on the safe context.  We should get a bug filed on that...

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703558 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints

>+  if (!JSVAL_IS_VOID(mandatory) && !JSVAL_IS_NULL(mandatory)) {

  if (!mandatory.isNullOrUndefined()) {

>+  if (!JSVAL_IS_VOID(optional) && !JSVAL_IS_NULL(optional)) {

Similar.

r=me

Comment 16

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

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

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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