From be4a5d91f38dacd8dd7406f7e3c4fcf55af5dbb7 Mon Sep 17 00:00:00 2001 From: Michael FIG Date: Tue, 21 Mar 2023 10:44:06 -0600 Subject: [PATCH] fix(ses): avoid holding deep stacks strongly --- packages/ses/src/error/note-log-args.js | 142 +++++++++++++++--------- 1 file changed, 89 insertions(+), 53 deletions(-) diff --git a/packages/ses/src/error/note-log-args.js b/packages/ses/src/error/note-log-args.js index ae715407e8..6f059ce611 100644 --- a/packages/ses/src/error/note-log-args.js +++ b/packages/ses/src/error/note-log-args.js @@ -1,3 +1,4 @@ +// @ts-check /* eslint-disable @endo/no-polymorphic-call */ /* eslint-disable no-restricted-globals */ @@ -7,14 +8,14 @@ const { freeze } = Object; const { isSafeInteger } = Number; /** + * @template Data * @typedef {object} DoublyLinkedCell * A cell of a doubly-linked ring, i.e., a doubly-linked circular list. * DoublyLinkedCells are not frozen, and so should be closely encapsulated by * any abstraction that uses them. - * @property {DoublyLinkedCell} next - * @property {DoublyLinkedCell} prev - * @property {object} key - * @property {any} value; + * @property {DoublyLinkedCell} next + * @property {DoublyLinkedCell} prev + * @property {Data} data */ /** @@ -22,21 +23,20 @@ const { isSafeInteger } = Number; * * To make the head sigil of a new initially-empty doubly-linked ring. * * To make a non-sigil cell to be `spliceAfter`ed. * - * @param {object} key - * @param {any} value - * @returns {DoublyLinkedCell} + * @template Data + * @param {Data} data + * @returns {DoublyLinkedCell} */ -const makeSelfCell = (key, value) => { - /** @type {Partial} */ +const makeSelfCell = data => { + /** @type {Partial>} */ const incompleteCell = { next: undefined, prev: undefined, - key, - value, + data, }; - incompleteCell.next = incompleteCell; - incompleteCell.prev = incompleteCell; - const selfCell = /** @type {DoublyLinkedCell} */ (incompleteCell); + const selfCell = /** @type {DoublyLinkedCell} */ (incompleteCell); + selfCell.next = selfCell; + selfCell.prev = selfCell; // Not frozen! return selfCell; }; @@ -46,8 +46,9 @@ const makeSelfCell = (key, value) => { * `prev` could be the head sigil, or it could be some other non-sigil * cell within a ring. * - * @param {DoublyLinkedCell} prev - * @param {DoublyLinkedCell} selfCell + * @template Data + * @param {DoublyLinkedCell} prev + * @param {DoublyLinkedCell} selfCell */ const spliceAfter = (prev, selfCell) => { if (prev === selfCell) { @@ -69,14 +70,12 @@ const spliceAfter = (prev, selfCell) => { }; /** - * @param {DoublyLinkedCell} cell - * Must be a non-sigil part of a ring, and therefore non-self-linked + * @template Data + * @param {DoublyLinkedCell} cell + * No-op if the cell is self-linked. */ const spliceOut = cell => { const { prev, next } = cell; - if (prev === cell || next === cell) { - throw TypeError('Expected non-self-linked cell'); - } prev.next = next; next.prev = prev; cell.prev = cell; @@ -87,28 +86,34 @@ const spliceOut = cell => { * The LRUCacheMap is used within the implementation of `assert` and so * at a layer below SES or harden. Thus, we give it a `WeakMap`-like interface * rather than a `WeakMapStore`-like interface. To work before `lockdown`, - * the implementation must use `freeze` manually, but still exhausively. - * - * It does not hold onto anything weakly. The only sense in which it is - * WeakMap-like is that it does not support enumeration. + * the implementation must use `freeze` manually, but still exhaustively. * - * TODO: Make parameterized `Key` and `Value` template types + * It implements the WeakMap interface, and holds its keys weakly. Cached + * values are only held while the key is held by the user and the key/value + * bookkeeping cell has not been pushed off the end of the cache by `budget` + * number of more recently referenced cells. If the key is dropped by the user, + * the value will no longer be held by the cache, but the bookkeeping cell + * itself will stay in memory. * + * @template {{}} K + * @template {unknown} V * @param {number} budget - * @returns {WeakMap} + * @returns {WeakMap} */ export const makeLRUCacheMap = budget => { if (!isSafeInteger(budget) || budget < 0) { throw new TypeError('budget must be a safe non-negative integer number'); } - /** @type {Map} */ - const map = new Map(); + /** @typedef {DoublyLinkedCell | undefined>} LRUCacheCell */ + /** @type {WeakMap} */ + const keyToCell = new WeakMap(); let size = 0; // `size` must remain <= `budget` - // As a sigil, `head` uniquely is not in the `map`. - const head = makeSelfCell(undefined, undefined); + // As a sigil, `head` uniquely is not in the `keyToCell` map. + /** @type {LRUCacheCell} */ + const head = makeSelfCell(undefined); const touchCell = key => { - const cell = map.get(key); + const cell = keyToCell.get(key); if (cell === undefined) { return undefined; } @@ -118,49 +123,79 @@ export const makeLRUCacheMap = budget => { return cell; }; + /** + * @param {K} key + */ const has = key => touchCell(key) !== undefined; freeze(has); + /** + * @param {K} key + */ // TODO Change to the following line, once our tools don't choke on `?.`. // See https://github.com/endojs/endo/issues/1514 - // const get = key => touchCell(key)?.value; + // const get = key => touchCell(key)?.data?.get(key); const get = key => { const cell = touchCell(key); - return cell && cell.value; + return cell && cell.data && cell.data.get(key); }; freeze(get); + /** + * @param {K} key + * @param {V} value + */ const set = (key, value) => { - if (budget >= 1) { - let cell = touchCell(key); - if (cell !== undefined) { - cell.value = value; - } else { - if (size >= budget) { - const condemned = head.prev; - spliceOut(condemned); // Drop least recently used - map.delete(condemned.key); - size -= 1; - } - size += 1; - cell = makeSelfCell(key, value); - map.set(key, cell); - spliceAfter(head, cell); // start most recently used + if (budget < 1) { + // eslint-disable-next-line no-use-before-define + return lruCacheMap; // Implements WeakMap.set + } + + let cell = touchCell(key); + if (cell === undefined) { + cell = makeSelfCell(undefined); + spliceAfter(head, cell); // start most recently used + } + if (!cell.data) { + // Either a fresh cell or a reused condemned cell. + size += 1; + // Add its data. + cell.data = new WeakMap(); + // Advertise the cell for this key. + keyToCell.set(key, cell); + while (size > budget) { + const condemned = head.prev; + spliceOut(condemned); // Drop least recently used + condemned.data = undefined; + size -= 1; } } + + // Update the data. + cell.data.set(key, value); + // eslint-disable-next-line no-use-before-define - return lruCacheMap; // Needed to be WeakMap-like + return lruCacheMap; // Implements WeakMap.set }; freeze(set); // "delete" is a keyword. + /** + * @param {K} key + */ const deleteIt = key => { - const cell = map.get(key); + const cell = keyToCell.get(key); if (cell === undefined) { return false; } spliceOut(cell); - map.delete(key); + keyToCell.delete(key); + if (cell.data === undefined) { + // Already condemned. + return false; + } + + cell.data = undefined; size -= 1; return true; }; @@ -171,6 +206,7 @@ export const makeLRUCacheMap = budget => { get, set, delete: deleteIt, + [Symbol.toStringTag]: 'LRUCacheMap', }); return lruCacheMap; }; @@ -210,7 +246,7 @@ export const makeNoteLogArgsArrayKit = (budget = defaultLogArgsBudget) => { /** * @param {Error} error - * @returns {LogArgs[]} + * @returns {LogArgs[] | undefined} */ const takeLogArgsArray = error => { const result = noteLogArgsArrayMap.get(error);