Skip to content

Conversation

@polina-c
Copy link
Contributor

No description provided.

@github-actions github-actions bot added framework flutter/packages/flutter repository. See also f: labels. f: routes Navigator, Router, and related APIs. labels Aug 31, 2023
@github-actions github-actions bot added the f: material design flutter/packages/flutter/material repository. label Aug 31, 2023
@polina-c polina-c marked this pull request as ready for review August 31, 2023 15:46
if (kFlutterMemoryAllocationsEnabled) {
MemoryAllocations.instance.dispatchObjectCreated(
library: 'package:flutter/widgets.dart',
className: '$Route<$T>',
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't notice this earlier, why would you need to provide a className if you already provide the object itself? Should the ObjectCreated uses the object.runtimeType if it needs the class name?

Copy link
Contributor Author

@polina-c polina-c Aug 31, 2023

Choose a reason for hiding this comment

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

runtimeType is different from type that owns instrumentation, because runtime type can be descender (like MaterialPageRoute). Does it help?

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks for explanation

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: why is Route<$T> a clearer name than MaterialPageRoute? I would have thought the more specific class name would be clearer for someone debugging.

/// used instead.
Route({ RouteSettings? settings }) : _settings = settings ?? const RouteSettings();
Route({ RouteSettings? settings }) : _settings = settings ?? const RouteSettings() {
if (kFlutterMemoryAllocationsEnabled) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry I have not brought up this earlier. Should these logic be wrapped in an assert?

Copy link
Contributor Author

@polina-c polina-c Aug 31, 2023

Choose a reason for hiding this comment

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

It is proven that this code is tree-shaked when kFlutterMemoryAllocationsEnabled is false, because it is a compile time constant.
So, it is ok to go without assert.
Does this answer your question?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, thanks for explanation

Comment on lines 642 to 645
const TestPage page = TestPage(name: 'page');
final Route<dynamic> route = page.createRoute(nav.currentContext!);
// ignore: invalid_use_of_protected_member
route.dispose();
Copy link
Contributor

Choose a reason for hiding this comment

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

you can do

nav.currentState!.push(MaterialPageRoute(builder: (_) => const PlaceHolder())); // This should create a route
await tester.pumpAndSettle();

nav.currentState!.pop();
await tester.pumpAndSettle(); // this should dispose the route.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nav.currentState is null on first line
Did i miss something?

Copy link
Contributor

Choose a reason for hiding this comment

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

you will need this first before push

await tester.pumpWidget(
      MaterialApp(
        navigatorKey: nav,
        home: const Scaffold(
          body: Text('home'),
        )
      )
    );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Applied. Thank you!

MemoryAllocations.instance.addListener(listener);

await createAndDisposeRoute();
expect(events, hasLength(2));
Copy link
Contributor

Choose a reason for hiding this comment

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

This is interesting, I would expect 3 events. One for MaterialPageRoute created from home page of MaterialApp. Two for creating and disposing the TestPage

Copy link
Contributor

Choose a reason for hiding this comment

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

ah nvm, the home page will create MaterialPageRoute which will be filter out in the listener

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes!

@polina-c polina-c requested review from chunhtai and removed request for goderbauer August 31, 2023 16:34
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

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

LGTM

)
);

nav.currentState!.push(MaterialPageRoute<void>(builder: (_) => const Placeholder())); // This should create a route
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
nav.currentState!.push(MaterialPageRoute<void>(builder: (_) => const Placeholder())); // This should create a route
nav.currentState!.push(MaterialPageRoute<void>(builder: (_) => const Placeholder())); // This should create a route.

await tester.pumpAndSettle();

nav.currentState!.pop();
await tester.pumpAndSettle(); // this should dispose the route.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
await tester.pumpAndSettle(); // this should dispose the route.
await tester.pumpAndSettle(); // This should dispose the route.

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 3, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 3, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 4, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 5, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 6, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 7, 2023
@polina-c polina-c added the a: leak tracking Issues and PRs related to memory leaks detected by leak_tracker label Sep 21, 2023
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 15, 2023
@polina-c polina-c changed the title Make Route dispatching memory events. Make Route dispatching memory events. [prod-leak-fix] Aug 29, 2024
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. f: routes Navigator, Router, and related APIs. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants