991368 - close() on already closed PeerConnection results in exception

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Nils Ohlmeier [:drno]]

If you call close() on a mozRTCPeerConnection object which is closed already it raises an exception of the form:
  Peer connection is closed

I think this is a bug: the user of the API instructs the system to go into the closed state. In which state the PeerConnection currently is does not really matter as long as the end result after the close() call will be that it is in the closed state. As the PeerConnection is already in the desired state the close() call should simply return without doing anything.

Comment 1

[Nils Ohlmeier [:drno]]

Attachment: Return on close() if PC is closed already.

This patch implements the suggested return on close() if the PC is in the closed state already.

Comment 2

[Nils Ohlmeier [:drno]]

Attachment: Avoid exception in case close() gets called twice.

Same patch with proper comment in it

Comment 3

[Nils Ohlmeier [:drno]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=f061308c661d

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

From the spec:

  When the RTCPeerConnection close() method is invoked, the user agent MUST run the following steps:

    If the RTCPeerConnection object's RTCPeerConnection signalingState is closed, abort these steps. 

So, no exception.  Sounds good to me.

Comment 5

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

Comment on attachment 8400959 [details] [diff] [review]
Avoid exception in case close() gets called twice.

Review of attachment 8400959 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/PeerConnection.js
@@ +785,1 @@
>      this._queueOrRun({ func: this._close, args: [false], wait: false });

Hmm, not related to this patch, but why do we _queueOrRun close? - This means that if say, createOffer has been called them close will be deferred until after createOffer's callbacks have been called. That seems wrong.

I figured I'd ask here since we have the right audience.

Comment 6

[Nils Ohlmeier [:drno]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #5)
> Hmm, not related to this patch, but why do we _queueOrRun close? - This
> means that if say, createOffer has been called them close will be deferred
> until after createOffer's callbacks have been called. That seems wrong.

That is indeed a good question. From looking at the function parameters in the spec close() looks like a synchronous function to me. On the other hand receiving a state callback for a sync function like Bug 989936 requests is a strange thing too (and I think the requester is right: if close() is async we should generate the event).
If we don't put it into the queue, the close() call would have to clear the queue and cancel any pending requests. Not sure if that is possible or a potential problem?!
But right now we have the middle thing: an a-sync call without an event.

Comment 7

[Adam Roach [:abr]]

Comment on attachment 8400959 [details] [diff] [review]
Avoid exception in case close() gets called twice.

Yep, looks correct to me. Thanks.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/58046fca8321

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/58046fca8321