Skip to content

Commit 7f9d45d

Browse files
authored
fix(reactivity): avoid mutating native array iterators (#15703)
fix #15643
1 parent 702fa61 commit 7f9d45d

2 files changed

Lines changed: 226 additions & 7 deletions

File tree

Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,189 @@
1+
import { setFlagsFromString } from 'node:v8'
2+
import {
3+
isProxy,
4+
isReactive,
5+
isReadonly,
6+
reactive,
7+
readonly,
8+
} from '../src/reactive'
9+
10+
// kept in its own file: the protector check below must run before anything
11+
// else in the file iterates a reactive array
12+
describe('reactivity/reactive/Array iterator', () => {
13+
// #15643
14+
test('iterating should not invalidate V8 array iteration fast paths', () => {
15+
setFlagsFromString('--allow-natives-syntax')
16+
const isProtectorIntact = new Function(
17+
'return %ArrayIteratorProtector()',
18+
) as () => boolean
19+
// the protector is isolate-wide and can only ever be invalidated, so it
20+
// has to be intact on entry for the check below to mean anything; each
21+
// test file runs in its own worker, so nothing else can have tripped it
22+
expect(isProtectorIntact()).toBe(true)
23+
24+
const seen: unknown[] = []
25+
const deep = reactive([{ val: 1 }])
26+
for (const item of deep) seen.push(item)
27+
for (const item of deep.values()) seen.push(item)
28+
for (const [, item] of deep.entries()) seen.push(item)
29+
for (const item of readonly([{ val: 1 }])) seen.push(item)
30+
for (const item of readonly(deep)) seen.push(item)
31+
for (const item of readonly(deep).values()) seen.push(item)
32+
for (const [, item] of readonly(deep).entries()) seen.push(item)
33+
expect(seen.length).toBe(7)
34+
expect(seen.every(isProxy)).toBe(true)
35+
36+
class Items extends Array {
37+
[Symbol.iterator]() {
38+
return super[Symbol.iterator]()
39+
}
40+
values() {
41+
return super.values()
42+
}
43+
entries() {
44+
return super.entries()
45+
}
46+
}
47+
const custom = reactive(new Items())
48+
custom.push({ val: 1 })
49+
const iter = custom[Symbol.iterator]()
50+
const values = custom.values()
51+
const entries = custom.entries()
52+
expect(isProtectorIntact()).toBe(true)
53+
expect(isReactive(iter.next().value)).toBe(true)
54+
expect(isReactive(values.next().value)).toBe(true)
55+
expect(isReactive(entries.next().value![1])).toBe(true)
56+
57+
expect(isProtectorIntact()).toBe(true)
58+
})
59+
60+
test('should keep native array iterator behavior', () => {
61+
const iter = reactive([{ val: 1 }, { val: 2 }])[Symbol.iterator]()
62+
// inherits from the shared native array iterator prototype
63+
expect(Object.prototype.toString.call(iter)).toBe('[object Array Iterator]')
64+
expect(iter[Symbol.iterator]()).toBe(iter)
65+
66+
const first = iter.next()
67+
expect(first.done).toBe(false)
68+
expect(isReactive(first.value)).toBe(true)
69+
iter.next()
70+
expect(iter.next()).toEqual({ value: undefined, done: true })
71+
})
72+
73+
test('should close custom iterators on early exit', () => {
74+
let closed = false
75+
class Items extends Array {
76+
*[Symbol.iterator]() {
77+
try {
78+
yield {}
79+
} finally {
80+
closed = true
81+
}
82+
return undefined
83+
}
84+
}
85+
86+
for (const item of reactive(new Items())) {
87+
expect(isReactive(item)).toBe(true)
88+
break
89+
}
90+
expect(closed).toBe(true)
91+
})
92+
93+
test('should close native iterators returned by custom methods on early exit', () => {
94+
let reads = 0
95+
let closed = false
96+
class Items extends Array {
97+
[Symbol.iterator]() {
98+
const iter = super[Symbol.iterator]()
99+
Object.defineProperty(iter, 'return', {
100+
get() {
101+
expect(this).toBe(iter)
102+
reads++
103+
return function (this: unknown) {
104+
expect(this).toBe(iter)
105+
closed = true
106+
return { value: undefined, done: true }
107+
}
108+
},
109+
})
110+
return iter
111+
}
112+
}
113+
114+
const items = new Items()
115+
items.push({})
116+
const iter = reactive(items)[Symbol.iterator]()
117+
expect(reads).toBe(0)
118+
for (const item of iter) {
119+
expect(isReactive(item)).toBe(true)
120+
expect(reads).toBe(0)
121+
break
122+
}
123+
expect(reads).toBe(1)
124+
expect(closed).toBe(true)
125+
})
126+
127+
test('should pass wrapped values to iterator helpers', () => {
128+
const raw = [{ val: 1 }]
129+
if (!('map' in raw.values())) return
130+
131+
class Items extends Array {
132+
*values() {
133+
yield* raw
134+
return undefined
135+
}
136+
}
137+
for (const array of [
138+
reactive(raw),
139+
readonly(reactive(raw)),
140+
reactive(new Items()),
141+
readonly(reactive(new Items())),
142+
]) {
143+
const iter = array.values()
144+
// @ts-expect-error iterator helpers are not in es2016
145+
const mapped = iter.map((item: (typeof raw)[number]) => {
146+
expect(isReactive(item)).toBe(true)
147+
expect(isReadonly(item)).toBe(isReadonly(array))
148+
return item.val
149+
})
150+
expect([...mapped]).toEqual([1])
151+
}
152+
})
153+
154+
test.each(['return', 'throw'] as const)(
155+
'should read the current native iterator %s method lazily',
156+
method => {
157+
const original = [{}].values()
158+
const initial = vi.fn()
159+
let closingMethod: unknown = initial
160+
const getMethod = vi.fn(() => closingMethod)
161+
Object.defineProperty(original, method, { get: getMethod })
162+
class Items extends Array {
163+
values() {
164+
return original
165+
}
166+
}
167+
168+
const iter: IterableIterator<unknown> = reactive(new Items()).values()
169+
expect(getMethod).not.toHaveBeenCalled()
170+
171+
const value = {}
172+
const result = { value, done: true }
173+
const close = vi.fn(() => result)
174+
closingMethod = close
175+
expect(iter[method]!(value)).toBe(result)
176+
expect(initial).not.toHaveBeenCalled()
177+
expect(close).toHaveBeenCalledExactlyOnceWith(value)
178+
expect(close.mock.contexts[0]).toBe(original)
179+
expect(getMethod).toHaveBeenCalledOnce()
180+
expect(getMethod.mock.contexts[0]).toBe(original)
181+
182+
for (const replacement of [undefined, null, 1]) {
183+
closingMethod = replacement
184+
expect(iter[method]).toBe(replacement)
185+
}
186+
expect(getMethod).toHaveBeenCalledTimes(4)
187+
},
188+
)
189+
})

‎packages/reactivity/src/arrayInstrumentations.ts‎

Lines changed: 37 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -254,22 +254,52 @@ function iterator(
254254
// given that JS iterator can only be read once, this doesn't seem like
255255
// a plausible use-case, so this tracking simplification seems ok.
256256
const arr = shallowReadArray(self)
257-
const iter = (arr[method] as any)() as IterableIterator<unknown> & {
257+
const methodFn = arr[method] as any
258+
const iter = methodFn.call(arr) as IterableIterator<unknown> & {
258259
_next: IterableIterator<unknown>['next']
259260
}
260261
if (arr !== self && !isShallow(self)) {
261-
iter._next = iter.next
262-
iter.next = () => {
263-
const result = iter._next()
264-
if (!result.done) {
265-
result.value = wrapValue(result.value)
262+
// #15643 an own `next` on a native array iterator permanently disables
263+
// V8's array iteration fast paths (spread, Array.from, etc.) for the whole
264+
// isolate, so it is wrapped instead. Checked on the iterator itself as
265+
// user-extended or cross-realm arrays can return native iterators too.
266+
if ((iter as any)[Symbol.toStringTag] === 'Array Iterator') {
267+
const wrapped = Object.create(Object.getPrototypeOf(iter))
268+
wrapped.next = () => wrapResult(iter.next(), wrapValue)
269+
if (methodFn !== arrayProto[method]) {
270+
Object.defineProperties(wrapped, {
271+
return: {
272+
get() {
273+
const fn = iter.return
274+
return typeof fn === 'function' ? fn.bind(iter) : fn
275+
},
276+
},
277+
throw: {
278+
get() {
279+
const fn = iter.throw
280+
return typeof fn === 'function' ? fn.bind(iter) : fn
281+
},
282+
},
283+
})
266284
}
267-
return result
285+
return wrapped
268286
}
287+
iter._next = iter.next
288+
iter.next = () => wrapResult(iter._next(), wrapValue)
269289
}
270290
return iter
271291
}
272292

293+
function wrapResult(
294+
result: IteratorResult<unknown>,
295+
wrapValue: (value: any) => unknown,
296+
) {
297+
if (!result.done) {
298+
result.value = wrapValue(result.value)
299+
}
300+
return result
301+
}
302+
273303
// in the codebase we enforce es2016, but user code may run in environments
274304
// higher than that
275305
type ArrayMethods = keyof Array<any> | 'findLast' | 'findLastIndex'

0 commit comments

Comments
 (0)