Storage: fix clear and removeItem (#1136)

* Storage: fix clear and removeItem

The mutations array usually contains an index into StringContext for
value of key/value in the operation. Historically, "delete" was
modeled as 0th index into the array. Unfortunately, this didn't make
sense if the first SET op was genuinely referring to the 0th string
value (See #1035).

While this fixed the first get/set operation, it also meant that deletes
were no longer possible! This PR now models the delete operation as a -1
in the unsigned mutation array, corresponding to 2^16-1.

* dont rely on overflow

* switch to special casing 0-value

* clean up test

* switch to isolating change within LocalStorage processor

* jridgewell found fix

Author: samouri

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.
       ]);
     },