Implemented FileReader::readAsArrayBuffer #13729

nikhilshagri · 2016-10-12T18:02:42Z

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Implement FileReader::readAsArrayBuffer #12555

There are tests for these changes

There's still some small issues, but I suppose most of the work is done:

test-tidy mentions a method declared in webidl is missing a comment with a specification link for the getResult method.
I get an 'unused code' warning for code present in UnionTypes.rs, which is auto-generated.

Passing tests:

FileAPI/reading-data-section/filereader_result.html
FileAPI/reading-data-section/filereader_readAsArrayBuffer.html
FileAPI/idlharness.html
FileAPI/reading-data-section/FileReader-multiple-reads.html

This change is

highfive · 2016-10-12T18:02:45Z

Heads up! This PR modifies the following files:

@KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/dom/webidls/FileReader.webidl, components/script/dom/filereader.rs

highfive · 2016-10-12T18:02:47Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

nikhilshagri

Not a review, but just some points I wanted to mention.

nikhilshagri

Forgot to add the comments;

nikhilshagri

There! hopefully this will work...

nikhilshagri · 2016-10-12T18:12:08Z

components/script/dom/filereader.rs

+
+    // https://w3c.github.io/FileAPI/#dfn-readAsArrayBuffer
+    #[allow(unsafe_code, unused_variables)]
+    fn perform_readasarraybuffer(cx: *mut JSContext, data: ReadMetaData, bytes: &[u8])


There might be more than a few logical errors in this function, as I haven't asked anyone to look at this piece of code since the compiler didn't complain.

nikhilshagri · 2016-10-12T18:12:15Z

components/script/dom/filereader.rs

