fix: too many setValue calls on storage service lead to race conditio…

…n with incorrect storage value persisted to disk after deinit
standardnotes · Jul 11, 2023 · d4e2979 · d4e2979
1 parent 02df94b
commit d4e2979
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 31 deletions.
diff --git a/packages/snjs/lib/Services/Features/FeaturesService.spec.ts b/packages/snjs/lib/Services/Features/FeaturesService.spec.ts
@@ -178,7 +178,7 @@ describe('FeaturesService', () => {
       const mock = (featuresService['notifyEvent'] = jest.fn())
 
       const newRoles = [...roles, RoleName.NAMES.PlusUser]
-      await featuresService.setOnlineRoles(newRoles)
+      featuresService.setOnlineRoles(newRoles)
 
       expect(mock.mock.calls[0][0]).toEqual(FeaturesEvent.UserRolesChanged)
     })
@@ -305,7 +305,7 @@ describe('FeaturesService', () => {
       const featuresService = createService()
 
       featuresService.hasFirstPartyOfflineSubscription = jest.fn().mockReturnValue(true)
-      await featuresService.setOfflineRoles([RoleName.NAMES.CoreUser, RoleName.NAMES.PlusUser])
+      featuresService.setOfflineRoles([RoleName.NAMES.CoreUser, RoleName.NAMES.PlusUser])
 
       expect(featuresService.getFeatureStatus(FeatureIdentifier.MidnightTheme)).toBe(FeatureStatus.Entitled)
       expect(featuresService.getFeatureStatus(FeatureIdentifier.TokenVaultEditor)).toBe(FeatureStatus.Entitled)

diff --git a/packages/snjs/lib/Services/Features/FeaturesService.ts b/packages/snjs/lib/Services/Features/FeaturesService.ts
@@ -63,7 +63,6 @@ export class SNFeaturesService
   private onlineRoles: string[] = []
   private offlineRoles: string[] = []
   private enabledExperimentalFeatures: FeatureIdentifier[] = []
-  private needsInitialFeaturesUpdate = true
 
   private getFeatureStatusUseCase = new GetFeatureStatusUseCase(this.items)
 
@@ -88,7 +87,7 @@ export class SNFeaturesService
       sockets.addEventObserver(async (eventName, data) => {
         if (eventName === WebSocketsServiceEvent.UserRoleMessageReceived) {
           const currentRoles = (data as UserRolesChangedEvent).payload.currentRoles
-          await this.didReceiveNewRolesFromServer(currentRoles)
+          void this.updateOnlineRolesWithNewValues(currentRoles)
         }
       }),
     )
@@ -114,9 +113,9 @@ export class SNFeaturesService
         if (sources.includes(source)) {
           const items = [...changed, ...inserted] as SNFeatureRepo[]
           if (this.sessions.isSignedIntoFirstPartyServer()) {
-            await this.migrateFeatureRepoToUserSetting(items)
+            void this.migrateFeatureRepoToUserSetting(items)
           } else {
-            await this.migrateFeatureRepoToOfflineEntitlements(items)
+            void this.migrateFeatureRepoToOfflineEntitlements(items)
           }
         }
       }),
@@ -151,25 +150,22 @@ export class SNFeaturesService
       }
 
       const { userRoles } = event.payload as MetaReceivedData
-      await this.didReceiveNewRolesFromServer(userRoles.map((role) => role.name))
+      void this.updateOnlineRolesWithNewValues(userRoles.map((role) => role.name))
     }
   }
 
-  private async didReceiveNewRolesFromServer(roles: string[]): Promise<void> {
-    await this.updateOnlineRolesWithNewValues(roles)
-  }
-
   override async handleApplicationStage(stage: ApplicationStage): Promise<void> {
-    await super.handleApplicationStage(stage)
-
     if (stage === ApplicationStage.FullSyncCompleted_13) {
       if (!this.hasFirstPartyOnlineSubscription()) {
         const offlineRepo = this.getOfflineRepo()
+
         if (offlineRepo) {
           void this.downloadOfflineRoles(offlineRepo)
         }
       }
     }
+
+    return super.handleApplicationStage(stage)
   }
 
   public enableExperimentalFeature(identifier: FeatureIdentifier): void {
@@ -241,7 +237,9 @@ export class SNFeaturesService
         } as FeatureRepoContent),
         true,
       )) as SNFeatureRepo
