Return InvalidStateError when calling addIceCandidate() too early

RESOLVED FIXED in Firefox 54

Status

()

Core
WebRTC: Signaling
P1
normal
Rank:
19
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: drno, Assigned: drno)

Tracking

52 Branch
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
https://w3c.github.io/webrtc-pc/#interface-definition says for addIceCandidate:
  If remoteDescription is null return a promise rejected with an InvalidStateError.
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
backlog: --- → webrtc/webaudio+
Rank: 19
now that raises the question when its ok to call addIceCandidate. The convenient thing here would be 
  "you can buffer candidates and add them in onsignalingstatechange"
(either have-remote-offer or stable).
Will that work? It would make working around this quite convenient.
(Assignee)

Comment 3

a year ago
(In reply to Philipp Hancke from comment #2)
> now that raises the question when its ok to call addIceCandidate. The
> convenient thing here would be 
>   "you can buffer candidates and add them in onsignalingstatechange"
> (either have-remote-offer or stable).
> Will that work? It would make working around this quite convenient.

Agreed. That would be the easier solution to the problem. And avoid that every single page/service/JS code having to come up with it's own buffering solution to avoid this potentially small, but probably fatal glitch.
And in the end the candidates need to be stored somewhere any way. Either in "user space JS land" or internally in the memory of the implementation.

But right now it looks like we would be violating the spec if we queue and thus not throw this error message as per spec ;-)
Why do the candidates need to be stored? The general rule is that the remote description they are associated with has to come first, which will happen as long as your signaling channel is ordered.
(Assignee)

Comment 5

a year ago
(In reply to Eric Rescorla (:ekr) from comment #4)
> Why do the candidates need to be stored? The general rule is that the remote
> description they are associated with has to come first, which will happen as
> long as your signaling channel is ordered.

... as long as your signaling channel is ordered and the outcome of the channel is synchronously feed into a synchronous API.

Or in other words: why does the spec have on the addIceCandidate() point 4.1 "If remoteDescription is null return a promise rejected with an InvalidStateError."?

After reading our JS code for a little bit I think my main concern got actually fixed by bug 1111666. Before that addIceCandidate() simply called directly into the C++ code no matter if any async call, for example SRD(), was still pending.
@jib: if I'm reading this correctly _chain in PeerConnection.js is suppose to implement the queuing required by the webrtc-pc spec, right?

Actually I think my current patch is not the best solution as it does not take the outcome of the SRD promise into account. I'll update it some time later.
Flags: needinfo?(jib)
(Assignee)

Updated

a year ago
Attachment #8829089 - Flags: review?(jib)
(In reply to Nils Ohlmeier [:drno] from comment #5)
> (In reply to Eric Rescorla (:ekr) from comment #4)
> > Why do the candidates need to be stored? The general rule is that the remote
> > description they are associated with has to come first, which will happen as
> > long as your signaling channel is ordered.
> 
> ... as long as your signaling channel is ordered 

Yes. That seems like a pretty minimal requirement.


> and the outcome of the
> channel is synchronously feed into a synchronous API.

No. The API has queueing for precisely this reason.





> Or in other words: why does the spec have on the addIceCandidate() point 4.1
> "If remoteDescription is null return a promise rejected with an
> InvalidStateError."?
> 
> After reading our JS code for a little bit I think my main concern got
> actually fixed by bug 1111666. Before that addIceCandidate() simply called
> directly into the C++ code no matter if any async call, for example SRD(),
> was still pending.
> @jib: if I'm reading this correctly _chain in PeerConnection.js is suppose
> to implement the queuing required by the webrtc-pc spec, right?
> 
> Actually I think my current patch is not the best solution as it does not
> take the outcome of the SRD promise into account. I'll update it some time
> later.

Comment 7

a year ago
mozreview-review
Comment on attachment 8829089 [details]
Bug 1332826: return error when calling addIceCandidate before setRemoteDescription.

https://reviewboard.mozilla.org/r/106286/#review107964

Basically what ekr said.

(In reply to Nils Ohlmeier [:drno] from comment #0)
>   If remoteDescription is null return a promise rejected with an
> InvalidStateError.

Directly ahead of that line in the spec [1] it says "Return the result of enqueuing the following steps:". The only way for the remote description to be null at that point would be if setRemoteDescription failed or was never called. In other words, callers need not worry about racing with setRemoteDescription. The candidate is effectively cached in the queue if you will.

::: dom/media/PeerConnection.js:1040
(Diff revision 1)
> +    if (this._remoteType === null) {
> +      throw new this._win.DOMException(
> +          "Remote ICE candidates can only be added after the remote SDP",
> +          "InvalidStateError");
> +    }
>      return await this._chain(() => new Promise((resolve, reject) => {
>        this._onAddIceCandidateSuccess = resolve;
>        this._onAddIceCandidateError = reject;

The if-check needs to be moved down here inside the chained steps, to match the spec [1].

[1] https://w3c.github.io/webrtc-pc/#dom-peerconnection-addicecandidate
Attachment #8829089 - Flags: review-
Flags: needinfo?(jib)
Comment hidden (mozreview-request)
(Assignee)

Comment 9

a year ago
Turns out that we throw this error from inside the JSEP C++ code. Not sure if it is worth moving this up here. I guess it saves the round trip to C++ and back.
@jib what do you think: is worth trying to catch this in JS already or should we stick to C++ handling this?
Flags: needinfo?(jib)

Comment 10

a year ago
mozreview-review
Comment on attachment 8829089 [details]
Bug 1332826: return error when calling addIceCandidate before setRemoteDescription.

https://reviewboard.mozilla.org/r/106286/#review108094
Attachment #8829089 - Flags: review?(jib) → review+
(In reply to Nils Ohlmeier [:drno] from comment #9)
> @jib what do you think: is worth trying to catch this in JS already or
> should we stick to C++ handling this?

Yes, the C++ code throws a cryptic error, not "InvalidStateError". We need this patch to be spec compliant.
Flags: needinfo?(jib)

Comment 12

a year ago
Pushed by drno@ohlmeier.org:
https://hg.mozilla.org/integration/autoland/rev/351df36e3a6b
return error when calling addIceCandidate before setRemoteDescription. r=jib

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/351df36e3a6b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.