Storage: fix clear and removeItem

src/main-thread/commands/storage.ts

@@ -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 > 0 ? strings.get(keyIndex - 1) : '';
+        const value = valueIndex > 0 ? strings.get(valueIndex - 1) : null;
 
         if (operation === GetOrSet.GET) {
           get(location, key);
@@ -82,8 +82,8 @@ export const StorageProcessor: CommandExecutorInterface = (strings, nodeContext,
       const keyIndex = mutations[startPosition + StorageMutationIndex.Key];
       const valueIndex = mutations[startPosition + StorageMutationIndex.Value];
 
-      const key = keyIndex >= 0 ? strings.get(keyIndex) : null;
-      const value = valueIndex >= 0 ? strings.get(valueIndex) : null;
+      const key = keyIndex > 0 ? strings.get(keyIndex - 1) : null;
+      const value = valueIndex > 0 ? strings.get(valueIndex - 1) : null;
 
       return {
         type: 'STORAGE',

src/main-thread/strings.ts

@@ -24,10 +24,11 @@ export class StringContext {
   /**
    * Stores a string in mapping and returns the index of the location.
    * @param value string to store
-   * @return location in map
+   * @return {number}
    */
-  store(value: string): void {
+  store(value: string): number {
     this.strings.push(value);
+    return this.strings.length - 1;
   }
 
   /**

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,
   };
 }
 
@@ -48,7 +56,7 @@ test('StorageProcessor sends storage value event to worker', async (t) => {
   const mutation: number[] = [];
   mutation[StorageMutationIndex.Operation] = GetOrSet.GET;
   mutation[StorageMutationIndex.Location] = StorageLocation.AmpState;
-  mutation[StorageMutationIndex.Key] = 0;
+  mutation[StorageMutationIndex.Key] = 1;
   const mutations = new Uint16Array(mutation);
 
   processor.execute(mutations, 0, true);
@@ -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] = 2;
+  mutation[StorageMutationIndex.Value] = 0;
+  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/main-thread/deserializeTransferrableObject.test.ts

@@ -55,7 +55,7 @@ test('Deserializes float arguments', (t) => {
 test('Deserializes string arguments', (t) => {
   const { stringContext, nodeContext } = t.context;
 
-  const serializedArgs = [TransferrableObjectType.String, storeString(stringContext, 'textArg')];
+  const serializedArgs = [TransferrableObjectType.String, stringContext.store('textArg')];
   const buffer = new Uint16Array(serializedArgs);
   const { args: deserializedArgs } = deserializeTransferrableObject(buffer, 0, 1, stringContext, nodeContext);
 
@@ -104,7 +104,7 @@ test('Deserializes varying types', (t) => {
 
   // argument 2: String
   const stringArg = 'textArg';
-  const stringId = storeString(stringContext, stringArg);
+  const stringId = stringContext.store(stringArg);
 
   // argument 3: Object
   const objectId = 7;
@@ -155,13 +155,6 @@ test('Returns the correct end offset', (t) => {
   t.is(buffer[endOffset], 32);
 });
 
-// main-thread's strings API does not return an ID when storing a string
-// so for convenience:
-function storeString(stringContext: StringContext, text: string, currentIndex = -1) {
-  stringContext.store(text);
-  return ++currentIndex;
-}
-
 /**
  * Used to compare float value similarity in tests.
  * @param expected

src/test/main-thread/object-creation.test.ts

@@ -95,7 +95,7 @@ test('Object creation with mutation at a zero offset', (t) => {
   objectCreationProcessor.execute(
     new Uint16Array([
       TransferrableMutationType.OBJECT_CREATION,
-      storeString(stringContext, methodName),
+      stringContext.store(methodName),
       objectId,
       4, // arg count
       ...serializedTarget,
@@ -141,7 +141,7 @@ test('Object creation with mutation at non-zero offset', (t) => {
 
   const mutation = [
     TransferrableMutationType.OBJECT_CREATION,
-    storeString(stringContext, methodName),
+    stringContext.store(methodName),
     objectId,
     4, // arg count
     ...serializedTarget,
@@ -180,7 +180,7 @@ test('Returns correct end offset', (t) => {
 
   const mutation = [
     TransferrableMutationType.OBJECT_CREATION,
-    storeString(stringContext, methodName),
+    stringContext.store(methodName),
     1, // object ID (not important for this test)
     4, // arg count
     TransferrableObjectType.CanvasRenderingContext2D,
@@ -196,8 +196,3 @@ test('Returns correct end offset', (t) => {
 
   t.is(mutationsArray[endOffset], 32);
 });
-
-function storeString(stringContext: StringContext, text: string, currentIndex = -1) {
-  stringContext.store(text);
-  return ++currentIndex;
-}

src/test/main-thread/object-mutation.test.ts

@@ -242,7 +242,7 @@ test('Mutation starts at a non-zero offset', (t) => {
 
   const mutation = [
     TransferrableMutationType.OBJECT_MUTATION,
-    storeString(stringContext, methodName),
+    stringContext.store(methodName),
     4, // arg count
     TransferrableObjectType.CanvasRenderingContext2D,
     canvasElement._index_,
@@ -280,7 +280,7 @@ test('Returns correct end offset', (t) => {
 
   const mutation = [
     TransferrableMutationType.OBJECT_MUTATION,
-    storeString(stringContext, methodName),
+    stringContext.store(methodName),
     4, // arg count
     TransferrableObjectType.CanvasRenderingContext2D,
     canvasElement._index_,
@@ -304,24 +304,10 @@ function executeCall(
   stringContext: StringContext,
   argCount: number,
   serializedArgs: number[],
-  stringsIndex?: number,
 ) {
   return mutationProcessor.execute(
-    new Uint16Array([
-      TransferrableMutationType.OBJECT_MUTATION,
-      storeString(stringContext, fnName, stringsIndex),
-      argCount,
-      ...serializedTargetObject,
-      ...serializedArgs,
-    ]),
+    new Uint16Array([TransferrableMutationType.OBJECT_MUTATION, stringContext.store(fnName), argCount, ...serializedTargetObject, ...serializedArgs]),
     0,
     /* allow */ true,
   );
 }
-
-// main-thread's strings API does not return an ID when storing a string
-// so for convenience:
-function storeString(stringContext: StringContext, text: string, currentIndex = -1) {
-  stringContext.store(text);
-  return ++currentIndex;
-}

src/test/main-thread/string-properties.test.ts

@@ -67,8 +67,8 @@ test.beforeEach((t) => {
 test('setting property to a new string', (t) => {
   const { stringContext, propertyProcessor, inputElement } = t.context;
   const newValue = 'new value';
-  const storedValueKey = storeString(stringContext, 'value');
-  const storedNewValue = storeString(stringContext, newValue, storedValueKey);
+  const storedValueKey = stringContext.store('value');
+  const storedNewValue = stringContext.store(newValue);
 
   propertyProcessor.execute(
     new Uint16Array([TransferrableMutationType.PROPERTIES, inputElement._index_, storedValueKey, NumericBoolean.FALSE, storedNewValue]),
@@ -82,9 +82,9 @@ test('setting property back to an empty string', (t) => {
   const { stringContext, propertyProcessor, inputElement } = t.context;
   const firstUpdateValue = 'new value';
   const secondUpdateValue = '';
-  const storedValueKey = storeString(stringContext, 'value');
-  const storedFirstUpdateValue = storeString(stringContext, firstUpdateValue, storedValueKey);
-  const storedSecondUpdateValue = storeString(stringContext, secondUpdateValue, storedFirstUpdateValue);
+  const storedValueKey = stringContext.store('value');
+  const storedFirstUpdateValue = stringContext.store(firstUpdateValue);
+  const storedSecondUpdateValue = stringContext.store(secondUpdateValue);
 
   propertyProcessor.execute(
     new Uint16Array([TransferrableMutationType.PROPERTIES, inputElement._index_, storedValueKey, NumericBoolean.FALSE, storedFirstUpdateValue]),
@@ -100,8 +100,3 @@ test('setting property back to an empty string', (t) => {
   );
   t.is(inputElement.value, secondUpdateValue);
 });
-
-function storeString(stringContext: StringContext, text: string, currentIndex = -1) {
-  stringContext.store(text);
-  return ++currentIndex;
-}

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

@@ -43,7 +43,13 @@ test.serial.cb('Storage.setItem', (t) => {
   const { document, storage } = t.context;
 
   expectMutations(document, (mutations) => {
-    t.deepEqual(mutations, [TransferrableMutationType.STORAGE, GetOrSet.SET, StorageLocation.Local, getForTesting('foo'), getForTesting('bar')]);
+    t.deepEqual(mutations, [
+      TransferrableMutationType.STORAGE,
+      GetOrSet.SET,
+      StorageLocation.Local,
+      getForTesting('foo')! + 1,
+      getForTesting('bar')! + 1,
+    ]);
     t.end();
   });
 
@@ -54,7 +60,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')! + 1, 0]);
     t.end();
   });
 

src/worker-thread/Storage.ts

@@ -51,7 +51,7 @@ export function createStorage(document: Document | DocumentStub, location: Stora
       const stringValue = String(value);
       this[key] = stringValue;
 
-      transfer(document, [TransferrableMutationType.STORAGE, GetOrSet.SET, location, store(key), store(stringValue)]);
+      transfer(document, [TransferrableMutationType.STORAGE, GetOrSet.SET, location, store(key) + 1, store(stringValue) + 1]);
     },
   });
   define(storage, 'removeItem', {
@@ -62,7 +62,7 @@ export function createStorage(document: Document | DocumentStub, location: Stora
         TransferrableMutationType.STORAGE,
         GetOrSet.SET,
         location,
-        store(key),
+        store(key) + 1,
         0, // value == 0 represents deletion.
       ]);
     },