Skip to content

Commit d1379a6

Browse files
tinchox5tinchox5
committed
fix: stop concurrent captures from sharing session maps and counter state
1 parent c390280 commit d1379a6

8 files changed

Lines changed: 171 additions & 52 deletions

File tree

‎__tests__/module.counter.test.js‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -126,24 +126,24 @@ describe('resolveCountersInContent', () => {
126126
})
127127
})
128128

129-
describe('buildCounterContext – epoch invalidation', () => {
130-
it('rebuilds the map when the session counter epoch changes', () => {
129+
describe('buildCounterContext – per-capture stability', () => {
130+
it('keeps its snapshot when another capture starts (no shared-epoch rebuild mid-capture)', () => {
131131
const root = document.createElement('div')
132132
const inner = document.createElement('span')
133133
inner.style.counterReset = 'x 5'
134134
root.appendChild(inner)
135135
document.body.appendChild(root)
136136

137-
cache.session = cache.session || {}
138-
cache.session.__counterEpoch = 1
139137
const ctx = buildCounterContext(root)
140138
expect(ctx.get(inner, 'x')).toBe(5)
141139

142-
// Mutate the DOM and bump the epoch: the next query must rebuild from the live tree.
140+
// A concurrently starting capture used to bump a shared epoch and force this context
141+
// to rebuild against a possibly-mutated document mid-capture. The context is
142+
// per-capture now: its snapshot must stay stable for the capture's lifetime.
143143
inner.style.counterReset = 'x 9'
144-
cache.session.__counterEpoch = 2
145-
expect(ctx.get(inner, 'x')).toBe(9)
146-
expect(ctx.getStack(inner, 'x')).toEqual([9])
144+
cache.session.__counterEpoch = (cache.session.__counterEpoch || 0) + 1
145+
expect(ctx.get(inner, 'x')).toBe(5)
146+
expect(ctx.getStack(inner, 'x')).toEqual([5])
147147
})
148148
})
149149

‎__tests__/regression.counterRace.test.js‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import { describe, it, expect, afterEach } from 'vitest'
2+
import { snapdom } from '../src/api/snapdom.js'
3+
4+
// Sibling pseudo-counter overrides used to live in a module-global WeakMap guarded by a
5+
// shared epoch that ANY capture start bumped — a concurrent capture wiped the accumulated
6+
// counters mid-sibling-walk and numbering restarted partway down the list (1,2,1,2…).
7+
// The state now lives on the per-capture sessionCache.
8+
9+
const IMG_URL = 'data:image/svg+xml,' + encodeURIComponent(
10+
'<svg xmlns="http://www.w3.org/2000/svg" width="8" height="8"><rect width="8" height="8" fill="teal"/></svg>'
11+
)
12+
13+
function makeCounterList(cls) {
14+
const style = document.createElement('style')
15+
style.textContent = `
16+
ul.${cls} { list-style: none; counter-reset: item; }
17+
ul.${cls} li::before { counter-increment: item; content: counter(item) ". "; }
18+
ul.${cls} li span.icon::after { content: url("${IMG_URL}"); }
19+
`
20+
document.head.appendChild(style)
21+
const ul = document.createElement('ul')
22+
ul.className = cls
23+
for (let i = 0; i < 4; i++) {
24+
const li = document.createElement('li')
25+
li.appendChild(Object.assign(document.createElement('span'), { className: 'icon' }))
26+
li.appendChild(document.createTextNode(' item'))
27+
ul.appendChild(li)
28+
}
29+
document.body.appendChild(ul)
30+
return { ul, style }
31+
}
32+
33+
describe('pseudo counter numbering under concurrent captures', () => {
34+
afterEach(() => { document.body.innerHTML = ''; document.head.querySelectorAll('style').forEach(s => s.remove()) })
35+
36+
it('both captures keep full 1..4 numbering', async () => {
37+
const a = makeCounterList('bh-race-a')
38+
const b = makeCounterList('bh-race-b')
39+
40+
const [resA, resB] = await Promise.all([snapdom(a.ul), snapdom(b.ul)])
41+
for (const res of [resA, resB]) {
42+
const svg = decodeURIComponent(res.url.split(',')[1])
43+
for (const n of ['1.', '2.', '3.', '4.']) expect(svg).toContain(n)
44+
}
45+
})
46+
})

‎__tests__/regression.sessionRace.test.js‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
import { describe, it, expect, afterEach } from 'vitest'
2+
import { snapdom } from '../src/api/snapdom.js'
3+
4+
// Concurrent captures used to share one cache.session nodeMap: applyCachePolicy runs at
5+
// captureDOM entry but the maps were only snapshotted inside prepareClone, several awaits
6+
// later — a second capture starting in that window reassigned the global, and each capture's
7+
// non-idempotent scroll-compensation loop then also processed the OTHER capture's clones
8+
// (double translate wrapper, content shifted by 2x the scroll).
9+
10+
function makeScroller() {
11+
const el = document.createElement('div')
12+
el.style.cssText = 'width:200px;height:100px;overflow:auto'
13+
const colors = ['red', 'lime', 'blue', 'orange', 'purple']
14+
for (let i = 0; i < 5; i++) {
15+
const line = document.createElement('div')
16+
line.style.cssText = `height:50px;background:${colors[i]}`
17+
el.appendChild(line)
18+
}
19+
document.body.appendChild(el)
20+
void el.offsetHeight // Firefox: flush layout so the element is scrollable before scrollTop lands
21+
el.scrollTop = 50
22+
return el
23+
}
24+
25+
async function centerPixel(result) {
26+
const canvas = await result.toCanvas()
27+
const ctx = canvas.getContext('2d')
28+
const px = ctx.getImageData(Math.floor(canvas.width / 2), 5, 1, 1).data
29+
return [px[0], px[1], px[2]]
30+
}
31+
32+
describe('concurrent captures own their session maps', () => {
33+
afterEach(() => { document.body.innerHTML = '' })
34+
35+
it('a scrolled element is wrapped exactly once under Promise.all of two captures', async () => {
36+
const plain = document.createElement('div')
37+
for (let i = 0; i < 100; i++) plain.appendChild(document.createElement('span')).textContent = 'x'
38+
document.body.appendChild(plain)
39+
const scroller = makeScroller()
40+
41+
const [_, resB] = await Promise.all([snapdom(plain), snapdom(scroller)])
42+
const svg = decodeURIComponent(resB.url.split(',')[1])
43+
const wraps = (svg.match(/translate\(0px,\s*-(?:4[5-9](?:\.\d+)?|50)px\)/g) || []).length
44+
expect(wraps).toBe(1)
45+
46+
// Fidelity: at scrollTop=50 the top row shows the middle of the 2nd (lime) band
47+
const [r, g, b] = await centerPixel(resB)
48+
expect(g).toBeGreaterThan(200)
49+
expect(r).toBeLessThan(100)
50+
expect(b).toBeLessThan(100)
51+
})
52+
53+
it('sequential control still wraps once', async () => {
54+
const scroller = makeScroller()
55+
const res = await snapdom(scroller)
56+
const svg = decodeURIComponent(res.url.split(',')[1])
57+
expect((svg.match(/translate\(0px,\s*-(?:4[5-9](?:\.\d+)?|50)px\)/g) || []).length).toBe(1)
58+
const [, g] = await centerPixel(res)
59+
expect(g).toBeGreaterThan(200)
60+
})
61+
62+
it('scroll compensation reaches clones from an iframe realm (instanceof HTMLElement is realm-bound)', async () => {
63+
const iframe = document.createElement('iframe')
64+
iframe.style.cssText = 'width:260px;height:160px;border:0'
65+
document.body.appendChild(iframe)
66+
const doc = iframe.contentDocument
67+
doc.open()
68+
doc.write(`<!DOCTYPE html><html><body style="margin:0">
69+
<div id="sc" style="width:200px;height:100px;overflow:auto">
70+
<div style="height:50px;background:red"></div>
71+
<div style="height:50px;background:lime"></div>
72+
<div style="height:50px;background:blue"></div>
73+
</div></body></html>`)
74+
doc.close()
75+
doc.getElementById('sc').scrollTop = 50
76+
await new Promise((r) => requestAnimationFrame(r))
77+
78+
const res = await snapdom(doc.body)
79+
const svg = decodeURIComponent(res.url.split(',')[1])
80+
expect((svg.match(/translate\(0px,\s*-(?:4[5-9](?:\.\d+)?|50)px\)/g) || []).length).toBe(1)
81+
})
82+
})

‎src/core/cache.js‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -84,7 +84,6 @@ export function normalizeCachePolicy(v) {
8484
* @param {"soft"|"auto"|"full"|"disabled"} policy
8585
*/
8686
export function applyCachePolicy(policy = 'soft') {
87-
cache.session.__counterEpoch = (cache.session.__counterEpoch || 0) + 1
8887
switch (policy) {
8988
case 'auto': {
9089
cache.session.styleMap = new Map()

‎src/core/capture.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,14 @@ function checkBurstAdvice(element) {
118118
export async function captureDOM(element, options) {
119119
if (!element) throw new Error('Element cannot be null or undefined')
120120
applyCachePolicy(options.cache)
121+
// cache.session is reassigned at every capture start: snapshot THIS capture's maps in the
122+
// same synchronous tick, before any await, or a concurrently started capture swaps them
123+
// and both captures share one nodeMap (double scroll-compensation, cross-contaminated CSS).
124+
options.__session = {
125+
styleMap: cache.session.styleMap,
126+
styleCache: cache.session.styleCache,
127+
nodeMap: cache.session.nodeMap
128+
}
121129
if (!options.burst) checkBurstAdvice(element)
122130
options.__resolveNodeHooks = collectResolveNodeHooks(options)
123131
const fast = options.fast

‎src/core/prepare.js‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,10 +24,13 @@ import { resolveClipRect, freezeViewportPositioned } from '../utils/capture.help
2424
*/
2525

2626
export async function prepareClone(element, options = {}) {
27+
// Prefer the snapshot captureDOM took synchronously at capture start — cache.session may
28+
// belong to a different in-flight capture by the time this runs (see capture.js).
29+
const session = options.__session || cache.session
2730
const sessionCache = {
28-
styleMap: cache.session.styleMap,
29-
styleCache: cache.session.styleCache,
30-
nodeMap: cache.session.nodeMap,
31+
styleMap: session.styleMap,
32+
styleCache: session.styleCache,
33+
nodeMap: session.nodeMap,
3134
options
3235
}
3336

@@ -152,7 +155,9 @@ export async function prepareClone(element, options = {}) {
152155
const scrollX = originalNode.scrollLeft
153156
const scrollY = originalNode.scrollTop
154157
const hasScroll = scrollX || scrollY
155-
if (hasScroll && cloneNode instanceof HTMLElement) {
158+
// Realm-safe HTML check: iframe-realm clones are not instances of this window's
159+
// HTMLElement, but their scroll still needs compensating.
160+
if (hasScroll && cloneNode?.nodeType === 1 && cloneNode.namespaceURI === 'http://www.w3.org/1999/xhtml') {
156161
cloneNode.style.overflow = 'hidden'
157162
cloneNode.style.scrollbarWidth = 'none'
158163
cloneNode.style.msOverflowStyle = 'none'
@@ -162,7 +167,7 @@ export async function prepareClone(element, options = {}) {
162167
try {
163168
const positioned = cloneNode.querySelectorAll('*')
164169
for (const child of positioned) {
165-
if (!(child instanceof HTMLElement)) continue
170+
if (child.nodeType !== 1 || child.namespaceURI !== 'http://www.w3.org/1999/xhtml') continue
166171
const pos = child.style.position
167172
if (pos === 'fixed' || pos === 'absolute') {
168173
const curTop = parseFloat(child.style.top) || 0

‎src/modules/counter.js‎

Lines changed: 2 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,3 @@
1-
import { cache } from '../core/cache'
21

32
/**
43
* Lightweight CSS counter resolver for SnapDOM.
@@ -77,8 +76,6 @@ function formatCounter(value, style) {
7776
* @returns {{ get(node: Element, name: string): number, getStack(node: Element, name: string): number[] }}
7877
*/
7978
export function buildCounterContext(root) {
80-
const getEpoch = () => (cache?.session?.__counterEpoch ?? 0)
81-
let run = getEpoch()
8279
const nodeCounters = new WeakMap()
8380
const rootEl = (root instanceof Document) ? root.documentElement : root
8481

@@ -213,24 +210,15 @@ export function buildCounterContext(root) {
213210
const empty = new Map()
214211
build(rootEl, empty, empty)
215212

216-
// Si cambió el epoch, reconstruimos el mapa antes de responder
217-
function ensureFresh() {
218-
const now = getEpoch()
219-
if (now !== run) {
220-
run = now
221-
const empty = new Map()
222-
build(rootEl, empty, empty)
223-
}
224-
}
225-
213+
// The context is per-capture (lazily built on sessionCache) and never outlives its
214+
// capture, so no cross-capture invalidation is needed here.
226215
return {
227216
/**
228217
* Get top value for counter name at given node.
229218
* @param {Element} node
230219
* @param {string} name
231220
*/
232221
get(node, name) {
233-
ensureFresh()
234222
const s = nodeCounters.get(node)?.get(name)
235223
return s && s.length ? s[s.length - 1] : 0
236224
},
@@ -240,7 +228,6 @@ export function buildCounterContext(root) {
240228
* @param {string} name
241229
*/
242230
getStack(node, name) {
243-
ensureFresh()
244231
const s = nodeCounters.get(node)?.get(name)
245232
return s ? s.slice() : []
246233
}

‎src/modules/pseudo.js‎

Lines changed: 15 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import {
2222
hasCounters
2323
} from '../modules/counter.js'
2424
import { snapFetch } from './snapFetch.js'
25-
import { cache } from '../core/cache.js'
2625

2726
/** Weak memo for per-document preflight results keyed by a cheap style fingerprint */
2827
const __preflightMemo = new WeakMap()
@@ -221,9 +220,6 @@ export function shouldProcessPseudos(doc = document, fp = styleFingerprint(doc))
221220
return false
222221
}
223222

224-
/** Acumulador de contadores por padre para propagar increments en pseudos entre hermanos */
225-
var __siblingCounters = new WeakMap() // parentElement -> Map<counterName, number>
226-
227223
/**
228224
* True if any single side paints a border. The `border-width`/`border-style` shorthands
229225
* can't be parsed with parseFloat: a `border-bottom` resolves to "0px 0px 1px 0px", whose
@@ -239,7 +235,6 @@ function hasPaintedBorder(style) {
239235
}
240236
return false
241237
}
242-
var __pseudoEpoch = -1
243238

244239
/**
245240
* Wraps buildCounterContext(doc) — an O(document) walk — behind a memoizing
@@ -310,9 +305,9 @@ function collapseCssContent(raw) {
310305
* @param {Element} node
311306
* @param {{get:Function, getStack:Function}} base
312307
*/
313-
function withSiblingOverrides(node, base) {
308+
function withSiblingOverrides(node, base, siblingCounters) {
314309
const parent = node.parentElement
315-
const map = parent ? __siblingCounters.get(parent) : null
310+
const map = parent && siblingCounters ? siblingCounters.get(parent) : null
316311
if (!map) return base
317312
return {
318313
get(n, name) {
@@ -417,15 +412,15 @@ function deriveCounterCtxForPseudo(node, pseudoStyle, baseCtx) {
417412
* @param {{get:Function, getStack:Function}} baseCtx
418413
* @returns {{ text: string, incs: Array<{name:string,num:number|undefined}> }}
419414
*/
420-
function resolvePseudoContentAndIncs(node, pseudo, baseCtx) {
415+
function resolvePseudoContentAndIncs(node, pseudo, baseCtx, siblingCounters) {
421416
let ps
422417
try { ps = getStyle(node, pseudo) } catch { }
423418
let raw = ps?.content
424419
if (!raw || raw === 'none' || raw === 'normal') return { text: '', incs: [] }
425420
raw = stripContentAltText(raw)
426421

427422
// 1) aplicar overrides de hermanos
428-
const baseWithSiblings = withSiblingOverrides(node, baseCtx)
423+
const baseWithSiblings = withSiblingOverrides(node, baseCtx, siblingCounters)
429424

430425
// 2) derivar (aplica reset/increment del pseudo)
431426
const derived = deriveCounterCtxForPseudo(node, ps, baseWithSiblings)
@@ -461,13 +456,10 @@ export async function inlinePseudoElements(source, clone, sessionCache, options)
461456
return
462457
}
463458

464-
// Reset per-capture: si cambió el epoch, limpiamos overrides de hermanos
465-
const epoch = (cache?.session?.__counterEpoch ?? 0)
466-
if (__pseudoEpoch !== epoch) {
467-
__siblingCounters = new WeakMap()
468-
if (sessionCache) sessionCache.__counterCtx = null
469-
__pseudoEpoch = epoch
470-
}
459+
// Sibling-counter overrides are per-capture state: they live on sessionCache (whose
460+
// lifetime is exactly one capture), not on a module global — a concurrently starting
461+
// capture used to wipe the shared global mid-traversal via an epoch bump.
462+
if (!sessionCache.__siblingCounters) sessionCache.__siblingCounters = new WeakMap()
471463

472464
// buildCounterContext walks the whole document once — defer it behind a lazy
473465
// wrapper so that cost is only paid the first time a pseudo actually declares
@@ -546,7 +538,7 @@ export async function inlinePseudoElements(source, clone, sessionCache, options)
546538
const isNoExplicitContent =
547539
rawContent === '' || rawContent === 'none' || rawContent === 'normal'
548540
const { text: cleanContent, incs } =
549-
resolvePseudoContentAndIncs(source, pseudo, counterCtx)
541+
resolvePseudoContentAndIncs(source, pseudo, counterCtx, sessionCache.__siblingCounters)
550542

551543
const bg = style.backgroundImage
552544
const bgColor = style.backgroundColor
@@ -587,18 +579,18 @@ const hasExplicitContent = !isNoExplicitContent && cleanContent !== ''
587579
if (!shouldRender) {
588580
// Aun si no renderizamos caja, si el pseudo tenía increments, propagar a hermanos
589581
if (incs && incs.length && source.parentElement) {
590-
const map = __siblingCounters.get(source.parentElement) || new Map()
582+
const map = sessionCache.__siblingCounters.get(source.parentElement) || new Map()
591583
// Para cada counter incrementado en el pseudo, guardar el valor resuelto final
592584
for (const { name } of incs) {
593585
if (!name) continue
594586
// reconstruir valor final desde derived: volvemos a pedirlo
595587
// Usamos withSiblingOverrides + derive para ser consistentes
596-
const baseWithSibs = withSiblingOverrides(source, counterCtx)
588+
const baseWithSibs = withSiblingOverrides(source, counterCtx, sessionCache.__siblingCounters)
597589
const derived = deriveCounterCtxForPseudo(source, getStyle(source, pseudo), baseWithSibs)
598590
const finalVal = derived.get(source, name)
599591
map.set(name, finalVal)
600592
}
601-
__siblingCounters.set(source.parentElement, map)
593+
sessionCache.__siblingCounters.set(source.parentElement, map)
602594
}
603595
continue
604596
}
@@ -717,15 +709,15 @@ const hasExplicitContent = !isNoExplicitContent && cleanContent !== ''
717709

718710
// Antes de insertar, si hubo increments en el pseudo, propagar valor final a los hermanos
719711
if (incs && incs.length && source.parentElement) {
720-
const map = __siblingCounters.get(source.parentElement) || new Map()
721-
const baseWithSibs = withSiblingOverrides(source, counterCtx)
712+
const map = sessionCache.__siblingCounters.get(source.parentElement) || new Map()
713+
const baseWithSibs = withSiblingOverrides(source, counterCtx, sessionCache.__siblingCounters)
722714
const derived = deriveCounterCtxForPseudo(source, getStyle(source, pseudo), baseWithSibs)
723715
for (const { name } of incs) {
724716
if (!name) continue
725717
const finalVal = derived.get(source, name)
726718
map.set(name, finalVal)
727719
}
728-
__siblingCounters.set(source.parentElement, map)
720+
sessionCache.__siblingCounters.set(source.parentElement, map)
729721
}
730722

731723
if (!hasVisibleBox) continue

0 commit comments

Comments
 (0)