Make error handling in decryptionLoop more generic (#3024)

Not everything is a `DecryptionError`, and there's no real reason that we
should only do retries for `DecryptionError`s

Author: richvdh

spec/unit/models/event.spec.ts

@@ -16,7 +16,6 @@ limitations under the License.
 
 import { MatrixEvent, MatrixEventEvent } from "../../../src/models/event";
 import { emitPromise } from "../../test-utils/test-utils";
-import { EventType } from "../../../src";
 import { Crypto } from "../../../src/crypto";
 
 describe("MatrixEvent", () => {
@@ -88,22 +87,6 @@ describe("MatrixEvent", () => {
         expect(ev.getWireContent().ciphertext).toBeUndefined();
     });
 
-    it("should abort decryption if fails with an error other than a DecryptionError", async () => {
-        const ev = new MatrixEvent({
-            type: EventType.RoomMessageEncrypted,
-            content: {
-                body: "Test",
-            },
-            event_id: "$event1:server",
-        });
-        await ev.attemptDecryption({
-            decryptEvent: jest.fn().mockRejectedValue(new Error("Not a DecryptionError")),
-        } as unknown as Crypto);
-        expect(ev.isEncrypted()).toBeTruthy();
-        expect(ev.isBeingDecrypted()).toBeFalsy();
-        expect(ev.isDecryptionFailure()).toBeFalsy();
-    });
-
     describe("applyVisibilityEvent", () => {
         it("should emit VisibilityChange if a change was made", async () => {
             const ev = new MatrixEvent({
@@ -134,6 +117,21 @@ describe("MatrixEvent", () => {
             });
         });
 
+        it("should report decryption errors", async () => {
+            const crypto = {
+                decryptEvent: jest.fn().mockRejectedValue(new Error("test error")),
+            } as unknown as Crypto;
+
+            await encryptedEvent.attemptDecryption(crypto);
+            expect(encryptedEvent.isEncrypted()).toBeTruthy();
+            expect(encryptedEvent.isBeingDecrypted()).toBeFalsy();
+            expect(encryptedEvent.isDecryptionFailure()).toBeTruthy();
+            expect(encryptedEvent.getContent()).toEqual({
+                msgtype: "m.bad.encrypted",
+                body: "** Unable to decrypt: Error: test error **",
+            });
+        });
+
         it("should retry decryption if a retry is queued", async () => {
             const eventAttemptDecryptionSpy = jest.spyOn(encryptedEvent, "attemptDecryption");
 

src/models/event.ts

@@ -828,17 +828,7 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
                     }
                 }
             } catch (e) {
-                if ((<Error>e).name !== "DecryptionError") {
-                    // not a decryption error: log the whole exception as an error
-                    // (and don't bother with a retry)
-                    const re = options.isRetry ? "re" : "";
-                    // For find results: this can produce "Error decrypting event (id=$ev)" and
-                    // "Error redecrypting event (id=$ev)".
-                    logger.error(`Error ${re}decrypting event (${this.getDetails()})`, e);
-                    this.decryptionPromise = null;
-                    this.retryDecryption = false;
-                    return;
-                }
+                const detailedError = e instanceof DecryptionError ? (<DecryptionError>e).detailedString : String(e);
 
                 err = e as Error;
 
@@ -858,10 +848,7 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
                 //
                 if (this.retryDecryption) {
                     // decryption error, but we have a retry queued.
-                    logger.log(
-                        `Error decrypting event (${this.getDetails()}), but retrying: ` +
-                            (<DecryptionError>e).detailedString,
-                    );
+                    logger.log(`Error decrypting event (${this.getDetails()}), but retrying: ${detailedError}`);
                     continue;
                 }
 
@@ -870,9 +857,9 @@ export class MatrixEvent extends TypedEventEmitter<MatrixEventEmittedEvents, Mat
                 //
                 // the detailedString already includes the name and message of the error, and the stack isn't much use,
                 // so we don't bother to log `e` separately.
-                logger.warn(`Error decrypting event (${this.getDetails()}): ` + (<DecryptionError>e).detailedString);
+                logger.warn(`Error decrypting event (${this.getDetails()}): ${detailedError}`);
 
-                res = this.badEncryptedMessage((<DecryptionError>e).message);
+                res = this.badEncryptedMessage(String(e));
             }
 
             // at this point, we've either successfully decrypted the event, or have given up