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

CB-13415 (iOS) Importing corrupt images using the Camera plugin crashes the app #310

Open
wants to merge 4 commits into
base: master
from

Conversation

9 participants
@alpesh12
Contributor

alpesh12 commented Jan 25, 2018

Platforms affected

  • iOS

What does this PR do?

  • Does not crash the Cordova app which uses the plugin with corrupted image

What testing has been done on this change?

Tested with cordova-ios : 4.5.4

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.
@alpesh12

This comment has been minimized.

Show comment
Hide comment
@alpesh12

alpesh12 Jan 26, 2018

Contributor

@jcesarmobile
Can you please check with travis test its failed because of "The Sauce VMs failed to start the browser or device."

Contributor

alpesh12 commented Jan 26, 2018

@jcesarmobile
Can you please check with travis test its failed because of "The Sauce VMs failed to start the browser or device."

@alsorokin

This comment has been minimized.

Show comment
Hide comment
@alsorokin

alsorokin Jan 26, 2018

Contributor

@alpesh12 from what I see, cd tests/ios && npm install && npm test has failed with the following error:

/Users/travis/build/apache/cordova-plugin-camera/src/ios/CDVCamera.m:373:36: error: variable is not assignable (missing __block type specifier)
                              data = imageData;
                              ~~~~ ^
1 error generated.
Contributor

alsorokin commented Jan 26, 2018

@alpesh12 from what I see, cd tests/ios && npm install && npm test has failed with the following error:

/Users/travis/build/apache/cordova-plugin-camera/src/ios/CDVCamera.m:373:36: error: variable is not assignable (missing __block type specifier)
                              data = imageData;
                              ~~~~ ^
1 error generated.
@alsorokin

This comment has been minimized.

Show comment
Hide comment
@alsorokin

alsorokin Jan 26, 2018

Contributor

All the following errors are because of the failed build. Not sure why paramedic continues the test run after a build failure though...

Contributor

alsorokin commented Jan 26, 2018

All the following errors are because of the failed build. Not sure why paramedic continues the test run after a build failure though...

Show outdated Hide outdated src/ios/CDVCamera.m Outdated

alpesh12 added some commits Jan 26, 2018

@shazron

This comment has been minimized.

Show comment
Hide comment
@shazron

shazron Jan 27, 2018

Contributor

Looks like the CI failures are paramedic related -- iOS tests pass which is good. I'll have to do a full review based on the new commits and test manually.

Contributor

shazron commented Jan 27, 2018

Looks like the CI failures are paramedic related -- iOS tests pass which is good. I'll have to do a full review based on the new commits and test manually.

@jcesarmobile

This comment has been minimized.

Show comment
Hide comment
@jcesarmobile

jcesarmobile Jan 28, 2018

Member

I've checked the code and the geolocation part won't work with this changes. Also adds code duplication and commented code. And the code to avoid the crash is only present for the case that there is no image changes (such as quality, resolution, etc).

BTW, as on the Android PR, the problem is not the corrupt image itself, but the increase of memory needed to handle it. The corrupt image works correctly with the old code in iPhone X and iPad pro, but crash the app on an iPod touch.

I like the idea of using Photos Framework to get the images, but it still requires more changes before it can be merged.

Member

jcesarmobile commented Jan 28, 2018

I've checked the code and the geolocation part won't work with this changes. Also adds code duplication and commented code. And the code to avoid the crash is only present for the case that there is no image changes (such as quality, resolution, etc).

BTW, as on the Android PR, the problem is not the corrupt image itself, but the increase of memory needed to handle it. The corrupt image works correctly with the old code in iPhone X and iPad pro, but crash the app on an iPod touch.

I like the idea of using Photos Framework to get the images, but it still requires more changes before it can be merged.

@alpesh12

This comment has been minimized.

Show comment
Hide comment
@alpesh12

alpesh12 Jan 29, 2018

Contributor

