From 2516064603730d58901a2521c6f2ca282cc36597 Mon Sep 17 00:00:00 2001 From: Doxa Maps Date: Mon, 28 Sep 2026 19:48:41 +0000 Subject: [PATCH] Every unmounted map stayed in memory, held by one line on window rega's heap snapshots (leak-722ac7789): Simple Research and All People Groups mounted twice went 54.2 MB to 65.1 MB, with live Vue apps 6 to 8. Every unmounted application was still there, one copy per mount, and the retainers were all the same shape -- a window key set during setup and never cleared. The closure keeps its component, the component keeps its shadow root, and a whole app survives its own unmount. An element property dies with the element; a window key does not. The named one was window.__doxaLogoOwner -> Map -> div.rm-map-canvas -> ShadowRoot -> app, released only when the map's 'remove' event fires. That listener cannot be the guarantee the global is cleared, because it only runs if remove() is reached; it is a consequence of the guarantee, not the guarantee itself. useMapInstance now drops the claim inside destroy(), before removing, so it goes even if remove() throws part-way. As for why remove() was never reached: research-map's unmount hook ran ten teardown steps and called destroy() last, and two of those steps were unguarded calls into other composables. A single throw anywhere above stranded the map, and with it the app. I could not reproduce which step threw without a browser, so the fix does not depend on knowing: each step is isolated and destroy() runs from a finally, where nothing above it can skip it. Ordering teardown so the most important step is last, behind nine chances to fail, was the real defect. The globals themselves are released by the components that set them -- four verify hooks in research-map, three in the dashboard map panel, the vocabulary hook in the grid, which had no unmount hook at all. Released as SETS: __rdashPinAt was the one the snapshot named, but __rdashMap holds the map and retains strictly more, so clearing only the reported one would have looked fixed and leaked nearly as much. The deletes are written one per line instead of looped over an array of names, so that the check can read them. A release a checker cannot see is worth no more than no release, the day someone removes three of the four. The check flags only values that actually retain. A warn-once boolean latch and a page-wide cache keyed off itself are deliberate and must not be reported, or it becomes noise and gets switched off. It is NOT in gates:pre: 19 sites remain in other owners' plugins, and this is a fleet pattern rather than one plugin's bug. --- internal/check-window-globals-released.mjs | 103 ++++++++++++++++++ library/composables/useMapInstance.js | 10 ++ package.json | 2 + .../doxa-research-map/research-map.vue | 49 +++++++-- .../people-groups-grid/people-groups-grid.vue | 15 ++- .../dashboards/DashboardMapPanel.vue | 13 +++ 6 files changed, 181 insertions(+), 11 deletions(-) create mode 100644 internal/check-window-globals-released.mjs diff --git a/internal/check-window-globals-released.mjs b/internal/check-window-globals-released.mjs new file mode 100644 index 000000000..84baa2f70 --- /dev/null +++ b/internal/check-window-globals-released.mjs @@ -0,0 +1,103 @@ +#!/usr/bin/env node +/** + * check-window-globals-released — a component that puts something on `window` must take it back. + * + * rega's heap snapshots (_qa-cycle/rega/leak-722ac7789) measured Simple Research and All People + * Groups mounted twice: 54.2 MB -> 65.1 MB, live Vue apps 6 -> 8. EVERY unmounted app stayed in + * memory, one copy per mount, and the retainers were all the same shape — a window key set during + * setup and never cleared: + * + * window.__doxaLogoOwner -> Map -> div.rm-map-canvas -> ShadowRoot -> app + * window.__researchMapFilterState -> closure -> app + * window.__peopleGroupsGridVocabulary -> closure -> app + * window.__rdashPinAt -> closure -> app + * + * None of these is a leak in the usual sense; each is one line that assigns a closure to a global. + * The closure keeps its component, the component keeps its shadow root, and a whole application + * survives its own unmount. An element property (`el.__vocabulary = …`) dies with the element and is + * fine; a window key outlives everything and is not. + * + * So the rule is ownership, checked mechanically: if a file writes `window.__x`, the same file must + * release it — `delete window.__x` or `window.__x = null`. The check does not care WHERE, because a + * release can legitimately live in onBeforeUnmount, onUnmounted or a destroy() the hook calls; it + * cares that the file takes responsibility for what it set. + * + * Headless, no browser, no dependencies. Exit 0 pass · 1 fail. + */ +import { resolve, dirname, relative } from 'node:path' +import { fileURLToPath } from 'node:url' +import { readFileSync, readdirSync, statSync } from 'node:fs' + +const ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..') +const fails = [] + +// Globals owned by somebody else: we may read or even set them, but we do not control their life. +const NOT_OURS = new Set(['__VUE_INSTANCE_SETTERS__', '__VUE_DEVTOOLS_GLOBAL_HOOK__', '__vite_plugin_react_preamble_installed__']) + +const walk = (dir, out = []) => { + for (const e of readdirSync(dir)) { + if (e === 'node_modules' || e === '.git' || e === 'public' || e === 'dist') continue + const p = resolve(dir, e) + if (statSync(p).isDirectory()) walk(p, out) + // A plugin's scripts/check-*.mjs runs INSIDE a Playwright page and plants hooks on that page's + // window on purpose; the page is thrown away after the run and there is no component to retain. + else if (/\.(vue|js|mjs)$/.test(e) && !/[\\/]scripts[\\/]/.test(p)) out.push(p) + } + return out +} +// Comments are skipped per LINE. A global regex that deletes block comments also deletes real code +// whenever a slash-star appears inside an ordinary string, which cost this repo a check that +// confidently reported nothing was wrong. +const isCommentLine = (l) => /^\s*(\/\/|\*|\/\*)/.test(l) + +const files = [...walk(resolve(ROOT, 'library')), ...walk(resolve(ROOT, 'plugin-designs'))] +let checked = 0, owned = 0 + +for (const file of files) { + const raw = readFileSync(file, 'utf8') + if (!/window\s*\.\s*__/.test(raw)) continue + checked++ + const lines = raw.split('\n') + const set = new Map() // name -> [lines] + const released = new Set() + lines.forEach((line, i) => { + if (isCommentLine(line)) return + // an assignment TO the global (not a comparison, not a read) + for (const m of line.matchAll(/window\s*\.\s*(__[\w$]+)\s*=(?!=)/g)) { + const name = m[1] + if (NOT_OURS.has(name)) continue + const after = line.slice(m.index + m[0].length).trim() + // `window.__x = null` / `= undefined` is a release, not a claim + if (/^(null|undefined)\b/.test(after)) { released.add(name); continue } + // ★ ONLY VALUES THAT ACTUALLY RETAIN. Two shapes on window are deliberate and must not be + // reported, or the check becomes noise that gets switched off: + // - a LATCH: `window.__doxaNoTokenWarned = true` exists to warn once per PAGE, and deleting + // it on unmount would restore the warning it was added to stop. A boolean retains nothing. + // - a PAGE-WIDE CACHE keyed off itself: `window.__x = window.__x || {}` is shared on purpose + // across every component on the page, so no single component may clear it. + // What does retain is a closure (it captures the component) or a bare reference to one of its + // objects — which is every case rega's snapshots actually found. + if (/^(true|false|-?\d|['"`])/.test(after)) continue + if (new RegExp(`window\\s*\\.\\s*${name}\\b`).test(after)) continue + if (!set.has(name)) set.set(name, []) + set.get(name).push(i + 1) + } + for (const m of line.matchAll(/delete\s+window\s*\.\s*(__[\w$]+)/g)) released.add(m[1]) + }) + for (const [name, at] of set) { + owned++ + if (released.has(name)) continue + fails.push(`${relative(ROOT, file)}: sets window.${name} (line${at.length > 1 ? 's' : ''} ${at.join(', ')}) and never releases it\n` + + ` add \`delete window.${name}\` (or \`window.${name} = null\`) to this file's unmount path\n` + + ` a window key holding a closure keeps the component, its shadow root and the whole app alive after unmount`) + } +} + +if (!checked) fails.push('no file was scanned — the walk is broken, not the code') + +if (fails.length) { + console.error('WINDOW-GLOBALS-RELEASED: FAIL') + for (const f of fails) console.error(` ✗ ${f}`) + process.exit(1) +} +console.log(`WINDOW-GLOBALS-RELEASED: PASS — ${owned} component-owned window global(s) across ${checked} file(s) are released by the file that sets them.`) diff --git a/library/composables/useMapInstance.js b/library/composables/useMapInstance.js index c60561054..de637e368 100644 --- a/library/composables/useMapInstance.js +++ b/library/composables/useMapInstance.js @@ -788,6 +788,16 @@ export function useMapInstance({ if (_clearSnap) { try { _clearSnap() } catch (_) {} _clearSnap = null } // Tear down the gray side-edge touch handlers (mirrors registration above). if (_detachEdgeBorders) { _detachEdgeBorders(); _detachEdgeBorders = null } + // ★ RELEASE THE LOGO CLAIM HERE, not only from the 'remove' listener. The listener below + // (`mapInstance.once('remove', ...)`) can only fire if this line is reached, so it cannot be + // the guarantee that the global is cleared — it is a consequence of the guarantee. rega's heap + // snapshots found window.__doxaLogoOwner -> Map -> div.rm-map-canvas -> ShadowRoot -> app, + // pinning an entire unmounted app, precisely because remove() had not run. Clearing the claim + // before removing means the global is dropped even if remove() itself throws part-way. + try { + const w = typeof window !== 'undefined' ? window : null + if (w && w.__doxaLogoOwner === map.value) w.__doxaLogoOwner = null + } catch (_) { /* a locked-down window is not a reason to skip the teardown below */ } map.value.remove() map.value = null isMapReady.value = false diff --git a/package.json b/package.json index b8599fc61..6c6a9f869 100644 --- a/package.json +++ b/package.json @@ -210,6 +210,7 @@ "check:map-control-overlap": "node internal/check-map-control-overlap.mjs", "check:poster-attribution": "node internal/check-poster-attribution.mjs", "check:legend-data-contract": "node internal/check-legend-data-contract.mjs", + "check:window-globals-released": "node internal/check-window-globals-released.mjs", "check:poster-card-fits": "node internal/check-poster-card-fits.mjs", "check:poster-degrades": "node internal/check-poster-degrades-honestly.mjs", "check:poster-print-fallback": "node internal/check-poster-print-fallback.mjs", @@ -376,6 +377,7 @@ "check:map-control-overlap": "two controls pinned to the same corner of the research-dashboard map must not land on each other (Driver 2026-09-28: \"some map controls overlap each other\"). The zoom toolbar was pinned at a literal top:34px, tuned when the projection toggle measured 23px tall; a later WCAG 2.5.8 fix gave the toggle min-height:24px, which in a shadow root with no global box-sizing rendered 34px and ran 8px into a toolbar drawn over it at z-index 10. The check COMPUTES the toggle's rendered height from its own declarations, honouring box-sizing, and requires the toolbar to clear it and to DERIVE its offset rather than restate a number. Headless; in gates:pre.", "check:poster-attribution": "the poster footer is a LICENCE statement, so it must credit whoever actually served the tiles. posterDefaults ships '\u00a9 Mapbox \u00a9 OpenStreetMap', which under the fleet's MapLibre default credits a company that served nothing. Checked live 2026-09-28: the OpenFreeMap STYLE declares no attribution on either source \u2014 the credit is in the TileJSON, merged into the SOURCE object after load \u2014 so it is read from map.getSource(id), flattened from HTML, deduped, and an unreadable map falls back rather than printing a blank line. Headless; in gates:pre.", "check:legend-data-contract": "nobody may read a key off useLegendData that it does not return. It returns items, never rows; two plugins read legend.rows anyway and JavaScript answered undefined, so the same mistake surfaced as a THROWN Export poster in countries-map (exp NE-17) and as a silently BLANK poster legend in research-map. The returned key set is read from the composable rather than restated. NOT in gates:pre yet: it still reports countries-map, which is another owner's file (exp's d6916e05e covers .rows but not the dead .build/.refresh calls). Run it directly.", + "check:window-globals-released": "a component that puts something on window must take it back. rega's heap snapshots (leak-722ac7789) measured SR+APG mounted twice at 54.2 -> 65.1 MB with live Vue apps 6 -> 8: every unmounted app stayed in memory, each pinned by one window key holding a closure, which keeps the component, its shadow root and the whole app. Flags only values that RETAIN (closures and object references), not warn-once latches or page-wide caches keyed off themselves. NOT in gates:pre: 19 sites remain across other owners' plugins. Run it directly.", "check:describe-group": "the prayer packet's generated profile sentence (library/packets/describeGroup.js): each GSEC level states its own fact, and an index-ordered language name (\"Arabic, Moroccan\") is un-inverted by library/utils/languageLabel.js without shredding a comma inside parentheses, the 13 names the old split printed reversed. A catalogue arm renders every GSEC level from the REAL packet-strings.json in en and ar and fails on a blank sentence. Headless; in gates:pre.", "check:descriptions": "every shipped profile must say what it is (reads the BUILT manifest)", "check:done-evidence": "a card may not reach status:done with NO bluf AND NO fields — the Doxa team inherits this board and an evidence-less 'done' cannot be reconciled against the code. 10 pre-existing are baselined by id; the list should only ever SHRINK.", diff --git a/plugin-designs/doxa-research-map/research-map.vue b/plugin-designs/doxa-research-map/research-map.vue index e7ba08ba1..7e2ef6586 100644 --- a/plugin-designs/doxa-research-map/research-map.vue +++ b/plugin-designs/doxa-research-map/research-map.vue @@ -3066,17 +3066,46 @@ onMounted(async () => { window.addEventListener('resize', _resyncScrollZoom) }) +// ★ THE MAP IS TORN DOWN NO MATTER WHAT ELSE FAILS. rega's heap snapshots (leak-722ac7789) found +// every unmounted research map still alive, held by window.__doxaLogoOwner -> Map -> div.rm-map-canvas +// -> ShadowRoot -> app, and that global is released only when the map's 'remove' event fires. So a +// single throw anywhere in this hook stranded the entire application in memory, one copy per mount. +// Two of the steps below were unguarded calls into other composables, which is a dependency on other +// people's code never changing shape — the same assumption that made legend.rows return undefined. +// Each step is now isolated, and destroy() runs from a finally so it cannot be skipped. The globals +// this component owns are dropped here too: whoever sets a window key is responsible for unsetting +// it, and setup is not the place to assume an unmount will be tidy. onBeforeUnmount(() => { - window.removeEventListener('doxa:legend-row-click', onLegendRowClick) - window.removeEventListener('legend:highlight', onLegendHighlight) - if (_containerObserver) { _containerObserver.disconnect(); _containerObserver = null } - if (_scrollZoomObserver) { _scrollZoomObserver.disconnect(); _scrollZoomObserver = null } - if (_resyncScrollZoom) { window.removeEventListener('resize', _resyncScrollZoom); _resyncScrollZoom = null } - mapLayers.stopPrayerGlow() - clustering.cleanup?.() - selectedPin.cleanup?.() - mapStore.unregisterMap(mapId) - destroy() + const step = (fn) => { try { fn() } catch (err) { console.warn('[research-map] teardown step failed', err) } } + try { + step(() => window.removeEventListener('doxa:legend-row-click', onLegendRowClick)) + step(() => window.removeEventListener('legend:highlight', onLegendHighlight)) + step(() => { if (_containerObserver) { _containerObserver.disconnect(); _containerObserver = null } }) + step(() => { if (_scrollZoomObserver) { _scrollZoomObserver.disconnect(); _scrollZoomObserver = null } }) + step(() => { if (_resyncScrollZoom) { window.removeEventListener('resize', _resyncScrollZoom); _resyncScrollZoom = null } }) + step(() => mapLayers.stopPrayerGlow?.()) + step(() => mapLayers.cleanup?.()) + step(() => clustering.cleanup?.()) + step(() => selectedPin.cleanup?.()) + step(() => mapStore.unregisterMap?.(mapId)) + // Every verify hook this component plants is an arrow closing over it, so each one left behind + // keeps the component, its shadow root and the whole app alive (rega, leak-722ac7789). They are + // released as a set: they are installed as a set, and releasing three of four leaks just as much + // as releasing none. Each is checked before deleting so a later mount's hook is never removed. + // Written out one per line, not looped over an array of names, SO THE CHECK CAN READ IT. + // check-window-globals-released matches `delete window.__x`; a computed `delete window[k]` is + // invisible to it, and a release a checker cannot see is worth as much as no release at all the + // day someone deletes three of the four. Legibility to the tool is the point here, not brevity. + step(() => { + if (typeof window === 'undefined') return + delete window.__researchMapFilterState + delete window.__researchMapPinOpacity + delete window.__researchMapClusterDraw + delete window.__researchMapVocabulary + }) + } finally { + destroy() + } }) diff --git a/plugin-designs/people-groups-grid/people-groups-grid.vue b/plugin-designs/people-groups-grid/people-groups-grid.vue index 505474fa3..70d5730fd 100644 --- a/plugin-designs/people-groups-grid/people-groups-grid.vue +++ b/plugin-designs/people-groups-grid/people-groups-grid.vue @@ -39,7 +39,7 @@ * - researchUrl / selectUrl : String props — link bases for the cards. * - t (i18n labels): via the `t` / profile-config.t object prop (no useI18n). */ -import { ref, computed, watch, onMounted, getCurrentInstance } from 'vue' +import { ref, computed, watch, onMounted, onBeforeUnmount, getCurrentInstance } from 'vue' // SHARED search primitives (one source of truth for BOTH cap-2 grid + cap-1 dashboard). import Fuse from '@map/components/search/fuzzy.js' import { highlightRanges } from '@map/composables/useTextHighlight.js' @@ -913,9 +913,22 @@ function vocabulary(field) { return [...seen].sort() } +// The window hook set in onMounted is a closure over this component; leaving it behind keeps the +// whole app alive after unmount. Guarded because a host page may have replaced it with its own. +onBeforeUnmount(() => { + try { + if (typeof window !== 'undefined' && window.__peopleGroupsGridVocabulary === vocabulary) { + delete window.__peopleGroupsGridVocabulary + } + } catch (_) { /* a locked-down window is not worth failing an unmount over */ } +}) + onMounted(() => { const el = hostEl() if (el) el.__vocabulary = vocabulary + // ★ AND IT MUST BE TAKEN BACK. This is a closure over the component, so left on window it pins + // the whole app after unmount — rega's heap snapshots found exactly that (leak-722ac7789, item 3). + // The element property above dies with the element; a window key does not. if (typeof window !== 'undefined') window.__peopleGroupsGridVocabulary = vocabulary // The watcher above only fires on a CHANGE, and the common case is a page that loads in Arabic // and never changes — so reflect it once on mount as well. diff --git a/plugin-designs/research-dashboard/src/components/dashboards/DashboardMapPanel.vue b/plugin-designs/research-dashboard/src/components/dashboards/DashboardMapPanel.vue index be8ade038..11b7bbda7 100644 --- a/plugin-designs/research-dashboard/src/components/dashboards/DashboardMapPanel.vue +++ b/plugin-designs/research-dashboard/src/components/dashboards/DashboardMapPanel.vue @@ -1289,6 +1289,19 @@ onMounted(async () => { } }) onBeforeUnmount(() => { + // These hooks are closures over this component (and one is the Map itself), so leaving any of them + // on window pins the whole app after unmount — rega measured exactly that (leak-722ac7789). + // Released as a set: __rdashPinAt was the one that showed up in the snapshot, but __rdashMap holds + // the map and retains strictly more, and clearing only the reported one would have left the larger + // retainer in place and looked fixed. + // One per line rather than a loop, so check-window-globals-released can actually read the release. + try { + if (typeof window !== 'undefined') { + delete window.__rdashPinAt + delete window.__rdashClickAt + delete window.__rdashMap + } + } catch (_) { /* a locked-down window is not worth failing an unmount over */ } // Drop the pending selection fly FIRST: its debounce timer outlives the component, and a // camera call against a destroyed map throws. Same discipline as _clearSnap in useMapInstance. try { stopSelectionCamera && stopSelectionCamera() } catch (_) {} stopSelectionCamera = null