CB-13415 (android) Importing corrupt images using the Camera plugin c… #309

alpesh12 · 2018-01-16T07:51:41Z

Platforms affected

Android

What does this PR do?

Throws an exception which can be caught by the error block in Cordova app which uses the plugin
Does not crash the Cordova app which uses the plugin with corrupted image

What testing has been done on this change?

Tested with cordova-android 7.0.0

Checklist

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.
Reported an issue in the JIRA database
Added automated test coverage as appropriate for this change.

…rashes the app

alpesh12 · 2018-01-18T13:05:50Z

@infil00p
Can you please merge this changes to master because i have not change anything related to iOS platform.

jcesarmobile · 2018-01-18T13:09:10Z

src/android/CameraLauncher.java

            try {
                fileStream = FileHelper.getInputStreamFromUriString(imageUrl, cordova);
                image = BitmapFactory.decodeStream(fileStream);
+            }  catch (OutOfMemoryError e) {


I think it's better to catch any exception and not only OutOfMemoryError, it might fail due to other reasons, and then in the error callbackContext.error provide the exception message

Indeed. Exception will catch all exceptions including OutOfMemoryError. So do I need to change or you will?

@alpesh12 You need to change this before I accept the PR.

@infil00p ok will do after testing.

@infil00p @jcesarmobile Now its done can you please merge changes

@infil00p i just check travis build failed for iOS can you please help or guide me to successful complete travis test
Thank you

…es the app

infil00p · 2018-01-22T19:13:51Z

We're still getting Travis issues on iOS 10 since the change to iOS.

alpesh12 · 2018-01-23T04:42:31Z

@infil00p Whats is the next step to pass Travis test any guideline or help will be appreciates

jcesarmobile · 2018-01-24T22:35:40Z

I would keep the iOS and the Android parts separated (i.e. creating a new issue for iOS and removing the iOS code from this PR, and then send a new PR with the iOS code)
I don't really like the iOS approach, there are probably better solutions than using ALAssetsLibrary. Also the possible errors are just being logged, but not returned to the user.

alpesh12 · 2018-01-25T05:55:44Z

@jcesarmobile
Removed iOS code from this PR.

alpesh12 · 2018-01-25T12:51:19Z

@jcesarmobile @infil00p

Can you please check appveyor test i think there are some issue with that, for all last build it will take only 2-3 minutes but this time it will take 5hr then also its on queue state.
https://ci.appveyor.com/project/ApacheSoftwareFoundation/cordova-plugin-camera/history
Thank you

jcesarmobile · 2018-01-25T12:55:21Z

Sometimes tests take a lot of time, I don't think we can control that.
The iOS file is still a bit different, can you remove it from the PR?

alpesh12 · 2018-01-25T13:05:31Z

@jcesarmobile Done
Also for iOS PR i am not able to fork new one, so i have to wait for complete this PR right?

jcesarmobile · 2018-01-25T13:11:19Z

Best thing is to work in branches, so you can create a new CB-13415 branch with your current content, and reset your master to match the apache master, then create a new branch from master and do the changes there

alpesh12 · 2018-01-27T07:07:37Z

@jcesarmobile
When this changes are merged to master?

jcesarmobile · 2018-01-27T13:57:46Z

When somebody has time to properly test it and merge it.
Please, relax and stop pinging people every day, this is a free open source project and we do things in our spare time (or me at least).

BTW, I still see the iOS file as modified, even though it shows not changes. Can you remove the file from the PR entirely?

jcesarmobile · 2018-01-28T11:22:53Z

I've finally tested and the problem is only present when Camera.DestinationType.DATA_URL, so I don't think the problem is the corrupted image itself, but the common out of memory problem when using DATA_URL. The sample image is 4MB, that shouldn't be a problem, but maybe being corrupted increase the memory needed to handle it.

In the docs you can read:

NOTE: Photo resolution on newer devices is quite good. Photos selected from the device's gallery are not downscaled to a lower quality, even if a quality parameter is specified. To avoid common memory problems, set Camera.destinationType to FILE_URI rather than DATA_URL.

So, I don't think you should hardcode the OutOfMemoryError to corrupted image as any big image not corrupted will also cause the OutOfMemoryError.
So, @infil00p do you think we should hardcode it to something else? or just return the e.getMessage() or e.getLocalizedMessage()?
The truth is the messages returned are not very helpful, is "failed to allocate xxxx byte allocation with yyyy free bytes and zzMB until OOM"

ShemrickFlannigan · 2018-01-28T15:52:15Z

@jcesarmobile @infil00p I agree that the OutOfMemoryError is a catch all that is not specific to corrupt images.
But in our testing, the OutOfMemoryError is the only exception which we've found that successfully captures the corrupt images.
In our testing, there is no correlation with file size. The corrupts sample image which has been provided is < 5MB. The Cordova Camera app can handle non-corrupt images which are 40MB.

Here are the sample images:

Corrupt 5MB image:
https://www.dropbox.com/s/s7jh3eay3y23oy1/5mb_13500x13500.D2%5B1%5D.jpg?dl=0

Valid 40MB image:
https://www.dropbox.com/s/cnxykbcvkbc2k1k/Airbus_Pleiades_50cm_8bit_RGB_Yogyakarta.jpg?dl=0

I would like to move forward with the generic OutOfMemoryError as a catch all exception within this Pull Request, as it solves a high impact error (corrupt images crashing the entire Cordova app). Later, if/when a more accurate exception is found, it can be applied as an enhanced exception.

jcesarmobile · 2018-01-28T15:59:45Z

I'm just talking about the error message returned
callbackContext.error("Corrupted image selected.");

I don't think we should hardcode the out of memory to "corrupted image selected".
In your tests a regular image of 40MB is not crashing, but probably in low end devices will crash, so with the update it will get the "Corrupted image selected." when that is not true.
In fact, I have tested your 40MB image in my moto G and I get the "Corrupted image selected.

ShemrickFlannigan · 2018-01-29T01:35:35Z

@jcesarmobile
Thanks for the clarification. I definitely agree that the error message should not be set to the fixed message "Corrupt image selected". I agree with your suggestion of returning the e.getMessage() or e.getLocalizedMessage() value

alpesh12 · 2018-01-29T05:32:31Z

@jcesarmobile @ShemrickFlannigan
added dynamic message as you suggest also @jcesarmobile for iOS code its showing me no changes can you please confirm.

alpesh12 · 2018-02-01T13:03:15Z

@jcesarmobile @infil00p
Any updates regarding merge

jcesarmobile · 2018-02-01T13:59:13Z

I would use the same e.getMessage() or e.getLocalizedMessage() for both, not one with e.getMessage() and the other e.getLocalizedMessage() (for consistency).

I've checked other plugins and cordova-plugin-file uses e.getLocalizedMessage(), so I would use e.getLocalizedMessage() for both.

The iOS file shows no changes, but appears as modified, so you should remove it from the PR, maybe with a git reset or something.

Ah, and remove the comments, I think there is no need.

dwlrathod · 2018-02-07T08:45:02Z

+1
This bug really need to fix. Facing same issue for big images.

jcesarmobile · 2018-02-07T23:16:49Z

@alpesh12 can you remove the code comments?

Removed comments from CameraLauncher.java

alpesh12 · 2018-02-08T07:09:41Z

@jcesarmobile comment removed as you suggested.

jcesarmobile · 2018-02-08T12:12:50Z

Merged, thanks!

Will try to take a look into the iOS one this weekend, but no promises

CB-13415 (android) Importing corrupt images using the Camera plugin c…

f46fc03

…rashes the app

jcesarmobile reviewed Jan 18, 2018

View reviewed changes

Added general exception handler with OutOfMemoryError.

951e445

alpesh12 changed the title ~~CB-13415 (android) Importing corrupt images using the Camera plugin c…~~ CB-13415 (android)(iOS) Importing corrupt images using the Camera plugin c… Jan 19, 2018

alpesh12 added 2 commits January 19, 2018 12:14

CB-13415 (iOS) Importing corrupt images using the Camera plugin crash…

1af5fa6

…es the app

solved error as per travis build log

4a4dfe6

Removed iOS Code

436ec74

alpesh12 changed the title ~~CB-13415 (android)(iOS) Importing corrupt images using the Camera plugin c…~~ CB-13415 (android)Importing corrupt images using the Camera plugin c… Jan 25, 2018

alpesh12 changed the title ~~CB-13415 (android)Importing corrupt images using the Camera plugin c…~~ CB-13415 (android) Importing corrupt images using the Camera plugin c… Jan 25, 2018

Removed minor spacing from iOS code

239b5cd

alpesh12 added 2 commits January 29, 2018 10:59

removed fixed error message and set to e.getLocalizedMessage()

4756c0a

Changed comment

49cdc57

jd0048 and others added 2 commits February 1, 2018 20:07

updated CDVCamera.m file permission

f76043e

Change message to e.getLocalizedMessage()

c9e1857

Removed comments from CameraLauncher.java

086dd4c

Removed comments from CameraLauncher.java

jcesarmobile merged commit bf935df into apache:master Feb 8, 2018

CB-13415 (android) Importing corrupt images using the Camera plugin c… #309

CB-13415 (android) Importing corrupt images using the Camera plugin c… #309

Uh oh!

Conversation

alpesh12 commented Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Platforms affected

What does this PR do?

What testing has been done on this change?

Checklist

Uh oh!

alpesh12 commented Jan 18, 2018

Uh oh!

jcesarmobile Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

alpesh12 Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

infil00p Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

alpesh12 Jan 19, 2018

Choose a reason for hiding this comment

Uh oh!

alpesh12 Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alpesh12 Jan 22, 2018

Choose a reason for hiding this comment

Uh oh!

infil00p commented Jan 22, 2018

Uh oh!

alpesh12 commented Jan 23, 2018

Uh oh!

jcesarmobile commented Jan 24, 2018

Uh oh!

alpesh12 commented Jan 25, 2018

Uh oh!

alpesh12 commented Jan 25, 2018

Uh oh!

jcesarmobile commented Jan 25, 2018

Uh oh!

alpesh12 commented Jan 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcesarmobile commented Jan 25, 2018

Uh oh!

alpesh12 commented Jan 27, 2018

Uh oh!

jcesarmobile commented Jan 27, 2018

Uh oh!

jcesarmobile commented Jan 28, 2018

Uh oh!

ShemrickFlannigan commented Jan 28, 2018

Uh oh!

jcesarmobile commented Jan 28, 2018

Uh oh!

ShemrickFlannigan commented Jan 29, 2018

Uh oh!

alpesh12 commented Jan 29, 2018

Uh oh!

alpesh12 commented Feb 1, 2018

Uh oh!

jcesarmobile commented Feb 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwlrathod commented Feb 7, 2018

Uh oh!

jcesarmobile commented Feb 7, 2018

Uh oh!

alpesh12 commented Feb 8, 2018

Uh oh!

jcesarmobile commented Feb 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

alpesh12 commented Jan 16, 2018 •

edited

Loading

alpesh12 Jan 22, 2018 •

edited

Loading

alpesh12 commented Jan 25, 2018 •

edited

Loading

jcesarmobile commented Feb 1, 2018 •

edited

Loading