@jcesarmobile Yes correct its old code working in iPhoneX and iPad pro also its issue of Out of memory exception iPhone5.
With Photos Framework there is no change for image quality and update code for geolocation

Contributor

alpesh12 commented Jan 29, 2018

@jcesarmobile Yes correct its old code working in iPhoneX and iPad pro also its issue of Out of memory exception iPhone5.
With Photos Framework there is no change for image quality and update code for geolocation

@alpesh12

This comment has been minimized.

Show comment
Hide comment
@alpesh12

alpesh12 Feb 1, 2018

Contributor

@jcesarmobile @shazron
Any updates regarding merge

Contributor

alpesh12 commented Feb 1, 2018

@jcesarmobile @shazron
Any updates regarding merge

@jcesarmobile

This comment has been minimized.

Show comment
Hide comment
@jcesarmobile

jcesarmobile Feb 1, 2018

Member

Sorry, I have no time to review the changes at this moment, as I told you on a previous PR, I do this in my spare time and I don't have spare time right now

Member

jcesarmobile commented Feb 1, 2018

Sorry, I have no time to review the changes at this moment, as I told you on a previous PR, I do this in my spare time and I don't have spare time right now

@JayeshThanki

This comment has been minimized.

Show comment
Hide comment
@JayeshThanki

JayeshThanki commented Feb 13, 2018

+1

@LiDhaval

This comment has been minimized.

Show comment
Hide comment
@LiDhaval

LiDhaval Feb 19, 2018

+1 It will be really helpful if we get this piece of code.

LiDhaval commented Feb 19, 2018

+1 It will be really helpful if we get this piece of code.

@alpesh12

This comment has been minimized.

Show comment
Hide comment
@alpesh12

alpesh12 Feb 22, 2018

Contributor

@jcesarmobile
Any updates for merge please?

Contributor

alpesh12 commented Feb 22, 2018

@jcesarmobile
Any updates for merge please?

@LiDhaval

This comment has been minimized.

Show comment
Hide comment
@LiDhaval

LiDhaval Feb 28, 2018

@jcesarmobile Please check if you got some free time this weekend. I have tested this and working fine for me.

LiDhaval commented Feb 28, 2018

@jcesarmobile Please check if you got some free time this weekend. I have tested this and working fine for me.

@ShemrickFlannigan

This comment has been minimized.

Show comment
Hide comment
@ShemrickFlannigan

ShemrickFlannigan Feb 28, 2018

@shazron +1 For this fix; it solves several issues that we're experiencing with corrupt and/or large images. Requesting a review of this Pull Request for a merge

ShemrickFlannigan commented Feb 28, 2018

@shazron +1 For this fix; it solves several issues that we're experiencing with corrupt and/or large images. Requesting a review of this Pull Request for a merge

@jd0048

This comment has been minimized.

Show comment
Hide comment
@jd0048

jd0048 Mar 5, 2018

@jcesarmobile @shazron @alsorokin
Have you got a chance to take a look and merge this code?
i am requesting you to merge code asap.
Thank you

jd0048 commented Mar 5, 2018

@jcesarmobile @shazron @alsorokin
Have you got a chance to take a look and merge this code?
i am requesting you to merge code asap.
Thank you

@alsorokin

This comment has been minimized.

Show comment
Hide comment
@alsorokin

alsorokin Mar 6, 2018

Contributor

There's still one test failure, is it expected?

