Merge pull request #504 from jitsi/misc_session_fixes

Misc session fixes

JitsiConference.js

@@ -6,7 +6,6 @@ import ConnectionQuality from './modules/connectivity/ConnectionQuality';
 import { getLogger } from 'jitsi-meet-logger';
 import GlobalOnErrorHandler from './modules/util/GlobalOnErrorHandler';
 import EventEmitter from 'events';
-import * as JingleSessionState from './modules/xmpp/JingleSessionState';
 import * as JitsiConferenceErrors from './JitsiConferenceErrors';
 import JitsiConferenceEventManager from './JitsiConferenceEventManager';
 import * as JitsiConferenceEvents from './JitsiConferenceEvents';
@@ -1235,8 +1234,6 @@ JitsiConference.prototype.onTransportInfo = function(session, transportInfo) {
  * @param {JitsiRemoteTrack} removedTrack
  */
 JitsiConference.prototype.onRemoteTrackRemoved = function(removedTrack) {
-    let consumed = false;
-
     this.getParticipants().forEach(participant => {
         const tracks = participant.getTracks();
 
@@ -1253,29 +1250,10 @@ JitsiConference.prototype.onRemoteTrackRemoved = function(removedTrack) {
                     this.transcriber.removeTrack(removedTrack);
                 }
 
-                consumed = true;
-
                 break;
             }
         }
     }, this);
