Storage: fix clear and removeItem

src/main-thread/commands/storage.ts

@@ -2,7 +2,7 @@ import { CommandExecutorInterface } from './interface';
 import { TransferrableMutationType, StorageMutationIndex } from '../../transfer/TransferrableMutation';
 import { StorageLocation } from '../../transfer/TransferrableStorage';
 import { TransferrableKeys } from '../../transfer/TransferrableKeys';
-import { MessageType, StorageValueToWorker, GetOrSet } from '../../transfer/Messages';
+import { MessageType, StorageValueToWorker, GetOrSet, DELETION_INDEX } from '../../transfer/Messages';
 
 export const StorageProcessor: CommandExecutorInterface = (strings, nodeContext, workerContext, objectContext, config) => {
   const allowedExecution = config.executorsAllowed.includes(TransferrableMutationType.STORAGE);
@@ -64,8 +64,8 @@ export const StorageProcessor: CommandExecutorInterface = (strings, nodeContext,
 
         // TODO(choumx): Clean up key/value strings (or don't store them in the first place)
         // to avoid leaking memory.
-        const key = keyIndex >= 0 ? strings.get(keyIndex) : '';
-        const value = valueIndex >= 0 ? strings.get(valueIndex) : null;
+        const key = keyIndex != DELETION_INDEX && keyIndex >= 0 ? strings.get(keyIndex) : '';
+        const value = valueIndex != DELETION_INDEX && valueIndex >= 0 ? strings.get(valueIndex) : null;
 
         if (operation === GetOrSet.GET) {
           get(location, key);

src/test/main-thread/commands/storage.test.ts

@@ -11,13 +11,17 @@ import { TransferrableKeys } from '../../../transfer/TransferrableKeys';
 
 const test = anyTest as TestInterface<{}>;
 
+type SetStorageMeta = { location: StorageLocation; key: string | null; value: string | null };
+
 function getStorageProcessor(strings: string[]): {
   processor: CommandExecutor;
   messages: Array<MessageToWorker>;
+  getLastSetStorage: () => any;
 } {
   const stringCtx = new StringContext();
   stringCtx.storeValues(strings);
   const messages: Array<MessageToWorker> = [];
+  let lastSetStorage: null | SetStorageMeta = null;
 
   const processor = StorageProcessor(
     stringCtx,
@@ -34,12 +38,16 @@ function getStorageProcessor(strings: string[]): {
         getStorage() {
           return Promise.resolve({ hello: 'world' });
         },
+        setStorage(location: StorageLocation, key: string | null, value: string | null) {
+          lastSetStorage = { location, key, value };
+        },
       } as unknown as Sanitizer,
     } as WorkerDOMConfiguration,
   );
   return {
     processor,
     messages,
+    getLastSetStorage: () => lastSetStorage,
   };
 }
 
@@ -63,3 +71,18 @@ test('StorageProcessor sends storage value event to worker', async (t) => {
   t.is(messages.length, 1);
   t.deepEqual(messages, [expectedMessage]);
 });
+
+test('StorageProcessor handles deletion event from worker', async (t) => {
+  const { processor, getLastSetStorage } = getStorageProcessor(['t', 'hello']);
+  const mutation: number[] = [];
+  mutation[StorageMutationIndex.Operation] = GetOrSet.SET;
+  mutation[StorageMutationIndex.Location] = StorageLocation.Local;
+  mutation[StorageMutationIndex.Key] = 1;
+  mutation[StorageMutationIndex.Value] = -1;
+  const mutations = new Uint16Array(mutation);
+
+  processor.execute(mutations, 0, true);
+  await Promise.resolve(setTimeout);
+
+  t.deepEqual(getLastSetStorage(), { location: StorageLocation.Local, key: 'hello', value: null });
+});

src/test/mutation-transfer/storage.test.ts

@@ -1,6 +1,6 @@
 import anyTest, { TestInterface } from 'ava';
 import { Document } from '../../worker-thread/dom/Document';
-import { GetOrSet } from '../../transfer/Messages';
+import { DELETION_INDEX, GetOrSet } from '../../transfer/Messages';
 import { Storage, createStorage } from '../../worker-thread/Storage';
 import { StorageLocation } from '../../transfer/TransferrableStorage';
 import { TransferrableMutationType } from '../../transfer/TransferrableMutation';
@@ -54,7 +54,7 @@ test.serial.cb('Storage.removeItem', (t) => {
   const { document, storage } = t.context;
 
   expectMutations(document, (mutations) => {
-    t.deepEqual(mutations, [TransferrableMutationType.STORAGE, GetOrSet.SET, StorageLocation.Local, getForTesting('foo'), 0]);
+    t.deepEqual(mutations, [TransferrableMutationType.STORAGE, GetOrSet.SET, StorageLocation.Local, getForTesting('foo'), DELETION_INDEX]);
     t.end();
   });
 
@@ -65,7 +65,7 @@ test.serial.cb('Storage.clear', (t) => {
   const { document, storage } = t.context;
 
   expectMutations(document, (mutations) => {
-    t.deepEqual(mutations, [TransferrableMutationType.STORAGE, GetOrSet.SET, StorageLocation.Local, 0, 0]);
+    t.deepEqual(mutations, [TransferrableMutationType.STORAGE, GetOrSet.SET, StorageLocation.Local, DELETION_INDEX, DELETION_INDEX]);
     t.end();
   });
 

src/transfer/Messages.ts

@@ -107,3 +107,7 @@ export const enum ResolveOrReject {
   RESOLVE = 1,
   REJECT = 2,
 }
+
+// Mutations are are modeled as Uint16Array.
+// Deletion is specified as a -1, which when placed into the array becomes 2^16-1.
+export const DELETION_INDEX = 2 ** 16 - 1;

src/worker-thread/Storage.ts

@@ -1,5 +1,5 @@
 import { Document } from './dom/Document';
-import { GetOrSet } from '../transfer/Messages';
+import { DELETION_INDEX, GetOrSet } from '../transfer/Messages';
 import { StorageLocation } from '../transfer/TransferrableStorage';
 import { TransferrableMutationType } from '../transfer/TransferrableMutation';
 import { store } from './strings';
@@ -58,13 +58,7 @@ export function createStorage(document: Document | DocumentStub, location: Stora
     value(key: string): void {
       delete this[key];
 
-      transfer(document, [
-        TransferrableMutationType.STORAGE,
-        GetOrSet.SET,
-        location,
-        store(key),
-        0, // value == 0 represents deletion.
-      ]);
+      transfer(document, [TransferrableMutationType.STORAGE, GetOrSet.SET, location, store(key), DELETION_INDEX]);
     },
   });
   define(storage, 'clear', {
@@ -73,13 +67,7 @@ export function createStorage(document: Document | DocumentStub, location: Stora
         delete this[key];
       });
 
-      transfer(document, [
-        TransferrableMutationType.STORAGE,
-        GetOrSet.SET,
-        location,
-        0, // key == 0 represents all keys.
-        0, // value == 0 represents deletion.
-      ]);
+      transfer(document, [TransferrableMutationType.STORAGE, GetOrSet.SET, location, DELETION_INDEX, DELETION_INDEX]);
     },
   });
   return storage as Storage;