-
Notifications
You must be signed in to change notification settings - Fork 5k
chore: support typed arrays in indexeddb #34949
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1837b1b
ee70854
dfd44ee
1610cca
37f1f9f
769fda6
bcf128b
2ba954a
662d82a
f3ac205
347afcb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,8 @@ | |
|
|
||
| import type { Builtins } from './builtins'; | ||
|
|
||
| type TypedArrayKind = 'i8' | 'ui8' | 'ui8c' | 'i16' | 'ui16' | 'i32' | 'ui32' | 'f32' | 'f64' | 'bi64' | 'bui64'; | ||
|
|
||
| export type SerializedValue = | ||
| undefined | boolean | number | string | | ||
| { v: 'null' | 'undefined' | 'NaN' | 'Infinity' | '-Infinity' | '-0' } | | ||
|
|
@@ -27,7 +29,8 @@ export type SerializedValue = | |
| { a: SerializedValue[], id: number } | | ||
| { o: { k: string, v: SerializedValue }[], id: number } | | ||
| { ref: number } | | ||
| { h: number }; | ||
| { h: number } | | ||
| { ta: { b: string, k: TypedArrayKind } }; | ||
|
|
||
| type HandleOrValue = { h: number } | { fallThrough: any }; | ||
|
|
||
|
|
@@ -70,6 +73,48 @@ export function source(builtins: Builtins) { | |
| } | ||
| } | ||
|
|
||
| function isTypedArray(obj: any, constructor: Function): boolean { | ||
| try { | ||
| return obj instanceof constructor || Object.prototype.toString.call(obj) === `[object ${constructor.name}]`; | ||
| } catch (error) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| const typedArrayConstructors: Record<TypedArrayKind, Function> = { | ||
| i8: Int8Array, | ||
| ui8: Uint8Array, | ||
| ui8c: Uint8ClampedArray, | ||
| i16: Int16Array, | ||
| ui16: Uint16Array, | ||
| i32: Int32Array, | ||
| ui32: Uint32Array, | ||
| // TODO: add Float16Array once it's in baseline | ||
| f32: Float32Array, | ||
| f64: Float64Array, | ||
| bi64: BigInt64Array, | ||
| bui64: BigUint64Array, | ||
| }; | ||
|
|
||
| function typedArrayToBase64(array: any) { | ||
| /** | ||
| * Firefox does not support iterating over typed arrays, so we use `.toBase64`. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps instead of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tried that, but doesn't work. It looks like any kind of iteration is forbidden: https://searchfox.org/mozilla-central/source/js/xpconnect/wrappers/XrayWrapper.cpp#576
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What exactly does this mean? You should be able to iterate arrays and use Array.from on them?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But not inside the utility world, apparently 🤷 |
||
| * Error: 'Accessing TypedArray data over Xrays is slow, and forbidden in order to encourage performant code. To copy TypedArrays across origin boundaries, consider using Components.utils.cloneInto().' | ||
| */ | ||
| if ('toBase64' in array) | ||
| return array.toBase64(); | ||
| const binary = Array.from(new Uint8Array(array.buffer, array.byteOffset, array.byteLength)).map(b => String.fromCharCode(b)).join(''); | ||
| return btoa(binary); | ||
| } | ||
|
|
||
| function base64ToTypedArray(base64: string, TypedArrayConstructor: any) { | ||
| const binary = atob(base64); | ||
| const bytes = new Uint8Array(binary.length); | ||
| for (let i = 0; i < binary.length; i++) | ||
| bytes[i] = binary.charCodeAt(i); | ||
| return new TypedArrayConstructor(bytes.buffer); | ||
| } | ||
|
|
||
| function parseEvaluationResultValue(value: SerializedValue, handles: any[] = [], refs: Builtins.Map<number, object> = new builtins.Map()): any { | ||
| if (Object.is(value, undefined)) | ||
| return undefined; | ||
|
|
@@ -121,6 +166,8 @@ export function source(builtins: Builtins) { | |
| } | ||
| if ('h' in value) | ||
| return handles[value.h]; | ||
| if ('ta' in value) | ||
| return base64ToTypedArray(value.ta.b, typedArrayConstructors[value.ta.k]); | ||
| } | ||
| return value; | ||
| } | ||
|
|
@@ -191,6 +238,10 @@ export function source(builtins: Builtins) { | |
| return { u: value.toJSON() }; | ||
| if (isRegExp(value)) | ||
| return { r: { p: value.source, f: value.flags } }; | ||
| for (const [k, ctor] of Object.entries(typedArrayConstructors) as [TypedArrayKind, Function][]) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd start with looking up constructor.name in a set instead of iterating
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that won't work on Firefox, see impl of
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It does work for me, are you saying this is also due to utility worlds? Something is fishy here. Do we have a bug in the way we initialize utility context? It needs to be proper JS context.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. as discussed in the meeting, firefox is just off here. let's not fix in this PR. |
||
| if (isTypedArray(value, ctor)) | ||
| return { ta: { b: typedArrayToBase64(value), k } }; | ||
| } | ||
|
|
||
| const id = visitorInfo.visited.get(value); | ||
| if (id) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like we need a bunch of page.evaluate tests for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests in
page-evaluate.spec.tsall go through the protocol serializer, which also doesn't yet have support. Do we test the server-side ofpage.evaluatein isolation?I was planning to add support to the protocol serializer in a follow-up, happy to add page.evaluate tests in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Landing this w/o coverage does not feel right, probably supporting the protocol serializer and testing e2e is the right approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added support to the protocol serializer and a bunch of page.evaluate tests.