Merge pull request #893 from threema-ch/fix-protocol-errors

Fix race conditions

src/controllers/status.ts

@@ -124,7 +124,7 @@ export class StatusController {
      */
     private onStateChange(newValue: threema.GlobalConnectionState,
                           oldValue: threema.GlobalConnectionState): void {
-        this.log.debug('State change:', oldValue, '->', newValue);
+        this.log.debug(`State change: ${oldValue} -> ${newValue} (attempt=${this.stateService.attempt})`);
         if (newValue === oldValue) {
             return;
         }
@@ -142,7 +142,7 @@ export class StatusController {
                     this.scheduleStatusBar();
                 }
                 this.webClientService.clearIsTypingFlags();
-                if (isRelayedData) {
+                if (this.stateService.attempt === 0 && isRelayedData) {
                     this.reconnectIos();
                 }
                 break;
@@ -189,10 +189,6 @@ export class StatusController {
             this.scheduleStatusBar();
         }
 
-        // Get original keys
-        const originalKeyStore = this.webClientService.salty.keyStore;
-        const originalPeerPermanentKeyBytes = this.webClientService.salty.peerPermanentKeyBytes;
-
         // Soft reconnect: Does not reset the loaded data
         this.webClientService.stop({
             reason: DisconnectReason.SessionStopped,
@@ -200,8 +196,8 @@ export class StatusController {
             close: false,
         });
         this.webClientService.init({
-            keyStore: originalKeyStore,
-            peerTrustedKey: originalPeerPermanentKeyBytes,
+            keyStore: 'reuse',
+            peerTrustedKey: 'reuse',
             resume: true,
         });
 
@@ -238,10 +234,6 @@ export class StatusController {
     private reconnectIos(): void {
         this.log.info(`Connection lost (iOS). Reconnect attempt #${++this.stateService.attempt}`);
 
-        // Get original keys
-        const originalKeyStore = this.webClientService.salty.keyStore;
-        const originalPeerPermanentKeyBytes = this.webClientService.salty.peerPermanentKeyBytes;
-
         // Delay connecting a bit to wait for old websocket to close
         // TODO: Make this more robust and hopefully faster
         const startTimeout = 500;
@@ -286,8 +278,8 @@ export class StatusController {
                 this.log.debug('Starting new connection without push');
             }
             this.webClientService.init({
-                keyStore: originalKeyStore,
-                peerTrustedKey: originalPeerPermanentKeyBytes,
+                keyStore: 'reuse',
+                peerTrustedKey: 'reuse',
                 resume: true,
             });
 

src/sass/sections/_status_bar.scss

@@ -89,6 +89,7 @@ body {
 .status-task-relayed-data {
     &.status-warning #status-bar {
         background-color: $status-ok;
+        height: 0;
     }
 }
 

src/services/peerconnection.ts

@@ -150,7 +150,8 @@ export class PeerConnectionHelper {
                             // situations which certainly would lead to ugly race conditions.
                             this.connectionFailedTimer = null;
                             this.log.debug('ICE connection considered failed');
-                            this.pc.close();
+                            this.setConnectionState(TaskConnectionState.Disconnected);
+                            this.close();
                         }, PeerConnectionHelper.CONNECTION_FAILED_TIMEOUT_MS, true, 'connectionFailedTimer');
                         break;
                     case 'connected':
@@ -160,6 +161,7 @@ export class PeerConnectionHelper {
                     case 'failed':
                     case 'closed':
                         this.setConnectionState(TaskConnectionState.Disconnected);
+                        this.close();
                         break;
                     default:
                         this.log.warn('Ignored ICE connection state change to',
@@ -285,7 +287,18 @@ export class PeerConnectionHelper {
      * Unbind all event handler and abruptly close the peer connection.
      */
     public close(): void {
+        // Cancel connection failed timer
+        if (this.connectionFailedTimer !== null) {
+            this.timeoutService.cancel(this.connectionFailedTimer);
+            this.connectionFailedTimer = null;
+        }
+
+        // Unbind all events
         this.webrtcTask.off();
+        this.sdc.dc.onopen = null;
+        this.sdc.dc.onclose = null;
+        this.sdc.dc.onerror = null;
+        this.sdc.dc.onmessage = null;
         this.pc.onnegotiationneeded = null;
         this.pc.onconnectionstatechange = null;
         this.pc.onsignalingstatechange = null;
@@ -294,6 +307,8 @@ export class PeerConnectionHelper {
         this.pc.oniceconnectionstatechange = null;
         this.pc.onicegatheringstatechange = null;
         this.pc.ondatachannel = null;
+
+        // Close peer connection
         this.pc.close();
     }
 }

src/services/webclient.ts

@@ -265,6 +265,8 @@ export class WebClientService {
     private drafts: threema.Container.Drafts;
     private pcHelper: PeerConnectionHelper = null;
     private trustedKeyStore: TrustedKeyStoreService;
+    private keyStore: saltyrtc.KeyStore | null = null;
+    private peerTrustedKey: Uint8Array | null = null;
     public clientInfo: threema.ClientInfo = null;
     public version = null;
 
@@ -445,18 +447,27 @@ export class WebClientService {
     /**
      * Initialize the webclient service.
      *
+     * @param keyStore Key store to be used. Will create a new one if `undefined` or reuse the currently used one if
+     *   `reuse`.
+     * @param peerTrustedKey The peer's trusted key. Will not use this feature if `undefined` or reuse the currently
+     *   used one if `reuse`.
+     * @param resume Whether the previous session can be resumed.
+     *
      * Warning: Do not call this with `flags.resume` set to `false` in case
      *          messages can be queued by the user.
      */
     public init(flags: {
-        keyStore?: saltyrtc.KeyStore,
-        peerTrustedKey?: Uint8Array,
+        keyStore?: saltyrtc.KeyStore | 'reuse',
+        peerTrustedKey?: Uint8Array | 'reuse',
         resume: boolean,
     }): void {
         let keyStore = flags.keyStore;
         let resumeSession = flags.resume;
-        this.log.info(`Initializing (keyStore=${keyStore !== undefined ? 'yes' : 'no'}, peerTrustedKey=` +
-            `${flags.peerTrustedKey !== undefined ? 'yes' : 'no'}, resume=${resumeSession})`);
+        let peerTrustedKey = flags.peerTrustedKey;
+        const keyStoreInfo = keyStore === 'reuse' ? 'reuse' : (keyStore !== undefined ? 'yes' : 'no');
+        const peerTrustedKeyInfo = peerTrustedKey === 'reuse' ? 'reuse' : (peerTrustedKey !== undefined ? 'yes' : 'no');
+        this.log.info(`Initializing (keyStore=${keyStoreInfo}, peerTrustedKey=${peerTrustedKeyInfo}, ` + `
+            resume=${resumeSession})`);
 
         // Reset fields, blob cache, pending requests and pending timeouts in case the session
         // should explicitly not be resumed
@@ -524,9 +535,16 @@ export class WebClientService {
         tasks.push(this.relayedDataTask);
 
         // Create new keystore if necessary
-        if (!keyStore) {
+        if (keyStore === 'reuse') {
+            if (this.keyStore === null) {
+                throw new Error('Key store cannot be reused as it is null');
+            }
+            keyStore = this.keyStore;
+        }
+        if (keyStore === undefined) {
             keyStore = new saltyrtcClient.KeyStore();
         }
+        this.keyStore = keyStore;
 
         // Determine SaltyRTC host, replace the inner prefix (if any)
         this.saltyRtcHost = this.config.SALTYRTC_HOST.replace('{prefix}', keyStore.publicKeyHex.substr(0, 2));
@@ -539,8 +557,15 @@ export class WebClientService {
             .withKeyStore(keyStore)
             .usingTasks(tasks)
             .withPingInterval(30);
-        if (flags.peerTrustedKey !== undefined) {
-            builder = builder.withTrustedPeerKey(flags.peerTrustedKey);
+        if (peerTrustedKey === 'reuse') {
+            if (this.peerTrustedKey === null) {
+                throw new Error('Peer trusted key cannot be reused as it is null');
+            }
+            peerTrustedKey = this.peerTrustedKey;
+        }
+        if (peerTrustedKey !== undefined) {
+            builder = builder.withTrustedPeerKey(peerTrustedKey);
+            this.peerTrustedKey = peerTrustedKey;
         }
         this.salty = builder.asInitiator();
         this.arpLog.info('Public key:', this.salty.permanentKeyHex);
@@ -577,6 +602,17 @@ export class WebClientService {
                         this.arpLog.warn('Unknown signaling state:', state);
                 }
             }
+
+            // Close the peer connection.
+            // Note: For some reason, we do not receive 'close' events for the main data channel,
+            //       so this is a fallback mechanism.
+            if (this.pcHelper !== null && state === 'closed') {
+                this.arpLog.debug('Closing peer connection');
+                this.stateService.updateTaskConnectionState(threema.TaskConnectionState.Disconnected);
+                this.pcHelper.onConnectionStateChange = null;
+                this.pcHelper.close();
+            }
+
             this.stateService.updateSignalingConnectionState(state, this.chosenTask, this.handoverDone);
         });
 
@@ -862,6 +898,9 @@ export class WebClientService {
      *   considered established.
      */
     private onTaskEstablished(resumeSession: boolean) {
+        // Store peer's key
+        this.peerTrustedKey = this.salty.peerPermanentKeyBytes;
+
         // Pushing complete
         this.resetPushSession(true);
 
@@ -1086,7 +1125,7 @@ export class WebClientService {
                 // Determine chunk length
                 this.secureDataChannelChunkLength = Math.min(
                     WebClientService.DATA_CHANNEL_MAX_CHUNK_SIZE, this.pcHelper.pc.sctp.maxMessageSize);
-                this.arpLog.debug(`Using chunk length: ${this.secureDataChannelChunkLength} for data channel` +
+                this.arpLog.debug(`Using chunk length: ${this.secureDataChannelChunkLength} for data channel ` +
                     dc.label);
 
                 // Connection established
@@ -1095,8 +1134,10 @@ export class WebClientService {
                 });
             };
             dc.onclose = () => {
-                this.arpLog.error(`Data channel ${dc.label} closed prematurely`);
-                this.failSession();
+                this.arpLog.info(`Data channel ${dc.label} closed, closing peer connection`);
+                this.stateService.updateTaskConnectionState(threema.TaskConnectionState.Disconnected);
+                this.pcHelper.onConnectionStateChange = null;
+                this.pcHelper.close();
             };
             dc.onerror = (event) => {
                 this.arpLog.error(`Data channel ${dc.label} error:`, event);
@@ -1344,6 +1385,8 @@ export class WebClientService {
 
         // Clear stored data (trusted key, push token, etc) if deleting the session
         if (remove) {
+            this.keyStore = null;
+            this.peerTrustedKey = null;
             this.trustedKeyStore.clearTrustedKey();
         }