Test Case '-[CameraTest testProcessImage]' started.
2018-02-28 14:10:55.751 xctest[2330:8469] -[CDVCamera processImage:info:options:]: unrecognized selector sent to instance 0x7976c770
/Users/travis/build/apache/cordova-plugin-camera/tests/ios/CDVCameraTest/CDVCameraLibTests/CameraTest.m:481: error: -[CameraTest testProcessImage] : failed: caught "NSInvalidArgumentException", "-[CDVCamera processImage:info:options:]: unrecognized selector sent to instance 0x7976c770"
(
	0   CoreFoundation                      0x00c96ded __exceptionPreprocess + 189
	1   libobjc.A.dylib                     0x00761d6f objc_exception_throw + 49
	2   CoreFoundation                      0x00d085b8 -[NSObject(NSObject) doesNotRecognizeSelector:] + 136
	3   CoreFoundation                      0x00c1ca8d ___forwarding___ + 941
	4   CoreFoundation                      0x00c1c6be _CF_forwarding_prep_0 + 14
	5   CDVCameraLibTests                   0x0edb4568 -[CameraTest testProcessImage] + 648
	6   CoreFoundation                      0x00c1df4d __invoking___ + 29
	7   CoreFoundation                      0x00c1de81 -[NSInvocation invoke] + 321
	8   XCTest                              0x0066442b __24-[XCTestCase invokeTest]_block_invoke_2 + 502
	9   XCTest                              0x006a4b0a -[XCTestContext performInScope:] + 229
	10  XCTest                              0x00664228 -[XCTestCase invokeTest] + 259
	11  XCTest                              0x00664bc5 -[XCTestCase performTest:] + 621
	12  XCTest                              0x006949b7 -[XCTest runTest] + 45
	13  XCTest                              0x00661a39 __27-[XCTestSuite performTest:]_block_invoke + 258
	14  XCTest                              0x0066160b -[XCTestSuite _performProtectedSectionForTest:testSection:] + 32
	15  XCTest                              0x0066181b -[XCTestSuite performTest:] + 213
	16  XCTest                              0x006949b7 -[XCTest runTest] + 45
	17  XCTest                              0x00661a39 __27-[XCTestSuite performTest:]_block_invoke + 258
	18  XCTest                              0x0066160b -[XCTestSuite _performProtectedSectionForTest:testSection:] + 32
	19  XCTest                              0x0066181b -[XCTestSuite performTest:] + 213
	20  XCTest                              0x006949b7 -[XCTest runTest] + 45
	21  XCTest                              0x00661a39 __27-[XCTestSuite performTest:]_block_invoke + 258
	22  XCTest                              0x0066160b -[XCTestSuite _performProtectedSectionForTest:testSection:] + 32
	23  XCTest                              0x0066181b -[XCTestSuite performTest:] + 213
	24  XCTest                              0x006949b7 -[XCTest runTest] + 45
	25  XCTest                              0x006b0bdf __44-[XCTTestRunSession runTestsAndReturnError:]_block_invoke + 52
	26  XCTest                              0x00671af7 -[XCTestObservationCenter _observeTestExecutionForBlock:] + 699
	27  XCTest                              0x006b0a72 -[XCTTestRunSession runTestsAndReturnError:] + 295
	28  XCTest                              0x0064be10 -[XCTestDriver runTestsAndReturnError:] + 255
	29  XCTest                              0x006a83a1 _XCTestMain + 790
	30  xctest                              0x0008bab2 xctest + 6834
	31  libdyld.dylib                       0x020e6779 start + 1
)
Test Case '-[CameraTest testProcessImage]' failed (0.229 seconds).
Contributor

alsorokin commented Mar 6, 2018

There's still one test failure, is it expected?

Test Case '-[CameraTest testProcessImage]' started.
2018-02-28 14:10:55.751 xctest[2330:8469] -[CDVCamera processImage:info:options:]: unrecognized selector sent to instance 0x7976c770
/Users/travis/build/apache/cordova-plugin-camera/tests/ios/CDVCameraTest/CDVCameraLibTests/CameraTest.m:481: error: -[CameraTest testProcessImage] : failed: caught "NSInvalidArgumentException", "-[CDVCamera processImage:info:options:]: unrecognized selector sent to instance 0x7976c770"
(
	0   CoreFoundation                      0x00c96ded __exceptionPreprocess + 189
	1   libobjc.A.dylib                     0x00761d6f objc_exception_throw + 49
	2   CoreFoundation                      0x00d085b8 -[NSObject(NSObject) doesNotRecognizeSelector:] + 136
	3   CoreFoundation                      0x00c1ca8d ___forwarding___ + 941
	4   CoreFoundation                      0x00c1c6be _CF_forwarding_prep_0 + 14
	5   CDVCameraLibTests                   0x0edb4568 -[CameraTest testProcessImage] + 648
	6   CoreFoundation                      0x00c1df4d __invoking___ + 29
	7   CoreFoundation                      0x00c1de81 -[NSInvocation invoke] + 321
	8   XCTest                              0x0066442b __24-[XCTestCase invokeTest]_block_invoke_2 + 502
	9   XCTest                              0x006a4b0a -[XCTestContext performInScope:] + 229
	10  XCTest                              0x00664228 -[XCTestCase invokeTest] + 259
	11  XCTest                              0x00664bc5 -[XCTestCase performTest:] + 621
	12  XCTest                              0x006949b7 -[XCTest runTest] + 45
	13  XCTest                              0x00661a39 __27-[XCTestSuite performTest:]_block_invoke + 258
	14  XCTest                              0x0066160b -[XCTestSuite _performProtectedSectionForTest:testSection:] + 32
	15  XCTest                              0x0066181b -[XCTestSuite performTest:] + 213
	16  XCTest                              0x006949b7 -[XCTest runTest] + 45
	17  XCTest                              0x00661a39 __27-[XCTestSuite performTest:]_block_invoke + 258
	18  XCTest                              0x0066160b -[XCTestSuite _performProtectedSectionForTest:testSection:] + 32
	19  XCTest                              0x0066181b -[XCTestSuite performTest:] + 213
	20  XCTest                              0x006949b7 -[XCTest runTest] + 45
	21  XCTest                              0x00661a39 __27-[XCTestSuite performTest:]_block_invoke + 258
	22  XCTest                              0x0066160b -[XCTestSuite _performProtectedSectionForTest:testSection:] + 32
	23  XCTest                              0x0066181b -[XCTestSuite performTest:] + 213
	24  XCTest                              0x006949b7 -[XCTest runTest] + 45
	25  XCTest                              0x006b0bdf __44-[XCTTestRunSession runTestsAndReturnError:]_block_invoke + 52
	26  XCTest                              0x00671af7 -[XCTestObservationCenter _observeTestExecutionForBlock:] + 699
	27  XCTest                              0x006b0a72 -[XCTTestRunSession runTestsAndReturnError:] + 295
	28  XCTest                              0x0064be10 -[XCTestDriver runTestsAndReturnError:] + 255
	29  XCTest                              0x006a83a1 _XCTestMain + 790
	30  xctest                              0x0008bab2 xctest + 6834
	31  libdyld.dylib                       0x020e6779 start + 1
)
Test Case '-[CameraTest testProcessImage]' failed (0.229 seconds).
@alsorokin

This comment has been minimized.

Show comment
Hide comment
@alsorokin

alsorokin Mar 6, 2018

Contributor

I've tried restarting tests and it's still there

Contributor

alsorokin commented Mar 6, 2018

I've tried restarting tests and it's still there

@jd0048

This comment has been minimized.

Show comment
Hide comment
@jd0048

jd0048 Mar 6, 2018

@alsorokin i think its not expected, iOS test already passed
This error comes because of TEST_DIR=./tests/ios in this test its referring old signature of function.

jd0048 commented Mar 6, 2018

@alsorokin i think its not expected, iOS test already passed
This error comes because of TEST_DIR=./tests/ios in this test its referring old signature of function.

@alsorokin

This comment has been minimized.

Show comment
Hide comment
@alsorokin

alsorokin Mar 6, 2018

Contributor

I see. Well, If you've changed the function signature, I guess you should also change the test to pass that new argument to it.

