965956 - File (<nsIFile>) and nsIDomFileReader.readAsArrayBuffer() perform synchronous reads of the disk

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Comment 1

[Steven MacLeod [:smacleod]]

Sorry, fat fingered enter key.

I'm unable to perform a strictly asynchronous read of the disk using nsiDOMFileReader.readAsArrayBuffer() in chrome code, as both of these calls check for file existence, instead of an error being triggered asynchronously.

It would be nice if File() was switched to lazily gather information about the file when it's requested. Also, making readAsArrayBuffer on a non-existent file asynchronously error would be helpful as well.

I'm not very familiar with it, but looking through the spec I couldn't find anything requiring the current behavior.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Can someone confirm that a lazy error would not break the spec?

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

http://dev.w3.org/2006/webapi/FileAPI/#file-error-read seems to suggest that would probably be fine.

Comment 4

[Jan Varga [:janv]]

So, the problem with readAsArrayBuffer() is that it calls file->GetSize() synchronously on the main thread, the size is needed for creating an empty array buffer object. I think the code could be modified to avoid getting the size synchronously.

Another problem is that nsDOMMultipartFile::InitChromeFile() calls file->Exists(), file->IsDirectory() and file->GetLeafName().
So it seem the File() throws an exception, if the file doesn't exist ?
That sounds unnecessary to me.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

I'll try and work on this. If someone has more time, please don't hesitate to steal the bug.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

So can make this all happen in a background thread? Async isn't good enough.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

This is definitely the objective.

Comment 8

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

We should probably remove the File(<nsIFile>) constructor. It will inevitably cause sync IO to get the filesize when the File object is used. There's nothing nsIDomFileReader.readAsArrayBuffer specific about this. A pile of other APIs that handle Blobs and Files will call GetSize().

We could add a File(nsIFile, long long size) constructor instead.

Comment 9

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

I don't know why or where we check for file existence though. That should definitely happen on a background thread such that any reads fail asynchronously rather than throw.

Comment 10

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

What we've talked about before is having a background thread that does the stat, and then dispatching something to do that and blocking on the thread if we need the data on the main thread before it finishes.

Comment 11

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

Sounds risky to me. Especially on systems with slow IO which is the same set of systems where you don't want the sync IO.

Seems safer to guarantee that no sync IO is happening by demanding that File creators supply a size.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

So your idea is to force the users to compute the size OMT before constructing a File object? That looks far from trivial on the user, plus it won't cover other sources of main thread I/O such as lastModifiedDate.

Now, I have the impression that we can find a simple solution that covers the main use case, i.e.:
1/ open a File;
2/ read its contents with a FileReader.

I haven't been able to find where in the code File or nsDOMFileReader checks for the existence of the file. Steven, can you point out where this happens?

As far as I understand, the current culprit is http://dxr.mozilla.org/mozilla-central/source/content/base/src/nsDOMFileReader.cpp#466, per which we do main-thread-io to compute the size of the file so as to allocate the array buffer from the main thread before opening the input stream. I am certain that we can compute this size on the IO thread. I am not quite clear how we can allocate the ArrayBuffer without causing an additional memory copy, but there is certainly a way.

Assuming this is possible, would this solution be acceptable for everyone?

Comment 13

[Jan Varga [:janv]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #12)
> So your idea is to force the users to compute the size OMT before
> constructing a File object? That looks far from trivial on the user, plus it
> won't cover other sources of main thread I/O such as lastModifiedDate.

so it means that lastModifiedData should be computed OMT too
but I agree, this approach is a bit harder

> 
> Now, I have the impression that we can find a simple solution that covers
> the main use case, i.e.:
> 1/ open a File;
> 2/ read its contents with a FileReader.
> 
> I haven't been able to find where in the code File or nsDOMFileReader checks
> for the existence of the file. Steven, can you point out where this happens?

take a look at:
nsDOMMultipartFile::InitChromeFile()

> 
> As far as I understand, the current culprit is
> http://dxr.mozilla.org/mozilla-central/source/content/base/src/
> nsDOMFileReader.cpp#466, per which we do main-thread-io to compute the size
> of the file so as to allocate the array buffer from the main thread before
> opening the input stream. I am certain that we can compute this size on the
> IO thread. I am not quite clear how we can allocate the ArrayBuffer without
> causing an additional memory copy, but there is certainly a way.

You can dispatch a runnable to a background thread to get the file size, then dispatch to the main thread and open the channel, etc.

Comment 14

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

We could make the "use File(nsIFile) constructor and then immediately pass the File to a FileReader" a happy-path which doesn't do any sync IO. However it means that we'll mostly likely fall off that happy path and do sync IO quite often.

Possibly we could make the .size getter throw when a File object is created using File(nsIFile). Hopefully that will make most operations fail, including things like blob.slice() and structured cloning. We'd just need to make sure that the FileReader still handles that case.

However the real fix is to add a Stream primitive with a Stream(nsIFile) constructor. This stream primitive wouldn't have .size getter at all and would never need to do sync IO. And would allow reading the data into memory. There's various people that are working on adding a Stream primitive to the web. Unclear when that will be done though.

Comment 15

[Steven MacLeod [:smacleod]]

(In reply to David Rajchenbach Teller [:Yoric] (please use "needinfo?") from comment #12)
> I haven't been able to find where in the code File or nsDOMFileReader checks
> for the existence of the file. Steven, can you point out where this happens?

It may not have been an explicit existence check. I was getting an error because the file
did not exist but that could have been coming from computing the size.

Comment 16

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

I think we should do the solution in the second paragraph of comment 14. I.e. remove any sync IO code which lazily gets the size or lastModifiedDate. When the File(<nsIFile>) constructor is used the .size/.lastModifiedDate getters should throw. As should the .slice() function and any attempts to structured-clone the file.

Then we just make sure that the FileReader is able to handle such a File.

This way we can also remove the mutable flag. All File and Blob instances will be immutable.

This way such a File kind'a works like a getto-Stream.

Comment 17

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

Ping? Any updates here?

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Sorry, working on something entirely different these days. I don't know when I'll have time to work on main thread I/O.

Comment 19

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1472046

Move all DOM bugs that haven't been updated in more than 3 years and has no one currently assigned to P5.

If you have questions, please contact :mdaly.