fix(SwingSet): VOM retains Remotables used in virtualized data

Userspace might store a locally-created Remotable (e.g. `Far('iface',
{methods..})` in the `state` of a virtual object, or somewhere in the value
of a vref-keyed `makeWeakStore()` entry. In either case, the data is
virtualized: serialized and written to disk. This serialized form obviously
cannot keep the Remotable JS `Object` alive directly, however userspace
reasonably expects to get the Remotable back if it reads the `state` or does
a `.get` on the store.

To ensure the Remotable can be looked up from the serialized vref, the
virtual object manager must retain a strong reference to the original
Remotable for as long as its vref is present anywhere in the virtualized
data.

For now, we simply add the Remotable to a strong Set the first time it is
added, and we never remove it. This is safe, but conservative.

To do better (and eventually release the Remotable), we'll need to add some
form of refcount to each vref. When the refcount of the Remotable's vref
drops to zero, the VOM can drop its strong reference to the Remotable.

closes #3132
refs #3106

packages/SwingSet/src/kernel/virtualObjectManager.js

@@ -196,6 +196,30 @@ export function makeVirtualObjectManager(
    */
   const kindTable = new Map();
 
+  /**
+   * Set of all Remotables which are reachable by our virtualized data, e.g.
+   * `makeWeakStore().set(key, remotable)` or `virtualObject.state.foo =
+   * remotable`. The serialization process stores the Remotable's vref to
+   * disk, but doesn't actually retain the Remotable. To correctly
+   * unserialize that offline data later, we must ensure the Remotable
+   * remains alive. This Set keeps a strong reference to the Remotable. We
+   * currently never remove anything from the set, but eventually refcounts
+   * will let us discover when it is no longer reachable, and we'll drop the
+   * strong reference.
+   */
+  /** @type {Set<Object>} of Remotables */
+  const reachableRemotables = new Set();
+  function addReachableRemotable(vref) {
+    const { type, virtual, allocatedByVat } = parseVatSlot(vref);
+    if (type === 'object' && !virtual && allocatedByVat) {
+      // exported non-virtual object: Remotable
+      const remotable = getValForSlot(vref);
+      assert(remotable, X`no remotable for ${vref}`);
+      // console.log(`adding ${vref} to reachableRemotables`);
+      reachableRemotables.add(remotable);
+    }
+  }
+
   /**
    * Produce a representative given a virtual object ID.  Used for
    * deserializing.
@@ -280,7 +304,9 @@ export function makeVirtualObjectManager(
             !syscall.vatstoreGet(vkey),
             X`${q(keyName)} already registered: ${key}`,
           );
-          syscall.vatstoreSet(vkey, JSON.stringify(m.serialize(value)));
+          const data = m.serialize(value);
+          data.slots.map(addReachableRemotable);
+          syscall.vatstoreSet(vkey, JSON.stringify(data));
         } else {
           assertKeyDoesNotExist(key);
           backingMap.set(key, value);
@@ -301,7 +327,9 @@ export function makeVirtualObjectManager(
         const vkey = virtualObjectKey(key);
         if (vkey) {
           assert(syscall.vatstoreGet(vkey), X`${q(keyName)} not found: ${key}`);
-          syscall.vatstoreSet(vkey, JSON.stringify(m.serialize(harden(value))));
+          const data = m.serialize(harden(value));
+          data.slots.map(addReachableRemotable);
+          syscall.vatstoreSet(vkey, JSON.stringify(data));
         } else {
           assertKeyExists(key);
           backingMap.set(key, value);
@@ -538,6 +566,7 @@ export function makeVirtualObjectManager(
             },
             set: value => {
               const serializedValue = m.serialize(value);
+              serializedValue.slots.map(addReachableRemotable);
               ensureState();
               innerSelf.rawData[prop] = serializedValue;
               innerSelf.dirty = true;
@@ -595,7 +624,9 @@ export function makeVirtualObjectManager(
       const rawData = {};
       for (const prop of Object.getOwnPropertyNames(initialData)) {
         try {
-          rawData[prop] = m.serialize(initialData[prop]);
+          const data = m.serialize(initialData[prop]);
+          data.slots.map(addReachableRemotable);
+          rawData[prop] = data;
         } catch (e) {
           console.error(`state property ${String(prop)} is not serializable`);
           throw e;

packages/SwingSet/test/virtualObjects/test-retain-remotable.js

@@ -0,0 +1,125 @@
+/* global WeakRef */
+import { test } from '../../tools/prepare-test-env-ava';
+
+// eslint-disable-next-line import/order
+import { Far } from '@agoric/marshal';
+
+import { gcAndFinalize } from '../../src/gc';
+import { makeFakeVirtualObjectManager } from '../../tools/fakeVirtualObjectManager';
+
+// empty object, used as makeWeakStore() key
+function makeKeyInstance(_state) {
+  return {
+    init() {},
+    self: Far('key'),
+  };
+}
+
+function makeHolderInstance(state) {
+  return {
+    init(held) {
+      state.held = held;
+    },
+    self: Far('holder', {
+      setHeld(held) {
+        state.held = held;
+      },
+      getHeld() {
+        return state.held;
+      },
+    }),
+  };
+}
+
+function makeHeld() {
+  const held = Far('held');
+  const wr = new WeakRef(held);
+  const ws = new WeakSet(); // note: real WeakSet, not vref-aware
+  ws.add(held);
+  function isHeld(obj) {
+    return ws.has(obj);
+  }
+  return { held, wr, isHeld };
+}
+
+function prepareEphemeral(vom) {
+  const ephemeral = Far('ephemeral');
+  vom.registerEntry('o+12345', ephemeral);
+  const wr = new WeakRef(ephemeral);
+  return { wr };
+}
+
+function stashRemotableOne(weakStore, key1) {
+  const { held, wr, isHeld } = makeHeld();
+  weakStore.init(key1, held);
+  return { wr, isHeld };
+}
+
+function stashRemotableTwo(weakStore, key1) {
+  const { held, wr, isHeld } = makeHeld();
+  weakStore.init(key1, 'initial');
+  weakStore.set(key1, held);
+  return { wr, isHeld };
+}
+
+function stashRemotableThree(holderMaker) {
+  const { held, wr, isHeld } = makeHeld();
+  const holder = holderMaker(held);
+  return { wr, isHeld, holder };
+}
+
+function stashRemotableFour(holderMaker) {
+  const { held, wr, isHeld } = makeHeld();
+  const holder = holderMaker('initial');
+  holder.setHeld(held);
+  return { wr, isHeld, holder };
+}
+
+test('remotables retained by virtualized data', async t => {
+  const vomOptions = { cacheSize: 3, weak: true };
+  const vom = makeFakeVirtualObjectManager(vomOptions);
+  const { makeWeakStore, makeKind } = vom;
+  const weakStore = makeWeakStore();
+  const keyMaker = makeKind(makeKeyInstance);
+  const holderMaker = makeKind(makeHolderInstance);
+
+  // create a Remotable and assign it a vref, then drop it, to make sure the
+  // fake VOM isn't holding onto a strong reference, which would cause a
+  // false positive in the subsequent test
+  const stash0 = prepareEphemeral(vom);
+  await gcAndFinalize();
+  t.falsy(stash0.wr.deref(), `caution: fake VOM didn't release Remotable`);
+
+  // stash a Remotable in the value of a weakStore
+  const key1 = keyMaker();
+  const stash1 = stashRemotableOne(weakStore, key1);
+  await gcAndFinalize();
+  // The weakStore virtualizes the values held under keys which are
+  // Representatives or Presences, so the value is not holding a strong
+  // reference to the Remotable. The VOM is supposed to keep it alive, via
+  // reachableRemotables.
+  t.truthy(stash1.wr.deref());
+  t.truthy(stash1.isHeld(weakStore.get(key1)));
+
+  // do the same, but exercise weakStore.set instead of .init
+  const key2 = keyMaker();
+  const stash2 = stashRemotableTwo(weakStore, key2);
+  await gcAndFinalize();
+  t.truthy(stash2.wr.deref());
+  t.truthy(stash2.isHeld(weakStore.get(key2)));
+
+  // now stash a Remotable in the state of a virtual object during init()
+  const stash3 = stashRemotableThree(holderMaker);
+  await gcAndFinalize();
+  // Each state property is virtualized upon write (via the generated
+  // setters). So again we rely on the VOM to keep the Remotable alive in
+  // case someone retreives it again.
+  t.truthy(stash3.wr.deref());
+  t.truthy(stash3.isHeld(stash3.holder.getHeld()));
+
+  // same, but stash after init()
+  const stash4 = stashRemotableFour(holderMaker);
+  await gcAndFinalize();
+  t.truthy(stash4.wr.deref());
+  t.truthy(stash4.isHeld(stash4.holder.getHeld()));
+});