Fix IconButton leaks its internal MaterialStatesController #130720

bleroux · 2023-07-17T14:41:30Z

Description

This PR adds a call to dispose the internal MaterialStatesController instantiated by _SelectableIconButtonState.

I found this memory leak while working on M2/M3 test update for about_test.dart. This memory leak only happens when using M3 because IconButton relies on _SelectableIconButton only when useMaterial3 is true:

flutter/packages/flutter/lib/src/material/icon_button.dart

Lines 671 to 721 in 3a1190a

    
           if (theme.useMaterial3) { 
        
             final Size? minSize = constraints == null 
        
                 ? null 
        
                 : Size(constraints!.minWidth, constraints!.minHeight); 
        
             final Size? maxSize = constraints == null 
        
                 ? null 
        
                 : Size(constraints!.maxWidth, constraints!.maxHeight); 
        
             ButtonStyle adjustedStyle = styleFrom( 
        
               visualDensity: visualDensity, 
        
               foregroundColor: color, 
        
               disabledForegroundColor: disabledColor, 
        
               focusColor: focusColor, 
        
               hoverColor: hoverColor, 
        
               highlightColor: highlightColor, 
        
               padding: padding, 
        
               minimumSize: minSize, 
        
               maximumSize: maxSize, 
        
               iconSize: iconSize, 
        
               alignment: alignment, 
        
               enabledMouseCursor: mouseCursor, 
        
               disabledMouseCursor: mouseCursor, 
        
               enableFeedback: enableFeedback, 
        
             ); 
        
             if (style != null) { 
        
               adjustedStyle = style!.merge(adjustedStyle); 
        
             } 
        
             Widget effectiveIcon = icon; 
        
             if ((isSelected ?? false) && selectedIcon != null) { 
        
               effectiveIcon = selectedIcon!; 
        
             } 
        
             Widget iconButton = effectiveIcon; 
        
             if (tooltip != null) { 
        
               iconButton = Tooltip( 
        
                 message: tooltip, 
        
                 child: effectiveIcon, 
        
               ); 
        
             } 
        
             return _SelectableIconButton( 
        
               style: adjustedStyle, 
        
               onPressed: onPressed, 
        
               autofocus: autofocus, 
        
               focusNode: focusNode, 
        
               isSelected: isSelected, 
        
               variant: _variant, 
        
               child: iconButton, 
        
             ); 
        
           }

Related Issue

Fixes #130708

Tests

Adds 1 test.

QuncCccccc

Thanks for add this fix! Just left one comment for the unit test:)

QuncCccccc · 2023-07-17T18:49:09Z

packages/flutter/test/material/icon_button_test.dart

Should we move this test to packages/flutter/test/material/about_test.dart? Seems the LicensePage() is only tested in that file.

Usually we keep the test as close as possible to the fix. I agree that relying on LicensePage is not great but I was not able to figure out another way to reproduce the memory leak.
It's also true that this very same test will be part of the PR I'm working on for about_test.dart M3 migration.

I'm not used to work on PR related to leaks, maybe someone can give us guidance?

I think to reproduce the issue, in icon_button_test.dart, we can try to pump an IconButton for the first time and then pump other widgets to see if there's any memory leak. Also checked other examples, looks like we also need to call TestWidgetsWithLeakingTracking instead of TestWidgets.

Try this. The memory leak can be reproduced when we don't have the fix.

testWidgetsWithLeakTracking('Material3 - IconButton memory leak', (WidgetTester tester) async { bool isSelected = false; Widget buildWidget(bool showIconButton) { return showIconButton ? MaterialApp( theme: ThemeData(useMaterial3: true), home: IconButton( onPressed: () { }, isSelected: isSelected, icon: const Icon(Icons.search), )) : const SizedBox(); } await tester.pumpWidget(buildWidget(true)); await tester.pumpWidget(buildWidget(false)); });

Revisiting this, I was able to write a test which does not depend on LicensePage.

Oh nice! I added a comment yesterday but didn't actually press the "submit review" button😂Sorry about that! But the response is similar to what you added.

Great! I really thought I tried something similar. This is way cleaner that my previous attempt, thanks!

QuncCccccc · 2023-07-18T16:42:06Z

packages/flutter/test/material/icon_button_test.dart

Seems we still didn't use IconButton here. I think the example I provided above might be enough to show there's no leaking.

QuncCccccc

LGTM:) Just left one nit below.

packages/flutter/test/material/icon_button_test.dart

bleroux · 2023-07-19T17:57:22Z

Oops I misclicked.

…lutter/flutter#130720)

This PR updates unit tests in `about_test.dart` for M3 migration. More info in #127064 - Two tests were failing in M3 due to a memory leak. As the memory leak is now fixed, see #130720, this two tests does not depend anymore on the Material version. - Created several M3 tests related to typography and rendering changes.

…lutter/flutter#130720)

Roll Flutter from d07e8ae to 9cfbf6b (58 revisions) flutter/flutter@d07e8ae...9cfbf6b 2023-07-21 [email protected] Roll Packages from 674179f to 2266a76 (6 revisions) (flutter/flutter#131058) 2023-07-21 [email protected] Add tests for navigation_drawer_theme_test.dart (flutter/flutter#130465) 2023-07-21 [email protected] Roll Flutter Engine from f812cf373b6b to f5c1650c7acc (1 revision) (flutter/flutter#131037) 2023-07-21 [email protected] Roll Flutter Engine from 264685f0aecb to f812cf373b6b (1 revision) (flutter/flutter#131032) 2023-07-21 [email protected] Roll Flutter Engine from 8ff10f5a7667 to 264685f0aecb (1 revision) (flutter/flutter#131031) 2023-07-21 [email protected] Roll Flutter Engine from b5a6b1c9cba5 to 8ff10f5a7667 (5 revisions) (flutter/flutter#131029) 2023-07-21 [email protected] Roll Flutter Engine from 218b71cd7a45 to b5a6b1c9cba5 (1 revision) (flutter/flutter#131025) 2023-07-21 [email protected] Roll Flutter Engine from ab7d424d4467 to 218b71cd7a45 (2 revisions) (flutter/flutter#131024) 2023-07-21 [email protected] Use downgraded analyze for flutter/packages (flutter/flutter#130878) 2023-07-21 [email protected] Roll pub packages (flutter/flutter#131022) 2023-07-20 [email protected] Roll Flutter Engine from 9b2ebf2afe00 to ab7d424d4467 (4 revisions) (flutter/flutter#131015) 2023-07-20 [email protected] Add applyFocusChangeIfNeeded, have menus restore focus before activating (flutter/flutter#130536) 2023-07-20 [email protected] Migrate more integration tests to process result matcher (flutter/flutter#130994) 2023-07-20 [email protected] Roll Flutter Engine from 062079ba30b6 to 9b2ebf2afe00 (2 revisions) (flutter/flutter#131013) 2023-07-20 [email protected] Trivial grammar and wrapping fix for docs (flutter/flutter#130955) 2023-07-20 [email protected] Updated the ThemeData API example (flutter/flutter#130954) 2023-07-20 [email protected] Roll Flutter Engine from a3fc18514cd6 to 062079ba30b6 (3 revisions) (flutter/flutter#131010) 2023-07-20 [email protected] Roll Flutter Engine from 6d7842d25f81 to a3fc18514cd6 (2 revisions) (flutter/flutter#131007) 2023-07-20 [email protected] Update `TextSelectionTheme`, `ThemeData`, `TimePicker`, and `TimePickerTheme` tests for M2/M3 (flutter/flutter#130547) 2023-07-20 [email protected] Roll Flutter Engine from c645eb6da8a9 to 6d7842d25f81 (1 revision) (flutter/flutter#130992) 2023-07-20 [email protected] Update AutoComplete test for M3 migration (flutter/flutter#130883) 2023-07-20 [email protected] Update about tests for M3 (flutter/flutter#130970) 2023-07-20 [email protected] Roll Packages from 209db21 to 674179f (4 revisions) (flutter/flutter#130989) 2023-07-20 [email protected] Roll Flutter Engine from e40995da7869 to c645eb6da8a9 (1 revision) (flutter/flutter#130988) 2023-07-20 [email protected] Upgrade leak_tracker. (flutter/flutter#130951) 2023-07-20 [email protected] Roll Flutter Engine from 2df3b9c4b2a4 to e40995da7869 (2 revisions) (flutter/flutter#130985) 2023-07-20 [email protected] Roll Flutter Engine from b494143fb0bc to 2df3b9c4b2a4 (3 revisions) (flutter/flutter#130973) 2023-07-20 [email protected] Fix chip delete button tap target spilling into the label. (flutter/flutter#130896) 2023-07-20 [email protected] Fix IconButton leaks its internal MaterialStatesController (flutter/flutter#130720) 2023-07-20 [email protected] Update banner_theme_test.dart for M3 (flutter/flutter#130884) 2023-07-20 [email protected] Roll Flutter Engine from 204625490ca1 to b494143fb0bc (1 revision) (flutter/flutter#130966) 2023-07-20 [email protected] Make PollingDeviceDiscovery start the initial poll faster. (flutter/flutter#130755) 2023-07-20 [email protected] Roll Flutter Engine from c902fec1e3ce to 204625490ca1 (1 revision) (flutter/flutter#130962) 2023-07-20 [email protected] More documentation for MediaQuery and friends (flutter/flutter#130509) 2023-07-20 [email protected] Roll Flutter Engine from 56e88e8b0eef to c902fec1e3ce (1 revision) (flutter/flutter#130960) 2023-07-20 [email protected] Automatically create the layer when setting hints in PaintingContext (flutter/flutter#130364) 2023-07-20 [email protected] Roll Flutter Engine from e2a36be2f084 to 56e88e8b0eef (1 revision) (flutter/flutter#130959) 2023-07-20 [email protected] Further clarify Stack documentation on overflowing (flutter/flutter#130776) 2023-07-20 [email protected] Roll Flutter Engine from eff70f7287f9 to e2a36be2f084 (2 revisions) (flutter/flutter#130956) 2023-07-20 [email protected] Can traverse if current focused node skips traversal (flutter/flutter#130812) 2023-07-20 [email protected] Document that you can't change initialRoute usefully (flutter/flutter#130450) 2023-07-20 [email protected] Roll Flutter Engine from 7671e2f2a9fc to eff70f7287f9 (3 revisions) (flutter/flutter#130953) 2023-07-19 [email protected] Roll Flutter Engine from 938140a974b0 to 7671e2f2a9fc (3 revisions) (flutter/flutter#130948) 2023-07-19 [email protected] Roll Flutter Engine from 0af285219809 to 938140a974b0 (1 revision) (flutter/flutter#130943) 2023-07-19 [email protected] Add docs to Route.maintainState (flutter/flutter#130638) 2023-07-19 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.20.4 to 2.21.0 (flutter/flutter#130941) ...

…30720) ## Description This PR adds a call to dispose the internal `MaterialStatesController` instantiated by `_SelectableIconButtonState`. I found this memory leak while working on M2/M3 test update for `about_test.dart`. This memory leak only happens when using M3 because `IconButton` relies on `_SelectableIconButton` only when useMaterial3 is true: https://github.com/flutter/flutter/blob/3a1190a5a85c3e6a0cf3a9c30f34548fdd48ac1e/packages/flutter/lib/src/material/icon_button.dart#L671-L721 ## Related Issue Fixes flutter#130708 ## Tests Adds 1 test.

