DraggableScrollableController should dispatch creation in constructor. #135423
Conversation
github-actions
bot
added
framework
| class DraggableScrollableController extends ChangeNotifier { | ||
| /// Creates a controller for [DraggableScrollableSheet]. | ||
| DraggableScrollableController() { | ||
| if (kFlutterMemoryAllocationsEnabled) { |
There was a problem hiding this comment.
I am less familiar here, what is kFlutterMemoryAllocationsEnabled? Will it be on by default in the future? If so, are there plans to come back and remove this?
There was a problem hiding this comment.
The kFlutterMemoryAllocationsEnabled is true only in the debug mode. I think setting it to true by default is not needed in production and won't be in the future. In the future, creation/disposal dispatching will be separated into Disposable mixin. You can read about future plans here: https://docs.google.com/document/d/1cjylBOczGk70x-sn1iZYVNAfpPZdtGWsm_KzoNQX0jc/edit#heading=h.b5wf7efqvos
@polina-c, please correct me if I'm wrong.
There was a problem hiding this comment.
True, this flag is only on, when leak tracking is needed.
By default it is on only in debug mode, to tree shake leak tracking code and to have better performance. When needed the flag can be enabled for release and profile mode by compiling with this flag.
ChangeNotifier is special disposable, because it is mixin and thus does not have constructor.
See more details in documentation for maybeDispatchObjectCreation:
Does it help?
There was a problem hiding this comment.
This will go away if we can enable object tacing without taking stack trace: https://docs.google.com/document/d/1cjylBOczGk70x-sn1iZYVNAfpPZdtGWsm_KzoNQX0jc/edit#bookmark=id.vp85epkbqmp9
There was a problem hiding this comment.
We need to add maybeDispatchObjectCreation, because DraggableScrollableController leaks in some tests and it is not possible to detect who is owner, that created, but did not dispose the object.
There was a problem hiding this comment.
Ah ok! So eventually in the future we won't need to include kFlutterMemoryAllocationsEnabled in every ChangeNotifier like this? It made me think, hm this should probably be in a lower level of the framework by default so we don't have to remember to include it every time we create a ChangeNotifier.
There was a problem hiding this comment.
Yes, this suggestion is exactly to address this at lower level: https://docs.google.com/document/d/1cjylBOczGk70x-sn1iZYVNAfpPZdtGWsm_KzoNQX0jc/edit#bookmark=id.vp85epkbqmp9
If we forget to include the invocation to the child of ChangeNotifier, it is not big deal, because it can be added after, per instruction for leak troubleshooting, when it is hard to detect who is owner: https://github.com/dart-lang/leak_tracker/blob/main/doc/TROUBLESHOOT.md#1-it-is-not-clear-who-owns-changenotifier
polina-c
added
the
a: leak tracking
flutter/flutter@57b5c3c...d3df8f6 2023-09-30 [email protected] Roll Flutter Engine from 68fc1a567195 to 01fc3c6624ce (1 revision) (flutter/flutter#135808) 2023-09-30 [email protected] Roll Flutter Engine from 4ed733d99cd5 to 68fc1a567195 (1 revision) (flutter/flutter#135807) 2023-09-30 [email protected] Roll Flutter Engine from d814c69abe26 to 4ed733d99cd5 (1 revision) (flutter/flutter#135801) 2023-09-30 [email protected] Roll Flutter Engine from f726438d4d65 to d814c69abe26 (1 revision) (flutter/flutter#135799) 2023-09-30 [email protected] Roll Flutter Engine from 0b96a82cc874 to f726438d4d65 (1 revision) (flutter/flutter#135796) 2023-09-30 [email protected] Roll Flutter Engine from f3a4cab69962 to 0b96a82cc874 (1 revision) (flutter/flutter#135793) 2023-09-30 [email protected] Roll Flutter Engine from e719b7957763 to f3a4cab69962 (2 revisions) (flutter/flutter#135792) 2023-09-30 [email protected] Roll Flutter Engine from 8939f9c1f4e4 to e719b7957763 (2 revisions) (flutter/flutter#135789) 2023-09-30 [email protected] Roll Flutter Engine from 6a13607008ed to 8939f9c1f4e4 (1 revision) (flutter/flutter#135788) 2023-09-30 [email protected] Add device ready check (flutter/flutter#135526) 2023-09-30 [email protected] Temporarily disable NexusLowRes, version=29 (flutter/flutter#135785) 2023-09-30 [email protected] Cover more test/widgets tests with leak tracking #12. (flutter/flutter#135385) 2023-09-30 [email protected] DraggableScrollableController should dispatch creation in constructor. (flutter/flutter#135423) 2023-09-30 [email protected] Roll Flutter Engine from d203e349781c to 6a13607008ed (2 revisions) (flutter/flutter#135780) 2023-09-29 [email protected] Roll Flutter Engine from 48973d7f9beb to d203e349781c (5 revisions) (flutter/flutter#135776) 2023-09-29 [email protected] ViewportOffset should dispatch creation in constructor. (flutter/flutter#135420) 2023-09-29 [email protected] Use platform dill from the engine when compiling with dart2wasm. (flutter/flutter#134970) 2023-09-29 [email protected] Roll Flutter Engine from d5fcd78a879b to 48973d7f9beb (5 revisions) (flutter/flutter#135765) 2023-09-29 [email protected] Upgrade leak_tracker to fix flackiness. (flutter/flutter#135760) 2023-09-29 [email protected] Roll Flutter Engine from c52251a8b2d0 to d5fcd78a879b (2 revisions) (flutter/flutter#135755) 2023-09-29 [email protected] Update `Drawer` tests for M2/M3 (flutter/flutter#135752) 2023-09-29 [email protected] Expose onAttach and onDetach in ScrollController subclasses (flutter/flutter#135721) 2023-09-29 [email protected] Update the wasm bootstrap to allow access to the emscripten wrapper to skwasm. (flutter/flutter#135723) 2023-09-29 [email protected] Update logic for getting `Analytics` instance from package:unified_analytics (flutter/flutter#134756) 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 Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: 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
3 participants
Description
This PR adds
DraggableScrollableControllercreation dispatching in the constructor for memory leak tracking according to dart-lang/leak_tracker#134.Tests
draggable_scrollable_sheet_test.dartto testDraggableScrollableControllercreation dispatch;draggable_scrollable_sheet_test.dartto add disposal ofDraggableScrollableControllers in tests;material/scaffold_test.dartto add disposal ofDraggableScrollableControllers in tests;Pre-launch Checklist
///).