Last Comment Bug 632255 - Implement FileReader.readAsArrayBuffer()
: Implement FileReader.readAsArrayBuffer()
Status: RESOLVED FIXED
[parity-chrome][parity-safari]
: dev-doc-complete
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: mozilla7
Assigned To: Shawn Gong
:
:
Mentors:
: 635274 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-02-07 17:36 PST by Patrick Walton (:pcwalton)
Modified: 2011-09-13 01:10 PDT (History)
16 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
The implementation of readAsArrayBuffer (20.01 KB, patch)
2011-05-19 10:20 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Changes according to bent's comment (6.40 KB, patch)
2011-05-19 13:14 PDT, Shawn Gong
bent.mozilla: review+
Details | Diff | Splinter Review
Smashed patch of the previous 2 (28.56 KB, patch)
2011-05-19 15:49 PDT, Shawn Gong
jonas: review-
Details | Diff | Splinter Review
Changes according to Jonas' comment (2.84 KB, patch)
2011-05-19 17:43 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Whole patch (may need to remove the empty test) (19.34 KB, patch)
2011-05-19 17:46 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Final patch candidate (19.31 KB, patch)
2011-05-20 10:20 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
readAsArrayBuffer patch for commit (19.38 KB, patch)
2011-05-20 14:11 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Added a temprory variable to hold mResult to fix the ReadAsBinaryString problem. (849 bytes, patch)
2011-05-20 18:30 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Full Patch for Commit (19.34 KB, patch)
2011-05-20 18:33 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Removed mResultArrayBufferRooted variable. Note it needs the patch 658683 being applied first. (19.06 KB, patch)
2011-05-23 15:32 PDT, Shawn Gong
jonas: review+
Details | Diff | Splinter Review
Patch for commit, with changes and a test due to Jonas' comment (19.43 KB, patch)
2011-05-23 17:14 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Added corresponding test for reading firstly as array buffer then read as binary string (20.04 KB, patch)
2011-05-23 17:37 PDT, Shawn Gong
jonas: review+
Details | Diff | Splinter Review
fixed - sorry have to make so many patches :) (20.04 KB, patch)
2011-05-23 18:04 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
latest attempt (19.92 KB, patch)
2011-05-24 04:27 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
no flags Details | Diff | Splinter Review
With FileReader added to dom_quickstub.qsconf should fix the crash (21.47 KB, patch)
2011-06-30 11:46 PDT, Shawn Gong
jonas: review+
Details | Diff | Splinter Review
Updated with the new jstypedarray.h change (21.66 KB, patch)
2011-06-30 14:08 PDT, Shawn Gong
no flags Details | Diff | Splinter Review
Final patch candidate (21.71 KB, patch)
2011-06-30 16:50 PDT, Shawn Gong
jonas: review+
Details | Diff | Splinter Review
Now with the correct comment (hopefully:) (21.76 KB, patch)
2011-06-30 17:53 PDT, Shawn Gong
jonas: review+
Details | Diff | Splinter Review

Description Patrick Walton (:pcwalton) 2011-02-07 17:36:37 PST
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 2011-02-07 18:02:58 PST
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 2 Boris Zbarsky [:bz] (still a bit busy) 2011-02-18 09:10:05 PST
*** Bug 635274 has been marked as a duplicate of this bug. ***
Comment 3 Kenneth Russell 2011-03-10 12:26:15 PST
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 2011-05-19 10:20:44 PDT
Created attachment 533697 [details] [diff] [review]
The implementation of readAsArrayBuffer
Comment 5 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-05-19 11:28:05 PDT
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 2011-05-19 13:14:47 PDT
Created attachment 533770 [details] [diff] [review]
Changes according to bent's comment
Comment 7 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-05-19 13:31:17 PDT
Comment on attachment 533770 [details] [diff] [review]
Changes according to bent's comment

