Closed Bug 834270 Opened 12 years ago Closed 12 years ago

Align PeerConnection error handling with WebRTC specification

Categories

(Core :: WebRTC: Signaling, defect, P1)

Product:
Core
Component:
WebRTC: Signaling
Version:
21 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla22

People

(Reporter: abr, Assigned: abr)

References

(Blocks 1 open bug)

Details

(Whiteboard: [WebRTC][blocking-webrtc+][spec-issue])

Attachments

(1 file, 3 obsolete files)

Fix PeerConnection error callbacks to return code/reason phrase
27.92 KB, patch
abr
: review+
Details | Diff | Splinter Review
Currently, our error callbacks for PeerConnection methods call the indicated callback with an integer, taken from the enumeration in peer_connection_types.h. In the WebRTC spec, these callbacks are intended to be called with an RTCError instance as their argument, with two strings (one selected from a spec-mandated enumeration; and an optional error string to provide additional details). We need to update our interface handling to match the spec. Ideally, we would define the enumeration as constants in IPeerConnection.idl, and convert them to strings in the javascript handling. Marking as P1/major, since this change is clearly visible to applications, and we want to break them earlier rather than later.
Assignee: nobody → adam
Depends on: 834100
As part of this change, we should also align our success callbacks -- for most of these, the spec specifies callbacks with no parameters rather than one integer parameter.
Similar problems arise with our exception handling. This is all pretty much part and parcel of the same problem.
Noming this for a triage discussion. There's enough complaints happening on this to make me think we need to have another discussion on this on whether this blocks or not.
Whiteboard: [WebRTC][blocking-webrtc-] → [WebRTC][blocking-webrtc?]
Blocks: 851293
Moving to blocking per meeting today - main thing that blocks is matching the spec for callbacks, not the exceptions.
Whiteboard: [WebRTC][blocking-webrtc?] → [WebRTC][blocking-webrtc+]
Whiteboard: [WebRTC][blocking-webrtc+] → [WebRTC][blocking-webrtc+][spec-issue]
Depends on: 855880
Attachment #730932 - Attachment is obsolete: true
Depends on: 855796
Comment on attachment 731028 [details] [diff] [review] Fix PeerConnection error callbacks to return code/reason phrase I think this patch has reached the wallpaper state needed for 22. We still need more plumbing long-term. Note that you must apply the patch from Bug 855796 along with this patch if you don't want the mochi tests to crash.
Attachment #731028 - Flags: review?(ekr)
Comment on attachment 731028 [details] [diff] [review] Fix PeerConnection error callbacks to return code/reason phrase Review of attachment 731028 [details] [diff] [review]: ----------------------------------------------------------------- Did a review of the test - only giving the review- on the change to unexpectedCallbackAndFinish, cause that may or may not break existing tests. The other changes are suggested code cleanup. ::: dom/media/tests/mochitest/head.js @@ +147,5 @@ > * The object fired back from the callback > */ > function unexpectedCallbackAndFinish(aObj) { > + ok(false, "Unexpected error callback with name = '" + aObj.name + > + "', message = '" + oObj.message + "'"); Many different tests are reusing this function. I don't think we can always assume "name" and "message" are present. I'd add some if checks here to make sure those properties are present before referencing them. ::: dom/media/tests/mochitest/test_peerConnection_bug834270.html @@ +1,1 @@ > +<!DOCTYPE HTML> FWIW, looks like landing this will end up fixing bug 850271. The tests look good, but we could do some cleanup here to avoid some duplication in case we want to add more "unexpected flow error tests" in the future. @@ +16,5 @@ > + function testCreateAnswerError() { > + var pc = new mozRTCPeerConnection(); > + > + pc.createAnswer( > + function() { If we target clean reusability, we could simplify every success function setup by doing: 1. add an info statement before each pc call describing what you have in the message below 2. set the success callback to unexpectedCallbackAndFinish instead @@ +20,5 @@ > + function() { > + ok (0, "createAnswer before offer should fail"); > + SimpleTest.finish(); > + }, > + function(err) { This error function is being repeated all over the place. Can we clean up the duplication? @@ +82,5 @@ > + }); > + }; > + > + // No test for createOffer errors -- there's nothing we can do at this > + // level to evoke an error in createOffer. The spec indicates that errors can be generated off of createOffer in scenarios such as: - Failed SDP generation process => error callback - Malformed Media Constraints => exception of RTCError type of INVALID_CONSTRAINTS_TYPE - Failure to apply media constraints => exception of RTCError type of INCOMPATIBLE_CONSTRIANTS Why won't one of those work here?
Attachment #731028 - Flags: review-
Blocks: 850271
(In reply to Jason Smith [:jsmith] from comment #9) > > ::: dom/media/tests/mochitest/test_peerConnection_bug834270.html > @@ +1,1 @@ > > +<!DOCTYPE HTML> > > FWIW, looks like landing this will end up fixing bug 850271. > Actually, let me retrack that statement. This adds flows to be tested that would fall under bug 850271, but there's more that would be needed to close that bug entirely.
Comment on attachment 731028 [details] [diff] [review] Fix PeerConnection error callbacks to return code/reason phrase Review of attachment 731028 [details] [diff] [review]: ----------------------------------------------------------------- I think this still needs some cleanup. ::: dom/media/PeerConnection.js @@ +731,5 @@ > QueryInterface: XPCOMUtils.generateQI([Ci.IPeerConnectionObserver, > Ci.nsISupportsWeakReference]), > > + // These strings must match those defined in the WebRTC spec. > + reasonName: [ Please separate errors and exceptions @@ +756,5 @@ > } > this._dompc._executeNext(); > }, > > + onCreateOfferError: function(code, message) { Can we remove the cut-and-paste here? @@ +761,4 @@ > if (this._dompc._onCreateOfferFailure) { > try { > + this._dompc._onCreateOfferFailure.onCallback({ > + name: this.reasonName[code], message: message, What happens if code or message are bogus in some way? @@ +761,5 @@ > if (this._dompc._onCreateOfferFailure) { > try { > + this._dompc._onCreateOfferFailure.onCallback({ > + name: this.reasonName[code], message: message, > + __exposedProps__: { name: "rw", message: "rw" } why is "rw" what we want here? ::: dom/media/bridge/IPeerConnection.idl @@ +96,5 @@ > const unsigned short kDataChannelPartialReliableRexmit = 1; > const unsigned short kDataChannelPartialReliableTimed = 2; > > + /* Constants for 'name' in error callbacks */ > + const unsigned long kNoError = 0; // Test driver only Can you please split out the errors from the exceptions? ::: dom/media/tests/mochitest/test_peerConnection_bug834270.html @@ +1,1 @@ > +<!DOCTYPE HTML> This looks like it could benefit from being named something more evocative, given thatit's testing the generla error funtionality. ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp @@ +149,3 @@ > switch (mCallState) { > case CREATEOFFER: > + CSFLogDebug(logTag, "Processing CREATEOFFER"); This log message isn't right. We are processing create offer success. There is a lot of cut-copy-paste here. Can we find a way to reduce it? Maybe a macro? Auto-gened code? ::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.h @@ +135,5 @@ > kRoleOfferer, > kRoleAnswerer > }; > > + enum Error { Do these values need to match those in the IDL? If so, can we get them from the IDL?
Attachment #731028 - Flags: review?(ekr) → review-
I resolved several of EKR's questions/comments in an IRC conversation. Excerpts below to document rationale. ------------------------------------------------------------ abr: I'm on the fence about the separation of errors and exceptions. The spec treats these as a single class, saying things like "The error name in the object provided MUST be picked from either the RTCExceptionName or RTCErrorName enums." ekr: where does it say that? abr: Section 4.6.1 ekr: I think that is terrible abr: I agree, but it's what we have. ekr: but I agree that it says that ekr: how about you send the list hatemail and leave it asis abr: Next... The "rw" thing is based on the proposition that no purpose is served by further restriction. I'm happy changing it, but what we lose is the ability for scripts to do something like doodle on the message and then pass it along to another function. I don't see any reason to prevent that. ekr: ok. abr: wrt "Processing CREATEOFFER" -- I'm going to consolidate the "Processing <messagename>" stuff by creating a constant-to-sring funcition, but it's still going to spit out the literal constant name that's being processed (meaning the output is unchanged). I agree that the constant is misnamed, but that's getting into a bit of swamp cleaning that I think is out of scope for this bug. ekr: ok abr: Finally, the constant replication from the IDL thing is a matter of choosing which particular flavor of ugly we want. I see three approaches: (1) use the IPeerConnection constants directly, in which case we lose the type altogether; (2) define the enum with matching numbers like I've done here (which is functionally equivalent to what's been done for things like IceState); or (3) explicitly bind the values together with something like this: > enum Error { > kNoError = IPeerConnection::kNoError, > kInvalidConstraintsType = IPeerConnection::kInvalidConstraintsType, > kInvalidCandidateType = IPeerConnection::kInvalidCandidateType, > kInvalidMediastreamTrack = IPeerConnection::kInvalidMediastreamTrack, > kInvalidState = IPeerConnection::kInvalidState, > kInvalidSessionDescription = IPeerConnection::kInvalidSessionDescription, > kIncompatibleSessionDescription = IPeerConnection::kIncompatibleSessionDescription, > kIncompatibleConstraints = IPeerConnection::kIncompatibleConstraints, > kIncompatibleMediaStreamTrack = IPeerConnection::kIncompatibleMediaStreamTrack, > kInternalError = IPeerConnection::kInternalError > }; (which will look pretty raw once I wrap it to fit in 80 columns) abr: The problem is that IDL doesn't support the notion of enumerations. abr: There's a somewhat ugly twist here, actually, in that we eventually need to expose these constants to c code, meaning that we'll have four (!) places to keep in sync. But I don't see much of a way around that without adding some really complicated machinery. ekr: yeah.... abr: (Three sites where the constants are defined, and one site to map from the constant to the WebRTC strings) ekr: it's all pretty aweful abr: it is. In any case, I decided to choose the variety of ugly that we've apparently already decided to use in PeerConnectionImpl, but with explicit integers to be just a smidgen safer about things. ekr: OK abr: Alright. So, from your review, I need to refactor the replicated code in PeerConnection.js, check for validity of the parameters to the error callbacks, rename the mochi test, and make the logging in the observer callback code more generic. Did I miss anything? ekr: I don't think so.
(In reply to Jason Smith [:jsmith] from comment #9) I'm not done processing the rest of your review, but I just wanted to respond to this comment by itself: > @@ +82,5 @@ > > + }); > > + }; > > + > > + // No test for createOffer errors -- there's nothing we can do at this > > + // level to evoke an error in createOffer. > > The spec indicates that errors can be generated off of createOffer in > scenarios such as: > > - Failed SDP generation process => error callback > - Malformed Media Constraints => exception of RTCError type of > INVALID_CONSTRAINTS_TYPE > - Failure to apply media constraints => exception of RTCError type of > INCOMPATIBLE_CONSTRIANTS > > Why won't one of those work here? The first scenario would involve forcing a failure in the SIPCC code, which isn't something the javascript can't induce. Simply put, unless we do something dodgy like introduce a constraint that means "I want this to fail," we can't induce this callback to fire. And adding such a constraint would be a fair bit of work for what amounts to very little gain. The second and third scenarios are exceptions, not error callbacks. We're testing the error callbacks here.
Whoops; I messed up my negatives there with excessive editing. My response should read "...which isn't something the javascript can induce."
FWIW, the second and third scenarios *are* tested in test_peerConnection_bug835370.html
Attachment #731028 - Attachment is obsolete: true
Attachment #731218 - Flags: review?(jsmith)
Attachment #731218 - Flags: review?(ekr)
Comment on attachment 731218 [details] [diff] [review] Fix PeerConnection error callbacks to return code/reason phrase Review of attachment 731218 [details] [diff] [review]: ----------------------------------------------------------------- review+ for the test with one small fix on the unexpectedCallbackAndFinish function ::: dom/media/tests/mochitest/head.js @@ +146,5 @@ > * @param {object} aObj > * The object fired back from the callback > */ > function unexpectedCallbackAndFinish(aObj) { > + if (aObj.name) { oObj can be undefined here, so what we want here is: if(aObj && aObj.name && aObj.message)
Attachment #731218 - Flags: review?(jsmith) → review+
Comment on attachment 731218 [details] [diff] [review] Fix PeerConnection error callbacks to return code/reason phrase Review of attachment 731218 [details] [diff] [review]: ----------------------------------------------------------------- did not review tests but lgtm
Attachment #731218 - Flags: review?(ekr) → review+
Attachment #731218 - Attachment is obsolete: true
Comment on attachment 731263 [details] [diff] [review] Fix PeerConnection error callbacks to return code/reason phrase Carrying r+ from ekr, jsmith
Attachment #731263 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/fb60b02f6281
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Keywords: verifyme
Marking verified per mochitest on check in.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: