1064074 - Seems like PeerConnection wont close properly in response to Peer Identity mismatch (code-inspection)

Status: NEW

(Core :: WebRTC, defect, P4)

Description

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

This was found by code-inspection while reviewing Bug 1063971, comment 7, so no STRs and not actually verified.

Probably not a big deal, but the code to close down a peerConnection in the event of a PeerIdentity mismatch looks like it wont work, which presumably means the peerConnection might hold on to resources it shouldn't hold on to if it is kept around for longer.

From http://hg.mozilla.org/mozilla-central/rev/50116088c244#l3.147 :

> 3.141 +        if (!idpGood) {
> 3.142 +          // iff we are waiting for a very specific peerIdentity
> 3.143 +          // call the error callback directly and then close
> 3.144 +          idpError = "Peer Identity mismatch, expected: " +
> 3.145 +            this._impl.peerIdentity;
> 3.146 +          this.callCB(onError, idpError);
> 3.147 +          this.close();
> 3.148 +        } else {
> 3.149 +          idpComplete = true;
> 3.150 +          allDone();
> 3.151 +        }

It calls this.close() but never calls allDone() which means executeNext() (inside allDone) is never called, which effectively freezes PeerConnection's queue. This is bad because close itself is queued, which means it never executes the c++ (resource-freeing) side of close:

From http://mxr.mozilla.org/mozilla-central/source/dom/media/PeerConnection.js#834 :

> close: function() {
>   if (this._closed) {
>     return;
>   }
>   this.changeIceConnectionState("closed");
>   this._queueOrRun({ func: this._close, args: [false], wait: false });
>   this._closed = true;
> },

Since the JS side of close completes, the peerConnection should act as if it's closed, so this may not be a big deal in practice, but I'm not sure. 

This is in 32.

Comment 1

[Martin Thomson [:mt:]]

I think that I can confirm this.  The fix seems trivial enough: add an executeNext call before calling close.

But that leads me to think...if there is anything else in the queue and it's not a call to close, that's not going to go well.  We should probably flush the queue when close is called.

Comment 2

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

(In reply to Martin Thomson [:mt] from comment #1)
> if there is anything else in the queue and it's not a call to close,
> that's not going to go well.

An excellent point. Calling close with anything on the queue seems problematic, especially since the _queueOrRun function itself calls this._checkClosed(), making it throw once this._closed == true.

This means the only time that close() in comment 0 will work is when nothing is on the queue, otherwise _close() is just queued and never executed, which actually seems true even if _queueOrRun didn't throw.

>  We should probably flush the queue when close is called.

And/or not queue close at all. Not sure why it was queued to begin with, but apparently it has always been. http://hg.mozilla.org/mozilla-central/rev/86aef70706f9

I forget what was decided in the workgroup on this, whether to fire outstanding error callbacks or not on close. Do you recall or have a link to a resolution?

Wanna do this in this bug or separate bug?

Comment 3

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #2)
> (In reply to Martin Thomson [:mt] from comment #1)
> > if there is anything else in the queue and it's not a call to close,
> > that's not going to go well.
> 
> An excellent point. Calling close with anything on the queue seems
> problematic, especially since the _queueOrRun function itself calls
> this._checkClosed(), making it throw once this._closed == true.
> 
> This means the only time that close() in comment 0 will work is when nothing
> is on the queue, otherwise _close() is just queued and never executed, which
> actually seems true even if _queueOrRun didn't throw.

Well, it's only going to set this.closed after the call to _queueOrRun(), so that's not a problem.

What I meant with the original message was that an error that is serious enough to require closing the PC is going to put the PC in a state that could cause problems if it tried to execute enqueued operations.  In general, I'd expect the code to be robust against that, but it's probably best to risk it.

> >  We should probably flush the queue when close is called.
> I forget what was decided in the workgroup on this, whether to fire
> outstanding error callbacks or not on close. Do you recall or have a link to
> a resolution?

We had a couple of rounds, and I can't remember why we settled on the queuing form.

Comment 4

[Bulk Bug Changes for mreavy's org]

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