WebRTC constraints implementation abuses JSAPI

RESOLVED FIXED in mozilla21

Status

()

P1
major
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bzbarsky, Assigned: jesup)

Tracking

unspecified
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 4 obsolete attachments)

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.
(Reporter)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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
(Reporter)

Comment 3

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

Yep.  The code added in bug 802694.
(Assignee)

Comment 4

6 years ago
Created attachment 703327 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints
(Assignee)

Comment 5

6 years ago
Created attachment 703331 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints
(Assignee)

Updated

6 years ago
Attachment #703327 - Attachment is obsolete: true
(Assignee)

Comment 6

6 years ago
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)
(Reporter)

Comment 7

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

r=me.  Thanks!
Attachment #703331 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 8

6 years ago
Created attachment 703451 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints
(Assignee)

Updated

6 years ago
Attachment #703331 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
Created attachment 703475 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints
(Assignee)

Updated

6 years ago
Attachment #703451 - Attachment is obsolete: true
(Assignee)

Comment 10

6 years ago
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-
(Assignee)

Comment 12

6 years ago
Created attachment 703558 [details] [diff] [review]
Clean up JSAPI error handling in PeerConnection constraints
(Assignee)

Updated

6 years ago
Attachment #703475 - Attachment is obsolete: true
(Assignee)

Comment 13

6 years ago
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+
(Assignee)

Comment 16

6 years ago
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
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Whiteboard: [WebRTC], [blocking-webrtc+] → [WebRTC], [blocking-webrtc+], [qa-]
(Assignee)

Updated

6 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.