From 26d17eb2c3b293f601d7cb5879fc4bb5af496497 Mon Sep 17 00:00:00 2001 From: Aofei Sheng Date: Wed, 21 Aug 2024 18:15:50 +0800 Subject: [PATCH] fix(Project): prevent potential data corruption in `saveToCloud` and `saveToLocalCache` (#777) Fixes #761 --- .../components/project/ProjectCreateModal.vue | 2 +- spx-gui/src/models/animation.test.ts | 2 +- spx-gui/src/models/project/history.ts | 9 ++-- spx-gui/src/models/project/index.test.ts | 14 ++++- spx-gui/src/models/project/index.ts | 52 +++++++++++-------- spx-gui/src/utils/disposable.ts | 16 ++++-- spx-gui/src/utils/exception.ts | 2 +- 7 files changed, 62 insertions(+), 35 deletions(-) diff --git a/spx-gui/src/components/project/ProjectCreateModal.vue b/spx-gui/src/components/project/ProjectCreateModal.vue index acd524e09..b0c9f2bb0 100644 --- a/spx-gui/src/components/project/ProjectCreateModal.vue +++ b/spx-gui/src/components/project/ProjectCreateModal.vue @@ -99,7 +99,7 @@ const handleSubmit = useMessageHandle( project.addSprite(sprite) await sprite.autoFit() // upload project content & call API addProject, TODO: maybe this should be extracted to `@/models`? - const files = project.export()[1] + const [, files] = await project.export() const { fileCollection } = await saveFiles(files) const projectData = await addProject({ name: form.value.name, diff --git a/spx-gui/src/models/animation.test.ts b/spx-gui/src/models/animation.test.ts index e107a080e..c422c2e3b 100644 --- a/spx-gui/src/models/animation.test.ts +++ b/spx-gui/src/models/animation.test.ts @@ -85,7 +85,7 @@ describe('Animation', () => { const project = makeProject() project.sprites[0].animations[0].setSound(project.sounds[0].name) - const [metadata, files] = project.export() + const [metadata, files] = await project.export() const delayedFiles: Files = Object.fromEntries( Object.entries(files).map(([path, file]) => [path, delayFile(file!, 50)]) ) diff --git a/spx-gui/src/models/project/history.ts b/spx-gui/src/models/project/history.ts index ce47f32b9..ea99d97ab 100644 --- a/spx-gui/src/models/project/history.ts +++ b/spx-gui/src/models/project/history.ts @@ -5,7 +5,6 @@ import { shallowReactive } from 'vue' import type { LocaleMessage } from '@/utils/i18n' -import Mutex from '@/utils/mutex' import type { Files } from '../common/file' import type { Project } from '.' @@ -27,8 +26,6 @@ export type State = { } export class History { - private mutex = new Mutex() - constructor( private project: Project, /** @@ -71,7 +68,7 @@ export class History { } redo() { - return this.mutex.runExclusive(() => this.goto(this.index + 1)) + return this.project.historyMutex.runExclusive(() => this.goto(this.index + 1)) } getUndoAction() { @@ -80,11 +77,11 @@ export class History { } undo() { - return this.mutex.runExclusive(() => this.goto(this.index - 1)) + return this.project.historyMutex.runExclusive(() => this.goto(this.index - 1)) } doAction(action: Action, fn: () => T | Promise): Promise { - return this.mutex.runExclusive(async () => { + return this.project.historyMutex.runExclusive(async () => { // history after current state (for redo) will be discarded on any action this.states.splice(this.index) diff --git a/spx-gui/src/models/project/index.test.ts b/spx-gui/src/models/project/index.test.ts index 207604361..3a836481b 100644 --- a/spx-gui/src/models/project/index.test.ts +++ b/spx-gui/src/models/project/index.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from 'vitest' +import { describe, it, expect, vi } from 'vitest' import { Sprite } from '../sprite' import { Animation } from '../animation' import { Sound } from '../sound' @@ -124,4 +124,16 @@ describe('Project', () => { project.removeSprite('Sprite2') expect(project.selected).toBeNull() }) + + it('should throw an error when saving a disposed project', async () => { + const project = makeProject() + const saveToLocalCacheMethod = vi.spyOn(project, 'saveToLocalCache' as any) + + project.dispose() + + await expect(project.saveToCloud()).rejects.toThrow('disposed') + + await expect((project as any).saveToLocalCache('key')).rejects.toThrow('disposed') + expect(saveToLocalCacheMethod).toHaveBeenCalledWith('key') + }) }) diff --git a/spx-gui/src/models/project/index.ts b/spx-gui/src/models/project/index.ts index 2d465c618..8ec026724 100644 --- a/spx-gui/src/models/project/index.ts +++ b/spx-gui/src/models/project/index.ts @@ -21,6 +21,7 @@ import { Sprite } from '../sprite' import { Sound } from '../sound' import type { RawWidgetConfig } from '../widget' import { History } from './history' +import Mutex from '@/utils/mutex' export type { Action } from './history' @@ -251,6 +252,7 @@ export class Project extends Disposable { } history: History + historyMutex = new Mutex() constructor() { super() @@ -267,10 +269,24 @@ export class Project extends Disposable { return reactiveThis } - private applyMetadata(metadata: Metadata) { + private loadMetadata(metadata: Metadata) { assign(this, metadata) } + private exportMetadata(): Metadata { + return { + id: this.id, + owner: this.owner, + name: this.name, + isPublic: this.isPublic, + version: this.version, + cTime: this.cTime, + uTime: this.uTime, + filesHash: this.filesHash, + lastSyncedFilesHash: this.lastSyncedFilesHash + } + } + async loadGameFiles(files: Files) { const configFile = files[projectConfigFilePath] const config: RawProjectConfig = {} @@ -330,25 +346,13 @@ export class Project extends Disposable { /** Load with metadata & game files */ async load(metadata: Metadata, files: Files) { - this.applyMetadata(metadata) + this.loadMetadata(metadata) await this.loadGameFiles(files) } /** Export metadata & game files */ - export(): [Metadata, Files] { - const metadata: Metadata = { - id: this.id, - owner: this.owner, - name: this.name, - isPublic: this.isPublic, - version: this.version, - cTime: this.cTime, - uTime: this.uTime, - filesHash: this.filesHash, - lastSyncedFilesHash: this.lastSyncedFilesHash - } - const files = this.exportGameFiles() - return [metadata, files] + async export(): Promise<[Metadata, Files]> { + return this.historyMutex.runExclusive(() => [this.exportMetadata(), this.exportGameFiles()]) } async loadGbpFile(file: globalThis.File) { @@ -363,7 +367,7 @@ export class Project extends Disposable { } async exportGbpFile() { - const [metadata, files] = this.export() + const [metadata, files] = await this.export() return await gbpHelper.save(metadata, files) } @@ -380,9 +384,10 @@ export class Project extends Disposable { /** Save to cloud */ async saveToCloud() { - const [metadata, files] = this.export() + const [metadata, files] = await this.export() + if (this.isDisposed) throw new Error('disposed') const saved = await cloudHelper.save(metadata, files) - this.applyMetadata(saved.metadata) + this.loadMetadata(saved.metadata) this.lastSyncedFilesHash = await hashFiles(files) } @@ -396,7 +401,8 @@ export class Project extends Disposable { /** Save to local cache */ private async saveToLocalCache(key: string) { - const [metadata, files] = this.export() + const [metadata, files] = await this.export() + if (this.isDisposed) throw new Error('disposed') await localHelper.save(key, metadata, files) } @@ -486,7 +492,11 @@ export class Project extends Disposable { else delazyLoadGameFiles() } })() - this.addDisposer(watch(() => this.export(), autoSaveToLocalCache, { immediate: true })) + this.addDisposer( + watch(() => [this.exportMetadata(), this.exportGameFiles()], autoSaveToLocalCache, { + immediate: true + }) + ) // watch for autoSaveMode switch, and trigger auto save accordingly this.addDisposer( diff --git a/spx-gui/src/utils/disposable.ts b/spx-gui/src/utils/disposable.ts index fcdddd464..48e14466b 100644 --- a/spx-gui/src/utils/disposable.ts +++ b/spx-gui/src/utils/disposable.ts @@ -6,15 +6,23 @@ export type Disposer = () => void export class Disposable { - _disposers: Disposer[] = [] + private disposers: Disposer[] = [] + + private _isDisposed = false + get isDisposed() { + return this._isDisposed + } addDisposer = (disposer: Disposer) => { - this._disposers.push(disposer) + if (this._isDisposed) throw new Error('disposed') + this.disposers.push(disposer) } dispose = () => { - while (this._disposers.length > 0) { - this._disposers.pop()?.() + if (this._isDisposed) return + this._isDisposed = true + while (this.disposers.length > 0) { + this.disposers.pop()?.() } } } diff --git a/spx-gui/src/utils/exception.ts b/spx-gui/src/utils/exception.ts index 0651d37a8..3d94bae70 100644 --- a/spx-gui/src/utils/exception.ts +++ b/spx-gui/src/utils/exception.ts @@ -30,7 +30,7 @@ export class DefaultException extends Exception { } /** - * Cancelled is a special exception, it stands for a "cancel operation" because of user ineraction. + * Cancelled is a special exception, it stands for a "cancel operation" because of user interaction. * Like other exceptions, it breaks normal flows, while it is supposed to be ignored by all user-feedback components, * so the user will not be notified of cancelled exceptions. */