Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix js error "The parameter is incorrect" #199

Merged
merged 2 commits into from
Oct 1, 2018
Merged

Conversation

trongrg
Copy link
Contributor

@trongrg trongrg commented Aug 8, 2018

Platforms affected

windows

What does this PR do?

Fix js error "The parameter is incorrect" when download progress is 0 in error handler

What testing has been done on this change?

Manual testing

Checklist

  • Reported an issue in the JIRA database
  • Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB-xxxx is the JIRA ID & "android" is the platform affected.
  • Added automated test coverage as appropriate for this change.

@janpio
Copy link
Member

janpio commented Aug 8, 2018

Hey @trongrg, thanks for this PR! Can you explain the motivation behind it a bit? When does this happen? What exactly is your code doing to fix it?

@trongrg
Copy link
Contributor Author

trongrg commented Aug 8, 2018

hey @janpio

So I have an UWP app using imgcache.js https://github.com/chrisben/imgcache.js to cache the images. imgcache.js use this plugin to download images.

When we load a page with 40 images, some of the images are broken, and it jumps to the error handler getTransferError. In my case, it does have a response however the download.progress.bytesReceived is 0, and Windows.Storage.Streams.DataReader throw an exception complaining about "The parameter is incorrect" for that 0

So this PR check download.progress.bytesReceived, and if it is 0, resolve the error, so no exception is thrown

@janpio
Copy link
Member

janpio commented Aug 8, 2018

What does "download.progress.bytesReceived is 0" mean, in layman terms?
Is it a good idea to just "swallow" that exception?

@trongrg
Copy link
Contributor Author

trongrg commented Aug 16, 2018

"download.progress.bytesReceived is 0" means the image is downloaded, but on hard disk its size is 0. This could be the result of a broken transfer.

I didn't swallow the exception (it is hard to digest anyway). This exception was thrown by a bug in the code, not intentional.

The purpose of that piece of code is to resolve a FILE_NOT_FOUND_ERR, with the downloaded payload. So it try to load the downloaded bytes, however when the bytesReceived is 0, WinJS throws an exception, and break the callback

After the change, it still resolves a FILE_NOT_FOUND_ERR, just without a payload in case bytesReceived is 0

@janpio
Copy link
Member

janpio commented Aug 16, 2018

Ok, way above my pay grade ($0) - let's hope someone who knows a bit about this comes along to merge this.

The tests on Travis are failing right now:

1) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.8 should download a file using https://
  - Expected spy httpFail not to have been called.
2) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.9 should not leave partial file due to abort
  - Expected spy httpWin not to have been called.
3) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.10 should be stopped by abort()
  - Expected spy httpWin not to have been called.

and

1) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.9 should not leave partial file due to abort
  - Expected spy httpWin not to have been called.
2) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.10 should be stopped by abort()
  - Expected spy httpWin not to have been called.

and

1) cordova-plugin-file-transfer-tests.tests >> FileTransfer methods download filetransfer.spec.9 should not leave partial file due to abort
  - Expected spy httpWin not to have been called.

Is this caused by these changes?

@trongrg
Copy link
Contributor Author

trongrg commented Aug 16, 2018

I don't think this PR cause those to fail, because the change is for windows platform, and the tests are failing for android platform

@janpio janpio merged commit 6c99dfa into apache:master Oct 1, 2018
mac89 added a commit to dualinventive/cordova-plugin-file-transfer that referenced this pull request Jan 29, 2019
* upstream/master: (56 commits)
  remove outdated translations (apache#211)
  [windows] Fix js error "The parameter is incorrect" (apache#199)
  also accept android sdk terms for android-27
  CB-13826 Incremented plugin version.
  CB-13826 Updated version and RELEASENOTES.md for release 1.7.1
  CB-13749: Add build-tools-26.0.2 to travis
  Update plugin.xml
  change dependency version to file >=5.0.0
  change dependency version to file 6.0.0
  Set VERSION to 1.7.1-dev (via coho)
  CB-13542 updated file plugin dependency
  Set VERSION to 1.7.1-dev (via coho)
  CB-13542 Updated version and RELEASENOTES.md for release 1.7.0 (via coho)
  Updated links in Deprecation Notice
  Updated README with Deprecated Status
  Revert "CB-13294 Remove cordova-plugin-compat"
  CB-13472: (CI) Fixed Travis Android builds again
  CB-13294 Remove cordova-plugin-compat
  CB-13299 (CI) Fix Android builds
  CB-12809: Google Play Blocker: Unsafe SSL TrustManager Defined
  ...

# Conflicts:
#	README.md
@PabbleDabble
Copy link

@janpio Hello. Apologies if this is the wrong place to ask / if this is a bad question. I see the 1.7.1 release was release Jan of 2018, and the 1.7.0 updated the readme with deprecated. I also see there have been merges in to master more recently than Jan of 2018 (specifically this one). Does this plugin being deprecated mean there will no longer be releases? I know I'm able to download this plugin manually, but was hoping to keep this installation using the ionic cordova add method to stay consistent with our process. Thank you so much for any information.

@janpio
Copy link
Member

janpio commented Mar 6, 2019

This is definitely the wrong place, also asking individual committers is not standard. Please create a new issue with your question.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants