Closed Bug 1072388 Opened 5 years ago Closed 9 months ago

Cannot call createOffer/setLocalDescription in "have-local-offer" state, nor createAnswer/setRemoteDescription in "have-remote-offer" state

Categories

(Core :: WebRTC, defect, P5, minor)

35 Branch
x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox68 --- fixed
Blocking Flags:

People

(Reporter: alan.ford, Assigned: bwc)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

Testing with Firefox Nightly (v35, 2014-09-24), I try to call PeerConnection.createOffer again once ICE candidate gathering is complete.

Multiple calls to createOffer appears to be perfectly valid according to my reading of http://www.w3.org/TR/webrtc/, yet I get an error back:

Object { name: "INVALID_STATE", message: "Cannot create offer in state HAVE_LOCAL_OFFER" }
Besides the question if multiple calls of createOffer() are valid or not, if you are interested in getting access to the updated SDP with the ICE candidates in it, simply hold a reference to the SDP you got on your first createOffer() call, the SDP in there will be updated with the candidates.
(In reply to Alan Ford from comment #0)
I've verified that setLocalDescription is broken the same way: "Cannot set local SDP in state HAVE_LOCAL_OFFER"

The signalingstate diagram [1] shows that it is legal to call setLocalDescription() in "have-local-offer", so by extension I would think you should be able to call createOffer as well in this state.

[1] http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtcpeerstate-enum
(In reply to Nils Ohlmeier [:drno] from comment #1)
> Besides the question if multiple calls of createOffer() are valid or not, if
> you are interested in getting access to the updated SDP with the ICE
> candidates in it, simply hold a reference to the SDP you got on your first
> createOffer() call, the SDP in there will be updated with the candidates.

I'm pretty sure that doesn't work, as RTCSessionDescription just holds an immutable string. See [1].


[1] http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js?rev=c885916823da#194
(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> (In reply to Alan Ford from comment #0)
> I've verified that setLocalDescription is broken the same way: "Cannot set
> local SDP in state HAVE_LOCAL_OFFER"
> 
> The signalingstate diagram [1] shows that it is legal to call
> setLocalDescription() in "have-local-offer", so by extension I would think
> you should be able to call createOffer as well in this state.

While I agree that there is no wording in the spec right now which describes how to handle multiple calls to createOffer(), and the diagram at least suggests multiple calls to setLocalDescription are possible I think multiple calls to createOffer() can be quite dangerous.
If you only call createOffer() multiple times without ever sending the offer to the far end it should probably save to do so. BUT if you send each of the offers created by createOffer() to the far end, you will easily at least confuse the offer answer process of SDP. This would get us into the space of SIP UPDATE's and stuff like that. SDP does not offer diff updates and therefore usually requires an answer to the first offer, before another offer can be made.
To avoid restrictions like described above (you can call createOffer() multiple times as long as you never send the offer anywhere), I think it is the safer choice to allow another call to createOffer() only from the Stable state.
To get the updated sdp, use pc.localDescription.
(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> To get the updated sdp, use pc.localDescription.

Correct. Sorry it was not the reference to the offer, but the pc.localDescription which gets updated.
(In reply to Nils Ohlmeier [:drno] from comment #4)
> To avoid restrictions like described above (you can call createOffer()
> multiple times as long as you never send the offer anywhere), I think it is
> the safer choice to allow another call to createOffer() only from the Stable
> state.

If you're talking about what to *allow* then that needs to be taken up on the list.

Also I'm not following your rationale. Isn't whatever people do outside of WebRTC their business? Isn't how things look in stable state all that matters to us?
(In reply to Jan-Ivar Bruaroey [:jib] from comment #7)
> If you're talking about what to *allow* then that needs to be taken up on
> the list.

Indeed. I think this is a topic which should be taken to the list, so that a clarification gets added to the spec. 
 
> Also I'm not following your rationale. Isn't whatever people do outside of
> WebRTC their business? Isn't how things look in stable state all that
> matters to us?

Exactly we only specify the API. Therefore the API spec should NOT have restrictions like "you are not allowed to send this somewhere unless...".

Lets say we add to the spec: createOffer() can be called multiple times and it will give you the most recent offer no matter in which signaling state you are in.
I'm pretty sure someone would then write some JS code which calls createOffer() in a loop until ICE gathering has finished and every time send the latest offer to the other side (instead of registering a callback to onicecandidate and just send the ICE candidates to the other side). But that would create race conditions on the answerer side: if the answerer side created an Answer already, but has not called setLocalDescription, and then receives a new Offer, what do you?
You could probably "require" that setLocalDescription() needs to be called first with the answer. But why are these functions then separate (instead of createAnswer() calling internally setLocalDescription() itself already).
And if you send the Answer to the offerer how does he know for which of his offers the answer is meant?

This is probably no big deal if the diff between multiple offers is only new ICE candidates. But we can't guarantee that. It could be that the 2nd offer added another m line...
(In reply to Nils Ohlmeier [:drno] from comment #8)
> Indeed. I think this is a topic which should be taken to the list, so that a
> clarification gets added to the spec. 

Feel free, though I think the spec is perfectly clear. createOffer isn't even in the state diagram.

> Lets say we add to the spec: createOffer() can be called multiple times and
> it will give you the most recent offer no matter in which signaling state
> you are in.

The spec is written for browsers, not users. It puts restrictions on browser behavior, not user behavior. A user can call a function as many times as they want. Since there's no language in createOffer [1] to suggest it should behave differently based on any state but "closed", it wont. It better work each time people call it. That's our first bug.

It also says browsers MUST allow setLocalDescription in "has-local-offer". That's our second bug.

> I'm pretty sure someone would then write some JS code which calls
> createOffer() in a loop until ICE gathering has finished and every time send
> the latest offer to the other side (instead of registering a callback to
> onicecandidate and just send the ICE candidates to the other side). But that
> would create race conditions on the answerer side: if the answerer side
> created an Answer already, but has not called setLocalDescription, and then
> receives a new Offer, what do you?

The spec also says browsers MUST allow setRemoteDescription in "has-remote-offer" so you should be fine to call createAnswer again too, and there's probably our third bug.

As to a race: I don't see a race. People are free to use whatever signaling/synchronization mechanism they choose, and it is not up to us to question or debug that. It may be perfectly synchronized without any races. It is extrinsic. The perfect abstraction of this is the local-loop test-call. Therefore, unless you can show a race in local-loop call, I claim they're extrinsic and not our business.

[1] http://dev.w3.org/2011/webrtc/editor/webrtc.html#widl-RTCPeerConnection-createOffer-void-RTCSessionDescriptionCallback-successCallback-RTCPeerConnectionErrorCallback-failureCallback-RTCOfferOptions-options
(In reply to Jan-Ivar Bruaroey [:jib] from comment #9)
> (In reply to Nils Ohlmeier [:drno] from comment #8)
> > Indeed. I think this is a topic which should be taken to the list, so that a
> > clarification gets added to the spec. 
> 
> Feel free, though I think the spec is perfectly clear. createOffer isn't
> even in the state diagram.

I don't think the spec is perfectly clear, but there was agreement
at the last interim that it was safe to call createOffer() repeatedly.


> > Lets say we add to the spec: createOffer() can be called multiple times and
> > it will give you the most recent offer no matter in which signaling state
> > you are in.
> 
> The spec is written for browsers, not users. It puts restrictions on browser
> behavior, not user behavior. A user can call a function as many times as
> they want. Since there's no language in createOffer [1] to suggest it should
> behave differently based on any state but "closed", it wont. It better work
> each time people call it. That's our first bug.
> 
> It also says browsers MUST allow setLocalDescription in "has-local-offer".
> That's our second bug.
> 
> > I'm pretty sure someone would then write some JS code which calls
> > createOffer() in a loop until ICE gathering has finished and every time send
> > the latest offer to the other side (instead of registering a callback to
> > onicecandidate and just send the ICE candidates to the other side). But that
> > would create race conditions on the answerer side: if the answerer side
> > created an Answer already, but has not called setLocalDescription, and then
> > receives a new Offer, what do you?
> 
> The spec also says browsers MUST allow setRemoteDescription in
> "has-remote-offer" so you should be fine to call createAnswer again too, and
> there's probably our third bug.
> 
> As to a race: I don't see a race. People are free to use whatever
> signaling/synchronization mechanism they choose, and it is not up to us to
> question or debug that. It may be perfectly synchronized without any races.
> It is extrinsic. The perfect abstraction of this is the local-loop
> test-call. Therefore, unless you can show a race in local-loop call, I claim
> they're extrinsic and not our business.
> 
> [1]
> http://dev.w3.org/2011/webrtc/editor/webrtc.html#widl-RTCPeerConnection-
> createOffer-void-RTCSessionDescriptionCallback-successCallback-
> RTCPeerConnectionErrorCallback-failureCallback-RTCOfferOptions-options
(In reply to Alan Ford from comment #0)
> Object { name: "INVALID_STATE", message: "Cannot create offer in state
> HAVE_LOCAL_OFFER" }

Alan, I'm going to assume you called setLocalDescription as well here, otherwise I can't reproduce or explain why you're in "have-local-offer" state. Please let us know if that's not the case.

Renaming subject to include all three problems (unless we want to break things up).
Summary: Cannot call PeerConnect.createOffer once offer is created → Cannot call createOffer/setLocalDescription in "have-local-offer" state, nor createAnswer/setRemoteDescription in "have-remote-offer" state
> pc.createOffer(function(offer) {
>   pc.setLocalDescription(offer, function() {
>     pc.setLocalDescription(pc.localDescription, function() {
>       // Works in chrome
>     }, function(error) {
>       // in FF: { name: "INVALID_STATE", message: "Cannot create offer in state HAVE_LOCAL_OFFER" }
>     });
>   });
> })

The following should work according to "http://www.w3.org/TR/webrtc/" but it doesn't. I understand that setting the same offer as local desciption twice makes no sense but generating this error (although the local sdp hasnt changed) is a little harsh.
Blocks: webrtc_spec
backlog: --- → webRTC+
Rank: 45
Priority: -- → P4
Duplicate of this bug: 1193724
Duplicate of this bug: 1081252
Mass change P4->P5 to align with new Mozilla triage process.
Priority: P4 → P5
Blocks: 1533020
Assignee: nobody → docfaraday
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eb773d96778e
Part 0: Re-enable some test-cases. r=jib
https://hg.mozilla.org/integration/autoland/rev/d9429bdabd04
Part 1: Allow createOffer to be called in have-local-offer, and allow repeated setting of offers. r=mjf
https://hg.mozilla.org/integration/autoland/rev/ec0a1e5ad360
Part 2: Don't stomp level assignments on local offer rollback, since those are set by createOffer, not sLD. r=mjf

Since this fix, I can no longer setLocalDescription with candidates.

I am not using Trickle ICE, so call createOffer and wait for the SDP back. Then setLocalDescription with the provided SDP, and get the error "Adding your own candidate attributes is not supported" back.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

No additional validation has been added in these patchsets. Please provide a test-case, and open a new bug.

Status: REOPENED → RESOLVED
Closed: 9 months ago9 months ago
Flags: needinfo?(alan.ford)
Resolution: --- → FIXED

Error was in my use of the API - the fixing of this bug cleared up discovering this! Thanks!

Flags: needinfo?(alan.ford)
You need to log in before you can comment on or make changes to this bug.