[image_picker] Fixes activity leak by 0xZOne · Pull Request #4439 · flutter/plugins

0xZOne · 2021-10-20T03:33:02Z

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

stuartmorgan-g · 2021-10-20T20:25:24Z

Thanks for the submission! You removed most of the PR checklist; it's there for a reason, and many of the items you removed are not optional. I'm going to mark this as a draft for now; please restore the original checklist, address the missing elements, then remove the draft designation so that we can get it reviewed.

0xZOne · 2021-10-21T01:45:10Z

Thanks for the submission! You removed most of the PR checklist; it's there for a reason, and many of the items you removed are not optional. I'm going to mark this as a draft for now; please restore the original checklist, address the missing elements, then remove the draft designation so that we can get it reviewed.

Sorry, I misunderstood. Those removed items are restored now.

stuartmorgan-g

I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.

I updated CHANGELOG.md to add a description of the change.

This change needs these steps. Please see the first link there.

I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.

As discussed at length in the contributing docs, this step is required.

0xZOne · 2021-10-22T04:28:09Z

I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy.

I updated CHANGELOG.md to add a description of the change.

This change needs these steps. Please see the first link there.

I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test exempt.

As discussed at length in the contributing docs, this step is required.

Just simply recycling resources in tearDown, I don't think there is a need for test cases.

It is hard to add tests here. Any guidance? Thanks. @stuartmorgan

stuartmorgan-g · 2021-10-22T18:16:52Z

Just simply recycling resources in tearDown, I don't think there is a need for test cases.

You appear to believe that the bug needs to be fixed, since you filed an issue and submitted a PR. Why wouldn't you want to prevent it from regressing again in the future?

