Add a high-level API for JS typed arrays #6779

jdm · 2015-07-26T15:46:20Z

This largely a Rust-y port of Gecko's API. No codegen support yet, but it's a lot easier to work with typed arrays properly now.

r? @Ms2ger

highfive · 2015-07-26T15:46:25Z

Warning

These commits modify unsafe code. Please review it carefully!

jdm · 2015-07-26T15:46:38Z

Requires servo/rust-mozjs#180.

Ms2ger · 2015-07-26T20:03:27Z

The first thing I'd ask is whether some of this could live in rust-mozjs; I sometimes dream of a fully safe api around SM.

Reviewed 2 of 2 files at r1, 2 of 2 files at r2, 2 of 2 files at r3, 1 of 1 files at r4.
Review status: 3 of 9 files reviewed at latest revision, 13 unresolved discussions, all commit checks successful.

^{components/canvas_traits/lib.rs, line 126 [r3] (raw file):}
I like issue numbers with my TODOs.

^{components/script/dom/bindings/typedarray.rs, line 47 [r1] (raw file):}
I don't imagine this can be private.

^{components/script/dom/bindings/typedarray.rs, line 137 [r1] (raw file):}
This is what Gecko does, but why not have fn new() -> Result<TypedArrayBase<T>, ()>?

^{components/script/dom/bindings/typedarray.rs, line 159 [r1] (raw file):}
This is unsafe; it may invoke UB by creating two objects from the same JS object.

^{components/script/dom/bindings/typedarray.rs, line 168 [r1] (raw file):}
Any reason not to do this in new()?

^{components/script/dom/bindings/typedarray.rs, line 180 [r1] (raw file):}
Limit to T: TypedArrayElement?

^{components/script/dom/bindings/typedarray.rs, line 250 [r1] (raw file):}
GC hazard?

^{components/script/dom/bindings/typedarray.rs, line 292 [r1] (raw file):}
Do we need the macro and trait?

^{components/script/dom/bindings/typedarray.rs, line 331 [r1] (raw file):}
Ditto

^{components/script/dom/bindings/typedarray.rs, line 358 [r1] (raw file):}
Does this cast adjust the length?

^{components/script/dom/bindings/typedarray.rs, line 364 [r1] (raw file):}
Ditto for length and UB

^{components/script/dom/imagedata.rs, line 51 [r2] (raw file):}
Followup to remove the argument?

^{components/script/dom/imagedata.rs, line 54 [r2] (raw file):}
[]

Comments from the review on Reviewable.io

jdm · 2015-07-27T02:06:27Z

The rooting can't live there, unfortunately, due to its reliance on the whole JSReflectable trait stuff and RootedJSTraceableSet.

Ms2ger · 2015-07-27T08:30:40Z

It probably wouldn't be a good idea to move everything except the rooting there. Maybe it could make sense to move most of trace.rs to rust-mozjs as well, though…

jdm · 2015-07-27T20:44:45Z

Review status: 2 of 9 files reviewed at latest revision, 13 unresolved discussions, all commit checks successful.

^{components/script/dom/bindings/typedarray.rs, line 47 [r1] (raw file):}
Correct. Private type exposed in public type bounds etc.

^{components/script/dom/bindings/typedarray.rs, line 168 [r1] (raw file):}
Consider void setValueCurveAtTime(Float32Array values, double startTime);, and code like setValueCurveAtTime(some_float32_array, /* object with getter that neuters some_float32_array */);
When this constructor moves into the bindings and the DOM methods are passed value: &Float32Array, its buffer pointer has been silently invalidated by startTime's getter and we'll now have a UAF.

^{components/script/dom/bindings/typedarray.rs, line 292 [r1] (raw file):}
It can be used in the future for SharedArrayBufferViews.

Comments from the review on Reviewable.io

bors-servo · 2015-07-28T16:46:14Z

