Revert PR #893 (#954)

The PR potentially introduced connectivity problems. Revert it to be
able to compare the behavior.

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} (attempt=${this.stateService.attempt})`);
+        this.log.debug('State change:', oldValue, '->', newValue);
         if (newValue === oldValue) {
             return;
         }
@@ -142,7 +142,7 @@ export class StatusController {
                     this.scheduleStatusBar();
                 }
                 this.webClientService.clearIsTypingFlags();
-                if (this.stateService.attempt === 0 && isRelayedData) {
+                if (isRelayedData) {
                     this.reconnectIos();
                 }
                 break;
@@ -189,6 +189,10 @@ 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,
@@ -196,8 +200,8 @@ export class StatusController {
             close: false,
         });
         this.webClientService.init({
-            keyStore: 'reuse',
-            peerTrustedKey: 'reuse',
+            keyStore: originalKeyStore,
+            peerTrustedKey: originalPeerPermanentKeyBytes,
             resume: true,
         });
 
@@ -234,6 +238,10 @@ 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;
@@ -278,8 +286,8 @@ export class StatusController {
                 this.log.debug('Starting new connection without push');
             }
             this.webClientService.init({
-                keyStore: 'reuse',
-                peerTrustedKey: 'reuse',
+                keyStore: originalKeyStore,
+                peerTrustedKey: originalPeerPermanentKeyBytes,
                 resume: true,
             });
 

src/sass/sections/_status_bar.scss

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

src/services/peerconnection.ts

@@ -150,8 +150,7 @@ export class PeerConnectionHelper {
                             // situations which certainly would lead to ugly race conditions.
                             this.connectionFailedTimer = null;
                             this.log.debug('ICE connection considered failed');
-                            this.setConnectionState(TaskConnectionState.Disconnected);
-                            this.close();
+                            this.pc.close();
                         }, PeerConnectionHelper.CONNECTION_FAILED_TIMEOUT_MS, true, 'connectionFailedTimer');
                         break;
                     case 'connected':
@@ -161,7 +160,6 @@ export class PeerConnectionHelper {
                     case 'failed':
                     case 'closed':
                         this.setConnectionState(TaskConnectionState.Disconnected);
-                        this.close();
                         break;
                     default:
                         this.log.warn('Ignored ICE connection state change to',
@@ -287,18 +285,7 @@ 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;
@@ -307,8 +294,6 @@ 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,8 +265,6 @@ 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;
 
@@ -447,27 +445,18 @@ 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 | 'reuse',
-        peerTrustedKey?: Uint8Array | 'reuse',
+        keyStore?: saltyrtc.KeyStore,
+        peerTrustedKey?: Uint8Array,
         resume: boolean,
     }): void {
         let keyStore = flags.keyStore;
         let resumeSession = flags.resume;
-        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})`);
+        this.log.info(`Initializing (keyStore=${keyStore !== undefined ? 'yes' : 'no'}, peerTrustedKey=` +
+            `${flags.peerTrustedKey !== undefined ? 'yes' : 'no'}, resume=${resumeSession})`);
 
         // Reset fields, blob cache, pending requests and pending timeouts in case the session
         // should explicitly not be resumed
@@ -535,16 +524,9 @@ export class WebClientService {
         tasks.push(this.relayedDataTask);
 
         // Create new keystore if necessary
-        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) {
+        if (!keyStore) {
             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));
@@ -557,15 +539,8 @@ export class WebClientService {
             .withKeyStore(keyStore)
             .usingTasks(tasks)
             .withPingInterval(30);
-        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;
+        if (flags.peerTrustedKey !== undefined) {
+            builder = builder.withTrustedPeerKey(flags.peerTrustedKey);
         }
         this.salty = builder.asInitiator();
         this.arpLog.info('Public key:', this.salty.permanentKeyHex);
@@ -602,17 +577,6 @@ 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);
         });
 
@@ -898,9 +862,6 @@ export class WebClientService {
      *   considered established.
      */
     private onTaskEstablished(resumeSession: boolean) {
-        // Store peer's key
-        this.peerTrustedKey = this.salty.peerPermanentKeyBytes;
-
         // Pushing complete
         this.resetPushSession(true);
 
@@ -1125,7 +1086,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
@@ -1134,10 +1095,8 @@ export class WebClientService {
                 });
             };
             dc.onclose = () => {
-                this.arpLog.info(`Data channel ${dc.label} closed, closing peer connection`);
-                this.stateService.updateTaskConnectionState(threema.TaskConnectionState.Disconnected);
-                this.pcHelper.onConnectionStateChange = null;
-                this.pcHelper.close();
+                this.arpLog.error(`Data channel ${dc.label} closed prematurely`);
+                this.failSession();
             };
             dc.onerror = (event) => {
                 this.arpLog.error(`Data channel ${dc.label} error:`, event);
@@ -1385,8 +1344,6 @@ 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();
         }