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

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P5)

Description

[Alan Ford]

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" }

Comment 1

[Nils Ohlmeier [:drno]]

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.

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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

Comment 4

[Nils Ohlmeier [:drno]]

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

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

To get the updated sdp, use pc.localDescription.

Comment 6

[Nils Ohlmeier [:drno]]

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

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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?

Comment 8

[Nils Ohlmeier [:drno]]

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

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(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

Comment 10

[Eric Rescorla (:ekr)]

(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

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

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

Comment 12

[Matthieu]

> 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.

Comment 15

[Bulk Bug Changes for mreavy's org]

Mass change P4->P5 to align with new Mozilla triage process.

Comment 16

[Byron Campen [:bwc]]

Attachment: Bug 1072388 - Part 0: Re-enable some test-cases. r?jib

Comment 17

[Byron Campen [:bwc]]

Attachment: Bug 1072388 - Part 1: Allow createOffer to be called in have-local-offer, and allow repeated setting of offers. r?mjf

Depends on D26779

Comment 18

[Byron Campen [:bwc]]

Attachment: Bug 1072388 - Part 2: Don't stomp level assignments on local offer rollback, since those are set by createOffer, not sLD. r?mjf

Depends on D26780

Comment 19

[Byron Campen [:bwc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d37dff0b917e4ee036ba6bad885e250e7754fa63

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/eb773d96778e
https://hg.mozilla.org/mozilla-central/rev/d9429bdabd04
https://hg.mozilla.org/mozilla-central/rev/ec0a1e5ad360

Comment 22

[Alan Ford]

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.

Comment 23

[Byron Campen [:bwc]]

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

Comment 24

[Alan Ford]

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