Closed Bug 826807 Opened 9 years ago Closed 8 years ago

WebRTC constraints implementation abuses JSAPI

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: bzbarsky, Assigned: jesup)

References

Details

(Whiteboard: [WebRTC], [blocking-webrtc+], [qa-])

Attachments

(1 file, 4 obsolete files)

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.
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.
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.
Whiteboard: [WebRTC], [blocking-webrtc+]
Assignee: nobody → rjesup
Priority: -- → P1
Blocks: 796463
> I assume this refers to the constraints code in PeerConnectionImpl.cpp

Yep.  The code added in bug 802694.
Attachment #703327 - Attachment is obsolete: true
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
Attachment #703331 - Flags: review?(bzbarsky)
Comment on attachment 703331 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints

r=me.  Thanks!
Attachment #703331 - Flags: review?(bzbarsky) → review+
Attachment #703331 - Attachment is obsolete: true
Attachment #703451 - Attachment is obsolete: true
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.
Attachment #703475 - Flags: review?(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.
Attachment #703475 - Flags: review?(bzbarsky) → review-
Attachment #703475 - Attachment is obsolete: true
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
Attachment #703558 - Flags: review?(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 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
Attachment #703558 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1ed018dd540
OS: Mac OS X → All
Hardware: x86 → All
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/f1ed018dd540
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.