Skip to content

Commit afcb306

Browse files
fix: WeakRefObjectMap can memory leak its keys, never cleaning them up (#40467)
## **Description** When updating `@types/node`, types used by `WeakRefObjectMap` became incompatible. When I went to refactor them I noticed several bugs in the implementation. I fixed: * `size` now prunes dead entries before reporting, so stale GC’d entries are no longer counted * `has` now checks liveness directly and removes stale entries instead of depending on `get` side effects * `keys()` now filters out stale entries (previously it returned raw map keys, including dead ones!) * `forEach`'s `thisArg` behavior was fixed. it previously wouldn't use the thisArg if the value of `thisArg` was `falsy` * Non-object property assignment now throws `TypeError`, which is a more correct error type, IMO. From a perf standpoint, its probably slower, since it does more work, but I also tried to make up for it by making it also do _less_ work: * internal storage moved from "object-of-weakrefs" to compact aligned arrays (keys[] + refs[]) for lower overhead and faster iteration (usually) * `set`/`get`/`has` hot paths now use indexed loops instead of `for...in`+ `hasOwnProperty` checks, so that's nice. * has no longer reconstructs full values and just checks weakref liveness * delete is now direct `Map.delete` instead of the more manual deref way * `entries()`/`values()` doesn't do eager array-building, and is much lazier now, it avoids intermediate objects now! * `forEach` now iterates internal entries directly and dereferences once, avoiding `map.forEach` + `get` calls I also added more tests to ensure proper behavior and coverage, and even enabled the garbage collection test to make sure this thing _actually_ works. I tried adding a `FinalizationRegistry` in an attempt to proactively clean up keys who's corresponding values have all been deleted, but I couldn't ever get it to actually run inside jest, so I deleted it. [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/40467?quickstart=1) ## **Changelog** <!-- If this PR is not End-User-Facing and should not show up in the CHANGELOG, you can choose to either: 1. Write `CHANGELOG entry: null` 2. Label with `no-changelog` If this PR is End-User-Facing, please write a short User-Facing description in the past tense like: `CHANGELOG entry: Added a new tab for users to see their NFTs` `CHANGELOG entry: Fixed a bug that was causing some NFTs to flicker` (This helps the Release Engineer do their job more quickly and accurately) --> CHANGELOG entry: null <!-- ## **Related issues** Fixes: ## **Manual testing steps** 1. Go to this page... 2. 3. ## **Screenshots/Recordings** ### **Before** ### **After** --> ## **Pre-merge author checklist** - [x] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [x] I've completed the PR template to the best of my ability - [x] I’ve included tests if applicable - [x] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [x] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **Pre-merge reviewer checklist** - [ ] I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed). - [ ] I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes core `WeakRefObjectMap` semantics around liveness/pruning (`size`/iterators/`delete`/`forEach`) and refactors its internal storage, which could affect consumers and performance. Unit test runner now uses `--expose-gc`, which may impact CI/runtime assumptions. > > **Overview** > Fixes `WeakRefObjectMap` to **eagerly prune stale (GC’d) entries** across `has`, `delete`, `size`, iterators (`entries`/`keys`/`values`), and `forEach`, preventing dead keys from lingering and aligning behavior with standard `Map` semantics. > > Refactors internal storage from an object-of-`WeakRef`s to a compact `{ keys[], refs[] }` entry and makes iteration **lazy** (no intermediate arrays); `set` now throws `TypeError` for non-object properties and `forEach` now correctly applies `thisArg` even when falsy. > > Expands `WeakRefObjectMap` unit coverage with explicit stale-entry injection and GC-based pruning assertions, and updates `test:unit` scripts to run Jest via `node --expose-gc` so GC tests can execute. > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 789c9cf. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
1 parent b491057 commit afcb306

File tree

3 files changed

+431
-104
lines changed

3 files changed

+431
-104
lines changed

app/scripts/lib/WeakRefObjectMap.test.ts

Lines changed: 235 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,48 @@
1+
import { beforeEach, describe, expect, it, jest } from '@jest/globals';
12
import { WeakRefObjectMap } from './WeakRefObjectMap';
23

3-
describe('WeakDomainProxyMap', () => {
4-
let map: WeakRefObjectMap<Record<string, object>>;
4+
type TestRecord = Record<string, object>;
5+
type InternalWeakEntry = {
6+
keys: (keyof TestRecord)[];
7+
refs: { deref: () => object | undefined }[];
8+
};
9+
10+
function injectInternalEntry(
11+
map: WeakRefObjectMap<TestRecord>,
12+
key: string,
13+
entry: InternalWeakEntry,
14+
) {
15+
(
16+
map as unknown as {
17+
map: Map<string, InternalWeakEntry>;
18+
}
19+
).map.set(key, entry);
20+
}
21+
22+
function hasInternalEntry(map: WeakRefObjectMap<TestRecord>, key: string) {
23+
return (
24+
map as unknown as {
25+
map: Map<string, InternalWeakEntry>;
26+
}
27+
).map.has(key);
28+
}
29+
30+
async function collectWithRetries(
31+
weakRef: WeakRef<object>,
32+
retries = 100,
33+
): Promise<boolean> {
34+
for (let i = 0; i < retries; i++) {
35+
global.gc?.();
36+
await new Promise((resolve) => setImmediate(resolve));
37+
if (weakRef.deref() === undefined) {
38+
return true;
39+
}
40+
}
41+
return false;
42+
}
43+
44+
describe('WeakRefObjectMap', () => {
45+
let map: WeakRefObjectMap<TestRecord>;
546

647
beforeEach(() => {
748
map = new WeakRefObjectMap();
@@ -52,6 +93,58 @@ describe('WeakDomainProxyMap', () => {
5293
expect(map.delete('nonExistentKey')).toBe(false);
5394
});
5495

96+
it('throws when setting non-object values', () => {
97+
const setInvalidValue = () =>
98+
map.set('bad', { objKey: null } as unknown as TestRecord);
99+
100+
expect(setInvalidValue).toThrow(TypeError);
101+
expect(setInvalidValue).toThrow(
102+
'Property objKey is not an object and cannot be weakly referenced.',
103+
);
104+
});
105+
106+
const itIfGc = global.gc ? it : it.skip;
107+
108+
itIfGc(
109+
'returns undefined and removes key when referenced value is collected',
110+
async () => {
111+
let staleTarget: object | null = {};
112+
map.set('stale', { objKey: staleTarget });
113+
const weakRef = new WeakRef(staleTarget);
114+
staleTarget = null;
115+
116+
const collected = await collectWithRetries(weakRef);
117+
if (!collected) {
118+
// Keep this test resilient in runtimes where collection is delayed.
119+
expect(map.has('stale')).toBe(true);
120+
return;
121+
}
122+
expect(map.get('stale')).toBeUndefined();
123+
expect(map.delete('stale')).toBe(false);
124+
},
125+
20_000,
126+
);
127+
128+
it('has removes stale internal entries', () => {
129+
injectInternalEntry(map, 'stale', {
130+
keys: ['objKey'],
131+
refs: [{ deref: () => undefined }],
132+
});
133+
134+
expect(map.has('stale')).toBe(false);
135+
expect(map.delete('stale')).toBe(false);
136+
});
137+
138+
it('delete returns false and removes stale internal entries', () => {
139+
injectInternalEntry(map, 'stale', {
140+
keys: ['objKey'],
141+
refs: [{ deref: () => undefined }],
142+
});
143+
144+
expect(map.delete('stale')).toBe(false);
145+
expect(hasInternalEntry(map, 'stale')).toBe(false);
146+
});
147+
55148
describe('iterators', () => {
56149
beforeEach(() => {
57150
map = new WeakRefObjectMap();
@@ -119,29 +212,146 @@ describe('WeakDomainProxyMap', () => {
119212
const iterator = map[Symbol.iterator]();
120213
expect(Array.from(iterator)).toEqual(Array.from(map.entries()));
121214
});
215+
216+
it('uses thisArg in forEach callback', () => {
217+
const thisArg = { calledWithKeys: [] as string[] };
218+
const callback = jest.fn(function (
219+
this: { calledWithKeys: string[] },
220+
_value: TestRecord,
221+
key: string,
222+
sourceMap: Map<string, TestRecord>,
223+
) {
224+
this.calledWithKeys.push(key);
225+
expect(sourceMap).toBe(map);
226+
});
227+
228+
map.forEach(callback, thisArg);
229+
230+
expect(thisArg.calledWithKeys).toEqual(['key1', 'key2']);
231+
expect(callback).toHaveBeenCalledTimes(2);
232+
});
233+
234+
it('uses falsy thisArg in forEach callback', () => {
235+
const thisArg = 0;
236+
const callback = jest.fn(function (
237+
this: number,
238+
_value: TestRecord,
239+
_key: string,
240+
sourceMap: Map<string, TestRecord>,
241+
) {
242+
expect(this).toBe(0);
243+
expect(sourceMap).toBe(map);
244+
});
245+
246+
map.forEach(callback, thisArg);
247+
248+
expect(callback).toHaveBeenCalledTimes(2);
249+
});
250+
251+
it('skips stale internal entries in entries, keys, and values', () => {
252+
const staleAwareMap = new WeakRefObjectMap<TestRecord>();
253+
const liveTarget = { detail: 'value1' };
254+
255+
injectInternalEntry(staleAwareMap, 'stale', {
256+
keys: ['objKey'],
257+
refs: [{ deref: () => undefined }],
258+
});
259+
staleAwareMap.set('live', { objKey: liveTarget });
260+
261+
expect(Array.from(staleAwareMap.entries())).toEqual([
262+
['live', { objKey: liveTarget }],
263+
]);
264+
expect(Array.from(staleAwareMap.keys())).toEqual(['live']);
265+
expect(Array.from(staleAwareMap.values())).toEqual([
266+
{ objKey: liveTarget },
267+
]);
268+
expect(staleAwareMap.delete('stale')).toBe(false);
269+
});
270+
271+
it('keys iterator continues when first internal entry is stale', () => {
272+
const staleAwareMap = new WeakRefObjectMap<TestRecord>();
273+
const liveTarget = { detail: 'value1' };
274+
275+
injectInternalEntry(staleAwareMap, 'stale', {
276+
keys: ['objKey'],
277+
refs: [{ deref: () => undefined }],
278+
});
279+
staleAwareMap.set('live', { objKey: liveTarget });
280+
281+
expect(Array.from(staleAwareMap.keys())).toEqual(['live']);
282+
expect(staleAwareMap.delete('stale')).toBe(false);
283+
});
284+
285+
it('values iterator continues when first internal entry is stale', () => {
286+
const staleAwareMap = new WeakRefObjectMap<TestRecord>();
287+
const liveTarget = { detail: 'value1' };
288+
289+
injectInternalEntry(staleAwareMap, 'stale', {
290+
keys: ['objKey'],
291+
refs: [{ deref: () => undefined }],
292+
});
293+
staleAwareMap.set('live', { objKey: liveTarget });
294+
295+
expect(Array.from(staleAwareMap.values())).toEqual([
296+
{ objKey: liveTarget },
297+
]);
298+
expect(staleAwareMap.delete('stale')).toBe(false);
299+
});
300+
301+
it('forEach skips stale internal entries and continues', () => {
302+
const staleAwareMap = new WeakRefObjectMap<TestRecord>();
303+
const liveTarget = { detail: 'value1' };
304+
305+
injectInternalEntry(staleAwareMap, 'stale', {
306+
keys: ['objKey'],
307+
refs: [{ deref: () => undefined }],
308+
});
309+
staleAwareMap.set('live', { objKey: liveTarget });
310+
311+
const callback = jest.fn();
312+
staleAwareMap.forEach(callback);
313+
314+
expect(callback).toHaveBeenCalledTimes(1);
315+
expect(callback).toHaveBeenCalledWith(
316+
{ objKey: liveTarget },
317+
'live',
318+
staleAwareMap,
319+
);
320+
expect(staleAwareMap.delete('stale')).toBe(false);
321+
});
122322
});
123323
});
124324

125-
// Commenting until we figure out how best to expose garbage collection in jest env
126-
// describe('WeakDomainProxyMap with garbage collection', () => {
127-
// it('cleans up weakly referenced objects after garbage collection', () => {
128-
// if ((global as any).gc) {
129-
// const map = new WeakDomainProxyMap();
130-
// let obj: object = { a: 1 };
131-
// map.set('key', { obj });
132-
133-
// expect(map.get('key')).toHaveProperty('obj', obj);
134-
135-
// obj = null!; // Remove the strong reference to the object
136-
137-
// (global as any).gc(); // Force garbage collection
138-
139-
// // The weakly referenced object should be gone after garbage collection.
140-
// expect(map.get('key')).toBeUndefined();
141-
// } else {
142-
// console.warn(
143-
// 'Garbage collection is not exposed. Run Node.js with the --expose-gc flag.',
144-
// );
145-
// }
146-
// });
147-
// });
325+
describe('WeakRefObjectMap with garbage collection', () => {
326+
const itIfGc = global.gc ? it : it.skip;
327+
328+
itIfGc(
329+
'prunes stale entries from has, size, and iterators after collection',
330+
async () => {
331+
const gcMap = new WeakRefObjectMap<TestRecord>();
332+
const liveTarget = {};
333+
let staleTarget: object | null = {};
334+
335+
gcMap.set('stale', { objKey: staleTarget });
336+
gcMap.set('live', { objKey: liveTarget });
337+
338+
const weakRef = new WeakRef(staleTarget);
339+
staleTarget = null;
340+
341+
const collected = await collectWithRetries(weakRef);
342+
if (!collected) {
343+
expect(gcMap.size).toBe(2);
344+
return;
345+
}
346+
347+
expect(gcMap.has('stale')).toBe(false);
348+
expect(gcMap.size).toBe(1);
349+
expect(Array.from(gcMap.entries())).toEqual([
350+
['live', { objKey: liveTarget }],
351+
]);
352+
expect(Array.from(gcMap.keys())).toEqual(['live']);
353+
expect(Array.from(gcMap.values())).toEqual([{ objKey: liveTarget }]);
354+
},
355+
20_000,
356+
);
357+
});

0 commit comments

Comments
 (0)