Looks good to me, over to sicking.
Comment 8 Shawn Gong 2011-05-19 15:49:16 PDT
Created attachment 533828 [details] [diff] [review]
Smashed patch of the previous 2
Comment 9 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-19 16:41:53 PDT
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 2011-05-19 17:42:54 PDT
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 2011-05-19 17:43:37 PDT
Created attachment 533858 [details] [diff] [review]
Changes according to Jonas' comment
Comment 12 Shawn Gong 2011-05-19 17:46:27 PDT
Created attachment 533859 [details] [diff] [review]
Whole patch (may need to remove the empty test)
Comment 13 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-19 17:52:38 PDT
(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 2011-05-20 10:20:11 PDT
Created attachment 534015 [details] [diff] [review]
Final patch candidate
Comment 15 Shawn Gong 2011-05-20 14:11:25 PDT
Created attachment 534105 [details] [diff] [review]
readAsArrayBuffer patch for commit
Comment 16 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-20 14:52:40 PDT
Checked in:

http://hg.mozilla.org/mozilla-central/rev/957f15b6e6bf
Comment 17 Shawn Gong 2011-05-20 18:30:35 PDT
Created attachment 534180 [details] [diff] [review]
Added a temprory variable to hold mResult to fix the ReadAsBinaryString problem.
Comment 18 Shawn Gong 2011-05-20 18:33:10 PDT
Created attachment 534181 [details] [diff] [review]
Full Patch for Commit
Comment 19 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-21 01:27:21 PDT
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 [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-05-21 10:59:42 PDT
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 2011-05-23 15:32:54 PDT
Created attachment 534589 [details] [diff] [review]
Removed mResultArrayBufferRooted variable. Note it needs the patch 658683 being applied first.
Comment 22 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-23 16:49:34 PDT
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 2011-05-23 17:14:40 PDT
Created attachment 534632 [details] [diff] [review]
Patch for commit, with changes and a test due to Jonas' comment
Comment 24 Shawn Gong 2011-05-23 17:37:23 PDT
Created attachment 534635 [details] [diff] [review]
Added corresponding test for reading firstly as array buffer then read as binary string
Comment 25 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-23 17:59:22 PDT
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 2011-05-23 18:04:14 PDT
Created attachment 534644 [details] [diff] [review]
fixed - sorry have to make so many patches :)
Comment 27 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-05-24 04:27:29 PDT
Created attachment 534723 [details] [diff] [review]
latest attempt

This seems to do it.
Comment 28 Mounir Lamouri (:mounir) 2011-05-24 06:50:44 PDT
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 2011-06-08 06:40:48 PDT
Temporary workaround available at https://gist.github.com/912082/c6a61eb804e554a7e30e6eb4d6eb59906670a0e0.
:)
Comment 30 Shawn Gong 2011-06-30 11:46:11 PDT
Created attachment 543207 [details] [diff] [review]
With FileReader added to dom_quickstub.qsconf should fix the crash

Pushed to tryServer, waiting for results
Comment 31 Shawn Gong 2011-06-30 11:47:45 PDT
Tbpl link: http://tbpl.mozilla.org/?tree=Try&rev=b2a58fc0abdd
Comment 32 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-06-30 12:26:48 PDT
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 2011-06-30 14:08:50 PDT
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
Comment 34 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-06-30 14:25:16 PDT
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 2011-06-30 16:50:24 PDT
Created attachment 543303 [details] [diff] [review]
Final patch candidate

The tryServer status looks so far so good
Comment 36 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-06-30 17:10:26 PDT
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 2011-06-30 17:53:39 PDT
Created attachment 543312 [details] [diff] [review]
Now with the correct comment (hopefully:)
Comment 38 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-06-30 18:02:12 PDT
Checked in to mozilla-inbound!

http://hg.mozilla.org/integration/mozilla-inbound/rev/02a734493229
Comment 39 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-06-30 18:02:45 PDT
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 2011-07-01 02:28:27 PDT
Great thanks a lot Jonas!
Comment 41 Marco Bonardo [::mak] 2011-07-02 02:35:20 PDT
http://hg.mozilla.org/mozilla-central/rev/02a734493229
Comment 42 Ioana (away) 2011-09-13 01:10:35 PDT
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

Note You need to log in before you can comment on or make changes to this bug.