Skip to content

Conversation

@elsewhat
Copy link

@elsewhat elsewhat commented Apr 7, 2016

Catch OutOfMemoryError for memory intensive operations and handle them
gracefully.

Tested on a Samsung SII which throws OutOfMemoryException when taking a photo and fetching it as a DATA_URL.Not able to test when selecting from gallery

Catch OutOfMemoryError for memory intensive operations and handle them
gracefully
@nikhilkh
Copy link
Contributor

nikhilkh commented Apr 8, 2016

Silently handling OutOfMemoryError is not a good idea - we should look to optimize the code so that it uses less memory instead.

@riknoll
Copy link
Contributor

riknoll commented Apr 9, 2016

Ah, I think I misunderstood your JIRA and I agree with @nikhilkh that optimizing our memory use is the right approach. Looking through the code, it seems we already consume that exception in a number of places when dealing with bitmaps which is probably a very bad thing. Google provides some guidance here on managing bitmap memory.

@riknoll
Copy link
Contributor

riknoll commented Apr 9, 2016

There are definitely cases where native exceptions should be surfaced to the javascript, but memory issues are the responsibility of the native developer. There isn't much that can be done from the Webview to correctly handle that situation

@elsewhat
Copy link
Author

elsewhat commented Apr 9, 2016

@riknoll @nikhilkh as an app developer I have to disagree with you.
I would expect a framework such as cordova to never crash an app under no circumstances if it's preventable.
Crashing an app is a very significant event which impacts users perception of the app very negatively and causes a significant effort wrt. end-user support for enterprise apps. It will in many cases cause entered data to be lost, and nothing frustrates users more.

There is lots the app can do to handle such an exception. In my case I can make sure the users understand the issue is because the size of the image and how they can perform workarounds.

Ideally, the CameraLauncher.failPicture would allow me to pass some semantics regarding the nature of the issue for example an error code. But since this was not in place already in the code, I didn't do any changes to it.

However, I totally agree that refactoring the code to reduce the memory footprint is an even better approach which actually tackles the root cause. But this doesn't negate the need to avoid OutOfMemoryErrors when performing memory intensive operations. Even with an exceptionally optimized memory handling of images in cordova-camera-plugin there will always be situations where an OutOfMemoryError occurs, for example if the app uses a lot of memory on the webview side and the device is old with a strict limit on heap size (a G1 had 16MB of heap)

@asfgit asfgit closed this in 2d2352f Nov 8, 2016
thehuijb added a commit to thehuijb/cordova-plugin-camera that referenced this pull request Nov 15, 2016
* master: (22 commits)
  CB-12086 Regenerate README.md from template
  Added NSPhotoLibraryUsageDescription parameter to example install command Fixing some usages of NSPhotoLibraryUsageDescriptionentry
  Close apache#124: I can crop fine with Photos.  We should not have adopted Crop, since it makes no sense on Android.
  Updating compat dependency to 1.1.0 or better
  Close apache#199.  We save photos to a shared Pictures directory, similar to the behaviour of the Twitter application
  Close apache#201.  Running out of memory shouldn't be graceful.
  Close apache#228.  We don't require these permissions on Camera, since we use intents.
  Close apache#241
  Bumping the CI
  CB-11625: Forgot to add CordovaUri.java to plugin.xml
  CB-11625: Files Provider does not work with Android 4.4.4 or lower, and I have no idea why.  Working around with CordovaUri
  CB-11625 (Android) : Make this work with previous versions of Cordova via cordova-plugin-compat
  CB-11917 - Remove pull request template checklist item: "iCLA has been submitted…"
  Closing invalid pull request: close apache#98
  CB-11832 Incremented plugin version.
  CB-11832 Updated version and RELEASENOTES.md for release 2.3.0
  BuildConfig from test project crept in source code thanks to Android Studio, removing
  WTF.  Directory, not file.  Not sure why I did that
  CB-11625: Managed to get Content Providers to work with a weird mix of Content Providers and non-Content Providers
  Adding provider_paths.xml so this works
  ...

# Conflicts:
#	src/android/CameraLauncher.java
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