feat(fetch): accept arraybuffer in consume_body #20406

kwonoj · 2018-03-23T22:40:27Z

Related to #20346.

I realized I am not sufficiently knowledgeable about codebases and have high confidence this PR is not ready to be accepted. Raising it as PR early to possibly ask some suggestions around codebases.

If this PR seems unrecoverable by code review, please feel freely close and unassign me from issue 🙏

This PR tries to implement #20346, updating Body idl and implements corresponding implementation in body.rs for fetch. Criteria for changes may includes

does run_array_buffer_data_algorithm implementation is legit for allocating arraybuffer? (probably not)
does run_array_buffer_data_algorithm implementation is acceptable for handling error, by naively returning Error::JSFailed?
there are some number of wpt test started to PASS with this PR. Is this legit side effect, or something incorrect by current implementation?
etcs, vice versa

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Implement the arraybuffer API for Fetch bodies #20346 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____
wpt test has changed in PR, need to be reviewed.

This change is

jdm

Your code was doing the right thing, just not in the most efficient way. All of the tests that now pass are tests that were either directly testing the arrayBuffer() behaviour or relying on arrayBuffer() to test the behaviour of the Fetch API, so this is a great result!

jdm · 2018-03-24T05:07:04Z

components/script/body.rs

 }

 pub enum FetchedData {
    Text(String),
    Json(JSValue),
    BlobData(DomRoot<Blob>),
    FormData(DomRoot<FormData>),
+    ArrayBuffer(Heap<JSVal>),


To ensure this code is GC-safe, we should use RootedTraceableBox<Heap<JSVal>>, and use it for the Json variant as well.

jdm · 2018-03-24T05:07:14Z

components/script/body.rs

 }

 // https://fetch.spec.whatwg.org/#concept-body-consume-body
 #[allow(unrooted_must_root)]
 pub fn consume_body<T: BodyOperations + DomObject>(object: &T, body_type: BodyType) -> Rc<Promise> {
    let promise = Promise::new(&object.global());

+


nit: remove this blank line.

jdm · 2018-03-24T05:07:41Z

components/script/body.rs

@@ -166,6 +176,26 @@ fn run_form_data_algorithm(root: &GlobalScope, bytes: Vec<u8>, mime: &[u8]) -> F
    }
 }

