-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[camera_android_camerax] Fixes crash on hot restart caused by stale observers #10529
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
base: main
Are you sure you want to change the base?
[camera_android_camerax] Fixes crash on hot restart caused by stale observers #10529
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses a crash on hot restart caused by stale observers. The fix involves adding a check in ObserverProxyApi.java to verify that an observer instance is still tracked in the InstanceManager before its onChanged callback is invoked. This prevents calls to Dart for observers that no longer have a corresponding Dart object. A new test has been added in ObserverTest.java to confirm that callbacks for untracked observers are ignored, and an existing test was updated to be more explicit about testing the valid observer path. The package version and changelog have been updated accordingly. The changes are correct and well-tested. I have one suggestion to refactor the test code to improve maintainability.
| @Test | ||
| public void onChanged_makesExpectedCallToDartCallback() { | ||
| final ObserverProxyApi mockApi = mock(ObserverProxyApi.class); | ||
| when(mockApi.getPigeonRegistrar()).thenReturn(new TestProxyApiRegistrar()); | ||
| final TestProxyApiRegistrar registrar = new TestProxyApiRegistrar(); | ||
| when(mockApi.getPigeonRegistrar()).thenReturn(registrar); | ||
|
|
||
| final ObserverProxyApi.ObserverImpl<String> instance = | ||
| new ObserverProxyApi.ObserverImpl<>(mockApi); | ||
|
|
||
| // Add the observer to the instance manager to simulate normal operation | ||
| registrar.getInstanceManager().addDartCreatedInstance(instance, 0); | ||
|
|
||
| final String value = "result"; | ||
| instance.onChanged(value); | ||
|
|
||
| verify(mockApi).onChanged(eq(instance), eq(value), any()); | ||
| } | ||
|
|
||
| @Test | ||
| public void onChanged_doesNotCallDartCallbackWhenObserverNotInInstanceManager() { | ||
| final ObserverProxyApi mockApi = mock(ObserverProxyApi.class); | ||
| final TestProxyApiRegistrar registrar = new TestProxyApiRegistrar(); | ||
| when(mockApi.getPigeonRegistrar()).thenReturn(registrar); | ||
|
|
||
| final ObserverProxyApi.ObserverImpl<String> instance = | ||
| new ObserverProxyApi.ObserverImpl<>(mockApi); | ||
|
|
||
| final String value = "result"; | ||
| instance.onChanged(value); | ||
|
|
||
| // Verify that the Dart callback is NOT invoked for stale observers | ||
| verify(mockApi, never()).onChanged(any(), any(), any()); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To improve maintainability and reduce code duplication, you can extract the common test setup into a method annotated with @org.junit.Before. This will make the tests cleaner and easier to read by separating setup logic from the test logic itself.
private ObserverProxyApi mockApi;
private TestProxyApiRegistrar registrar;
private ObserverProxyApi.ObserverImpl<String> instance;
@org.junit.Before
public void setUp() {
mockApi = mock(ObserverProxyApi.class);
registrar = new TestProxyApiRegistrar();
when(mockApi.getPigeonRegistrar()).thenReturn(registrar);
instance = new ObserverProxyApi.ObserverImpl<>(mockApi);
}
@Test
public void onChanged_makesExpectedCallToDartCallback() {
// Add the observer to the instance manager to simulate normal operation
registrar.getInstanceManager().addDartCreatedInstance(instance, 0);
final String value = "result";
instance.onChanged(value);
verify(mockApi).onChanged(eq(instance), eq(value), any());
}
@Test
public void onChanged_doesNotCallDartCallbackWhenObserverNotInInstanceManager() {
final String value = "result";
instance.onChanged(value);
// Verify that the Dart callback is NOT invoked for stale observers
verify(mockApi, never()).onChanged(any(), any(), any());
}
camsim99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this fix! I think this would definitely catch the issue and I'm open to submitting it. My only concern is that I think there's also a way to better address the root of the problem.
The root of the problem seems to be that the plugin does not handle hot restart appropriately. I see that on dispose, we remove the observers, but this won't get called on hot restart. I think we need to handle this case when the camera is re-initialized, likely in one of the createCameraWithSettings or initialize methods. Did you possibly look into this?
|
Some thing like this inside createCameraWithSettings? |
|
I think a better solution for this would be to update ProxyApiRegistrar.ignoreCallsToDart is called on the native side when the plugin is detached to prevent this crash, but it looks like we need something that also works for hot restarts. Also there is still a small chance that this crash can still happen with this fix if the Observer makes a call to Dart before it is cleared from the |
…the instance manager calls to Dart (#10552) In the event of a hot restart, the native `InstanceManagers` are cleared. There is then a chance for a class to make a callback when it is no in the instance manager which leads to a null exception in the codec. Part of fixing flutter/flutter#176451 Pigeon side fix for #10529 ## Pre-Review Checklist **Note**: The Flutter team is currently trialing the use of [Gemini Code Assist for GitHub](https://developers.google.com/gemini-code-assist/docs/review-github-code). Comments from the `gemini-code-assist` bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed. [^1]: Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling.
fixes Issue #176451
Pre-Review Checklist
[shared_preferences]pubspec.yamlwith an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under[^1].CHANGELOG.mdto add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under[^1].///).Description