W3C home > Mailing lists > Public > public-webrtc@w3.org > November 2013

Re: WebRTC spec error message review

From: Jan-Ivar Bruaroey <jib@mozilla.com>
Date: Mon, 11 Nov 2013 05:45:55 -0500
Message-ID: <5280B563.30909@mozilla.com>
To: Dan Burnett <dburnett@voxeo.com>, public-webrtc@w3.org
On 11/9/13 12:29 PM, Dan Burnett wrote:

> In preparation for discussion this week, I was asked to collect together all the places in the WebRTC spec where errors or exceptions occur and to also carefully review for other errors and/or exceptions a web application developer might want.
>
> This is the list.  If text is in quotes it is from the spec directly.  If I don't propose a change to it, my suggestion is that we keep it as-is.
>
> You are welcome to read this and comment by email, but I plan to go through this list, in order, during the face-to-face meeting this coming week.

Thanks, this is very helpful!

> -- dan
>
>
> RTCPeerConnection, section 4.3.1
>
>    Review Issue 1 -- does it suggest a need for an exception or at least an event?

The client already gets an iceconnectionstatechange event in this case. The state will be "failed" or "closed" depending on whether the close method MUST be called as is dictated here (if it must, you'll never see "failed", which seems odd).


On a side-note, should we enumerate exceptions from new RTCPeerConnection(configuration), or do people generally get that APIs throw on bad args?

(Note: since I'm mentally leaning on our implementation, whenever I say "we" or "our" below, I mean Firefox)

For reference, we have (forgive me, all our exceptions are still vanilla with a message):

  - "RTCPeerConnection constructor passed invalid RTCConfiguration"
  - "RTCPeerConnection constructor passed invalid RTCConfiguration - missing url"
  - "RTCPeerConnection constructor passed invalid RTCConfiguration - malformed URI"
  - "RTCPeerConnection constructor passed invalid RTCConfiguration - improper scheme"
  - "RTCPeerConnection constructor passed invalid RTCConfiguration - missing username"
  - "RTCPeerConnection constructor passed invalid RTCConfiguration - missing credential"
  - "Can't create RTCPeerConnections when the network is down"

Additionally, our webidl bindings throw on incorrect args for us.

> addIceCandidate(), section 4.3.2.3
>
>    "If the [ICE] candidate could not be successfully applied, the user agent MUST queue a task to invoke failureCallback with a DOMError object whose name attribute has the value TBD."
>    What kinds of situations might cause this, and what do developers care about finding out when they occur?  Simple proposal:  "invalidCandidate" as the name.
>    If we queue this method, does this introduce new error cases?

We already queue it fwiw.
We throw on bad arg (null candidate or zero sdpMLineIndex): "Invalid candidate passed to addIceCandidate".

> addStream(), section 4.3.2.3
>
>    "If connection's RTCPeerConnection signalingState is closed, throw an invalidStateError exception and abort these steps."
> Also, no failureCallback, so how to report IncompatibleConstraintsError?

Can you give me an example of when this should report IncompatibleConstraintsError? There's no mention of what constraints are to be applied, or how.

> close(), section 4.3.2.3
>
>    "If connection's RTCPeerConnection signalingState is closed, throw an invalidStateError exception and abort these steps."
> createAnswer(), section 4.3.2.3
>
>    Need to figure out how to add the Constrainable interface here, which will introduce the need to report constraint failures.  Currently the text says
>     - "If the constraints parameter is malformed, throw a SyntaxError exception and abort these steps." and
>     - "If the constraints could not be successfully applied, the user agent MUST queue a task to invoke failureCallback with a DOMError object whose name attribute has the value IncompatibleConstraintError."
>
>    Also, "If the SDP generation process failed for any reason, the user agent MUST queue a task to invoke failureCallback with a DOMError object of type TBD as its argument."  Simple proposal:  name should be "answerGenerationFailure".
>
>
> createOffer(), section 4.3.2.3
>
>    Need to figure out how to add the Constrainable interface here, which will introduce the need to report constraint failures.  Currently the text says
>     - "If the constraints parameter is malformed, throw a SyntaxError exception and abort these steps." and
>     - "If the constraints could not be successfully applied, the user agent MUST queue a task to invoke failureCallback with a DOMError object whose name attribute has the value IncompatibleConstraintError."
>
>    Also, "If the SDP generation process failed for any reason, the user agent MUST queue a task to invoke failureCallback with a DOMError object of type TBD as its argument."  Simple proposal:  name should be "offerGenerationFailure".

I don't think Constrainable fits here, as I've mentioned http://lists.w3.org/Archives/Public/public-webrtc/2013Oct/0038.html

I see no constraining algorithm for known capabilities, no set to constrain/apply to, hence no overconstrained condition possible, and no event-firing mechanism needed. - Furthermore, the requirement to preserve mandatory/optional state and optional order becomes onerous since the information is useless.

I feel I "get" gUM constraints, but I find the conflation of what are constraints ("MediaConstraints" vs. "MediaStreamConstraints") a source of confusion when reading this spec.

Here in PeerConnection, "OfferToReceiveAudio" etc. are just dictionary settings imho (and should be called "RTCSettings" or something, not "MediaConstraints").

If we added a way to query the API about what it supports, we wouldn't even need mandatory/optional, removing a webidl pain-point.

> removeStream(), section 4.3.2.3
>
>    "If connection's RTCPeerConnection signalingState is closed, throw an invalidStateError exception and abort these steps."
> setLocalDescription() , section 4.3.2.3
>
>    "If connection's RTCPeerConnection signalingState is closed, throw an invalidStateError exception and abort these steps."
>
>    "If a local description contains a different set of ICE credentials, then the ICE Agent MUST trigger an ICE restart."
>    Should this throw an event of some sort other than "onicestatechange"?
>
>    If the session description could not be applied because the content was invalid or the type was wrong for the current signaling state, then the failureCallback will be called with a DOMError whose name = InvalidSessionDescriptionError.
>
>    If the session description was valid but could not be applied at the media labor, rollback will be attempted.
>    If it succeeds (or was not necessary), the failureCallback will be called with a DOMError whose name = IncompatibleSessionDescriptionError.
>    If rollback fails, the failureCallback will be called with a DOMError whose name = InternalError.

We throw on bad args fwiw: "Invalid type " + desc.type + " provided to setLocalDescription".

> setRemoteDescription(), section 4.3.2.3
>
>    "If a=identity attributes, are present, the browser verifies the identity following the procedures in [XREF sec.identity-proxy-assertion-request]."
>    What failures can occur in this process?
>
>    "If any tracks on the PeerConnection have a peerIdentity constraint and either the PeerConnection connection has no peer identity or that identity is not equal to the specified peerIdentity, the user agent must queue a task to invoke failureCallback with a DOMError object whose name attribute has the value IncompatibleConstraintsError."
>
>    Additionally, all the errors for setLocalDescription above are relevant here as well.
>
>
> updateIce(), section 4.3.2.3
>
>    Need to figure out how to add the Constrainable interface here, which will introduce the need to report constraint failures.  Currently the text says
>     - "If the constraints parameter is malformed, throw a SyntaxError exception and abort these steps."
>    
>    Also, no failureCallback, so how to report IncompatibleConstraintsError?
>
>    Does this method need to be queued?  If so, what additional error conditions might that introduce?
>
>
> createDataChannel(), section 5.1.2
>
>    "If connection's RTCPeerConnection signalingState is closed, throw an invalidStateError exception and abort these steps."
>
>    "If both the maxRetransmitTime and maxRetransmits attributes are set (not null), then throw a SyntaxError exception and abort these steps."
>    Should there be any limits on these values other than being unsigned short integers?
>
>    "If the value of the protocol attribute fails to match the requirements of the WebRTC DataChannel Protocol specification, then throw a SyntaxError exception and abort these steps."
>    IS THIS DETAILED ENOUGH?  It seems underspecified to me.  For example, it might be nice to get a useful message about what's wrong with the protocol setting.
>
>    "if the value of the id attribute is taken by an existing RTCDataChannel, throw a TBD exception and abort these steps."
>    Simple proposal:  throw an "IDInUse" exception.   The text should probably clarify that this only occurs for a non-negotiated channel or for a negotiated one that has already been negotiated.
>
>
> (remote-created data channel), section 5.2
>
>    The spec says
>    - Let configuration be an information bundle received from the other peer as a part of the process to establish the underlying data channeldescribed by the WebRTC DataChannel Protocol specification.
>    - Initialize channel's label, ordered, maxRetransmitTime, maxRetransmits, protocol, negotiated and id attributes to their corresponding values in configuration.
>
>    It seems that there could be errors or failures here, somewhere between receiving the configuration and applying it.  What could occur, and would we want local exceptions to be raised if so?  I think probably not, but we should discuss.
>
>    The spec says "When a RTCDataChannel object's underlying data transport has been closed, . . . If the transport was closed with an error, fire an error event at channel."   This is way way underspecified.  What kinds of transport errors can occur?  Which might matter to JS developers?
>
>
> binaryType attribute, section 5.2.1
>
>    The spec currently allows arbitrary DOMString values.  Is that what we want?  What entries might be invalid, and should we return an error for them?
>    Simple proposal:  Yes, throw a SyntaxError exception for illegal values.
>
>
> protocol attribute, section 5.2.1
>
>    The spec currently allows arbitrary DOMString values.  Is that what we want?  What entries might be invalid, and should we return an error for them?
>    Simple proposal:  Yes, throw a SyntaxError exception for illegal values.
>
>
> send, section 5.2.2
>
>    If the channel was created by the remote peer in negotiated form, there might be a data connection that we can receive from but not send to.  Thus, send() might not work.  I'm not sure what to do here, but perhaps an exception is appropriate.
>
>    "If channel's readyState attribute is 'connecting', throw an InvalidStateError exception and abort these steps."
>
>    "Attempt to send data on channelís underlying data transport; if the data cannot be sent, e.g. because it would need to be buffered but the buffer is full, the user agent must abruptly close channelís underlying data transport with an error."
>    This implies that Data channel sends are synchronous.  Is that correct?  Also, the error needs to be defined.  Simple proposal: "sendFailure" exception.
>
>
> createDTMFSender, section 6.1.1
>
>    "The MediaStreamTrackmust be an element of a MediaStream that's currently in the RTCPeerConnection object's local streams set; if not, throw anInvalidMediaStreamTrackError exception and abort these steps."
>
>
> insertDTMF(), section 6.2.2
>
>    There should be an error message for values outside of the allowed ranges.  Simple proposal:  "SyntaxError" exception.
>
>    The spec says "If the associated MediaStreamTrack is not connected to the associated RTCPeerConnection, return.  If the canInsertDTMF attribute is false, return."
>    Are we okay with no errors being thrown here?
>
>
> statistics model, section 7.1
>
>    The spec says "The selector may, for example, be a MediaStreamTrack. For a track to be a valid selector, it must be a member of a MediaStream that is sent or received by the RTCPeerConnection object on which the stats request was issued."
>    What other kinds of selectors might there be?  Do we want special error messages for each (see getStats() below)?
>
>
> getStats(), section 7.2.1
>
>    "If the RTCPeerConnection object's RTCPeerConnection signalingState is closed, throw an invalidStateError exception."  NOTE WELL:  unlike in most of the other methods, this text does not require aborting after throwing the exception. I think it should.

I agree. Even though abort is implied by throw, I find the redundant prose helpful when reading these plain-language algorithms.

The same text is also missing on close() btw.

>    "If selectorArg is an invalid selector, the user agent must queue a task to invoke the failure callback (the method's third argument)."
>
>
> RTCStats dictionary, section 7.5
>
>    "Thus, applications must be prepared to deal with unknown stats."
>    What does this mean?  In particular, what behavior is appropriate when an unknown stat is encountered?

Ignore and skip on enumeration.

> idP messaging channel, section 8.1.1
>
>    The text indicates some requirements on the existence and behavior of a special signaling channel between the Peer Connection and the IdP.  What errors, if any, should be passed up to the application?  Remember that this identity checking can happen with no explicit action by the web application code, so it is unclear where any exceptions would be targeted.
>
>
> getIdentityAssertion, section 9.2
>
>    "If the RTCPeerConnection object's RTCPeerConnection signalingState is closed, abort these steps."
>
>    I think there should be an AssertionFailure exception for the case where an identity assertion cannot be obtained after calling this method.
>
>
> setIdentityProvider, section 9.2

I spot a throw bug:

"2. If the RTCPeerConnection object's RTCPeerConnection signalingState is stable, and any of the identity settings have changed, queue a task to run the following substeps:

     1. If the connection's RTCPeerConnection signalingState is closed, throw an InvalidStateError exception and abort these steps."

Generally, one cannot throw from a queued task.

>    "If the connection's RTCPeerConnection signalingState is closed, throw an invalidStateError exception and abort these steps."
>
>    As an application developer I want feedback if something I set was wrong.  For example, InvalidProvider, InvalidProtocol, and InvalidUsername.  Since these may take time to determine, this might require a failureCallback instead of exceptions.

I'd prefer choosing exception vs. failureCallback based solely on whether the action can be done synchronously or not.

Thanks again,

.: Jan-Ivar :.
Received on Monday, 11 November 2013 10:46:27 UTC

This archive was generated by hypermail 2.3.1 : Monday, 23 October 2017 15:19:36 UTC