+        -> StringOrObject {
+        unsafe {
+            let length = bytes.len() as u32;
+            let newArrayBuffer = JS_NewArrayBuffer(cx, length);


Both FileAPI/reading-data-section/filereader_readAsArrayBuffer.html and FileAPI/reading-data-section/filereader_result.html crash at this point. There's very little information in the log to debug it, it only shows a stack trace. I still have to test for additional tests which have been mentioned in the issue.

We need to create a JSAutoCompartment value in this method before calling JS_NewArrayBuffer.

nikhilshagri · 2016-10-12T18:12:20Z

components/script/dom/filereader.rs

+        if let Some(FileReaderResult::ArrayBuffer_(ref heap)) = *fr.result.borrow() {
+            if let StringOrObject::Object(obj) = output {
+                unsafe {
+                    let tempr = heap.get_unsafe();


This can be done in a much better manner, but I don't know how.

Use heap.set instead.

I had tried that, but I throws a cannot borrow immutable borrowed content *heap as mutable error. Trying that with a mutable reference throws a cannot borrow anonymous inner field as mutable error.

Oh, use borrow_mut.

I don't understand....heap doesn't have a borrow_mut method.

*fr.result.borrow_mut() and ref mut heap should work :)

@emilio it works. Thanks!

jdm · 2016-10-12T18:37:13Z

I suspect the crash is caused by a missing JS compartment. Try adding let _ac = JSAutoCompartment::new(fr.global().r().get_cx(), &*fr.reflector().get_jsobject()); before the call to perform_readasarraybuffer?

jdm · 2016-10-14T21:13:52Z

components/script/dom/bindings/codegen/CodegenRust.py

@@ -4191,7 +4197,7 @@ def get_match(name):

        hasObjectTypes = interfaceObject or arrayObject or dateObject or nonPlatformObject or object or mozMapObject
        if hasObjectTypes:
-            assert interfaceObject or arrayObject or mozMapObject
+            assert interfaceObject or arrayObject or mozMapObject or object


Let's update this to match Gecko's Codegen.py:

# "object" is not distinguishable from other types assert not object or not (interfaceObject or arrayObject or dateObject or callbackObject or mozMapObject)

jdm · 2016-10-14T21:14:47Z

components/script/dom/filereader.rs

+#[derive(HeapSizeOf, JSTraceable)]
+pub enum FileReaderResult {
+    ArrayBuffer_(Heap<JSVal>),
+    String_(DOMString),


Let's remove the _s from the names here.

jdm · 2016-10-14T21:15:11Z

components/script/dom/filereader.rs

@@ -159,6 +173,7 @@ impl FileReader {
    }

    // https://w3c.github.io/FileAPI/#dfn-readAsText
+    #[allow(unsafe_code, unused_variables)]


Let's get rid of the unused_variables.

jdm · 2016-10-14T21:17:52Z

components/script/dom/filereader.rs

@@ -176,14 +191,26 @@ impl FileReader {
        fr.change_ready_state(FileReaderReadyState::Done);
        // Step 8.2

-        let output = match data.function {
+        let output: StringOrObject = match data.function {


I have an idea that will lead to cleaner code - let's make the perform_foo methods take a &self argument, and have them store the value in self.result, rather than returning it.

jdm · 2016-10-14T21:18:51Z

components/script/dom/filereader.rs

+        if let Some(FileReaderResult::ArrayBuffer_(ref heap)) = *fr.result.borrow() {
+            if let StringOrObject::Object(obj) = output {
+                unsafe {
+                    let tempr = heap.get_unsafe();


Use heap.set instead.

jdm · 2016-10-14T21:19:50Z

components/script/dom/filereader.rs

+        -> StringOrObject {
+        unsafe {
+            let length = bytes.len() as u32;
+            let newArrayBuffer = JS_NewArrayBuffer(cx, length);


We need to create a JSAutoCompartment value in this method before calling JS_NewArrayBuffer.

jdm · 2016-10-14T21:22:56Z

components/script/dom/filereader.rs

+        -> StringOrObject {
+        unsafe {
+            let length = bytes.len() as u32;
+            let newArrayBuffer = JS_NewArrayBuffer(cx, length);


Let's use the new safe typed array APIs implemented in servo/rust-mozjs#304 . See the tests added there for examples.

I can't figure what the fourth parameter in create should be... should I use the same macro to create it as specified in the tests or something else?

Something like

rooted!(in(cx) let mut buffer = ptr::null_mut()); assert!(Uint32Array::create(cx, length, Some(bytes), buffer.handle_mut()).is_ok()); StringOrObject::Object(buffer.get())

jdm · 2016-10-14T21:23:34Z

components/script/dom/filereader.rs

@@ -303,8 +355,19 @@ impl FileReaderMethods for FileReader {
    }

    // https://w3c.github.io/FileAPI/#dfn-result
-    fn GetResult(&self) -> Option<DOMString> {
-        self.result.borrow().clone()
+    #[allow(unsafe_code, unused_variables)]


unused_variables

jdm · 2016-10-14T21:25:32Z

components/script/dom/filereader.rs

-        self.result.borrow().clone()
+    #[allow(unsafe_code, unused_variables)]
+    fn GetResult(&self, cx: *mut JSContext) -> Option<StringOrObject> {
+        match *self.result.borrow() {


self.result.borrow().as_ref().map(|r| match r { FileReaderResult::String(string) => StringOrObject::String(string.clone()), FileReaderResult::ArrayBuffer(arr_buffer) => { ... } })

jdm · 2016-10-14T21:25:55Z

components/script/dom/webidls/FileReader.webidl

-  //readonly attribute FileReaderResult? result;
-  readonly attribute DOMString? result;
+  readonly attribute FileReaderResult? result;
+  //readonly attribute DOMString? result;


We can remove this now.

nikhilshagri · 2016-10-24T23:26:31Z

I implemented all the requested changes as a temporary commit so that it's easier to view the changes since the first commit. It would be a good idea to bors-servo: try this PR if there are no more changes, because there are some failing tests and we can have a record of them here.

KiChjang · 2016-10-27T21:48:35Z

@bors-servo try

bors-servo · 2016-10-27T21:48:38Z

⌛ Trying commit d8c6656 with merge ca2e93f...

Implemented FileReader::readAsArrayBuffer  ---  - [X] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #12555  - [X] There are tests for these changes There's still some small issues, but I suppose most of the work is done: - test-tidy mentions a `method declared in webidl is missing a comment with a specification link` for the `getResult` method. - I get an 'unused code' warning for code present in `UnionTypes.rs`, which is auto-generated. Passing tests: - [x] `FileAPI/reading-data-section/filereader_result.html` - [x] `FileAPI/reading-data-section/filereader_readAsArrayBuffer.html` - [x] `FileAPI/idlharness.html` - [ ] `FileAPI/reading-data-section/FileReader-multiple-reads.html`   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13729)

bors-servo · 2016-10-27T22:19:48Z

💔 Test failed - linux-rel-wpt

KiChjang · 2016-10-27T22:25:28Z

@bors-servo retry

homu throws a fit

bors-servo · 2016-10-27T22:25:31Z

⌛ Trying commit d8c6656 with merge adbf85b...

Implemented FileReader::readAsArrayBuffer  ---  - [X] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #12555  - [X] There are tests for these changes There's still some small issues, but I suppose most of the work is done: - test-tidy mentions a `method declared in webidl is missing a comment with a specification link` for the `getResult` method. - I get an 'unused code' warning for code present in `UnionTypes.rs`, which is auto-generated. Passing tests: - [x] `FileAPI/reading-data-section/filereader_result.html` - [x] `FileAPI/reading-data-section/filereader_readAsArrayBuffer.html` - [x] `FileAPI/idlharness.html` - [ ] `FileAPI/reading-data-section/FileReader-multiple-reads.html`   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13729)

bors-servo · 2016-10-27T22:59:39Z

💔 Test failed - linux-rel-wpt

jdm · 2016-10-28T04:14:39Z

@bors-servo: retry

bors-servo · 2016-10-28T04:14:41Z

⌛ Trying commit d8c6656 with merge b262a94...

Implemented FileReader::readAsArrayBuffer  ---  - [X] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #12555  - [X] There are tests for these changes There's still some small issues, but I suppose most of the work is done: - test-tidy mentions a `method declared in webidl is missing a comment with a specification link` for the `getResult` method. - I get an 'unused code' warning for code present in `UnionTypes.rs`, which is auto-generated. Passing tests: - [x] `FileAPI/reading-data-section/filereader_result.html` - [x] `FileAPI/reading-data-section/filereader_readAsArrayBuffer.html` - [x] `FileAPI/idlharness.html` - [ ] `FileAPI/reading-data-section/FileReader-multiple-reads.html`   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13729)

nikhilshagri · 2016-10-30T16:15:12Z

@bors-servo retry

see Intermittent failure in /css21_dev/html4/abs-pos-non-replaced-vlr-137.htm , /css21_dev/html4/abs-pos-non-replaced-vrl-136.htm, /css21_dev/html4/abs-pos-non-replaced-vrl-144.htm, /css21_dev/html4/abs-pos-non-replaced-vlr-149.htm, /css21_dev/html4/abs-pos-non-replaced-vlr-033.htm, /css21_dev/html4/abs-pos-non-replaced-vrl-064.htm, /css21_dev/html4/abs-pos-non-replaced-vlr-065.htm, /css21_dev/html4/abs-pos-non-replaced-vlr-145.htm #13919

bors-servo · 2016-10-30T16:15:15Z

⌛ Trying commit c39939b with merge b79f881...

Implemented FileReader::readAsArrayBuffer  ---  - [X] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #12555  - [X] There are tests for these changes There's still some small issues, but I suppose most of the work is done: - test-tidy mentions a `method declared in webidl is missing a comment with a specification link` for the `getResult` method. - I get an 'unused code' warning for code present in `UnionTypes.rs`, which is auto-generated. Passing tests: - [x] `FileAPI/reading-data-section/filereader_result.html` - [x] `FileAPI/reading-data-section/filereader_readAsArrayBuffer.html` - [x] `FileAPI/idlharness.html` - [ ] `FileAPI/reading-data-section/FileReader-multiple-reads.html`   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13729)

bors-servo · 2016-10-30T16:57:48Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

jdm · 2016-10-30T17:18:25Z

@cynicaldevil Generally it's not necessary to re-run try runs if the reported failures are all known intermittent ones.

nikhilshagri · 2016-10-30T17:30:05Z

@jdm Does homu stop running after the first encountered error, or does it run the build on all platforms and then report all errors at once?

KiChjang · 2016-10-30T19:47:15Z

@cynicaldevil Homu runs on all platforms, but only displays error results from one.

nikhilshagri · 2016-10-31T07:10:15Z

I see. So what now? Is the PR ready to be merged?

KiChjang · 2016-10-31T09:14:58Z

If you're ever in doubt about the status of a PR, the purple labels should help you understand. Right now it says S-awaiting-review, and your reviewer is @jdm.

Ms2ger · 2016-10-31T15:06:36Z

r=me once you squash all commits together. Thanks!

Stealing review

nikhilshagri · 2016-10-31T18:59:41Z

@Ms2ger Done!

KiChjang · 2016-10-31T19:01:16Z

@bors-servo r=Ms2ger

bors-servo · 2016-10-31T19:01:17Z

📌 Commit 51ef05b has been approved by Ms2ger

bors-servo · 2016-10-31T19:01:19Z

⌛ Testing commit 51ef05b with merge ceb18e7...

Implemented FileReader::readAsArrayBuffer  ---  - [X] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #12555  - [X] There are tests for these changes There's still some small issues, but I suppose most of the work is done: - test-tidy mentions a `method declared in webidl is missing a comment with a specification link` for the `getResult` method. - I get an 'unused code' warning for code present in `UnionTypes.rs`, which is auto-generated. Passing tests: - [x] `FileAPI/reading-data-section/filereader_result.html` - [x] `FileAPI/reading-data-section/filereader_readAsArrayBuffer.html` - [x] `FileAPI/idlharness.html` - [ ] `FileAPI/reading-data-section/FileReader-multiple-reads.html`   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/13729)

bors-servo · 2016-10-31T19:58:01Z

☀️ Test successful - arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-dev

highfive assigned jdm Oct 12, 2016

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 12, 2016

nikhilshagri commented Oct 12, 2016

View reviewed changes

jdm previously requested changes Oct 14, 2016

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 Oct 14, 2016

emilio mentioned this pull request Oct 20, 2016

Integrate highfive with GitHub's reviews. servo/highfive#160

Open

nikhilshagri force-pushed the readAsArrayBuffer branch from 7fbf9b0 to 23def27 Compare October 24, 2016 22:15

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 Oct 24, 2016

nikhilshagri force-pushed the readAsArrayBuffer branch from 23def27 to d8c6656 Compare October 24, 2016 23:23

highfive added the S-tests-failed The changes caused existing tests to fail. label Oct 27, 2016

Ms2ger added S-needs-squash Some (or all) of the commits in the PR should be combined. and removed S-awaiting-review There is new code that needs to be reviewed. labels Oct 31, 2016

Ms2ger assigned Ms2ger and unassigned jdm Oct 31, 2016

Implemented FileReader::readAsArrayBuffer

51ef05b

nikhilshagri force-pushed the readAsArrayBuffer branch from c39939b to 51ef05b Compare October 31, 2016 18:56

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Oct 31, 2016

highfive removed the S-awaiting-review There is new code that needs to be reviewed. label Oct 31, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-needs-squash Some (or all) of the commits in the PR should be combined. labels Oct 31, 2016

bors-servo merged commit 51ef05b into servo:master Oct 31, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Oct 31, 2016

servo deleted a comment from AppleMango23 Feb 1, 2019

Implemented FileReader::readAsArrayBuffer #13729

Implemented FileReader::readAsArrayBuffer #13729

Conversation

nikhilshagri commented Oct 12, 2016 • edited

highfive commented Oct 12, 2016

highfive commented Oct 12, 2016

nikhilshagri left a comment

Choose a reason for hiding this comment

nikhilshagri left a comment

Choose a reason for hiding this comment

nikhilshagri 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

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

jdm commented Oct 12, 2016

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

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

nikhilshagri commented Oct 24, 2016

KiChjang commented Oct 27, 2016

bors-servo commented Oct 27, 2016

bors-servo commented Oct 27, 2016

KiChjang commented Oct 27, 2016

bors-servo commented Oct 27, 2016

bors-servo commented Oct 27, 2016

jdm commented Oct 28, 2016

bors-servo commented Oct 28, 2016

nikhilshagri commented Oct 30, 2016

bors-servo commented Oct 30, 2016

bors-servo commented Oct 30, 2016

jdm commented Oct 30, 2016

nikhilshagri commented Oct 30, 2016

KiChjang commented Oct 30, 2016 • edited

nikhilshagri commented Oct 31, 2016

KiChjang commented Oct 31, 2016

Ms2ger commented Oct 31, 2016

nikhilshagri commented Oct 31, 2016

KiChjang commented Oct 31, 2016

bors-servo commented Oct 31, 2016

bors-servo commented Oct 31, 2016

bors-servo commented Oct 31, 2016

nikhilshagri commented Oct 12, 2016 •

edited

KiChjang commented Oct 30, 2016 •

edited