ElementR: fix emoji verification stalling when both ends hit start at the same time

src/rust-crypto/verification.ts

@@ -34,6 +34,7 @@ import { OutgoingRequest, OutgoingRequestProcessor } from "./OutgoingRequestProc
 import { TypedReEmitter } from "../ReEmitter";
 import { MatrixEvent } from "../models/event";
 import { EventType, MsgType } from "../@types/event";
+import { defer, IDeferred } from "../utils";
 
 /**
  * An incoming, or outgoing, request to verify a user or a device via cross-signing.
@@ -85,7 +86,7 @@ export class RustVerificationRequest
                 if (this._verifier === undefined || this._verifier instanceof RustQrCodeVerifier) {
                     this.setVerifier(new RustSASVerifier(verification, this, outgoingRequestProcessor));
                 } else if (this._verifier instanceof RustSASVerifier) {
-                    this._verifier.setInner(verification);
+                    this._verifier.replaceInner(verification);
                 }
             } else if (verification instanceof RustSdkCryptoJs.Qr && this._verifier === undefined) {
                 this.setVerifier(new RustQrCodeVerifier(verification, outgoingRequestProcessor));
@@ -455,53 +456,39 @@ abstract class BaseRustVerifer<InnerType extends RustSdkCryptoJs.Qr | RustSdkCry
     VerifierEventHandlerMap & VerificationRequestEventHandlerMap
 > {
     /** A promise which completes when the verification completes (or rejects when it is cancelled/fails) */
-    protected readonly completionPromise: Promise<void>;
-    private onChangeCallback!: () => Promise<void>;
+    protected readonly completionDeferred: IDeferred<void>;
 
     public constructor(
         protected inner: InnerType,
         protected readonly outgoingRequestProcessor: OutgoingRequestProcessor,
     ) {
         super();
 
-        this.completionPromise = new Promise<void>((resolve, reject) => {
-            this.onChangeCallback = async (): Promise<void> => {
-                this.onChange();
-
-                if (this.inner.isDone()) {
-                    resolve(undefined);
-                } else if (this.inner.isCancelled()) {
-                    const cancelInfo = this.inner.cancelInfo()!;
-                    reject(
-                        new Error(
-                            `Verification cancelled by ${
-                                cancelInfo.cancelledbyUs() ? "us" : "them"
-                            } with code ${cancelInfo.cancelCode()}: ${cancelInfo.reason()}`,
-                        ),
-                    );
-                }
-
-                this.emit(VerificationRequestEvent.Change);
-            };
-            inner.registerChangesCallback(this.onChangeCallback);
+        this.completionDeferred = defer();
+        inner.registerChangesCallback(async () => {
+            await this.onChangeCallback();
         });
         // stop the runtime complaining if nobody catches a failure
-        this.completionPromise.catch(() => null);
-    }
-
-    /**
-     * Replace the inner Rust verifier with a different one.
-     *
-     * @param inner - the new Rust verifier
-     * @internal
-     */
-    public setInner(inner: InnerType): void {
-        if (this.inner != inner) {
-            this.inner = inner;
-            inner.registerChangesCallback(this.onChangeCallback);
-            this.onSetInner();
-            this.onChangeCallback();
+        this.completionDeferred.promise.catch(() => null);
+    }
+
+    protected async onChangeCallback(): Promise<void> {
+        this.onChange();
+
+        if (this.inner.isDone()) {
+            this.completionDeferred.resolve(undefined);
+        } else if (this.inner.isCancelled()) {
+            const cancelInfo = this.inner.cancelInfo()!;
+            this.completionDeferred.reject(
+                new Error(
+                    `Verification cancelled by ${
+                        cancelInfo.cancelledbyUs() ? "us" : "them"
+                    } with code ${cancelInfo.cancelCode()}: ${cancelInfo.reason()}`,
+                ),
+            );
         }
+
+        this.emit(VerificationRequestEvent.Change);
     }
 
     /**
@@ -511,11 +498,6 @@ abstract class BaseRustVerifer<InnerType extends RustSdkCryptoJs.Qr | RustSdkCry
      */
     protected onChange(): void {}
 
-    /**
-     * Hook which is called when the underlying rust class is changed by calling `setInner`.
-     */
-    protected onSetInner(): void {}
-
     /**
      * Returns true if the verification has been cancelled, either by us or the other side.
      */
@@ -599,7 +581,7 @@ export class RustQrCodeVerifier extends BaseRustVerifer<RustSdkCryptoJs.Qr> impl
             this.emit(VerifierEvent.ShowReciprocateQr, this.callbacks);
         }
         // Nothing to do here but wait.
-        await this.completionPromise;
+        await this.completionDeferred.promise;
     }
 
     /**
@@ -657,7 +639,6 @@ export class RustQrCodeVerifier extends BaseRustVerifer<RustSdkCryptoJs.Qr> impl
 /** A Verifier instance which is used if we are exchanging emojis */
 export class RustSASVerifier extends BaseRustVerifer<RustSdkCryptoJs.Sas> implements Verifier {
     private callbacks: ShowSasCallbacks | null = null;
-    private accepted: boolean = false;
 
     public constructor(
         inner: RustSdkCryptoJs.Sas,
@@ -677,18 +658,12 @@ export class RustSASVerifier extends BaseRustVerifer<RustSdkCryptoJs.Sas> implem
      *    or times out.
      */
     public async verify(): Promise<void> {
-        this.accepted = true;
-        try {
-            await this.sendAccept();
-        } catch (e) {
-            this.accepted = false;
-            throw e;
-        }
-        await this.completionPromise;
+        await this.sendAccept();
+        await this.completionDeferred.promise;
     }
 
     /**
-     * Send the accept event, if it hasn't already been sent
+     * Send the accept or start event, if it hasn't already been sent
      */
     private async sendAccept(): Promise<void> {
         const req: undefined | OutgoingRequest = this.inner.accept();
@@ -734,14 +709,6 @@ export class RustSASVerifier extends BaseRustVerifer<RustSdkCryptoJs.Sas> implem
         }
     }
 
-    public onSetInner(): void {
-        if (this.accepted) {
-            // if the verification was already accepted, but the inner verifier
-            // got changed, will need to re-accept
-            this.sendAccept();
-        }
-    }
-
     /**
      * Calculate an appropriate VerificationPhase for a VerificationRequest where this is the verifier.
      */
@@ -758,6 +725,25 @@ export class RustSASVerifier extends BaseRustVerifer<RustSdkCryptoJs.Sas> implem
     public getShowSasCallbacks(): ShowSasCallbacks | null {
         return this.callbacks;
     }
+
+    /**
+     * Replace the inner Rust verifier with a different one.
+     *
+     * @param inner - the new Rust verifier
+     * @internal
+     */
+    public replaceInner(inner: RustSdkCryptoJs.Sas): void {
+        if (this.inner != inner) {
+            this.inner = inner;
+            inner.registerChangesCallback(async () => {
+                await this.onChangeCallback();
+            });
+            // replaceInner will only get called if we started the verification at the same time as the other side, and we lost
+            // the tie breaker.  So we need to re-accept their verification.
+            this.sendAccept();
+            this.onChangeCallback();
+        }
+    }
 }
 
 /** For each specced verification method, the rust-side `VerificationMethod` corresponding to it */