Contributor

alsorokin commented Mar 6, 2018

I see. Well, If you've changed the function signature, I guess you should also change the test to pass that new argument to it.

@jd0048

This comment has been minimized.

Show comment
Hide comment
@jd0048

jd0048 Mar 6, 2018

How can i do that?

jd0048 commented Mar 6, 2018

How can i do that?

@alsorokin

This comment has been minimized.

Show comment
Hide comment
Contributor

alsorokin commented Mar 6, 2018

@jd0048

This comment has been minimized.

Show comment
Hide comment
@jd0048

jd0048 Mar 6, 2018

But this iOS test already passed.

jd0048 commented Mar 6, 2018

But this iOS test already passed.

@alsorokin

This comment has been minimized.

Show comment
Hide comment
@alsorokin

alsorokin Mar 6, 2018

Contributor

I'm not sure what you mean.
it is failed here:
https://travis-ci.org/apache/cordova-plugin-camera/jobs/334579377#L1781

Contributor

alsorokin commented Mar 6, 2018

I'm not sure what you mean.
it is failed here:
https://travis-ci.org/apache/cordova-plugin-camera/jobs/334579377#L1781

@jd0048

This comment has been minimized.

Show comment
Hide comment
@jd0048

jd0048 Mar 6, 2018

@alsorokin
i had test locally its showing build success for me.

jd0048 commented Mar 6, 2018

@alsorokin
i had test locally its showing build success for me.

@jd0048

This comment has been minimized.

Show comment
Hide comment
@jd0048

jd0048 Mar 6, 2018

@alsorokin
Also check above comment from @shazron

jd0048 commented Mar 6, 2018

@alsorokin
Also check above comment from @shazron

@alsorokin

This comment has been minimized.

Show comment
Hide comment
@alsorokin

alsorokin Mar 6, 2018

Contributor

Well I guess we have to wait until he reviews it then.
I don't have enough expertise in iOS to merge it anyway :)

Contributor

alsorokin commented Mar 6, 2018

Well I guess we have to wait until he reviews it then.
I don't have enough expertise in iOS to merge it anyway :)

@jd0048

This comment has been minimized.

Show comment
Hide comment
@jd0048

jd0048 Mar 7, 2018

@shazron Can you please check and merge code.

jd0048 commented Mar 7, 2018

@shazron Can you please check and merge code.

@alpesh12

This comment has been minimized.

Show comment
Hide comment
@alpesh12

alpesh12 Mar 22, 2018

Contributor

@jcesarmobile @shazron
Can you please help me to merge this code to the master, i am waiting from last one month.

Contributor

alpesh12 commented Mar 22, 2018

@jcesarmobile @shazron
Can you please help me to merge this code to the master, i am waiting from last one month.

@jcesarmobile

This comment has been minimized.

Show comment
Hide comment
@jcesarmobile

jcesarmobile Mar 22, 2018

Member

I have a long list of things to review/merge, and every time somebody pings me or comment with +1 on the PR trying to rush it, I move it to the bottom of my list.

For people commenting with +1, please, stop doing it. GitHub added the reactions long time ago, if you reaction with +1 on the PR instead of a +1 comment, then we can sort the PRs by the reactions and know which one has more +1, while a comment just spams everybody subscribed and piss off collaborators like me.

Member

jcesarmobile commented Mar 22, 2018

I have a long list of things to review/merge, and every time somebody pings me or comment with +1 on the PR trying to rush it, I move it to the bottom of my list.

For people commenting with +1, please, stop doing it. GitHub added the reactions long time ago, if you reaction with +1 on the PR instead of a +1 comment, then we can sort the PRs by the reactions and know which one has more +1, while a comment just spams everybody subscribed and piss off collaborators like me.

@alpesh12

This comment has been minimized.

Show comment
Hide comment
@alpesh12

alpesh12 Mar 22, 2018

Contributor

