apache / cordova-plugin-file

Apache Cordova File Plugin
https://cordova.apache.org/
Apache License 2.0
741 stars 756 forks source link

CB-6994 - Windows 8 does not save big file transferred using Blob #54

Closed fberton closed 9 years ago

fberton commented 9 years ago

When trying to save Blob using FileWriter.write method, if the blob's size is too high, we get a stack overflow exception during the conversion of the blob into an basic array.

In order to avoid this exception, we pass the Blob or File directly in FileProxy (avoid the conversion to ArrayBuffer) and we use a specific method in FileProxy to save the Blob directly to the filesystem by using Streams (safe).

More info at : https://issues.apache.org/jira/browse/CB-6994

purplecabbage commented 9 years ago

Is this still valid? It is almost impossible to tell what has changed with all the whitespace noise in FileProxy.js. If this is still valid, please rebase as there are currently merge conflicts.

BBosman commented 9 years ago

You can see a commit diff ignoring whitespace changes by appending ?w=1 to the querystring.

So for this pull request's commit that would be: https://github.com/Touchit/cordova-plugin-file/commit/823d2ff100757b21411e990633d084482bbca52e?w=1

Doesn't change the fact that it needs rebasing, but allows for a clean(er) view of the intended changes.

purplecabbage commented 9 years ago

Thanks for the github lesson @BBosman, extremely useful! I will use this lots.

sgrebnov commented 9 years ago

@Bosman, thx for sharing this trick!

SomaticIT commented 9 years ago

Hey @sgrebnov, I did not have any time to fix this issue before. I just rebased previous commit and add a new one to synchronize changes with the global Windows Platform. Could you review please.

SomaticIT commented 9 years ago

Hi, Just to ask for review again. This bug is really blocking in Windows 8 application : the application crash with a stackoverflow exception while trying to write files < 1 MB. Thanks

sgrebnov commented 9 years ago

Maxime, I'll be able to review and test this later today, thx for the fix!

SomaticIT commented 9 years ago

@sgrebnov Thanks

purplecabbage commented 9 years ago

I'll have a look too, if it is not already merged.

Sent from my iPhone

On Oct 7, 2014, at 2:14 AM, Maxime LUCE notifications@github.com wrote:

@sgrebnov Thanks

\ Reply to this email directly or view it on GitHub.

sgrebnov commented 9 years ago

Just an additional proposed improvement to improve readability (not critical): it will be great if blob/array buffer writing logic is wrapped to separate functions (writeArrayBufferAsync/writeBlobAsync), similar to Windows.Storage.FileIO.writeBytesAsync Windows.Storage.FileIO.writeTextAsync

So we can continue using logic with writePromise:

if (data instanceof ArrayBuffer) { writePromise = writeArrayBufferAsync; } else if (data instanceof Blob) { writePromise = writeBlobAsync; } else if ...

SomaticIT commented 9 years ago

I created 2 shortcuts to writeBytesAsync and writeTextAsync and a new function writeBlobAsync.

Then I do :

if (data instanceof Blob) { writePromise = writeBlobAsync; } else if (isBinary) { writePromise = writeBytesAsync; } else { writePromise = writeTextAsync; }

sgrebnov commented 9 years ago

Thank you, Maxime. I'll test updated version as soon as I have a chance (most likely in a few hrs) and let you know. Thx!

sgrebnov commented 9 years ago

Reviewed, lgtm except some inline note and failed test 'file.spec.106 should be able to write a File to a FileWriter' . This is because 'File' is cordova specific implementation so does not have msDetachStream method in comparion to native Blob.

I'll merge this version tomorrow and do one more iteration improving FileProxy since I see many other tests failed (another reasons). Thank you, @SomaticIT for this patch!

SomaticIT commented 9 years ago

Ok I understand that File is Cordova specific. Is there a way to access the blob inside the File Object ?

sgrebnov commented 9 years ago

merged, I'll do another round of testing this code on windows this week (or beginning of next week)

SomaticIT commented 9 years ago

@sgrebnov I reviewed your changes and it's OK for me. Thanks for merging.

shenzhuxi commented 9 years ago

In Android, I tried to copy a 14MB file from html file input with org.apache.cordova.file 1.3.1. The App crashed and a 0 byte file was copied. I'm not sure is it the same problem.

clelland commented 9 years ago

Probably not the same issue, @shenzhuxi -- Android hasn't used that technique for binary data in at least a year. (Not saying that you don't have a legitimate bug; if you can reproduce it, please open another issue for it, though)

SomaticIT commented 9 years ago

It's not the same issue.

This issue was only applicable to Windows 8 which tried to serialize files into plain old arrays and create stack overflow exception.