[camera_android] Guard capture-session operations against teardown races#12224
[camera_android] Guard capture-session operations against teardown races#12224kwikwag wants to merge 2 commits into
Conversation
## Summary Prevents crashes caused by `CameraCaptureSession` being cleared, replaced, or closed while an asynchronous camera operation is still in progress. This addresses: * `NullPointerException` in `unlockAutoFocus` when the session is cleared between being checked and used. * `IllegalStateException` when an operation is submitted to a session whose underlying `CameraDevice` has already closed. * Stale capture callbacks attempting to operate on a newer capture session. Fixes flutter/flutter#82031 Fixes flutter/flutter#158878 ## Changes * Snapshot the active capture session and consistently use that snapshot throughout the camera operation, including passing it into helper methods. * Mark `captureSession` as `volatile` to ensure that updates are visible across the background and platform threads. * Ignore still-capture completion callbacks from sessions that are no longer current. * Handle `IllegalStateException` from stale or closed sessions. * Report an appropriate camera error when a still-capture request cannot continue * Add regression tests covering null, closed, replaced, and mid-operation session changes. * Mark `captureFile` as `@VisibleForTesting` for the new video-recording teardown tests. * Remove `throws` from `Camera.pausePreview` and handle exceptions internally. Some things I didn't change which might be wanted: * The AI assistant really wanted to invoke `cameraCaptureCallback.setCameraState(CameraState.STATE_PREVIEW)` on failure conditions so as not to reach bad state. However, existing exception handling didn't reset state, so this is out of scope for what this pull request is trying to fix. * The responsibility for exception handling is divided inconsistently between `Camera` and `CameraApiImpl`. Specifically this was apparent when applying the fix to `pausePreview()`, as it would be the only function where I wouldn't deal with the exception internally in `Camera`. I decided to match `pausePreview()` with `resumePreview()` in the sense that it handles exceptions internally. Generally, I think exception handling should be made consistent, either in `Camera` or in `CameraApiImpl`. In addition, FlutterError error names should be made more consistent - e.g. `CameraAccessException` sometimes bubbles up under the same name and sometime as `CameraAccess`. * Some places such as `runPrecaptureSequence` silently swallow exceptions rather than raising it to Flutter. I followed their lead, but it might be wrong. ## Testing Added Android unit tests covering: * Session removal between related capture calls. * Session replacement while an operation is in progress. * Completion callbacks arriving from stale sessions. * Closed sessions throwing `IllegalStateException`. * Null-session handling during still capture and recording.
There was a problem hiding this comment.
Code Review
This pull request addresses race conditions and crashes in the Android camera plugin by making captureSession volatile and using local session snapshots across camera operations to ensure thread safety. It also adds robust handling for IllegalStateException when the camera device is closed concurrently, along with corresponding unit tests. Feedback recommends moving the refreshPreviewCaptureSession call inside the try block of unlockAutoFocus to prevent potential unhandled IllegalStateException crashes if the session is invalidated during execution.
| void unlockAutoFocus(@NonNull CameraCaptureSession session) { | ||
| try { | ||
| // Cancel existing AF state. | ||
| previewRequestBuilder.set( | ||
| CaptureRequest.CONTROL_AF_TRIGGER, CameraMetadata.CONTROL_AF_TRIGGER_CANCEL); | ||
| captureSession.capture(previewRequestBuilder.build(), null, backgroundHandler); | ||
| session.capture(previewRequestBuilder.build(), null, backgroundHandler); | ||
|
|
||
| // Set AF state to idle again. | ||
| previewRequestBuilder.set( | ||
| CaptureRequest.CONTROL_AF_TRIGGER, CameraMetadata.CONTROL_AF_TRIGGER_IDLE); | ||
|
|
||
| captureSession.capture(previewRequestBuilder.build(), null, backgroundHandler); | ||
| session.capture(previewRequestBuilder.build(), null, backgroundHandler); | ||
| } catch (CameraAccessException e) { | ||
| String message = | ||
| (e.getMessage() == null) | ||
| ? "CameraAccessException occurred while unlocking autofocus." | ||
| : e.getMessage(); | ||
| dartMessenger.sendCameraErrorEvent(message); | ||
| return; | ||
| } catch (IllegalStateException e) { | ||
| if (BuildConfig.DEBUG) { | ||
| Log.i(TAG, "[unlockAutoFocus] capture() failed, capture session no longer valid: " + e); | ||
| } | ||
| String message = | ||
| (e.getMessage() == null) | ||
| ? "IllegalStateException occurred while unlocking autofocus." | ||
| : e.getMessage(); | ||
| dartMessenger.sendCameraErrorEvent(message); | ||
| return; | ||
| } | ||
|
|
||
| refreshPreviewCaptureSession( | ||
| session, | ||
| null, | ||
| (errorCode, errorMessage) -> | ||
| dartMessenger.error(flutterResult, errorCode, errorMessage, null)); |
There was a problem hiding this comment.
The refreshPreviewCaptureSession call is currently placed outside of the try-catch block in unlockAutoFocus. If the capture session is closed or invalidated after the initial capture calls but before refreshPreviewCaptureSession executes, session.setRepeatingRequest can throw an unhandled IllegalStateException, causing a crash. Moving refreshPreviewCaptureSession inside the try block ensures that any IllegalStateException thrown during the preview refresh is caught and handled gracefully.
void unlockAutoFocus(@NonNull CameraCaptureSession session) {
try {
// Cancel existing AF state.
previewRequestBuilder.set(
CaptureRequest.CONTROL_AF_TRIGGER, CameraMetadata.CONTROL_AF_TRIGGER_CANCEL);
session.capture(previewRequestBuilder.build(), null, backgroundHandler);
// Set AF state to idle again.
previewRequestBuilder.set(
CaptureRequest.CONTROL_AF_TRIGGER, CameraMetadata.CONTROL_AF_TRIGGER_IDLE);
session.capture(previewRequestBuilder.build(), null, backgroundHandler);
refreshPreviewCaptureSession(
session,
null,
(errorCode, errorMessage) ->
dartMessenger.error(flutterResult, errorCode, errorMessage, null));
} catch (CameraAccessException e) {
String message =
(e.getMessage() == null)
? "CameraAccessException occurred while unlocking autofocus."
: e.getMessage();
dartMessenger.sendCameraErrorEvent(message);
} catch (IllegalStateException e) {
if (BuildConfig.DEBUG) {
Log.i(TAG, "[unlockAutoFocus] capture() failed, capture session no longer valid: " + e);
}
String message =
(e.getMessage() == null)
? "IllegalStateException occurred while unlocking autofocus."
: e.getMessage();
dartMessenger.sendCameraErrorEvent(message);
}
}
Summary
Prevents crashes caused by
CameraCaptureSessionbeing cleared, replaced, or closed while an asynchronous camera operation is still in progress.This addresses:
NullPointerExceptioninunlockAutoFocuswhen the session is cleared between being checked and used.IllegalStateExceptionwhen an operation is submitted to a session whose underlyingCameraDevicehas already closed.Fixes flutter/flutter#82031
Fixes flutter/flutter#158878
Changes
captureSessionasvolatileto ensure that updates are visible across the background and platform threads.IllegalStateExceptionfrom stale or closed sessions.captureFileas@VisibleForTestingfor the new video-recording teardown tests.throwsfromCamera.pausePreviewand handle exceptions internally.Some things I didn't change which might be wanted:
cameraCaptureCallback.setCameraState(CameraState.STATE_PREVIEW)on failure conditions so as not to reach bad state. However, existing exception handling didn't reset state, so this is out of scope for what this pull request is trying to fix.CameraandCameraApiImpl. Specifically this was apparent when applying the fix topausePreview(), as it would be the only function where I wouldn't deal with the exception internally inCamera. I decided to matchpausePreview()withresumePreview()in the sense that it handles exceptions internally. Generally, I think exception handling should be made consistent, either inCameraor inCameraApiImpl. In addition, FlutterError error names should be made more consistent - e.g.CameraAccessExceptionsometimes bubbles up under the same name and sometime asCameraAccess.runPrecaptureSequencesilently swallow exceptions rather than raising it to Flutter. I followed their lead, but it might be wrong.Testing
Added Android unit tests covering:
IllegalStateException.Pre-Review Checklist
[shared_preferences]///).