Fix account & room settings race condition (#7953)

t3chguy · web-flow · commit afbe3d16b4fd · 2022-03-03T22:09:06.000Z
diff --git a/src/settings/handlers/AccountSettingsHandler.ts b/src/settings/handlers/AccountSettingsHandler.ts
@@ -17,8 +17,8 @@ limitations under the License.
 
 import { ClientEvent, MatrixClient } from "matrix-js-sdk/src/client";
 import { MatrixEvent } from "matrix-js-sdk/src/models/event";
+import { defer } from "matrix-js-sdk/src/utils";
 
-import { MatrixClientPeg } from '../../MatrixClientPeg';
 import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler";
 import { objectClone, objectKeyChanges } from "../../utils/objects";
 import { SettingLevel } from "../SettingLevel";
@@ -30,6 +30,7 @@ const BREADCRUMBS_EVENT_TYPES = [BREADCRUMBS_LEGACY_EVENT_TYPE, BREADCRUMBS_EVEN
 const RECENT_EMOJI_EVENT_TYPE = "io.element.recent_emoji";
 const INTEG_PROVISIONING_EVENT_TYPE = "im.vector.setting.integration_provisioning";
 const ANALYTICS_EVENT_TYPE = "im.vector.analytics";
+const DEFAULT_SETTINGS_EVENT_TYPE = "im.vector.web.settings";
 
 /**
  * Gets and sets settings at the "account" level for the current user.
@@ -45,10 +46,7 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa
     }
 
     public initMatrixClient(oldClient: MatrixClient, newClient: MatrixClient) {
-        if (oldClient) {
-            oldClient.removeListener(ClientEvent.AccountData, this.onAccountData);
-        }
-
+        oldClient?.removeListener(ClientEvent.AccountData, this.onAccountData);
         newClient.on(ClientEvent.AccountData, this.onAccountData);
     }
 
@@ -62,9 +60,9 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa
             }
 
             this.watchers.notifyUpdate("urlPreviewsEnabled", null, SettingLevel.ACCOUNT, val);
-        } else if (event.getType() === "im.vector.web.settings" || event.getType() === ANALYTICS_EVENT_TYPE) {
+        } else if (event.getType() === DEFAULT_SETTINGS_EVENT_TYPE || event.getType() === ANALYTICS_EVENT_TYPE) {
             // Figure out what changed and fire those updates
-            const prevContent = prevEvent ? prevEvent.getContent() : {};
+            const prevContent = prevEvent?.getContent() ?? {};
             const changedSettings = objectKeyChanges<Record<string, any>>(prevContent, event.getContent());
             for (const settingName of changedSettings) {
                 const val = event.getContent()[settingName];
@@ -136,72 +134,81 @@ export default class AccountSettingsHandler extends MatrixClientBackedSettingsHa
         return preferredValue;
     }
 
-    public async setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
-        // Special case URL previews
-        if (settingName === "urlPreviewsEnabled") {
-            const content = this.getSettings("org.matrix.preview_urls") || {};
-            content['disable'] = !newValue;
-            await MatrixClientPeg.get().setAccountData("org.matrix.preview_urls", content);
-            return;
+    // helper function to set account data then await it being echoed back
+    private async setAccountData(
+        eventType: string,
+        field: string,
+        value: any,
+        legacyEventType?: string,
+    ): Promise<void> {
+        let content = this.getSettings(eventType);
+        if (legacyEventType && !content?.[field]) {
+            content = this.getSettings(legacyEventType);
         }
 
-        // Special case for breadcrumbs
-        if (settingName === "breadcrumb_rooms") {
-            // We read the value first just to make sure we preserve whatever random keys might be present.
-            let content = this.getSettings(BREADCRUMBS_EVENT_TYPE);
-            if (!content || !content['recent_rooms']) {
-                content = this.getSettings(BREADCRUMBS_LEGACY_EVENT_TYPE);
-            }
-            if (!content) content = {}; // If we still don't have content, make some
-
-            content['recent_rooms'] = newValue;
-            await MatrixClientPeg.get().setAccountData(BREADCRUMBS_EVENT_TYPE, content);
-            return;
+        if (!content) {
+            content = {};
         }
 
-        // Special case recent emoji
-        if (settingName === "recent_emoji") {
-            const content = this.getSettings(RECENT_EMOJI_EVENT_TYPE) || {};
-            content["recent_emoji"] = newValue;
-            await MatrixClientPeg.get().setAccountData(RECENT_EMOJI_EVENT_TYPE, content);
-            return;
-        }
+        content[field] = value;
 
-        // Special case integration manager provisioning
-        if (settingName === "integrationProvisioning") {
-            const content = this.getSettings(INTEG_PROVISIONING_EVENT_TYPE) || {};
-            content['enabled'] = newValue;
-            await MatrixClientPeg.get().setAccountData(INTEG_PROVISIONING_EVENT_TYPE, content);
-            return;
-        }
+        await this.client.setAccountData(eventType, content);
 
-        // Special case analytics
-        if (settingName === "pseudonymousAnalyticsOptIn") {
-            const content = this.getSettings(ANALYTICS_EVENT_TYPE) || {};
-            content[settingName] = newValue;
-            await MatrixClientPeg.get().setAccountData(ANALYTICS_EVENT_TYPE, content);
-            return;
-        }
+        const deferred = defer<void>();
+        const handler = (event: MatrixEvent) => {
+            if (event.getType() !== eventType || event.getContent()[field] !== value) return;
+            this.client.off(ClientEvent.AccountData, handler);
+            deferred.resolve();
+        };
+        this.client.on(ClientEvent.AccountData, handler);
 
-        const content = this.getSettings() || {};
-        content[settingName] = newValue;
-        await MatrixClientPeg.get().setAccountData("im.vector.web.settings", content);
+        await deferred.promise;
+    }
+
+    public setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
+        switch (settingName) {
+            // Special case URL previews
+            case "urlPreviewsEnabled":
+                return this.setAccountData("org.matrix.preview_urls", "disable", !newValue);
+
+            // Special case for breadcrumbs
+            case "breadcrumb_rooms":
+                return this.setAccountData(
+                    BREADCRUMBS_EVENT_TYPE,
+                    "recent_rooms",
+                    newValue,
+                    BREADCRUMBS_LEGACY_EVENT_TYPE,
+                );
+
+            // Special case recent emoji
+            case "recent_emoji":
+                return this.setAccountData(RECENT_EMOJI_EVENT_TYPE, "recent_emoji", newValue);
+
+            // Special case integration manager provisioning
+            case "integrationProvisioning":
+                return this.setAccountData(INTEG_PROVISIONING_EVENT_TYPE, "enabled", newValue);
+
+            // Special case analytics
+            case "pseudonymousAnalyticsOptIn":
+                return this.setAccountData(ANALYTICS_EVENT_TYPE, "pseudonymousAnalyticsOptIn", newValue);
+
+            default:
+                return this.setAccountData(DEFAULT_SETTINGS_EVENT_TYPE, settingName, newValue);
+        }
     }
 
     public canSetValue(settingName: string, roomId: string): boolean {
         return true; // It's their account, so they should be able to
     }
 
     public isSupported(): boolean {
-        const cli = MatrixClientPeg.get();
-        return cli !== undefined && cli !== null && !cli.isGuest();
+        return this.client && !this.client.isGuest();
     }
 
     private getSettings(eventType = "im.vector.web.settings"): any { // TODO: [TS] Types on return
-        const cli = MatrixClientPeg.get();
-        if (!cli) return null;
+        if (!this.client) return null;
 
-        const event = cli.getAccountData(eventType);
+        const event = this.client.getAccountData(eventType);
         if (!event || !event.getContent()) return null;
         return objectClone(event.getContent()); // clone to prevent mutation
     }
diff --git a/src/settings/handlers/LocalEchoWrapper.ts b/src/settings/handlers/LocalEchoWrapper.ts
@@ -40,31 +40,37 @@ export default class LocalEchoWrapper extends SettingsHandler {
     }
 
     public getValue(settingName: string, roomId: string): any {
-        const cacheRoomId = roomId ? roomId : "UNDEFINED"; // avoid weird keys
+        const cacheRoomId = roomId ?? "UNDEFINED"; // avoid weird keys
         const bySetting = this.cache[settingName];
-        if (bySetting && bySetting.hasOwnProperty(cacheRoomId)) {
+        if (bySetting?.hasOwnProperty(cacheRoomId)) {
             return bySetting[cacheRoomId];
         }
 
         return this.handler.getValue(settingName, roomId);
     }
 
-    public setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
+    public async setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
         if (!this.cache[settingName]) this.cache[settingName] = {};
         const bySetting = this.cache[settingName];
 
-        const cacheRoomId = roomId ? roomId : "UNDEFINED"; // avoid weird keys
+        const cacheRoomId = roomId ?? "UNDEFINED"; // avoid weird keys
         bySetting[cacheRoomId] = newValue;
 
         const currentValue = this.handler.getValue(settingName, roomId);
         const handlerPromise = this.handler.setValue(settingName, roomId, newValue);
         this.handler.watchers?.notifyUpdate(settingName, roomId, this.level, newValue);
-        return Promise.resolve(handlerPromise).catch(() => {
+
+        try {
+            await handlerPromise;
+        } catch (e) {
             // notify of a rollback
             this.handler.watchers?.notifyUpdate(settingName, roomId, this.level, currentValue);
-        }).finally(() => {
-            delete bySetting[cacheRoomId];
-        });
+        } finally {
+            // only expire the cache if our value hasn't been overwritten yet
+            if (bySetting[cacheRoomId] === newValue) {
+                delete bySetting[cacheRoomId];
+            }
+        }
     }
 
     public canSetValue(settingName: string, roomId: string): boolean {
diff --git a/src/settings/handlers/MatrixClientBackedSettingsHandler.ts b/src/settings/handlers/MatrixClientBackedSettingsHandler.ts
@@ -15,7 +15,6 @@ limitations under the License.
 */
 
 import { MatrixClient } from "matrix-js-sdk/src/client";
-import { logger } from "matrix-js-sdk/src/logger";
 
 import SettingsHandler from "./SettingsHandler";
 
@@ -49,7 +48,5 @@ export default abstract class MatrixClientBackedSettingsHandler extends Settings
         return MatrixClientBackedSettingsHandler._matrixClient;
     }
 
-    protected initMatrixClient(oldClient: MatrixClient, newClient: MatrixClient) {
-        logger.warn("initMatrixClient not overridden");
-    }
+    protected abstract initMatrixClient(oldClient: MatrixClient, newClient: MatrixClient);
 }
