Skip to content
This repository was archived by the owner on Feb 22, 2023. It is now read-only.

Fixed image picker crash when used with android alarm manager #1125

Merged
cyanglaz merged 7 commits into from
Feb 8, 2019
Merged

Fixed image picker crash when used with android alarm manager #1125

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
62784a1
do not allow image picker to be registered when there is no activity
Jan 28, 2019
5019a28
reword changelog
Jan 28, 2019
7a1266f
added a unit test
Jan 29, 2019
03fc4f4
format fix
Feb 6, 2019
7d9f868
merge master
Feb 6, 2019
b438927
add comments
Feb 8, 2019
32ad05a
merge master
Feb 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/image_picker/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
## 0.5.0+2

* Fixed am Android crash when Image Picker is registered without an activity.

## 0.5.0+1

* Fix a crash when user calls the plugin in quick succession on Android.
Expand Down
3 changes: 3 additions & 0 deletions .../image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ public class ImagePickerPlugin implements MethodChannel.MethodCallHandler {
private final ImagePickerDelegate delegate;

public static void registerWith(PluginRegistry.Registrar registrar) {
if (registrar.activity() == null) {
Copy link
Contributor

@amirh amirh Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will prevent the crash but IIUC leaves us with an unregistered plugin, as even if the activity asks the GeneratedPluginRegistrant to register again it will short circuit.

Unless I'm missing something this will trade the startup crash with a crash the first time the plugin is used (as the method channel isn't registered).

I believe the only eager use of activity is getExternalFilesDir and this can be done with the application context which is guaranteed to be available.
It looks like ImagePickerDelegate is caching the activity that's being passed to it from here, I believe instead of doing that we can make ImagePickerDelegate always ask the activity from the registrar when it needs an activity.

@cyanglaz @Hixie am I getting this wrong? is the current fix actually leaving the picker in a working state? are you interested in merging this for now just to prevent the early crash or doing a little more refactoring here?

Copy link
Contributor

@Hixie Hixie Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to test if the picker was in a working state with this fix -- the problem I was seeing was that when the app was starting in the background (for the alarm manager), nothing was working, because the plugin would crash and prevent anything else from happening. In that case, I don't actually care about the image picker.

I've no idea if it's possible for the app to then get re-used in the foreground. If so, then I agree that this wouldn't work.

Copy link
Contributor

@amirh amirh Feb 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point for the scenario that the app is executed in the background and then quits (and the Android APP is destroyed) it's ok to live with the plugin unregistered. So this fix at least improves some cases, we can land it now. We should probably follow-up and figure what happens if e.g the app's background code is calling startActivity and starts an activity of the same (Android) application with a FlutterView.

return;
}
final MethodChannel channel = new MethodChannel(registrar.messenger(), CHANNEL);

final File externalFilesDirectory =
Expand Down
9 changes: 9 additions & 0 deletions ...ample/android/app/src/test/java/io/flutter/plugins/imagepicker/ImagePickerPluginTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.flutter.plugins.imagepicker;

import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.verifyZeroInteractions;
import static org.mockito.Mockito.when;
Expand Down Expand Up @@ -95,6 +96,14 @@ public void onMethodCall_WhenSourceIsCamera_InvokesTakeImageWithCamera() {
verifyZeroInteractions(mockResult);
}

@Test
public void onResiter_WhenAcitivityIsNull_ShouldNotCrash() {
when(mockRegistrar.activity()).thenReturn(null);
ImagePickerPlugin.registerWith((mockRegistrar));
assertTrue(
"No exception thrown when ImagePickerPlugin.registerWith ran with activity = null", true);
}

private MethodCall buildMethodCall(final int source) {
final Map<String, Object> arguments = new HashMap<>();
arguments.put("source", source);
Expand Down