@jcesarmobile
when you are available to check this PR and merge?
i am waiting from last 30 days.

Contributor

alpesh12 commented Mar 22, 2018

@jcesarmobile
when you are available to check this PR and merge?
i am waiting from last 30 days.

@jcesarmobile

This comment has been minimized.

Show comment
Hide comment
@jcesarmobile

jcesarmobile Mar 22, 2018

Member

So, you didn't read my comment or you didn't care and pinged me again trying to rush me, so I just removed it from my list.
I understand that you need this and you want this merged as soon as possible, but there are hundreds of PRs at this moment, most of them are older than yours. We don't have enough resources to review and merge everything, we have to prioritize. And after you comment, this PR is no longer a priority for me.
I'm unsubscribing from the notifications, please, don't ping me again.

Member

jcesarmobile commented Mar 22, 2018

So, you didn't read my comment or you didn't care and pinged me again trying to rush me, so I just removed it from my list.
I understand that you need this and you want this merged as soon as possible, but there are hundreds of PRs at this moment, most of them are older than yours. We don't have enough resources to review and merge everything, we have to prioritize. And after you comment, this PR is no longer a priority for me.
I'm unsubscribing from the notifications, please, don't ping me again.

@alpesh12

This comment has been minimized.

Show comment
Hide comment
@alpesh12

alpesh12 Mar 22, 2018

Contributor

@jcesarmobile
i am really sorry. i didn’t mean to disturb you. Just wanted to follow up with you regarding merge. But i understand your work load so please take care of it when you get a time. Again sorry for bugging you.
Thank you

Contributor

alpesh12 commented Mar 22, 2018

@jcesarmobile
i am really sorry. i didn’t mean to disturb you. Just wanted to follow up with you regarding merge. But i understand your work load so please take care of it when you get a time. Again sorry for bugging you.
Thank you

@janpio janpio added this to ⛔ Blocked: Tests failing in Apache Cordova: Plugin Pull Requests (1) Sep 16, 2018

@janpio

This comment has been minimized.

Show comment
Hide comment
@janpio

janpio Oct 2, 2018

Contributor

1 test failure left:

Test Case '-[CameraTest testProcessImage]' failed (0.174 seconds).
Test Case '-[CameraTest testRetrieveImage]' started.
Test Case '-[CameraTest testRetrieveImage]' passed (0.316 seconds).
Test Suite 'CameraTest' failed at 2018-09-30 01:20:33.535.
	 Executed 7 tests, with 1 failure (1 unexpected) in 5.831 (5.848) seconds
Test Suite 'CDVCameraLibTests.xctest' failed at 2018-09-30 01:20:33.537.
	 Executed 7 tests, with 1 failure (1 unexpected) in 5.831 (5.852) seconds
Test Suite 'All tests' failed at 2018-09-30 01:20:33.543.
	 Executed 7 tests, with 1 failure (1 unexpected) in 5.831 (5.858) seconds
Failing tests:
	-[CameraTest testProcessImage]
Contributor

janpio commented Oct 2, 2018

1 test failure left:

Test Case '-[CameraTest testProcessImage]' failed (0.174 seconds).
Test Case '-[CameraTest testRetrieveImage]' started.
Test Case '-[CameraTest testRetrieveImage]' passed (0.316 seconds).
Test Suite 'CameraTest' failed at 2018-09-30 01:20:33.535.
	 Executed 7 tests, with 1 failure (1 unexpected) in 5.831 (5.848) seconds
Test Suite 'CDVCameraLibTests.xctest' failed at 2018-09-30 01:20:33.537.
	 Executed 7 tests, with 1 failure (1 unexpected) in 5.831 (5.852) seconds
Test Suite 'All tests' failed at 2018-09-30 01:20:33.543.
	 Executed 7 tests, with 1 failure (1 unexpected) in 5.831 (5.858) seconds
Failing tests:
	-[CameraTest testProcessImage]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment