[pigeon] Prevents premature finalization from unused Dart instance for ProxyApis #9231
Conversation
bparrishMines
changed the title
| @@ -41,6 +41,7 @@ | |||
| - io | |||
| - js | |||
| - json_serializable | |||
| - leak_tracker | |||
There was a problem hiding this comment.
Added to use forceGC
packages/pigeon/platform_tests/shared_test_plugin_code/test/instance_manager_test.dart
Outdated
Show resolved
Hide resolved
| @@ -110,8 +110,17 @@ class $dartInstanceManagerClassName { | |||
| /// | |||
| /// Returns the randomly generated id of the [instance] added. | |||
| int addDartCreatedInstance($proxyApiBaseClassName instance) { | |||
| assert(getIdentifier(instance) == null); | |||
There was a problem hiding this comment.
small feedback: adding a msg to this assert
There was a problem hiding this comment.
It is usually my preference that adding a message is dependent on whether the assertion is self explanatory to the person who uses the method. InstanceManager.addDartCreatedInstance should only ever be called by the generated code, so I assume a maintainer of pigeon would understand this error without a message. While a user of pigeon would not get much value from an error message that states the InstanceManager already contains the instance.
But I'm biased since I wrote this, so I could be convinced if the intention of the check is not immediately clear to a new maintainer.
|
I'm testing this a bit, and noticed that any proxy api class that has a @static property is giving me this assertion error when doing a hot restart:
|
|
@jointhejourney Thanks for catching this. I added a fix that detaches the finalizers when the Swift |
bparrishMines
added
the
autosubmit
…nstance for ProxyApis (flutter/packages#9231)
flutter/packages@1468581...2dff621 2025-05-15 10687576+bparrishMines@users.noreply.github.com [pigeon] Prevents premature finalization from unused Dart instance for ProxyApis (flutter/packages#9231) 2025-05-14 louisehsu@google.com [in_app_purchase] Bump `in_app_purchase_storekit` version (flutter/packages#9247) 2025-05-14 engine-flutter-autoroll@skia.org Roll Flutter from 336a7ec to 0b9f928 (22 revisions) (flutter/packages#9251) 2025-05-14 10687576+bparrishMines@users.noreply.github.com [webview_flutter_android][webview_flutter_wkwebview] Adds support to set whether to draw the scrollbar (flutter/packages#9249) 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 flutter-ecosystem@google.com 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://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
…r ProxyApis (flutter#9231) 1. The Dart `InstanceManager.addHostCreatedInstance` is only ever used to respond to native calls that create a new instance right before the instance is returned in a host method call or passed in a flutter method call. So, `addHostCreatedInstance` now only adds a strong referenced instance and not the weak referenced instance. This will ensure the weak referenced instance is not created until it is being passed to a user immediately. 2. The Dart `InstanceManager.remove` now asserts that the strong referenced instance isn't removed before the weak referenced instance. This should prevent future scenarios where the user is holding a weak referenced instance while the native side no longer has any reference. Fixes flutter/flutter#168531 ## Pre-Review Checklist [^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.
InstanceManager.addHostCreatedInstanceis only ever used to respond to native calls that create a new instance right before the instance is returned in a host method call or passed in a flutter method call. So,addHostCreatedInstancenow only adds a strong referenced instance and not the weak referenced instance. This will ensure the weak referenced instance is not created until it is being passed to a user immediately.InstanceManager.removenow asserts that the strong referenced instance isn't removed before the weak referenced instance. This should prevent future scenarios where the user is holding a weak referenced instance while the native side no longer has any reference.Fixes flutter/flutter#168531
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 under1.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 under1.///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2 ↩3