☔ The latest upstream changes (presumably #6699) made this pull request unmergeable. Please resolve the merge conflicts.

Ms2ger · 2015-07-30T08:22:05Z

Reviewed 1 of 1 files at r5, 6 of 6 files at r6, 6 of 6 files at r7.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

^{components/script/dom/bindings/typedarray.rs, line 110 [r7] (raw file):}
Replace by Option<(*mut _, u32)>?

^{components/script/dom/bindings/typedarray.rs, line 168 [r1] (raw file):}
Bah. How about we make these types entirely opaque, and give them a method that returns a more transparent struct in a Result?

^{components/script/dom/bindings/typedarray.rs, line 188 [r6] (raw file):}
Can't tenacious enforce that?

^{components/script/dom/bindings/typedarray.rs, line 442 [r6] (raw file):}
No need for the parens

Comments from the review on Reviewable.io

jdm · 2015-07-30T11:16:03Z

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.

^{components/script/dom/bindings/typedarray.rs, line 166 [r1] (raw file):}
That's a really good idea.

^{components/script/dom/bindings/typedarray.rs, line 189 [r6] (raw file):}
Paging @Manishearth

Comments from the review on Reviewable.io

…Fixes servo#6602. Fixes servo#6601.

Manishearth · 2015-07-30T15:25:16Z

Review status: 2 of 9 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

^{components/script/dom/bindings/typedarray.rs, line 194 [r6] (raw file):}
Yes, but keep the assert there too.

Comments from the review on Reviewable.io

jdm · 2015-07-30T15:52:20Z

Review status: 2 of 9 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

^{components/script/dom/bindings/typedarray.rs, line 194 [r6] (raw file):}
I looked into this, and I'd prefer not to rely on tenacious since it requires introducing another intermediary type:

11:48 <jdm> but that makes the client code pretty clunky
11:48 <jdm> let rooter = TypedArrayRooter::new();
11:48 <jdm> let typed_array = TypedArray::from(foo, &mut rooter);
11:48 <jdm> let inited_typed_array = typed_array.init();
11:48 <jdm> let buffer = inited_typed_array.extract();
11:48 <jdm> buffer.as_slice() //woohoo an actual piece of data!

Comments from the review on Reviewable.io

jdm · 2015-07-30T19:06:47Z

@bzbarsky Do you know why the Gecko code uses JS_CallUnbarrieredObjectTracer?

bzbarsky · 2015-07-30T19:47:07Z

Probably because I was told to do that by terrence? Note that this is only in cases when we're dealing with a stack-based autorooter, right?

jdm · 2015-07-30T20:07:15Z

Yeah, that's the conclusion that mwu came to as well. We may need to do something different here, since apparently our RootedTraceableSet has some different characteristics from the CustomAutoRooter stuff.

Ms2ger · 2015-08-03T12:56:19Z

Reviewed 5 of 7 files at r8.
Review status: 7 of 9 files reviewed at latest revision, 5 unresolved discussions, all commit checks successful.

^{components/script/dom/bindings/typedarray.rs, line 182 [r8] (raw file):}
I think we should be able to avoid this unwrap.

^{components/script/dom/bindings/typedarray.rs, line 385 [r8] (raw file):}
Missing space

Comments from the review on Reviewable.io

…h, and make that operation infallible.

jdm · 2015-08-03T14:54:02Z

Updated.

jdm · 2015-08-04T23:21:18Z

Note: due to the concerns about unbarriered tracing, I'm intending to implement equivalent support for SM's CustomAutoRooter instead.

bors-servo · 2015-08-05T18:51:20Z

☔ The latest upstream changes (presumably #6974) made this pull request unmergeable. Please resolve the merge conflicts.

cc servo#6779 (this adds an unsafe construction of a typed array)

…ary. There is one mangling-related removal (of va_args-related functions), but these were never called and I'm not convinced they would have worked on any platform except for Linux 64 bit.

frewsxcv · 2015-11-15T00:28:37Z

@jdm Any updates on this?

jdm · 2015-11-15T00:35:29Z

Not since my related mailing list post. No particular point in leaving this open.

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jul 26, 2015

Ms2ger self-assigned this Jul 26, 2015

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 27, 2015

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels Jul 27, 2015

jdm added the S-needs-rebase There are merge conflict errors. label Jul 28, 2015

jdm added 7 commits July 30, 2015 07:35

Port Gecko's TypedArray.h API to Rust.

7beea80

Convert ImageData to use safe typed array API.

1cff7a5

Replace unsafe code in WebGLRenderingContext with new typed array API. …

b4633ed

…Fixes servo#6602. Fixes servo#6601.

Replace unsafe JSAPI in Crypto with the new typed array APIs.

bdb09d1

Replace unsafe JSAPI in TextDecoder with the new typed array APIs.

504de81

Make the typed array wrappers GC-safe by introducing stack rooting.

53ecd52

Address review comments.

82ee22c

jdm force-pushed the typedarray2 branch from 5b0eb67 to d8b3366 Compare July 30, 2015 13:40

jdm removed the S-needs-rebase There are merge conflict errors. label Jul 30, 2015

Make it impossible to get a usable buffer without computing the lengt…

0fd2085

…h, and make that operation infallible.

jdm force-pushed the typedarray2 branch from d8b3366 to 0fd2085 Compare August 3, 2015 14:51

tomjakubowski mentioned this pull request Aug 13, 2015

Implement WebSocket#binaryType. #7098

Closed

jdm added S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. and removed S-awaiting-review There is new code that needs to be reviewed. labels Aug 14, 2015

tomjakubowski added a commit to tomjakubowski/servo that referenced this pull request Aug 14, 2015

respect WebSocket BinaryType in message event data

ccc2c3e

cc servo#6779 (this adds an unsafe construction of a typed array)

tomjakubowski added a commit to tomjakubowski/servo that referenced this pull request Aug 14, 2015

respect WebSocket BinaryType in message event data

a3541f5

cc servo#6779 (this adds an unsafe construction of a typed array)

tomjakubowski added a commit to tomjakubowski/servo that referenced this pull request Aug 14, 2015

respect WebSocket binaryType in message event data

36caea2

cc servo#6779 (this adds an unsafe construction of a typed array)

nox added S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors. labels Sep 14, 2015

jdm closed this Nov 15, 2015

emilio mentioned this pull request Jan 5, 2016

webgl: Make a general way to get data from a JS array buffer view #8970

Merged

jdm mentioned this pull request Feb 15, 2016

Implement bindings for WebIDL typed arrays #5014

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a high-level API for JS typed arrays #6779

Add a high-level API for JS typed arrays #6779

jdm commented Jul 26, 2015

highfive commented Jul 26, 2015

jdm commented Jul 26, 2015

Ms2ger commented Jul 26, 2015

jdm commented Jul 27, 2015

Ms2ger commented Jul 27, 2015

jdm commented Jul 27, 2015

bors-servo commented Jul 28, 2015

Ms2ger commented Jul 30, 2015

jdm commented Jul 30, 2015

Manishearth commented Jul 30, 2015

jdm commented Jul 30, 2015

jdm commented Jul 30, 2015

bzbarsky commented Jul 30, 2015

jdm commented Jul 30, 2015

Ms2ger commented Aug 3, 2015

jdm commented Aug 3, 2015

jdm commented Aug 4, 2015

bors-servo commented Aug 5, 2015

frewsxcv commented Nov 15, 2015

jdm commented Nov 15, 2015

Add a high-level API for JS typed arrays #6779

Add a high-level API for JS typed arrays #6779

Conversation

jdm commented Jul 26, 2015

highfive commented Jul 26, 2015

jdm commented Jul 26, 2015

Ms2ger commented Jul 26, 2015

jdm commented Jul 27, 2015

Ms2ger commented Jul 27, 2015

jdm commented Jul 27, 2015

bors-servo commented Jul 28, 2015

Ms2ger commented Jul 30, 2015

jdm commented Jul 30, 2015

Manishearth commented Jul 30, 2015

jdm commented Jul 30, 2015

jdm commented Jul 30, 2015

bzbarsky commented Jul 30, 2015

jdm commented Jul 30, 2015

Ms2ger commented Aug 3, 2015

jdm commented Aug 3, 2015

jdm commented Aug 4, 2015

bors-servo commented Aug 5, 2015

frewsxcv commented Nov 15, 2015

jdm commented Nov 15, 2015