-
-    if (!consumed) {
-        if ((this.isP2PActive() && !removedTrack.isP2P)
-             || (!this.isP2PActive() && removedTrack.isP2P)) {
-            // A remote track can be removed either as a result of
-            // 'source-remove' or the P2P logic which removes remote tracks
-            // explicitly when switching between JVB and P2P connections.
-            // The check above filters out the P2P logic case which should not
-            // result in an error (which just goes over all remote tracks).
-            return;
-        }
-        logger.error(
-            'Failed to match remote track on remove'
-                + ' with any of the participants',
-            removedTrack.getStreamId(),
-            removedTrack.getParticipantId());
-    }
 };
 
 /**
@@ -1352,7 +1330,14 @@ JitsiConference.prototype.onIncomingCall
     try {
         jingleSession.acceptOffer(
             jingleOffer,
-            null /* success */,
+            () => {
+                // If for any reason invite for the JVB session arrived after
+                // the P2P has been established already the media transfer needs
+                // to be turned off here.
+                if (this.isP2PActive() && this.jvbJingleSession) {
+                    this._suspendMediaTransferForJvbConnection();
+                }
+            },
             error => {
                 GlobalOnErrorHandler.callErrorHandler(error);
                 logger.error(
@@ -1414,13 +1399,15 @@ JitsiConference.prototype._rejectIncomingCall
 
     // Terminate  the jingle session with a reason
     jingleSession.terminate(
-        options && options.reasonTag,
-        options && options.reasonMsg,
         null /* success callback => we don't care */,
         error => {
             logger.warn(
                 'An error occurred while trying to terminate'
                     + ' invalid Jingle session', error);
+        }, {
+            reason: options.reasonTag,
+            reasonDescription: options.reasonMsg,
+            sendSessionTerminate: true
         });
 };
 
@@ -2264,7 +2251,7 @@ JitsiConference.prototype._maybeStartOrStopP2P = function(userLeftEvent) {
             logger.info(`Will start P2P with: ${jid}`);
             this._startP2PSession(jid);
         }
-    } else if (isModerator && this.p2pJingleSession && !shouldBeInP2P) {
+    } else if (this.p2pJingleSession && !shouldBeInP2P) {
         logger.info(`Will stop P2P with: ${this.p2pJingleSession.peerjid}`);
 
         // Log that there will be a switch back to the JVB connection
@@ -2295,7 +2282,9 @@ JitsiConference.prototype._stopP2PSession
 
     // Swap remote tracks, but only if the P2P has been fully established
     if (wasP2PEstablished) {
-        this._resumeMediaTransferForJvbConnection();
+        if (this.jvbJingleSession) {
+            this._resumeMediaTransferForJvbConnection();
+        }
 
         // Remove remote P2P tracks
         this._removeRemoteP2PTracks();
@@ -2305,23 +2294,39 @@ JitsiConference.prototype._stopP2PSession
     logger.info('Stopping remote stats for P2P connection');
     this.statistics.stopRemoteStats(this.p2pJingleSession.peerconnection);
     logger.info('Stopping CallStats for P2P connection');
-    this.statistics.stopCallStats(
-        this.p2pJingleSession.peerconnection);
+    this.statistics.stopCallStats(this.p2pJingleSession.peerconnection);
 
-    if (JingleSessionState.ENDED !== this.p2pJingleSession.state) {
-        this.p2pJingleSession.terminate(
-            reason ? reason : 'success',
-            reasonDescription
-                ? reasonDescription : 'Turing off P2P session',
-            () => {
-                logger.info('P2P session terminate RESULT');
-            },
-            error => {
-                logger.warn(
+    this.p2pJingleSession.terminate(
+        () => {
+            logger.info('P2P session terminate RESULT');
+        },
+        error => {
+            // Because both initiator and responder are simultaneously
+            // terminating their JingleSessions in case of the 'to JVB switch'
+            // when 3rd participant joins, both will dispose their sessions and
+            // reply with 'item-not-found' (see strophe.jingle.js). We don't
+            // want to log this as an error since it's expected behaviour.
+            //
+            // We want them both to terminate, because in case of initiator's
+            // crash the responder would stay in P2P mode until ICE fails which
+            // could take up to 20 seconds.
+            //
+            // NOTE lack of 'reason' is considered as graceful session terminate
+            // where both initiator and responder terminate their sessions
+            // simultaneously.
+            if (reason) {
+                logger.error(
                     'An error occurred while trying to terminate'
-                    + ' P2P Jingle session', error);
-            });
-    }
+                        + ' P2P Jingle session', error);
+            }
+        }, {
+            reason: reason ? reason : 'success',
+            reasonDescription: reasonDescription
+                ? reasonDescription : 'Turing off P2P session',
+            sendSessionTerminate: this.room
+                && this.getParticipantById(
+                    Strophe.getResourceFromJid(this.p2pJingleSession.peerjid))
+        });
 
     this.p2pJingleSession = null;
 

modules/RTC/DataChannels.js

@@ -49,7 +49,7 @@ DataChannels.prototype.onDataChannel = function(event) {
     const self = this;
 
     dataChannel.onopen = function() {
-        logger.info('Data channel opened by the Videobridge!', dataChannel);
+        logger.info('Data channel opened by the Videobridge!');
 
         // Code sample for sending string and/or binary data
         // Sends String message to the bridge

modules/RTC/TraceablePeerConnection.js

@@ -666,8 +666,7 @@ TraceablePeerConnection.prototype._createRemoteTrack
     }
     remoteTracksMap.set(mediaType, remoteTrack);
 
-    // FIXME not cool to use RTC's eventEmitter
-    this.rtc.eventEmitter.emit(RTCEvents.REMOTE_TRACK_ADDED, remoteTrack);
+    this.eventEmitter.emit(RTCEvents.REMOTE_TRACK_ADDED, remoteTrack);
 };
 
 /* eslint-enable max-params */
@@ -728,7 +727,7 @@ TraceablePeerConnection.prototype._remoteTrackRemoved
         return;
     }
 
-    if (!this._removeRemoteTrack(streamId, trackId)) {
+    if (!this._removeRemoteTrackById(streamId, trackId)) {
         // NOTE this warning is always printed when user leaves the room,
         // because we remove remote tracks manually on MUC member left event,
         // before the SSRCs are removed by Jicofo. In most cases it is fine to
@@ -800,6 +799,26 @@ TraceablePeerConnection.prototype.removeRemoteTracks = function(owner) {
     return removedTracks;
 };
 
+/**
+ * Removes and disposes given <tt>JitsiRemoteTrack</tt> instance. Emits
+ * {@link RTCEvents.REMOTE_TRACK_REMOVED}.
+ * @param {JitsiRemoteTrack} toBeRemoved
+ */
+TraceablePeerConnection.prototype._removeRemoteTrack = function(toBeRemoved) {
+    toBeRemoved.dispose();
+    const participantId = toBeRemoved.getParticipantId();
+    const remoteTracksMap = this.remoteTracks.get(participantId);
+
+    if (!remoteTracksMap) {
+        logger.error(
+            `removeRemoteTrack: no remote tracks map for ${participantId}`);
+    } else if (!remoteTracksMap.delete(toBeRemoved.getType())) {
+        logger.error(
+            `Failed to remove ${toBeRemoved} - type mapping messed up ?`);
+    }
+    this.eventEmitter.emit(RTCEvents.REMOTE_TRACK_REMOVED, toBeRemoved);
+};
+
 /**
  * Removes and disposes <tt>JitsiRemoteTrack</tt> identified by given stream and
  * track ids.
@@ -810,26 +829,12 @@ TraceablePeerConnection.prototype.removeRemoteTracks = function(owner) {
  * <tt>undefined</tt> if no track matching given stream and track ids was
  * found.
  */
-TraceablePeerConnection.prototype._removeRemoteTrack
+TraceablePeerConnection.prototype._removeRemoteTrackById
 = function(streamId, trackId) {
     const toBeRemoved = this._getRemoteTrackById(streamId, trackId);
 
     if (toBeRemoved) {
-        toBeRemoved.dispose();
-
-        const remoteTracksMap
-            = this.remoteTracks.get(toBeRemoved.getParticipantId());
-
-        // If _getRemoteTrackById succeeded it must be a valid value or
-        // we're good to crash
-        if (!remoteTracksMap.delete(toBeRemoved.getType())) {
-            logger.error(
-                `Failed to remove ${toBeRemoved} - type mapping messed up ?`);
-        }
-
-        // FIXME not cool to use RTC's eventEmitter
-        this.rtc.eventEmitter.emit(
-            RTCEvents.REMOTE_TRACK_REMOVED, toBeRemoved);
+        this._removeRemoteTrack(toBeRemoved);
     }
 
     return toBeRemoved;
@@ -1435,7 +1440,7 @@ TraceablePeerConnection.prototype.setLocalDescription
 
             if (localUfrag !== this.localUfrag) {
                 this.localUfrag = localUfrag;
-                this.rtc.eventEmitter.emit(
+                this.eventEmitter.emit(
                     RTCEvents.LOCAL_UFRAG_CHANGED, this, localUfrag);
             }
             successCallback();
@@ -1508,7 +1513,7 @@ TraceablePeerConnection.prototype.setRemoteDescription
 
             if (remoteUfrag !== this.remoteUfrag) {
                 this.remoteUfrag = remoteUfrag;
-                this.rtc.eventEmitter.emit(
+                this.eventEmitter.emit(
                     RTCEvents.REMOTE_UFRAG_CHANGED, this, remoteUfrag);
             }
             successCallback();
@@ -1543,6 +1548,11 @@ TraceablePeerConnection.prototype.clearRecvonlySsrc = function() {
     this.sdpConsistency.clearVideoSsrcCache();
 };
 
+/**
+ * Closes underlying WebRTC PeerConnection instance and removes all remote
+ * tracks by emitting {@link RTCEvents.REMOTE_TRACK_REMOVED} for each one of
+ * them.
+ */
 TraceablePeerConnection.prototype.close = function() {
     this.trace('stop');
 
@@ -1552,6 +1562,13 @@ TraceablePeerConnection.prototype.close = function() {
     this.signalingLayer.off(
         SignalingEvents.PEER_VIDEO_TYPE_CHANGED, this._peerVideoTypeChanged);
 
+    for (const peerTracks of this.remoteTracks.values()) {
+        for (const remoteTrack of peerTracks.values()) {
+            this._removeRemoteTrack(remoteTrack);
+        }
+    }
+    this.remoteTracks.clear();
+
     if (!this.rtc._removePeerConnection(this)) {
         logger.error('RTC._removePeerConnection returned false');
     }

modules/xmpp/JingleSession.js

@@ -138,20 +138,20 @@ export default class JingleSession {
      */
     removeSources(contents) {}
 
-    /* eslint-disable max-params */
-
     /**
      * Terminates this Jingle session by sending session-terminate
-     * @param reason XMPP Jingle error condition
-     * @param text some meaningful error message
      * @param success a callback called once the 'session-terminate' packet has
      * been acknowledged with RESULT.
      * @param failure a callback called when either timeout occurs or ERROR
      * response is received.
+     * @param {Object} options
+     * @param {string} [options.reason] XMPP Jingle error condition
+     * @param {string} [options.reasonDescription] some meaningful error message
+     * @param {boolean} [options.sendSessionTerminate=true] set to false to skip
+     * sending session-terminate. It may not make sense to send it if the XMPP
+     * connection has been closed already or if the remote peer has disconnected
      */
-    terminate(reason, text, success, failure) {}
-
-    /* eslint-enable max-params */
+    terminate(success, failure, options) {}
 
     /**
      * Handles an offer from the remote peer (prepares to accept a session).

modules/xmpp/JingleSessionPC.js

@@ -674,8 +674,6 @@ export default class JingleSessionPC extends JingleSession {
         this.setOfferAnswerCycle(
             jingleOffer,
             () => {
-                this.state = JingleSessionState.ACTIVE;
-
                 // FIXME we may not care about RESULT packet for session-accept
                 // then we should either call 'success' here immediately or
                 // modify sendSessionAccept method to do that
@@ -697,22 +695,49 @@ export default class JingleSessionPC extends JingleSession {
         if (!this.isInitiator) {
             throw new Error('Trying to invite from the responder session');
         }
-        for (const localTrack of localTracks) {
-            this.peerconnection.addTrack(localTrack);
-        }
-        this.peerconnection.createOffer(
-            this.sendSessionInitiate.bind(this),
-            error => logger.error('Failed to create offer', error),
-            this.mediaConstraints);
+        const workFunction = finishedCallback => {
+            for (const localTrack of localTracks) {
+                this.peerconnection.addTrack(localTrack);
+            }
+            this.peerconnection.createOffer(
+                sdp => {
+                    this.sendSessionInitiate(
+                        sdp,
+                        finishedCallback,
+                        finishedCallback
+                    );
+                },
+                error => {
+                    logger.error(
+                        'Failed to create an offer',
+                        error,
+                        this.mediaConstraints);
+                    finishedCallback(error);
+                },
+                this.mediaConstraints);
+        };
+
+        this.modificationQueue.push(
+            workFunction,
+            error => {
+                if (error) {
+                    logger.error('invite error', error);
+                } else {
+                    logger.debug('invite executed - OK');
+                }
+            });
     }
 
     /**
      * Sends 'session-initiate' to the remote peer.
      * @param {object} sdp the local session description object as defined by
      * the WebRTC standard.
+     * @param {function} success executed when the operation succeeds.
+     * @param {function(error)} failure executed when the operation fails with
+     * an error passed as an argument.
      * @private
      */
-    sendSessionInitiate(sdp) {
+    sendSessionInitiate(sdp, success, failure) {
         logger.log('createdOffer', sdp);
         const sendJingle = () => {
             let init = $iq({
@@ -740,12 +765,17 @@ export default class JingleSessionPC extends JingleSession {
                     logger.error('"session-initiate" error', error);
                 },
                 IQ_TIMEOUT);
+
+            // NOTE the callback is executed immediately as we don't want to
+            // wait for the XMPP response which would delay the startup process.
+            success();
         };
 
         this.peerconnection.setLocalDescription(
             sdp, sendJingle,
             error => {
                 logger.error('session-init setLocalDescription failed', error);
+                failure(error);
             }
         );
     }
@@ -761,7 +791,6 @@ export default class JingleSessionPC extends JingleSession {
         this.setOfferAnswerCycle(
             jingleAnswer,
             () => {
-                this.state = JingleSessionState.ACTIVE;
                 logger.info('setAnswer - succeeded');
             },
             error => {
@@ -795,9 +824,27 @@ export default class JingleSessionPC extends JingleSession {
 
             const newRemoteSdp
                 = this._processNewJingleOfferIq(jingleOfferAnswerIq);
+            const oldLocalSdp
+                = this.peerconnection.localDescription.sdp;
 
-            this._renegotiate(newRemoteSdp)
+            this._renegotiate(newRemoteSdp.raw)
                 .then(() => {
+                    if (this.state === JingleSessionState.PENDING) {
+                        this.state = JingleSessionState.ACTIVE;
+                    }
+
+                    // Old local SDP will be available when we're setting answer
+                    // for the first time, but not when offer and it's fine
+                    // since we're generating an answer now it will contain all
+                    // our SSRCs
+                    if (oldLocalSdp) {
+                        const newLocalSdp
+                            = new SDP(this.peerconnection.localDescription.sdp);
+
+                        this.notifyMySSRCUpdate(
+                            new SDP(oldLocalSdp), newLocalSdp);
+                    }
+
                     finishedCallback();
                 }, error => {
                     logger.error(
@@ -1004,45 +1051,51 @@ export default class JingleSessionPC extends JingleSession {
             IQ_TIMEOUT);
     }
 
-    /* eslint-disable max-params */
-
     /**
      * @inheritDoc
      */
-    terminate(reason, text, success, failure) {
-        let sessionTerminate = $iq({
-            to: this.peerjid,
-            type: 'set'
-        })
-        .c('jingle', {
-            xmlns: 'urn:xmpp:jingle:1',
-            action: 'session-terminate',
-            initiator: this.initiator,
-            sid: this.sid
-        })
-        .c('reason')
-        .c(reason || 'success');
-
-        if (text) {
-            // eslint-disable-next-line newline-per-chained-call
-            sessionTerminate.up().c('text').t(text);
+    terminate(success, failure, options) {
+        if (this.state === JingleSessionState.ENDED) {
+            return;
         }
 
-        // Calling tree() to print something useful
-        sessionTerminate = sessionTerminate.tree();
-        logger.info('Sending session-terminate', sessionTerminate);
+        if (!options || Boolean(options.sendSessionTerminate)) {
+            let sessionTerminate
+                = $iq({
+                    to: this.peerjid,
+                    type: 'set'
+                })
+                .c('jingle', {
+                    xmlns: 'urn:xmpp:jingle:1',
+                    action: 'session-terminate',
+                    initiator: this.initiator,
+                    sid: this.sid
+                })
+                .c('reason')
+                .c((options && options.reason) || 'success');
+
+            if (options && options.reasonDescription) {
+                sessionTerminate.up()
+                    .c('text')
+                    .t(options.reasonDescription);
+            }
 
-        this.connection.sendIQ(
-            sessionTerminate,
-            success,
-            this.newJingleErrorHandler(sessionTerminate, failure), IQ_TIMEOUT);
+            // Calling tree() to print something useful
+            sessionTerminate = sessionTerminate.tree();
+            logger.info('Sending session-terminate', sessionTerminate);
+            this.connection.sendIQ(
+                sessionTerminate,
+                success,
+                this.newJingleErrorHandler(sessionTerminate, failure),
+                IQ_TIMEOUT);
+        } else {
+            logger.info(`Skipped sending session-terminate for ${this}`);
+        }
 
         // this should result in 'onTerminated' being called by strope.jingle.js
         this.connection.jingle.terminate(this.sid);
     }
 
-    /* eslint-enable max-params */
-
     /**
      *
      * @param reasonCondition
@@ -1197,7 +1250,7 @@ export default class JingleSessionPC extends JingleSession {
                     ? this._processRemoteAddSource(addOrRemoveSsrcInfo)
                     : this._processRemoteRemoveSource(addOrRemoveSsrcInfo);
 
-            this._renegotiate(newRemoteSdp)
+            this._renegotiate(newRemoteSdp.raw)
                 .then(() => {
                     const newLocalSdp
                         = new SDP(this.peerconnection.localDescription.sdp);
@@ -1302,7 +1355,7 @@ export default class JingleSessionPC extends JingleSession {
 
     /**
      * Do a new o/a flow using the existing remote description
-     * @param {SDP object} optionalRemoteSdp optional remote sdp
+     * @param {string} [optionalRemoteSdp] optional, raw remote sdp
      *  to use.  If not provided, the remote sdp from the
      *  peerconnection will be used
      * @returns {Promise} promise which resolves when the
@@ -1311,11 +1364,17 @@ export default class JingleSessionPC extends JingleSession {
      */
     _renegotiate(optionalRemoteSdp) {
         const remoteSdp
-            = optionalRemoteSdp
-                || new SDP(this.peerconnection.remoteDescription.sdp);
+            = optionalRemoteSdp || this.peerconnection.remoteDescription.sdp;
+
+        if (!remoteSdp) {
+            return Promise.reject(
+                'Can not renegotiate without remote description,'
+                    + `- current state: ${this.state}`);
+        }
+
         const remoteDescription = new RTCSessionDescription({
             type: this.isInitiator ? 'answer' : 'offer',
-            sdp: remoteSdp.raw
+            sdp: remoteSdp
         });
 
         return new Promise((resolve, reject) => {
@@ -1461,7 +1520,8 @@ export default class JingleSessionPC extends JingleSession {
                 this.peerconnection.addTrack(newTrack);
             }
 
-            if ((oldTrack || newTrack) && oldLocalSdp) {
+            if ((oldTrack || newTrack)
+                && this.state === JingleSessionState.ACTIVE) {
                 this._renegotiate()
                     .then(() => {
                         const newLocalSDP
@@ -1695,6 +1755,16 @@ export default class JingleSessionPC extends JingleSession {
      */
     setMediaTransferActive(active) {
         const workFunction = finishedCallback => {
+
+            // Because the value is modified on the queue it's impossible to
+            // check it's final value reliably prior to submitting the task.
+            // The rule here is that the last submitted state counts.
+            // Check the value here to avoid unnecessary renegotiation cycle.
+            if (this.mediaTransferActive === active) {
+                finishedCallback();
+
+                return;
+            }
             this.mediaTransferActive = active;
             if (this.peerconnection) {
                 this.peerconnection.setMediaTransferActive(
@@ -1844,9 +1914,10 @@ export default class JingleSessionPC extends JingleSession {
             // FIXME: Maybe we can include part of the session object
             // error.session = this;
 
-            logger.error('Jingle error', error);
             if (failureCb) {
                 failureCb(error);
+            } else {
+                logger.error('Jingle error', error);
             }
         };
     }

modules/xmpp/strophe.jingle.js

@@ -4,7 +4,6 @@ import { getLogger } from 'jitsi-meet-logger';
 const logger = getLogger(__filename);
 
 import JingleSessionPC from './JingleSessionPC';
-import * as JingleSessionState from './JingleSessionState';
 import XMPPEvents from '../../service/xmpp/XMPPEvents';
 import GlobalOnErrorHandler from '../util/GlobalOnErrorHandler';
 import RandomUtil from '../util/RandomUtil';
@@ -188,7 +187,6 @@ class JingleConnectionPlugin extends ConnectionPlugin {
                     = $(iq).find('>jingle>reason>:first')[0].tagName;
                 reasonText = $(iq).find('>jingle>reason>text').text();
             }
-            sess.state = JingleSessionState.ENDED;
             this.terminate(sess.sid, reasonCondition, reasonText);
             this.eventEmitter.emit(XMPPEvents.CALL_ENDED,
                 sess, reasonCondition, reasonText);