diff --git a/src/settings/handlers/RoomAccountSettingsHandler.ts b/src/settings/handlers/RoomAccountSettingsHandler.ts
@@ -18,14 +18,15 @@ limitations under the License.
 import { MatrixClient } from "matrix-js-sdk/src/client";
 import { MatrixEvent } from "matrix-js-sdk/src/models/event";
 import { Room, RoomEvent } from "matrix-js-sdk/src/models/room";
+import { defer } from "matrix-js-sdk/src/utils";
 
-import { MatrixClientPeg } from '../../MatrixClientPeg';
 import MatrixClientBackedSettingsHandler from "./MatrixClientBackedSettingsHandler";
 import { objectClone, objectKeyChanges } from "../../utils/objects";
 import { SettingLevel } from "../SettingLevel";
 import { WatchManager } from "../WatchManager";
 
 const ALLOWED_WIDGETS_EVENT_TYPE = "im.vector.setting.allowed_widgets";
+const DEFAULT_SETTINGS_EVENT_TYPE = "im.vector.web.settings";
 
 /**
  * Gets and sets settings at the "room-account" level for the current user.
@@ -55,7 +56,7 @@ export default class RoomAccountSettingsHandler extends MatrixClientBackedSettin
             }
 
             this.watchers.notifyUpdate("urlPreviewsEnabled", roomId, SettingLevel.ROOM_ACCOUNT, val);
-        } else if (event.getType() === "im.vector.web.settings") {
+        } else if (event.getType() === DEFAULT_SETTINGS_EVENT_TYPE) {
             // Figure out what changed and fire those updates
             const prevContent = prevEvent ? prevEvent.getContent() : {};
             const changedSettings = objectKeyChanges<Record<string, any>>(prevContent, event.getContent());
@@ -87,43 +88,62 @@ export default class RoomAccountSettingsHandler extends MatrixClientBackedSettin
         return settings[settingName];
     }
 
-    public async setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
-        // Special case URL previews
-        if (settingName === "urlPreviewsEnabled") {
-            const content = this.getSettings(roomId, "org.matrix.room.preview_urls") || {};
-            content['disable'] = !newValue;
-            await MatrixClientPeg.get().setRoomAccountData(roomId, "org.matrix.room.preview_urls", content);
-            return;
+    // helper function to send room account data then await it being echoed back
+    private async setRoomAccountData(
+        roomId: string,
+        eventType: string,
+        field: string | null,
+        value: any,
+    ): Promise<void> {
+        let content: ReturnType<RoomAccountSettingsHandler["getSettings"]>;
+
+        if (field === null) {
+            content = value;
+        } else {
+            const content = this.getSettings(roomId, eventType) || {};
+            content[field] = value;
         }
 
-        // Special case allowed widgets
-        if (settingName === "allowedWidgets") {
-            await MatrixClientPeg.get().setRoomAccountData(roomId, ALLOWED_WIDGETS_EVENT_TYPE, newValue);
-            return;
-        }
+        await this.client.setRoomAccountData(roomId, eventType, content);
+
+        const deferred = defer<void>();
+        const handler = (event: MatrixEvent) => {
+            if (event.getRoomId() !== roomId || event.getType() !== eventType) return;
+            if (field !== null && event.getContent()[field] !== value) return;
+            this.client.off(RoomEvent.AccountData, handler);
+            deferred.resolve();
+        };
+        this.client.on(RoomEvent.AccountData, handler);
 
-        const content = this.getSettings(roomId) || {};
-        content[settingName] = newValue;
-        await MatrixClientPeg.get().setRoomAccountData(roomId, "im.vector.web.settings", content);
+        await deferred.promise;
     }
 
-    public canSetValue(settingName: string, roomId: string): boolean {
-        const room = MatrixClientPeg.get().getRoom(roomId);
+    public setValue(settingName: string, roomId: string, newValue: any): Promise<void> {
+        switch (settingName) {
+            // Special case URL previews
+            case "urlPreviewsEnabled":
+                return this.setRoomAccountData(roomId, "org.matrix.room.preview_urls", "disable", !newValue);
+
+            // Special case allowed widgets
+            case "allowedWidgets":
+                return this.setRoomAccountData(roomId, ALLOWED_WIDGETS_EVENT_TYPE, null, newValue);
 
+            default:
+                return this.setRoomAccountData(roomId, DEFAULT_SETTINGS_EVENT_TYPE, settingName, newValue);
+        }
+    }
+
+    public canSetValue(settingName: string, roomId: string): boolean {
         // If they have the room, they can set their own account data
-        return room !== undefined && room !== null;
+        return !!this.client.getRoom(roomId);
     }
 
     public isSupported(): boolean {
-        const cli = MatrixClientPeg.get();
-        return cli !== undefined && cli !== null && !cli.isGuest();
+        return this.client && !this.client.isGuest();
     }
 
-    private getSettings(roomId: string, eventType = "im.vector.web.settings"): any { // TODO: [TS] Type return
-        const room = MatrixClientPeg.get().getRoom(roomId);
-        if (!room) return null;
-
-        const event = room.getAccountData(eventType);
+    private getSettings(roomId: string, eventType = DEFAULT_SETTINGS_EVENT_TYPE): any { // TODO: [TS] Type return
+        const event = this.client.getRoom(roomId)?.getAccountData(eventType);
         if (!event || !event.getContent()) return null;
         return objectClone(event.getContent()); // clone to prevent mutation
     }
diff --git a/src/settings/handlers/RoomSettingsHandler.ts b/src/settings/handlers/RoomSettingsHandler.ts
diff --git a/test/test-utils/test-utils.ts b/test/test-utils/test-utils.ts

Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,6 @@ limitations under the License.`
`15`	`15`	`*/`
`16`	`16`
`17`	`17`	`import { MatrixClient } from "matrix-js-sdk/src/client";`
`18`		`-import { logger } from "matrix-js-sdk/src/logger";`
`19`	`18`
`20`	`19`	`import SettingsHandler from "./SettingsHandler";`
`21`	`20`
`@@ -49,7 +48,5 @@ export default abstract class MatrixClientBackedSettingsHandler extends Settings`
`49`	`48`	`return MatrixClientBackedSettingsHandler._matrixClient;`
`50`	`49`	`}`
`51`	`50`
`52`		`- protected initMatrixClient(oldClient: MatrixClient, newClient: MatrixClient) {`
`53`		`- logger.warn("initMatrixClient not overridden");`
`54`		`- }`
	`51`	`+ protected abstract initMatrixClient(oldClient: MatrixClient, newClient: MatrixClient);`
`55`	`52`	`}`