Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix memory leak in TabPageSelector #147403

Conversation

ValentinVignal
Copy link
Contributor

Part of #141198

Pre-launch Checklist

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

@ValentinVignal
Copy link
Contributor Author

cc @polina-c

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Apr 26, 2024
@polina-c polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Apr 26, 2024
@@ -41,7 +41,7 @@ Future<void> testExecutable(FutureOr<void> Function() testMain) {
// receive the event.
WidgetController.hitTestWarningShouldBeFatal = true;

if (_isLeakTrackingEnabled()) {
// if (_isLeakTrackingEnabled()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revert this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops ! Very sorry about that :( I reverted it in refactor: Revert bad commit

@polina-c polina-c merged commit af27093 into flutter:master Apr 30, 2024
71 checks passed
@chingjun
Copy link
Contributor

chingjun commented May 1, 2024

Reason for revert: Causing an internal test to fail, see b/338159496 for details

@chingjun chingjun added the revert Autorevert PR (with "Reason for revert:" comment) label May 1, 2024
auto-submit bot pushed a commit that referenced this pull request May 1, 2024
@auto-submit auto-submit bot removed the revert Autorevert PR (with "Reason for revert:" comment) label May 1, 2024
auto-submit bot added a commit that referenced this pull request May 1, 2024
Reverts: #147403
Initiated by: chingjun
Reason for reverting: Causing an internal test to fail, see b/338159496 for details
Original PR Author: ValentinVignal

Reviewed By: {polina-c}

This change reverts the following previous change:
Part of #141198
@ValentinVignal
Copy link
Contributor Author

@chingjun Sorry for that. What can I do / check to make sure no internal tests fail ? (ex: what does "see b/338159496 for details" mean? )

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request May 1, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request May 1, 2024
flutter/flutter@b597dd2...d33bb8f

2024-05-01 sokolovskyi.konstantin@gmail.com Add tests for single_activator.0.dart API example. (flutter/flutter#147426)
2024-05-01 Dhankechakishan@gmail.com Added missing code block language in docs (flutter/flutter#147481)
2024-05-01 zanderso@users.noreply.github.com Move docs_test and docs_publish to bringup (flutter/flutter#147645)
2024-05-01 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Fix memory leak in `TabPageSelector`  (#147403)" (flutter/flutter#147622)
2024-04-30 47866232+chunhtai@users.noreply.github.com Revert "add a new PopScope.onPopWithResultInvoke widget to replace Po� (flutter/flutter#147597)
2024-04-30 andrewrkolos@gmail.com include exception details in tool exit displayed when adb call fails (flutter/flutter#147498)
2024-04-30 32538273+ValentinVignal@users.noreply.github.com Fix memory leak in `TabPageSelector`  (flutter/flutter#147403)

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 dit@google.com,rmistry@google.com,stuartmorgan@google.com 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://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
@chingjun
Copy link
Contributor

chingjun commented May 1, 2024

The information is available to Google employees. @polina-c can you help with this? Thanks!

@polina-c
Copy link
Contributor

polina-c commented May 1, 2024

Error:

The following assertion was thrown building ...:
		dependOnInheritedWidgetOfExactType<_TabControllerScope>() or dependOnInheritedElement() was called
		before _TabPageSelectorState.initState() completed.
#744    MultiChildRenderObjectElement.inflateWidget (third_party/dart/flutter/lib/src/widgets/framework.dart:6898:36)
#745    MultiChildRenderObjectElement.mount (third_party/dart/flutter/lib/src/widgets/framework.dart:6910:32)
...

I suggest to create PR to redo the change.
And in description specify that we want to make sure the bot 'Google testing' passed. For some reasons Google testing was not activated in this PR.
And we will see what 'Google testing' catches.

@ValentinVignal
Copy link
Contributor Author

OK thank you for the insights. I created a new PR: #147689. Let's see what is happening there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants