[webview_flutter_android] [camera_android_camerax] Updates internal Java InstanceManager to only stop finalization callbacks when stopped #3571
Conversation
bparrishMines
changed the title
bparrishMines
changed the title
| handler.removeCallbacks(this::releaseAllFinalizedInstances); | ||
| isClosed = true; | ||
| clear(); | ||
| hasFinalizationListenerStopped = true; |
There was a problem hiding this comment.
Just to make sure I understand correctly, can you confirm the following is correct?
After this method called, objects that have been added to this instance manager can be garbage collected. Thus, objects might be contained by the instance manager (because we aren't releasing the resources/calling clear() anymore), but also might not. So basically, the instance manager is now unreliable, but with this change, it's up the developer to be aware of that versus getting a null, -1, etc. value back when attempting to call a method?
There was a problem hiding this comment.
When a new instance is added to the manager, the manager creates a strong reference and a weak reference to the instance. When remove(Object) is called, the strong reference is removed. Therefore, the weak reference to the instance will remain until it is garbage collected. The manager now functions this way whether this value is true or false. The only difference now is that the finalizationListener that was passed in InstanceManager.create is no longer called when the weak reference to an instance is claimed by garbage collection.
There was a problem hiding this comment.
Oh, I see. This makes sense!
There was a problem hiding this comment.
Is it possible to test the case where the weak reference was claimed by garbage collection to see that the finalizationListener is no longer called? I figure it's not but asking just in case!
There was a problem hiding this comment.
It is possible with the Runtime.getRuntime().gc(); method. I tried to add a unit test, but the Android Handler class is just a stub when running unit tests. So I created an integration test.
camsim99
left a comment
camsim99
left a comment
There was a problem hiding this comment.
LGTM! Just left a suggestion about a test
| handler.removeCallbacks(this::releaseAllFinalizedInstances); | ||
| isClosed = true; | ||
| clear(); | ||
| hasFinalizationListenerStopped = true; |
There was a problem hiding this comment.
Oh, I see. This makes sense!
| handler.removeCallbacks(this::releaseAllFinalizedInstances); | ||
| isClosed = true; | ||
| clear(); | ||
| hasFinalizationListenerStopped = true; |
There was a problem hiding this comment.
Is it possible to test the case where the weak reference was claimed by garbage collection to see that the finalizationListener is no longer called? I figure it's not but asking just in case!
...flutter_android/android/src/main/java/io/flutter/plugins/webviewflutter/InstanceManager.java
Show resolved
Hide resolved
| handler.removeCallbacks(this::releaseAllFinalizedInstances); | ||
| isClosed = true; | ||
| clear(); | ||
| hasFinalizationListenerStopped = true; |
There was a problem hiding this comment.
Is it possible to derive this value instead of using a boolean?
For example if handler.removeCallbacks does not have any more callbacks?
There was a problem hiding this comment.
It looks like there is a Handler.hasCallbacks, but it requires Android 29+ unfortunately.
bparrishMines
added
the
autosubmit
…ternal Java InstanceManager to only stop finalization callbacks when stopped (flutter/packages#3571)
flutter/packages@e4ec155...0277f2a 2023-04-16 [email protected] Roll Flutter from 00171b0 to 50171bb (7 revisions) (flutter/packages#3723) 2023-04-15 [email protected] Roll Flutter from f740544 to 00171b0 (17 revisions) (flutter/packages#3717) 2023-04-15 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.2.9 to 2.2.12 (flutter/packages#3711) 2023-04-15 49699333+dependabot[bot]@users.noreply.github.com Bump actions/checkout from 3.5.0 to 3.5.2 (flutter/packages#3710) 2023-04-14 [email protected] [path_provider] Fix Android lint warnings (flutter/packages#3706) 2023-04-14 [email protected] [webview_flutter_android] [camera_android_camerax] Updates internal Java InstanceManager to only stop finalization callbacks when stopped (flutter/packages#3571) 2023-04-14 [email protected] [shared_preferences] Fix Android Java version issue (flutter/packages#3712) 2023-04-14 [email protected] [image_picker][android] Non-bitmap images now return path instead of null (flutter/packages#3590) 2023-04-14 [email protected] Roll Flutter from be45eb2 to f740544 (24 revisions) (flutter/packages#3713) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…ava InstanceManager to only stop finalization callbacks when stopped (flutter#3571)
3 participants
Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.