632255 - Implement FileReader.readAsArrayBuffer()

Status: RESOLVED FIXED

(Core :: DOM: Events, enhancement)

Description

[Patrick Walton (:pcwalton)]

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.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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).

Comment 3

[Kenneth Russell]

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.

Comment 4

[Shawn Gong]

Attachment: The implementation of readAsArrayBuffer

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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");

Comment 6

[Shawn Gong]

Attachment: Changes according to bent's comment

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 533770 [details] [diff] [review]
Changes according to bent's comment

Looks good to me, over to sicking.

Comment 8

[Shawn Gong]

Attachment: Smashed patch of the previous 2

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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);

Comment 10

[Shawn Gong]

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?

Comment 11

[Shawn Gong]

Attachment: Changes according to Jonas' comment

Comment 12

[Shawn Gong]

Attachment: Whole patch (may need to remove the empty test)

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(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.

Comment 14

[Shawn Gong]

Attachment: Final patch candidate

Comment 15

[Shawn Gong]

Attachment: readAsArrayBuffer patch for commit

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in:

http://hg.mozilla.org/mozilla-central/rev/957f15b6e6bf

Comment 17

[Shawn Gong]

Attachment: Added a temprory variable to hold mResult to fix the ReadAsBinaryString problem.

Comment 18

[Shawn Gong]

Attachment: Full Patch for Commit

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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

Comment 20

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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.

Comment 21

[Shawn Gong]

Attachment: Removed mResultArrayBufferRooted variable. Note it needs the patch 658683 being applied first.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 23

[Shawn Gong]

Attachment: Patch for commit, with changes and a test due to Jonas' comment

Comment 24

[Shawn Gong]

Attachment: Added corresponding test for reading firstly as array buffer then read as binary string

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 26

[Shawn Gong]

Attachment: fixed - sorry have to make so many patches :)

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: latest attempt

This seems to do it.

Comment 28

[Mounir Lamouri (:mounir)]

Pushed by Jonas then backed out because of orange on Windows Mochitest-1.
See:
http://tbpl.mozilla.org/?tree=Firefox&rev=0acfb7c8331b

Comment 29

[Ashok Mathew]

Temporary workaround available at https://gist.github.com/912082/c6a61eb804e554a7e30e6eb4d6eb59906670a0e0.
:)

Comment 30

[Shawn Gong]

Attachment: With FileReader added to dom_quickstub.qsconf should fix the crash

Pushed to tryServer, waiting for results

Comment 31

[Shawn Gong]

Tbpl link: http://tbpl.mozilla.org/?tree=Try&rev=b2a58fc0abdd

Comment 32

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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?

Comment 33

[Shawn Gong]

Attachment: 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

Comment 34

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 35

[Shawn Gong]

Attachment: Final patch candidate

The tryServer status looks so far so good

Comment 36

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 543303 [details] [diff] [review]
Final patch candidate

Yay!

Please do attach a new patch with a better checkin comment though.

Comment 37

[Shawn Gong]

Attachment: Now with the correct comment (hopefully:)

Comment 38

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Checked in to mozilla-inbound!

http://hg.mozilla.org/integration/mozilla-inbound/rev/02a734493229

Comment 39

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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 :) )

Comment 40

[hyou]

Great thanks a lot Jonas!

Comment 41

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/02a734493229

Comment 42

[Ioana (away)]

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