This PR updates unit tests in `about_test.dart` for M3 migration. More info in flutter#127064 - Two tests were failing in M3 due to a memory leak. As the memory leak is now fixed, see flutter#130720, this two tests does not depend anymore on the Material version. - Created several M3 tests related to typography and rendering changes.

github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Jul 17, 2023

bleroux requested a review from QuncCccccc July 17, 2023 15:21

QuncCccccc reviewed Jul 17, 2023

View reviewed changes

bleroux force-pushed the IconButton_leak branch 3 times, most recently from ca9fcb9 to 061c65b Compare July 18, 2023 07:43

bleroux requested a review from QuncCccccc July 18, 2023 08:41

QuncCccccc reviewed Jul 18, 2023

View reviewed changes

bleroux force-pushed the IconButton_leak branch from 061c65b to bd4e4b7 Compare July 19, 2023 07:04

bleroux requested a review from QuncCccccc July 19, 2023 07:39

QuncCccccc approved these changes Jul 19, 2023

View reviewed changes

packages/flutter/test/material/icon_button_test.dart Outdated Show resolved Hide resolved

packages/flutter/test/material/icon_button_test.dart Outdated Show resolved Hide resolved

bleroux closed this Jul 19, 2023

bleroux force-pushed the IconButton_leak branch from bd4e4b7 to a417a69 Compare July 19, 2023 17:54

bleroux reopened this Jul 19, 2023

bleroux force-pushed the IconButton_leak branch from 2fec655 to cc05856 Compare July 19, 2023 19:08

Fix IconButton leaks its internal MaterialController

0fc0b13

bleroux force-pushed the IconButton_leak branch from cc05856 to 0fc0b13 Compare July 20, 2023 06:45

bleroux added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 20, 2023

auto-submit bot merged commit 0aba94f into flutter:master Jul 20, 2023

bleroux deleted the IconButton_leak branch July 20, 2023 08:00

bleroux mentioned this pull request Jul 20, 2023

Update about tests for M3 #130970

Merged

engine-flutter-autoroll mentioned this pull request Jul 20, 2023

Roll Flutter from d07e8aece184 to e8a975e884c1 (34 revisions) flutter/packages#4529

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 20, 2023

0aba94f Fix IconButton leaks its internal MaterialStatesController (f…

3a6fb3b

…lutter/flutter#130720)

engine-flutter-autoroll mentioned this pull request Jul 20, 2023

Roll Flutter from d07e8aece184 to 1a804207d801 (35 revisions) flutter/packages#4531

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 20, 2023

0aba94f Fix IconButton leaks its internal MaterialStatesController (f…

5b55445

…lutter/flutter#130720)

engine-flutter-autoroll mentioned this pull request Jul 20, 2023

Roll Flutter from d07e8aece184 to 0ed17dd5a246 (40 revisions) flutter/packages#4532

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 20, 2023

0aba94f Fix IconButton leaks its internal MaterialStatesController (f…

c0ff6b0

…lutter/flutter#130720)

engine-flutter-autoroll mentioned this pull request Jul 21, 2023

Roll Flutter from d07e8aece184 to aafdbc6d12cd (57 revisions) flutter/packages#4542

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 21, 2023

0aba94f Fix IconButton leaks its internal MaterialStatesController (f…

7f4d3a6

…lutter/flutter#130720)

engine-flutter-autoroll mentioned this pull request Jul 21, 2023

Roll Flutter from d07e8aece184 to 9cfbf6b9fd40 (58 revisions) flutter/packages#4544

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Jul 21, 2023

0aba94f Fix IconButton leaks its internal MaterialStatesController (f…

e01e891

…lutter/flutter#130720)

HansMuller mentioned this pull request Jul 21, 2023

Flutter Framework Team: Notable Commits #121415

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix IconButton leaks its internal MaterialStatesController #130720

Fix IconButton leaks its internal MaterialStatesController #130720

Uh oh!

bleroux commented Jul 17, 2023

Uh oh!

QuncCccccc left a comment

Uh oh!

QuncCccccc Jul 17, 2023

Uh oh!

bleroux Jul 17, 2023 •

edited

Loading

Uh oh!

QuncCccccc Jul 17, 2023

Uh oh!

bleroux Jul 18, 2023

Uh oh!

QuncCccccc Jul 18, 2023

Uh oh!

bleroux Jul 19, 2023

Uh oh!

QuncCccccc Jul 18, 2023

Uh oh!

QuncCccccc left a comment

Uh oh!

Uh oh!

Uh oh!

bleroux commented Jul 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if (theme.useMaterial3) {
	final Size? minSize = constraints == null
	? null
	: Size(constraints!.minWidth, constraints!.minHeight);
	final Size? maxSize = constraints == null
	? null
	: Size(constraints!.maxWidth, constraints!.maxHeight);

	ButtonStyle adjustedStyle = styleFrom(
	visualDensity: visualDensity,
	foregroundColor: color,
	disabledForegroundColor: disabledColor,
	focusColor: focusColor,
	hoverColor: hoverColor,
	highlightColor: highlightColor,
	padding: padding,
	minimumSize: minSize,
	maximumSize: maxSize,
	iconSize: iconSize,
	alignment: alignment,
	enabledMouseCursor: mouseCursor,
	disabledMouseCursor: mouseCursor,
	enableFeedback: enableFeedback,
	);
	if (style != null) {
	adjustedStyle = style!.merge(adjustedStyle);
	}

	Widget effectiveIcon = icon;
	if ((isSelected ?? false) && selectedIcon != null) {
	effectiveIcon = selectedIcon!;
	}

	Widget iconButton = effectiveIcon;
	if (tooltip != null) {
	iconButton = Tooltip(
	message: tooltip,
	child: effectiveIcon,
	);
	}

	return _SelectableIconButton(
	style: adjustedStyle,
	onPressed: onPressed,
	autofocus: autofocus,
	focusNode: focusNode,
	isSelected: isSelected,
	variant: _variant,
	child: iconButton,
	);
	}

Fix IconButton leaks its internal MaterialStatesController #130720

Fix IconButton leaks its internal MaterialStatesController #130720

Uh oh!

Conversation

bleroux commented Jul 17, 2023

Description

Related Issue

Tests

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

bleroux Jul 17, 2023 • 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!

QuncCccccc Jul 17, 2023

Choose a reason for hiding this comment

Uh oh!

bleroux Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

bleroux Jul 19, 2023

Choose a reason for hiding this comment

Uh oh!

QuncCccccc Jul 18, 2023

Choose a reason for hiding this comment

Uh oh!

QuncCccccc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bleroux commented Jul 19, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bleroux Jul 17, 2023 •

edited

Loading