From 4c341d92aacb013e55fa38806403835f684a8518 Mon Sep 17 00:00:00 2001
From: Brian Warner <warner@lothar.com>
Date: Thu, 10 Jun 2021 18:28:43 -0700
Subject: [PATCH] fix(swingset): implement dispatch.retireExports for
 Remotables

This updates liveslots to implement kernel-delivered
`dispatch.retireExports`, but only when the object being retired is a
Remotable (virtual objects are left alone for now).

Retiring an exported Remotable means we remove it from the `slotToVal` and
`valToSlot` tables, and unregister it from the `droppedRegistry`. At this
point, vat code might still hold a strong reference to the Remoteable, but
liveslots is unaware of it. If the vat re-exports the object, it will get a
new vref.
---
 packages/SwingSet/src/kernel/liveSlots.js | 45 ++++++++++++++-
 packages/SwingSet/test/test-liveslots.js  | 67 +++++++++++++++++++++++
 2 files changed, 109 insertions(+), 3 deletions(-)

diff --git a/packages/SwingSet/src/kernel/liveSlots.js b/packages/SwingSet/src/kernel/liveSlots.js
index 152ce4dfb946..f7880669ec19 100644
--- a/packages/SwingSet/src/kernel/liveSlots.js
+++ b/packages/SwingSet/src/kernel/liveSlots.js
@@ -847,11 +847,50 @@ function build(
     }
   }
 
+  function retireOneExport(vref) {
+    insistVatType('object', vref);
+    const { virtual, allocatedByVat, type } = parseVatSlot(vref);
+    assert(allocatedByVat);
+    assert.equal(type, 'object');
+    if (virtual) {
+      // virtual object: ignore for now, but TODO we must still not make
+      // syscall.retireExport for vrefs that were already retired by the
+      // kernel
+      // console.log(`-- liveslots ignoring retireExports ${vref}`);
+    } else {
+      // Remotable
+      // console.log(`-- liveslots acting on retireExports ${vref}`);
+      const wr = slotToVal.get(vref);
+      if (wr) {
+        const val = wr.deref();
+        if (val) {
+          // it's fine to still have a value, that just means the kernel
+          // (and other vats) have completely forgotten about this, but we
+          // still know about it
+
+          if (exportedRemotables.has(val)) {
+            // however this is weird: we still think the Remotable is
+            // reachable, otherwise we would have removed it from
+            // exportedRemotables. The kernel was supposed to send
+            // dispatch.dropExports first.
+            console.log(`err: kernel retired undropped ${vref}`);
+            // TODO: find a way to make this more severe, it's cause for
+            // panicing the kernel, except that vats don't have that
+            // authority. It's *not* cause for terminating the vat, since
+            // it wasn't necessarily our fault.
+            return;
+          }
+          valToSlot.delete(val);
+          droppedRegistry.unregister(val);
+        }
+        slotToVal.delete(vref);
+      }
+    }
+  }
+
   function retireExports(vrefs) {
     assert(Array.isArray(vrefs));
-    vrefs.map(vref => insistVatType('object', vref));
-    vrefs.map(vref => assert(parseVatSlot(vref).allocatedByVat));
-    console.log(`-- liveslots ignoring retireExports`);
+    vrefs.forEach(retireOneExport);
   }
 
   function retireImports(vrefs) {
diff --git a/packages/SwingSet/test/test-liveslots.js b/packages/SwingSet/test/test-liveslots.js
index d2a835ef8978..9c466dfdf11a 100644
--- a/packages/SwingSet/test/test-liveslots.js
+++ b/packages/SwingSet/test/test-liveslots.js
@@ -1103,3 +1103,70 @@ test('GC dispatch.dropExports', async t => {
   });
   t.deepEqual(log, []);
 });
+
+test('GC dispatch.retireExports inhibits syscall.retireExports', async t => {
+  const { log, syscall } = buildSyscall();
+  let wr;
+  function build(_vatPowers) {
+    let ex1;
+    const root = Far('root', {
+      hold() {
+        ex1 = Far('export', {});
+        wr = new WeakRef(ex1);
+        return ex1;
+      },
+      two() {},
+      drop() {
+        // eslint-disable-next-line no-unused-vars
+        ex1 = undefined; // drop the last userspace strongref
+      },
+    });
+    return root;
+  }
+  const dispatch = makeDispatch(syscall, build, 'vatA', true);
+  t.deepEqual(log, []);
+  const rootA = 'o+0';
+
+  // rp1 = root~.hold()
+  // ex1 = await rp1
+  const rp1 = 'p-1';
+  await dispatch(makeMessage(rootA, 'hold', capargs([]), rp1));
+  const l1 = log.shift();
+  const ex1 = l1.resolutions[0][2].slots[0];
+  t.deepEqual(l1, {
+    type: 'resolve',
+    resolutions: [[rp1, false, capdataOneSlot(ex1)]],
+  });
+  t.deepEqual(log, []);
+
+  // the exported Remotable should be held in place by exportedRemotables
+  // until we tell the vat we don't need it any more
+  t.truthy(wr.deref());
+
+  // an intermediate message will trigger GC, but the presence is still held
+  await dispatch(makeMessage(rootA, 'two', capargs([])));
+  t.truthy(wr.deref());
+
+  // now tell the vat we don't need a strong reference to that export.
+  await dispatch(makeDropExports(ex1));
+
+  // that removes the liveslots strongref, but the vat's remains in place
+  t.truthy(wr.deref());
+
+  // now the kernel tells the vat we can't even recognize the export
+  await dispatch(makeRetireExports(ex1));
+
+  // that ought to delete the table entry, but doesn't affect the vat
+  // strongref
+  t.truthy(wr.deref());
+
+  // now tell the vat to drop its strongref
+  await dispatch(makeMessage(rootA, 'drop', capargs([])));
+
+  // which should let the export be collected
+  t.falsy(wr.deref());
+
+  // the vat should *not* emit `syscall.retireExport`, because it already
+  // received a dispatch.retireExport
+  t.deepEqual(log, []);
+});