But regardless, PRs here follow Flutter policy (which you've indicated in the checklist that you have read), and personal opinions about the value of tests isn't one of the exemptions listed in that policy.

It is hard to add tests here. Any guidance?

There are lots of possible ways to test this. In order to make the code as future-proof as possible, I would probably move all of this activity-lifetime-bound state into a helper object, so that setup and tearDown would just become constructor and finalize calls of the helper object. Then you could put a package-level accessor on the plugin to see if that helper object is null after the activity is detached, which ensures that all state references have been cleared.

/cc @blasten in case he has alternate preferences.

0xZOne · 2021-10-23T01:41:43Z

@stuartmorgan Thank you for your rapid reply and patient.

stuartmorgan-g

This change needs a version bump since it's changing code behavior, but otherwise looks good to me at a high level.

@blasten Please assign a primary reviewer from the Android side.

stuartmorgan-g

LGTM with nits

stuartmorgan-g · 2022-01-24T17:44:47Z


 * Updates Android compileSdkVersion to 31.
 * Fix iOS RunnerUITests search paths.
+* Fix Activity leak


Please add the missing period at the end.

Also this should be "Fixes" per the repo's new CHANGELOG style guide. (It would be great if you could update the line above it the same way.)

stuartmorgan-g · 2022-01-24T17:46:03Z

-    channel = null;
-    application.unregisterActivityLifecycleCallbacks(observer);
-    application = null;
+    activityState.release();


Doesn't this need a null check?

stuartmorgan-g · 2022-02-07T20:39:16Z

@blasten Ping for second review here?

blasten · 2022-02-24T02:11:48Z

+        // V1 embedding setup for activity listeners.
+        application.registerActivityLifecycleCallbacks(observer);
+        registrar.addActivityResultListener(delegate);
+        registrar.addRequestPermissionsResultListener(delegate);


I will just throw here. We planned on removing the v1 embedding this quarter. cc @GaryQian

It is more appropriate to remove the v1 embedding in another PR.

blasten · 2022-02-24T02:17:54Z

+      }
+    }
+
+    ActivityState(final ImagePickerDelegate delegate, final Activity activity) {


this needs javadocs. when is this constructor preferred over the other one?

blasten · 2022-02-24T02:18:32Z

+    // This is null when not using v2 embedding;
+    private Lifecycle lifecycle;
+
+    ActivityState(


needs java doc

the behavior of ActivityState#release() depends upon which constructor is used.

blasten · 2022-02-24T02:22:11Z

  }

+  @Test
+  public void onDetachedFromActivity_ShouldReleaseActivityState() {


what about testing ActivityState#release() - Is that covered somehow?

ActivityState#release() is an internal helper function and does not need to add special test cases. Here we just need to make sure that ActivityState is released as expected after onDetachedFromActivity is called.

blasten

LGTM

stuartmorgan-g

No version change: The PR does not involve external interface changes.

That's not how our version change policy works. This needs a version change. (I've removed this from the description so that our CI checks will work correctly.)

Please read the explanation of versioning linked from the checklist item about versions.

stuartmorgan-g · 2022-03-08T19:02:15Z

 * Updates Android compileSdkVersion to 31.
-* Fix iOS RunnerUITests search paths.
+* Fixes iOS RunnerUITests search paths.
+* Fixes Activity leak.


Version 0.8.4+5, which is now 5 versions old, did not include this fix.

stuartmorgan-g · 2022-03-09T02:49:23Z

 * Improves the documentation on handling MainActivity being killed by the Android OS.
 * Updates Android compileSdkVersion to 31.
-* Fix iOS RunnerUITests search paths.
+* Fixes iOS RunnerUITests search paths.


Please don't change old versions' notes.

stuartmorgan-g · 2022-03-09T02:50:09Z

@@ -1,3 +1,7 @@
+## NEXT


As I said before, and as it says in the document I referenced above, this PR needs a version change.

stuartmorgan-g

LGTM

0xZOne requested a review from cyanglaz as a code owner October 20, 2021 03:33

google-cla bot added the cla: yes label Oct 20, 2021

github-actions bot added p: image_picker platform-android labels Oct 20, 2021

stuartmorgan-g marked this pull request as draft October 20, 2021 20:25

0xZOne force-pushed the task/image_picker_leaks branch from f565703 to 0b4c890 Compare October 21, 2021 01:08

0xZOne marked this pull request as ready for review October 21, 2021 01:58

stuartmorgan-g suggested changes Oct 21, 2021

View reviewed changes

stuartmorgan-g added the needs tests label Oct 21, 2021

0xZOne force-pushed the task/image_picker_leaks branch 2 times, most recently from fc3b3a6 to b096c1c Compare October 22, 2021 04:23

0xZOne requested a review from stuartmorgan-g October 22, 2021 04:29

0xZOne force-pushed the task/image_picker_leaks branch from c9cfde7 to 68f0c30 Compare October 24, 2021 07:15

stuartmorgan-g suggested changes Oct 27, 2021

View reviewed changes

Comment thread ...ker/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerPlugin.java Outdated

stuartmorgan-g reviewed Oct 29, 2021

View reviewed changes

Comment thread ...ker/image_picker/android/src/main/java/io/flutter/plugins/imagepicker/ImagePickerPlugin.java

0xZOne force-pushed the task/image_picker_leaks branch from c52d2de to c7108c1 Compare October 30, 2021 01:44

0xZOne requested a review from stuartmorgan-g November 5, 2021 01:33

0xZOne marked this pull request as draft December 6, 2021 01:16

0xZOne marked this pull request as ready for review December 6, 2021 11:12

0xZOne force-pushed the task/image_picker_leaks branch 2 times, most recently from b966ec3 to 1067b74 Compare December 9, 2021 11:38

godofredoc changed the base branch from master to main January 6, 2022 22:46

stuartmorgan-g suggested changes Jan 21, 2022

View reviewed changes

stuartmorgan-g requested review from blasten and removed request for cyanglaz January 21, 2022 17:24

stuartmorgan-g approved these changes Jan 24, 2022

View reviewed changes

0xZOne force-pushed the task/image_picker_leaks branch from 2ec0b45 to 60559bb Compare January 25, 2022 01:47

0xZOne changed the title ~~[image_picker] Fix activity leak~~ [image_picker] Fixes activity leak Jan 25, 2022

0xZOne force-pushed the task/image_picker_leaks branch 2 times, most recently from 1860782 to c113535 Compare January 26, 2022 06:22

blasten reviewed Feb 24, 2022

View reviewed changes

0xZOne force-pushed the task/image_picker_leaks branch 2 times, most recently from 670c612 to b191e9c Compare March 4, 2022 07:20

0xZOne requested a review from blasten March 4, 2022 07:21

blasten approved these changes Mar 8, 2022

View reviewed changes

stuartmorgan-g suggested changes Mar 8, 2022

View reviewed changes

0xZOne force-pushed the task/image_picker_leaks branch from b191e9c to 833d9ae Compare March 9, 2022 01:40

0xZOne requested a review from stuartmorgan-g March 9, 2022 01:42

stuartmorgan-g suggested changes Mar 9, 2022

View reviewed changes

0xZOne requested a review from stuartmorgan-g March 9, 2022 05:44

0xZOne added 2 commits March 10, 2022 09:43

[image_picker] Fixes activity leak

cdda38f

change version

86a271e

0xZOne force-pushed the task/image_picker_leaks branch from 72ff357 to 86a271e Compare March 10, 2022 01:44

stuartmorgan-g removed the needs tests label Mar 10, 2022

stuartmorgan-g approved these changes Mar 10, 2022

View reviewed changes

stuartmorgan-g added the waiting for tree to go green (Use "autosubmit") This PR is approved and tested, but waiting for the tree to be green to land. label Mar 10, 2022

fluttergithubbot merged commit 6ff862f into flutter:main Mar 10, 2022

engine-flutter-autoroll mentioned this pull request Mar 11, 2022

Roll Plugins from 20e231b0529c to 0ade756afed0 (3 revisions) flutter/flutter#99979

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2022

6ff862fa7 [image_picker] Fixes activity leak (flutter/plugins#4439)

025c3cd

engine-flutter-autoroll mentioned this pull request Mar 11, 2022

Roll Plugins from 20e231b0529c to 0ade756afed0 (3 revisions) flutter/flutter#99990

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 11, 2022

6ff862fa7 [image_picker] Fixes activity leak (flutter/plugins#4439)

e98bb71

Conversation

0xZOne commented Oct 20, 2021 • edited by stuartmorgan-g Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

stuartmorgan-g commented Oct 20, 2021

Uh oh!

0xZOne commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

0xZOne commented Oct 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stuartmorgan-g commented Oct 22, 2021

Uh oh!

0xZOne commented Oct 23, 2021

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g commented Feb 7, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blasten left a comment

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

0xZOne commented Oct 20, 2021 •

edited by stuartmorgan-g

Loading

0xZOne commented Oct 21, 2021 •

edited

Loading

0xZOne commented Oct 22, 2021 •

edited

Loading

stuartmorgan-g left a comment •

edited

Loading