Element-R: Fix resource leaks in verification logic (#4263)

* Move `RustVerificationRequest.onChange` out to a method

The only reason it was an inner function in the first place was to avoid
storing a reference in the class to `outgoingRequestProcessor`. That changed
with d1dec4c.

* Fix reference cycles in rust verification code

Author: richvdh

src/rust-crypto/verification.ts

@@ -72,30 +72,40 @@ export class RustVerificationRequest
         private readonly supportedVerificationMethods: string[],
     ) {
         super();
-
         this.reEmitter = new TypedReEmitter(this);
 
-        const onChange = async (): Promise<void> => {
-            const verification: RustSdkCryptoJs.Qr | RustSdkCryptoJs.Sas | undefined = this.inner.getVerification();
-
-            // Set the _verifier object (wrapping the rust `Verification` as a js-sdk Verifier) if:
-            // - we now have a `Verification` where we lacked one before
-            // - we have transitioned from QR to SAS
-            // - we are verifying with SAS, but we need to replace our verifier with a new one because both parties
-            //   tried to start verification at the same time, and we lost the tie breaking
-            if (verification instanceof RustSdkCryptoJs.Sas) {
-                if (this._verifier === undefined || this._verifier instanceof RustQrCodeVerifier) {
-                    this.setVerifier(new RustSASVerifier(verification, this, outgoingRequestProcessor));
-                } else if (this._verifier instanceof RustSASVerifier) {
-                    this._verifier.replaceInner(verification);
-                }
-            } else if (verification instanceof RustSdkCryptoJs.Qr && this._verifier === undefined) {
-                this.setVerifier(new RustQrCodeVerifier(verification, outgoingRequestProcessor));
+        // Obviously, the Rust object maintains a reference to the callback function. If the callback function maintains
+        // a reference to the Rust object, then we have a reference cycle which means that `RustVerificationRequest`
+        // will never be garbage-collected, and hence the underlying rust object will never be freed.
+        //
+        // To avoid this reference cycle, use a weak reference in the callback function. If the `RustVerificationRequest`
+        // gets garbage-collected, then there is nothing to update!
+        const weakThis = new WeakRef(this);
+        inner.registerChangesCallback(async () => weakThis.deref()?.onChange());
+    }
+
+    /**
+     * Hook which is called when the underlying rust class notifies us that there has been a change.
+     */
+    private onChange(): void {
+        const verification: RustSdkCryptoJs.Qr | RustSdkCryptoJs.Sas | undefined = this.inner.getVerification();
+
+        // Set the _verifier object (wrapping the rust `Verification` as a js-sdk Verifier) if:
+        // - we now have a `Verification` where we lacked one before
+        // - we have transitioned from QR to SAS
+        // - we are verifying with SAS, but we need to replace our verifier with a new one because both parties
+        //   tried to start verification at the same time, and we lost the tie breaking
+        if (verification instanceof RustSdkCryptoJs.Sas) {
+            if (this._verifier === undefined || this._verifier instanceof RustQrCodeVerifier) {
+                this.setVerifier(new RustSASVerifier(verification, this, this.outgoingRequestProcessor));
+            } else if (this._verifier instanceof RustSASVerifier) {
+                this._verifier.replaceInner(verification);
             }
+        } else if (verification instanceof RustSdkCryptoJs.Qr && this._verifier === undefined) {
+            this.setVerifier(new RustQrCodeVerifier(verification, this.outgoingRequestProcessor));
+        }
 
-            this.emit(VerificationRequestEvent.Change);
-        };
-        inner.registerChangesCallback(onChange);
+        this.emit(VerificationRequestEvent.Change);
     }
 
     private setVerifier(verifier: RustSASVerifier | RustQrCodeVerifier): void {
@@ -473,9 +483,12 @@ abstract class BaseRustVerifer<InnerType extends RustSdkCryptoJs.Qr | RustSdkCry
         super();
 
         this.completionDeferred = defer();
-        inner.registerChangesCallback(async () => {
-            this.onChange();
-        });
+
+        // As with RustVerificationRequest, we need to avoid a reference cycle.
+        // See the comments in RustVerificationRequest.
+        const weakThis = new WeakRef(this);
+        inner.registerChangesCallback(async () => weakThis.deref()?.onChange());
+
         // stop the runtime complaining if nobody catches a failure
         this.completionDeferred.promise.catch(() => null);
     }
@@ -752,9 +765,12 @@ export class RustSASVerifier extends BaseRustVerifer<RustSdkCryptoJs.Sas> implem
     public replaceInner(inner: RustSdkCryptoJs.Sas): void {
         if (this.inner != inner) {
             this.inner = inner;
-            inner.registerChangesCallback(async () => {
-                this.onChange();
-            });
+
+            // As with RustVerificationRequest, we need to avoid a reference cycle.
+            // See the comments in RustVerificationRequest.
+            const weakThis = new WeakRef(this);
+            inner.registerChangesCallback(async () => weakThis.deref()?.onChange());
+
             // 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();