+
       void this.sync.sync()
+
       return this.downloadOfflineRoles(offlineRepo)
     } catch (err) {
       return new ClientDisplayableError(`${API_MESSAGE_FAILED_OFFLINE_ACTIVATION}, ${err}`)
@@ -285,7 +283,7 @@ export class SNFeaturesService
       return result
     }
 
-    await this.setOfflineRoles(result.roles)
+    this.setOfflineRoles(result.roles)
   }
 
   public async migrateFeatureRepoToUserSetting(featureRepos: SNFeatureRepo[] = []): Promise<void> {
@@ -320,36 +318,28 @@ export class SNFeaturesService
     return hasFirstPartyOfflineSubscription || new URL(offlineRepo.content.offlineFeaturesUrl).hostname === 'localhost'
   }
 
-  async updateOnlineRolesWithNewValues(roles: string[]): Promise<{
-    didChangeRoles: boolean
-  }> {
+  async updateOnlineRolesWithNewValues(roles: string[]): Promise<void> {
     const previousRoles = this.onlineRoles
 
     const userRolesChanged =
       roles.some((role) => !this.onlineRoles.includes(role)) || this.onlineRoles.some((role) => !roles.includes(role))
 
     const isInitialLoadRolesChange = previousRoles.length === 0 && userRolesChanged
 
-    if (!userRolesChanged && !this.needsInitialFeaturesUpdate) {
-      return {
-        didChangeRoles: false,
-      }
+    if (!userRolesChanged) {
+      return
     }
 
-    await this.setOnlineRoles(roles)
+    this.setOnlineRoles(roles)
 
     if (userRolesChanged && !isInitialLoadRolesChange) {
       if (this.onlineRolesIncludePaidSubscription()) {
         await this.notifyEvent(FeaturesEvent.DidPurchaseSubscription)
       }
     }
-
-    return {
-      didChangeRoles: true,
-    }
   }
 
-  async setOnlineRoles(roles: string[]): Promise<void> {
+  setOnlineRoles(roles: string[]): void {
     const rolesChanged = !arraysEqual(this.onlineRoles, roles)
 
     this.onlineRoles = roles
@@ -361,7 +351,7 @@ export class SNFeaturesService
     this.storage.setValue(StorageKey.UserRoles, this.onlineRoles)
   }
 
-  async setOfflineRoles(roles: string[]): Promise<void> {
+  setOfflineRoles(roles: string[]): void {
     const rolesChanged = !arraysEqual(this.offlineRoles, roles)
 
     this.offlineRoles = roles

diff --git a/packages/snjs/mocha/auth-fringe-cases.test.js b/packages/snjs/mocha/auth-fringe-cases.test.js
@@ -27,7 +27,7 @@ describe('auth fringe cases', () => {
     localStorage.clear()
   })
 
-  const clearApplicationLocalStorage = function () {
+  const clearApplicationLocalStorageOfNonItems = function () {
     const keys = Object.keys(localStorage)
     for (const key of keys) {
       if (!key.toLowerCase().includes('item')) {
@@ -43,7 +43,7 @@ describe('auth fringe cases', () => {
       const context = await createContext()
       await context.application.register(context.email, context.password)
       const note = await Factory.createSyncedNote(context.application)
-      clearApplicationLocalStorage()
+      clearApplicationLocalStorageOfNonItems()
 
       console.warn("Expecting errors 'Unable to find operator for version undefined'")
 
@@ -58,7 +58,7 @@ describe('auth fringe cases', () => {
       const context = await createContext()
       await context.application.register(context.email, context.password)
       const note = await Factory.createSyncedNote(context.application)
-      clearApplicationLocalStorage()
+      clearApplicationLocalStorageOfNonItems()
       const restartedApplication = await Factory.restartApplication(context.application)
 
       console.warn(