+#[allow(unsafe_code)]
+fn run_array_buffer_data_algorithm(cx: *mut JSContext, bytes: Vec<u8>) -> Fallible<FetchedData> {


This function should be marked unsafe, since it accepts a raw pointer.

I've specified unsafe scope here as similar to other places - mainly cause specifying fn itself as unsafe will raises compilation error as unsafe invocation is propagated in line 116.

New errors are fine; we can add #[allow(unsafe_code)] on the containing functions. We want to avoid declaring functions as safe when it is trivially possible to pass an invalid pointer that would trigger a crash.

jdm · 2018-03-24T05:14:06Z

components/script/body.rs

+    unsafe {
+        match ArrayBuffer::create(cx, CreateWith::Slice(&bytes), array_buffer_ptr.handle_mut()) {
+            Ok(_) => {
+                let mut arraybuffer = Some(FetchedData::ArrayBuffer(Heap::default()));


This part is a bit jumbled, but the basic idea is correct. What about:

rooted!(in(cx) let mut array_buffer_ptr = ptr::null_mut::<JSObject>()); let arraybuffer = ArrayBuffer::create(cx, CreateWith::Slice(&bytes), array_buffer_ptr.handle_mut()); if arraybuffer.is_err() { return Err(Error::JSFailed); } let rooted_heap = RootedTraceableBox::from_box(Heap::boxed(array_buffer_ptr.get())); Ok(FetchedData::ArrayBuffer(rooted_heap))

kwonoj

@jdm Appreciate for taking time to review! 🙇‍♂️ I tried to update code as much as suggested, and left few question / comments around changes.

kwonoj · 2018-03-24T06:53:33Z

components/script/body.rs

@@ -166,6 +176,26 @@ fn run_form_data_algorithm(root: &GlobalScope, bytes: Vec<u8>, mime: &[u8]) -> F
    }
 }

+#[allow(unsafe_code)]
+fn run_array_buffer_data_algorithm(cx: *mut JSContext, bytes: Vec<u8>) -> Fallible<FetchedData> {


I've specified unsafe scope here as similar to other places - mainly cause specifying fn itself as unsafe will raises compilation error as unsafe invocation is propagated in line 116.

kwonoj · 2018-03-24T06:56:11Z

components/script/body.rs

+        if arraybuffer.is_err() {
+            return Err(Error::JSFailed);
+        }
+        let rooted_heap = RootedTraceableBox::from_box(Heap::boxed(jsval::ObjectValue(array_buffer_ptr.get())));


appreciate for suggestion and tried to update code as suggested, except creating boxed heap using ObjectValue - since type of ArrayBuffer is RootedTraceableBox<Heap<JSVal>>, using ptr directly creates RootedTraceableBox<Heap<mut *JSObject>>.

Oh, let's make it a bit cleaner then - store RootedTraceableBox<Heap<*mut JSObject>> and get rid of the jsval::ObjectValue.

kwonoj · 2018-03-24T06:59:55Z

components/script/body.rs

 }

 pub enum FetchedData {
    Text(String),
    Json(JSValue),
    BlobData(DomRoot<Blob>),
    FormData(DomRoot<FormData>),
+    ArrayBuffer(RootedTraceableBox<Heap<JSVal>>),


I'm bit unsure if I understood #20406 (comment) fully around use it for the Json variant as well. - would you mind elaborate bit more?

I mean that the existing Json(JSValue) is similarly GC-unsafe, and it should store a RootedTraceableBox<Heap<JSVal>> instead.

kwonoj

@jdm now type for Arraybuffer updated to Heap<*mut JSObect>, also added commit for updating types for JSON as well.

kwonoj · 2018-03-24T15:59:50Z

components/script/body.rs

 }

 pub enum FetchedData {
    Text(String),
-    Json(JSValue),
+    Json(RootedTraceableBox<Heap<JSValue>>),


Updated Json to store RootedTraceableBox<Heap> as suggested. Still it stores JSValue instead of JSVal, by JSVal doesn't meet trait bounds for RootedTraceableBox.

kwonoj · 2018-03-24T16:00:18Z

components/script/body.rs

@@ -104,6 +112,9 @@ fn run_package_data_algorithm<T: BodyOperations + DomObject>(object: &T,
        BodyType::Json => run_json_data_algorithm(cx, bytes),
        BodyType::Blob => run_blob_data_algorithm(&global, bytes, mime),
        BodyType::FormData => run_form_data_algorithm(&global, bytes, mime),
+        BodyType::ArrayBuffer => unsafe {
+            run_array_buffer_data_algorithm(cx, bytes)


now run_array_buffer_data_algorithm is declared as unsafe fn, and this match arm uses unsafe scope.

jdm · 2018-03-24T16:24:55Z

@bors-servo r+
Thanks!

bors-servo · 2018-03-24T16:24:56Z

📌 Commit 80304a1 has been approved by jdm

KiChjang · 2018-03-24T16:42:00Z

tests/wpt/metadata/fetch/api/response/response-stream-disturbed-5.html.ini

@@ -1,5 +1,3 @@
 [response-stream-disturbed-5.html]
  type: testharness
-  [Getting a body reader after consuming as arrayBuffer]
-    expected: FAIL


Shouldn't this file be deleted altogether?

It will be automatically in the future.

bors-servo · 2018-03-24T17:46:22Z

⌛ Testing commit 80304a1 with merge 23b2f42...

feat(fetch): accept arraybuffer in consume_body  Related to #20346. I realized I am not sufficiently knowledgeable about codebases and have high confidence this PR is not ready to be accepted. Raising it as PR early to possibly ask some suggestions around codebases. If this PR seems unrecoverable by code review, please feel freely close and unassign me from issue 🙏 This PR tries to implement #20346, updating `Body` idl and implements corresponding implementation in `body.rs` for `fetch`. Criteria for changes may includes - does `run_array_buffer_data_algorithm` implementation is legit for allocating arraybuffer? (probably not) - does `run_array_buffer_data_algorithm` implementation is acceptable for handling error, by naively returning `Error::JSFailed`? - there are some number of wpt test started to PASS with this PR. Is this legit side effect, or something incorrect by current implementation? - etcs, vice versa ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20346 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ - wpt test has changed in PR, need to be reviewed.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20406)

bors-servo · 2018-03-24T18:34:10Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 23b2f42 to master...

jdm added the S-awaiting-review There is new code that needs to be reviewed. label Mar 24, 2018

jdm self-assigned this Mar 24, 2018

jdm requested changes Mar 24, 2018

View reviewed changes

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 Mar 24, 2018

feat(webidl): expose arraybuffer overload in body idl

c0b5eee

kwonoj force-pushed the feat-fetch-body-arraybuffer branch from 2a12369 to f2b0cd4 Compare March 24, 2018 06:44

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 Mar 24, 2018

kwonoj commented Mar 24, 2018

View reviewed changes

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 Mar 24, 2018

kwonoj added 3 commits March 24, 2018 08:33

feat(consume_body): implement consume_body accepts arraybuffer

168f610

refactor(consume_body): json stored in roottraceablebox

e6de213

test(wpt): update fetch test metadata

80304a1

kwonoj force-pushed the feat-fetch-body-arraybuffer branch from f2b0cd4 to 80304a1 Compare March 24, 2018 15:57

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 Mar 24, 2018

kwonoj commented Mar 24, 2018

View reviewed changes

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 24, 2018

KiChjang reviewed Mar 24, 2018

View reviewed changes

bors-servo merged commit 80304a1 into servo:master Mar 24, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 24, 2018

kwonoj deleted the feat-fetch-body-arraybuffer branch March 27, 2018 05:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(fetch): accept arraybuffer in consume_body #20406

feat(fetch): accept arraybuffer in consume_body #20406

kwonoj commented Mar 23, 2018 •

edited by SimonSapin

jdm left a comment

jdm Mar 24, 2018

jdm Mar 24, 2018

jdm Mar 24, 2018

kwonoj Mar 24, 2018

jdm Mar 24, 2018

jdm Mar 24, 2018

kwonoj left a comment

kwonoj Mar 24, 2018

kwonoj Mar 24, 2018

jdm Mar 24, 2018

kwonoj Mar 24, 2018

jdm Mar 24, 2018

kwonoj left a comment

kwonoj Mar 24, 2018

kwonoj Mar 24, 2018

jdm commented Mar 24, 2018

bors-servo commented Mar 24, 2018

KiChjang Mar 24, 2018

jdm Mar 24, 2018

bors-servo commented Mar 24, 2018

bors-servo commented Mar 24, 2018

feat(fetch): accept arraybuffer in consume_body #20406

feat(fetch): accept arraybuffer in consume_body #20406

Conversation

kwonoj commented Mar 23, 2018 • edited by SimonSapin

jdm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwonoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwonoj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdm commented Mar 24, 2018

bors-servo commented Mar 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Mar 24, 2018

bors-servo commented Mar 24, 2018

kwonoj commented Mar 23, 2018 •

edited by SimonSapin