Implement FileReader.readAsArrayBuffer()

RESOLVED FIXED in mozilla7

Status

()

Core
DOM: Events
--
enhancement
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: pcwalton, Assigned: Shawn Gong)

Tracking

({dev-doc-complete})

unspecified
mozilla7
dev-doc-complete
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [parity-chrome][parity-safari])

Attachments

(1 attachment, 17 obsolete attachments)

21.76 KB, patch
sicking
: review+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
The FileReader interface contains a readAsArrayBuffer() function:

https://developer.mozilla.org/en/DOM/FileReader#readAsArrayBuffer%28%29

It'd be really nice to have this; data URLs are a pain.
Even better would be to implement something based on Dave Hermans binary-data harmony proposal. Not sure if we want to do both (in which case I shouldn't hijack this bug), or if we want to just do one (in which case this bug is probably the place to discuss which we should do).

Updated

7 years ago
Duplicate of this bug: 635274

Comment 3

7 years ago
I've received feedback from colleagues that they would like to see this method implemented in Firefox.

Given our recent meeting about the evolution of the typed array spec, it seems it and the binary data proposal will coexist for some time, so it would be great if support for readAsArrayBuffer could be added to Gecko.
(Assignee)

Updated

6 years ago
Assignee: nobody → sgong
(Assignee)

Comment 4

6 years ago
Created attachment 533697 [details] [diff] [review]
The implementation of readAsArrayBuffer
Attachment #533697 - Flags: review?(jonas)
Comment on attachment 533697 [details] [diff] [review]
The implementation of readAsArrayBuffer

Review of attachment 533697 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDOMFileReader.cpp
@@ +116,5 @@
>  NS_IMPL_CYCLE_COLLECTION_UNLINK_END
>  
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(nsDOMFileReader)
> +  if(tmp->mResultArrayBuffer) {
> +    NS_IMPL_CYCLE_COLLECTION_TRACE_JS_CALLBACK(tmp->mResultArrayBuffer, "mResultArrayBuffer")

Nit: This line exceeds 80 chars.

@@ +174,5 @@
>      mReadyState(nsIDOMFileReader::EMPTY),
>      mProgressEventWasDelayed(PR_FALSE),
>      mTimerIsActive(PR_FALSE),
> +    mReadTotal(0), mReadTransferred(0),
> +    mResultArrayBufferRooted(false)

The order of initialization here is wrong, mResultArrayBufferRooted should be set after mResultArrayBuffer, or change the header to match.

@@ +279,5 @@
> +  if (mDataFormat == FILE_AS_ARRAYBUFFER) {
> +    if (mReadyState == nsIDOMFileReader::DONE) {
> +      *aResult = mResultArrayBuffer ?
> +                 OBJECT_TO_JSVAL(mResultArrayBuffer) :
> +                 JSVAL_NULL;

if readyState is DONE then you must have an array buffer, so no need to check it here.

@@ +508,5 @@
>  
>    nsresult rv = NS_OK;
>    switch (mDataFormat) {
> +    case FILE_AS_ARRAYBUFFER:
> +      // Intentionally fall-through to FILE_AS_BINARY.

This no longer makes sense (didn't reuse the FILE_AS_BINARY implementation). Just replace that with a break line.

@@ +550,5 @@
>    //Implicit abort to clear any other activity going on
>    Abort();
>    mError = nsnull;
>    SetDOMStringToNull(mResult);
> +  mResultArrayBuffer = nsnull;

Abort() will set this to null, so this line is unnecessary.

@@ +590,5 @@
> +  if (mDataFormat == FILE_AS_ARRAYBUFFER) {
> +    RootResultArrayBuffer();
> +    mResultArrayBuffer = js_CreateArrayBuffer(aCx, mReadTotal);
> +    if (!mResultArrayBuffer) {
> +      return NS_ERROR_FAILURE;

I would issue a warning here so that if this ever fails we can notice in the console.

::: content/base/src/nsDOMFileReader.h
@@ +79,5 @@
>  
>    NS_DECL_ISUPPORTS_INHERITED
>  
>    NS_DECL_NSIDOMFILEREADER
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS_INHERITED(nsDOMFileReader, nsXHREventTarget)

Nit: Exceeds 80 chars.

@@ +120,5 @@
>      FILE_AS_TEXT,
>      FILE_AS_DATAURL
>    };
>  
> +  nsresult ReadFileContent(JSContext* aCx, nsIDOMBlob *aFile, const nsAString &aCharset, eDataFormat aDataFormat); 

Nit: Exceeds 80 chars.

::: content/base/test/test_fileapi.html
@@ +28,5 @@
>  } catch (e) {
>    ok(true, "Threw on an unprivileged attempt to construct a File");
>  }
>  
> +const minFileSize = 2000;

I would not make this change, see below.

@@ +374,5 @@
> +    
> +    u8v = new Uint8Array(event.target.result);
> +    for (var i = 0; i < expectedLength; i++) {
> +      is(u8v[i], expectedResult.charCodeAt(i), "array buffer char code");
> +    }

Here, instead of calling is() inside the for loop, just do something like this:

  var ok = true;
  for (var i = 0; i < expectedLength; i++) {
    if (u8v[i] != expectedResult.charCodeAt(i)) {
      ok = false;
    }
  }
  is(ok, true, "array buffer char code");
(Assignee)

Comment 6

6 years ago
Created attachment 533770 [details] [diff] [review]
Changes according to bent's comment
Attachment #533770 - Flags: review?(bent.mozilla)
Comment on attachment 533770 [details] [diff] [review]
Changes according to bent's comment

Looks good to me, over to sicking.
Attachment #533770 - Flags: review?(jonas)
Attachment #533770 - Flags: review?(bent.mozilla)
Attachment #533770 - Flags: review+
(Assignee)

Comment 8

6 years ago
Created attachment 533828 [details] [diff] [review]
Smashed patch of the previous 2
(Assignee)

Updated

6 years ago
Attachment #533828 - Flags: review?(jonas)
Attachment #533697 - Flags: review?(jonas)
Attachment #533770 - Flags: review?(jonas)
Comment on attachment 533828 [details] [diff] [review]
Smashed patch of the previous 2

Review of attachment 533828 [details] [diff] [review]:
-----------------------------------------------------------------

Just small fixes, but would still like to see a final patch.

::: content/base/src/nsDOMFileReader.cpp
@@ +120,5 @@
> +                                               "mResultArrayBuffer")
> +  }
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END
> +
> +

Just one blank line is enough.

@@ +589,5 @@
> +    mResultArrayBuffer = js_CreateArrayBuffer(aCx, mReadTotal);
> +    if (!mResultArrayBuffer) {
> +      NS_WARNING("Failed to create JS array buffer");
> +      return NS_ERROR_FAILURE;
> +    }

Probably safer to call the RootResultArrayBuffer() function only if the js_CreateArrayBuffer call succeeds.

::: content/base/src/nsXMLHttpRequest.cpp
@@ +290,5 @@
>  NS_IMPL_CYCLE_COLLECTION_CLASS(nsXHREventTarget)
>  
>  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN_INHERITED(nsXHREventTarget,
>                                                    nsDOMEventTargetWrapperCache)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS

This seems to be part of another patch. Please revert all the XHR changes.

::: content/base/test/test_fileapi.html
@@ +138,5 @@
>  
> +// Test get result without reading
> +r = new FileReader();
> +is(r.readyState, FileReader.EMPTY,
> +  "readyState in test reader get result without reading");

Why this addition?

@@ +378,5 @@
> +      if (u8v[i] != expectedResult.charCodeAt(i)) {
> +        match = false;
> +      } 
> +    }
> +    is(match, true, "array buffer char code");

You can write this as:

is(event.target.result.byteLength, expectedLength, "array buffer size in test " + testName);
var u8v = new Uint8Array(event.target.result);
is(String.fromCharCode.apply(String, u8v), expectedResult, "array buffer contents in test " + testName);
Attachment #533828 - Flags: review?(jonas) → review-
(Assignee)

Comment 10

6 years ago
The test addition on line 138 is to make sure for a new FileReader, even if we don't read anything, the GetResult() call should return a NULL - should I remove it?
(Assignee)

Comment 11

6 years ago
Created attachment 533858 [details] [diff] [review]
Changes according to Jonas' comment
Attachment #533858 - Flags: review?(jonas)
(Assignee)

Comment 12

6 years ago
Created attachment 533859 [details] [diff] [review]
Whole patch (may need to remove the empty test)
Attachment #533859 - Flags: review?(jonas)
(In reply to comment #10)
> The test addition on line 138 is to make sure for a new FileReader, even if
> we don't read anything, the GetResult() call should return a NULL - should I
> remove it?

Ah, excellent. However there's no need to add these two lines:

+testHasRun();
+expectedTestCount++;

The purpose of that function and variable is to manage asynchronous tests, but since you can run these tests synchronously they are not needed.
(Assignee)

Updated

6 years ago
Attachment #533697 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #533770 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #533828 - Attachment is obsolete: true
(Assignee)

Comment 14

6 years ago
Created attachment 534015 [details] [diff] [review]
Final patch candidate
Attachment #534015 - Flags: review?
(Assignee)

Updated

6 years ago
Attachment #534015 - Flags: review? → review?(jonas)
(Assignee)

Updated

6 years ago
Attachment #533859 - Attachment is obsolete: true
Attachment #533859 - Flags: review?(jonas)
(Assignee)

Comment 15

6 years ago
Created attachment 534105 [details] [diff] [review]
readAsArrayBuffer patch for commit
(Assignee)

Updated

6 years ago
Attachment #534105 - Flags: review?(jonas)
Checked in:

http://hg.mozilla.org/mozilla-central/rev/957f15b6e6bf
Keywords: dev-doc-needed
(Assignee)

Comment 17

6 years ago
Created attachment 534180 [details] [diff] [review]
Added a temprory variable to hold mResult to fix the ReadAsBinaryString problem.
Attachment #533858 - Attachment is obsolete: true
Attachment #534015 - Attachment is obsolete: true
Attachment #534105 - Attachment is obsolete: true
Attachment #534180 - Flags: review?(jonas)
Attachment #533858 - Flags: review?(jonas)
Attachment #534015 - Flags: review?(jonas)
Attachment #534105 - Flags: review?(jonas)
(Assignee)

Comment 18

6 years ago
Created attachment 534181 [details] [diff] [review]
Full Patch for Commit
Attachment #534181 - Flags: review?(jonas)
I pushed this and bug 658683 to tryserver and got one failure and one seemingly random crash:

http://tbpl.mozilla.org/?tree=Try&rev=e2514be69c2d
The crash is definitely not random.  The stack is in nsXMLHttpRequest's CC helper, so there's some sort of problem with the patch for bug 658683.
(Assignee)

Comment 21

6 years ago
Created attachment 534589 [details] [diff] [review]
Removed mResultArrayBufferRooted variable. Note it needs the patch 658683 being applied first.
Attachment #534180 - Attachment is obsolete: true
Attachment #534181 - Attachment is obsolete: true
Attachment #534589 - Flags: review?(jonas)
Attachment #534180 - Flags: review?(jonas)
Attachment #534181 - Flags: review?(jonas)
(Assignee)

Updated

6 years ago
Attachment #534589 - Attachment description: Removed mResultArrayBufferRooted variable. Noted need the patch 658683 being applied first. → Removed mResultArrayBufferRooted variable. Note it needs the patch 658683 being applied first.
Comment on attachment 534589 [details] [diff] [review]
Removed mResultArrayBufferRooted variable. Note it needs the patch 658683 being applied first.

Review of attachment 534589 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with this change.

::: content/base/src/nsDOMFileReader.cpp
@@ +178,5 @@
>  nsDOMFileReader::~nsDOMFileReader()
>  {
> +  if (mResultArrayBuffer) {
> +    UnrootResultArrayBuffer();
> +  }

You should do the Unroot call here even if mResultArrayBuffer is null. Consider the case when you first read as an ArrayBuffer, then read as something else, and then the reader is deleted. In that case mResultArrayBuffer is null, but the wrappercache won't unroot since it was never rooting.

Same thing in the unlink call.

And please add a test for this.
Attachment #534589 - Flags: review?(jonas) → review+
(Assignee)

Comment 23

6 years ago
Created attachment 534632 [details] [diff] [review]
Patch for commit, with changes and a test due to Jonas' comment
Attachment #534589 - Attachment is obsolete: true
Attachment #534632 - Flags: review?(jonas)
(Assignee)

Comment 24

6 years ago
Created attachment 534635 [details] [diff] [review]
Added corresponding test for reading firstly as array buffer then read as binary string
(Assignee)

Updated

6 years ago
Attachment #534635 - Flags: review?(jonas)
Attachment #534632 - Attachment is obsolete: true
Attachment #534632 - Flags: review?(jonas)
Comment on attachment 534635 [details] [diff] [review]
Added corresponding test for reading firstly as array buffer then read as binary string

Review of attachment 534635 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that fixed.

::: content/base/test/test_fileapi.html
@@ +250,5 @@
> +                          testBinaryData.length,
> +                          "to-be-reused reading arrayBuffer")
> +var makeAnotherReadListener5 = function(event) {
> +  r = event.target;
> +  r.removeEventListener("load", makeAnotherReadListener4, false);

That should be makeAnotherReadListener5.
Attachment #534635 - Flags: review?(jonas) → review+
(Assignee)

Comment 26

6 years ago
Created attachment 534644 [details] [diff] [review]
fixed - sorry have to make so many patches :)
Attachment #534635 - Attachment is obsolete: true
Attachment #534644 - Flags: review?(jonas)
Created attachment 534723 [details] [diff] [review]
latest attempt

This seems to do it.
Attachment #534644 - Attachment is obsolete: true
Attachment #534644 - Flags: review?(jonas)
Pushed by Jonas then backed out because of orange on Windows Mochitest-1.
See:
http://tbpl.mozilla.org/?tree=Firefox&rev=0acfb7c8331b

Comment 29

6 years ago
Temporary workaround available at https://gist.github.com/912082/c6a61eb804e554a7e30e6eb4d6eb59906670a0e0.
:)
(Assignee)

Comment 30

6 years ago
Created attachment 543207 [details] [diff] [review]
With FileReader added to dom_quickstub.qsconf should fix the crash

Pushed to tryServer, waiting for results
Attachment #534723 - Attachment is obsolete: true
Attachment #543207 - Flags: review?(jonas)
(Assignee)

Comment 31

6 years ago
Tbpl link: http://tbpl.mozilla.org/?tree=Try&rev=b2a58fc0abdd
Comment on attachment 543207 [details] [diff] [review]
With FileReader added to dom_quickstub.qsconf should fix the crash

Review of attachment 543207 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with that! Though please attach a new patch with a better commit comment.

Yay! Very glad we figured this one out, thanks for your help with all the testing!

::: content/base/src/nsDOMFileReader.cpp
@@ +141,5 @@
>  
> +void
> +nsDOMFileReader::RootResultArrayBuffer()
> +{
> +  nsContentUtils::PreserveWrapper(static_cast<nsPIDOMEventTarget*>(this), this);

This won't compile once you update to tip. You should cast to nsIDOMEventTarget rather than nsPIDOMEventTarget.

@@ +167,3 @@
>  {
>    nsLayoutStatics::AddRef();
> +  SetDOMStringToNull(mResult);

Why is this needed?
Attachment #543207 - Flags: review?(jonas) → review+
(Assignee)

Comment 33

6 years ago
Created attachment 543250 [details] [diff] [review]
Updated with the new jstypedarray.h change

You're right! I did the change but somehow they were lost in the patch. Now the changes according to the new jstypedarray is applied.

New tbpl: http://tbpl.mozilla.org/?tree=Try&rev=24a704daf853
Attachment #543207 - Attachment is obsolete: true
Attachment #543250 - Flags: review?(jonas)
Comment on attachment 543250 [details] [diff] [review]
Updated with the new jstypedarray.h change

Review of attachment 543250 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDOMFileReader.cpp
@@ +430,5 @@
>                                 &bytesRead);
>      NS_ASSERTION(bytesRead == aCount, "failed to read data");
>    }
> +  else if (mDataFormat == FILE_AS_ARRAYBUFFER) {
> +    JSObject* abuf = js::ArrayBuffer::getArrayBuffer(mResultArrayBuffer);

I don't understand this line. You're converting from a JSObject* to a JSObject*? Are they different objects somehow?

I guess I'd like to get Nikhil to verify that this is required by the new arraybuffer API.

::: content/base/test/test_fileapi.html
@@ +142,5 @@
> +  "readyState in test reader get result without reading");
> +is(r.error, null,
> +  "no error in test reader get result without reading");
> +is(r.result, "",
> +  "result in test reader get result without reading");

Ah, this should indeed be null, so add back the SetDOMStringToNull(mResult) that you had added and change this back to null.
(Assignee)

Comment 35

6 years ago
Created attachment 543303 [details] [diff] [review]
Final patch candidate

The tryServer status looks so far so good
Attachment #543250 - Attachment is obsolete: true
Attachment #543303 - Flags: review?(jonas)
Attachment #543250 - Flags: review?(jonas)
Comment on attachment 543303 [details] [diff] [review]
Final patch candidate

Yay!

Please do attach a new patch with a better checkin comment though.
Attachment #543303 - Attachment is patch: true
Attachment #543303 - Flags: review?(jonas) → review+
(Assignee)

Comment 37

6 years ago
Created attachment 543312 [details] [diff] [review]
Now with the correct comment (hopefully:)
Attachment #543303 - Attachment is obsolete: true
Attachment #543312 - Flags: review?(jonas)
Checked in to mozilla-inbound!

http://hg.mozilla.org/integration/mozilla-inbound/rev/02a734493229
Comment on attachment 543312 [details] [diff] [review]
Now with the correct comment (hopefully:)

(by the way, you don't really need to ask for review when fixing the checkin message :) )
Attachment #543312 - Flags: review?(jonas) → review+

Comment 40

6 years ago
Great thanks a lot Jonas!
http://hg.mozilla.org/mozilla-central/rev/02a734493229
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7

Updated

6 years ago
Keywords: dev-doc-needed → dev-doc-complete

Comment 42

6 years ago
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Build ID: 20110908135051

I ran the content/base/test/test_fileapi.html test to verify this bug but it failed:
Passed: 210
Failed: 1
Todo: 1
todo | shouldn't throw when opening nonexistent file, should fire error instead
failed | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: SpecialPowers is not defined at file:///D:/test%20cases/tc/test_fileapi.html:407
You need to log in before you can comment on or make changes to this bug.