[pigeon] Prevents premature finalization from unused Dart instance for ProxyApis by bparrishMines · Pull Request #9231 · flutter/packages

bparrishMines · 2025-05-08T22:57:20Z

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.
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.

Pre-Review Checklist

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

Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩² ↩³

…_finalize

bparrishMines · 2025-05-09T18:35:53Z

 - io
 - js
 - json_serializable
+- leak_tracker


Added to use forceGC

tarrinneal

little nit

jointhejourney · 2025-05-10T02:05:12Z

  ///
  /// Returns the randomly generated id of the [instance] added.
  int addDartCreatedInstance($proxyApiBaseClassName instance) {
+    assert(getIdentifier(instance) == null);


small feedback: adding a msg to this assert

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.

jointhejourney · 2025-05-10T02:29:06Z

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:

_AssertionError (Failed assertion: line 258 pos 7: 'instance == null': A strong instance with identifier 0 is being removed despite the weak reference still existing: Instance of 'JourneyProvider')

stuartmorgan-g

LGTM

…_finalize

bparrishMines · 2025-05-13T19:49:33Z

@jointhejourney Thanks for catching this. I added a fix that detaches the finalizers when the Swift InstanceManager.removeAllObjects is called.

…tance for ProxyApis (flutter/packages#9231)

flutter/packages@1468581...2dff621 2025-05-15 [email protected] [pigeon] Prevents premature finalization from unused Dart instance for ProxyApis (flutter/packages#9231) 2025-05-14 [email protected] [in_app_purchase] Bump `in_app_purchase_storekit` version (flutter/packages#9247) 2025-05-14 [email protected] Roll Flutter from 336a7ec to 0b9f928 (22 revisions) (flutter/packages#9251) 2025-05-14 [email protected] [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 [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://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.

flutter/packages@1468581...2dff621 2025-05-15 [email protected] [pigeon] Prevents premature finalization from unused Dart instance for ProxyApis (flutter/packages#9231) 2025-05-14 [email protected] [in_app_purchase] Bump `in_app_purchase_storekit` version (flutter/packages#9247) 2025-05-14 [email protected] Roll Flutter from 470f481 to 5557d57 (22 revisions) (flutter/packages#9251) 2025-05-14 [email protected] [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 [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://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

bparrishMines added 2 commits May 8, 2025 17:47

fix premature finalization

5976f79

handle adding differently

594ede7

github-actions Bot added the p: pigeon label May 8, 2025

add leak tracker

e03c89d

bparrishMines changed the title ~~Manager early finalize~~ [pigeon] Prevents premature finalization from unused Dart instance for ProxyApis May 8, 2025

bparrishMines added 2 commits May 8, 2025 19:53

change leak tracker to any

380f98d

update changelog

54f6f59

bparrishMines marked this pull request as ready for review May 9, 2025 18:34

bparrishMines requested a review from tarrinneal as a code owner May 9, 2025 18:34

Merge branch 'main' of github.com:flutter/packages into manager_early…

eb5e0e1

…_finalize

bparrishMines commented May 9, 2025

View reviewed changes

bparrishMines requested a review from stuartmorgan-g May 9, 2025 18:38

tarrinneal approved these changes May 9, 2025

View reviewed changes

Comment thread packages/pigeon/platform_tests/shared_test_plugin_code/test/instance_manager_test.dart Outdated

jointhejourney reviewed May 10, 2025

View reviewed changes

stuartmorgan-g approved these changes May 12, 2025

View reviewed changes

bparrishMines added 2 commits May 12, 2025 16:47

clear finalization listeners too

61285a9

Merge branch 'main' of github.com:flutter/packages into manager_early…

f2d5a01

…_finalize

bparrishMines requested review from LouiseHsu and hellohuanlin as code owners May 12, 2025 20:47

github-actions Bot added platform-android platform-ios platform-macos labels May 12, 2025

bparrishMines added 4 commits May 12, 2025 17:21

fix finalizer name

48aa328

make delegate nil as well

ee3743e

Merge branch 'main' of github.com:flutter/packages into manager_early…

13bcf19

…_finalize

fix test

1678acb

fix if

349d01b

bparrishMines added the autosubmit Merge PR when tree becomes green via auto submit App label May 15, 2025

auto-submit Bot merged commit 2dff621 into flutter:main May 15, 2025

bparrishMines deleted the manager_early_finalize branch May 15, 2025 00:59

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request May 15, 2025

2dff621 [pigeon] Prevents premature finalization from unused Dart ins…

0eb10bb

…tance for ProxyApis (flutter/packages#9231)

engine-flutter-autoroll mentioned this pull request May 15, 2025

Roll Packages from 14685811ed39 to 2dff6213a892 (4 revisions) flutter/flutter#168908

Merged

bparrishMines mentioned this pull request May 21, 2025

[webview_flutter_wkwebview] Crash when restarting in debug mode on iOS – Host platform returned null value for non-null return value flutter/flutter#168306

Closed

Navigation Menu

Repository navigation

[pigeon] Prevents premature finalization from unused Dart instance for ProxyApis - #9231

[pigeon] Prevents premature finalization from unused Dart instance for ProxyApis#9231
auto-submit[bot] merged 13 commits into
flutter:mainfrom
bparrishMines:manager_early_finalize

bparrishMines commented May 8, 2025 •

edited

Loading

Uh oh!

bparrishMines May 9, 2025

Uh oh!

tarrinneal left a comment

Uh oh!

Uh oh!

jointhejourney May 10, 2025

Uh oh!

bparrishMines May 13, 2025 •

edited

Loading

Uh oh!

jointhejourney commented May 10, 2025

Uh oh!

stuartmorgan-g left a comment

Uh oh!

bparrishMines commented May 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

bparrishMines commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-Review Checklist

Footnotes

Uh oh!

bparrishMines May 9, 2025

Choose a reason for hiding this comment

Uh oh!

tarrinneal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jointhejourney May 10, 2025

Choose a reason for hiding this comment

Uh oh!

bparrishMines May 13, 2025 • 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!

jointhejourney commented May 10, 2025

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

bparrishMines commented May 13, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bparrishMines commented May 8, 2025 •

edited

Loading

bparrishMines May 13, 2025 •

edited

Loading