Repository navigation
Conversation
This is just a simple fix where we return null of a library URI is not a `package:` URI. Fixes flutter#6734
Contributor
There was a problem hiding this comment.
Code Review
This pull request updates RootInfo in isolate_state.dart to return null for the package property when the library URI is not a package: URI (e.g., file: or dart: URIs). It also adds comprehensive unit tests covering these scenarios and updates the CHANGELOG. No review comments were provided, and the implementation is correct and well-tested, so there is no further feedback.
null from RootInfo.package for non-package: root libraries
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6734
Summary
RootInfo.packagenow returnsnullwhen the root library is not apackage:URI. Before, it returned everything up to the first/, so the root libraryfile:///app/bin/main.dartof a Dart CLI app produced the "package"file:. That is not a package, and it is what #6734 reported.Changing
RootInfois a one-line fix, but it was not safe on its own: the Memory screen depended on the old behavior. This PR also makes the Memory screen keep behaving exactly as it did before.Why this was more than a one-line fix
The Memory screen is the only place in this repo that reads
RootInfo.package, and it does not treat the value as a package name. It uses it as a prefix:HeapClassName.classTypeclassifies a class as a project class (ClassType.rootPackage) when its library URI starts with the prefix. For afile:root library, the old valuefile:made classes fromfile:libraries count as project classes.RootInfochangeRootInfo.packageforfile:///app/bin/main.dartfile:nullnullfile:null(crash)file:MemoryControllercrashed during initialization, atrootInfoNow().package!. This is what CI caught.!would have been worse, because it would fail silently. The app's own classes would be classified as runtime classes instead of project classes. That turns off live evaluation and "drop to console" for them (LiveClassSampler.isEvalEnabled), changes their icons and what the$projectand$runtimefilters match, and makes exported snapshots impossible to import again (DiffPaneController.fromJsonreads the root package withas String).HeapClassNamecaches itsClassTypethe first time it is asked, so every call site must compute the root package the same way. Patching only the crashing line would not be enough.So
RootInfo.packagestays strict, as #6734 asks, and the Memory screen getsRootInfoMemoryExtension.rootPackagePrefix, which is the old computation moved out ofRootInfo. For every root library it returns exactly whatRootInfo.packagereturned before this PR, and all five Memory screen call sites use it, so the Memory screen behaves as it does on master. It lives indevtools_apprather than onRootInfobecause it is a Memory screen heuristic, not something to add to the public API of a published package.Why only some tests noticed
The first version of this PR only changed
RootInfoand its tests. The unit and widget tests passed, and only two CI jobs failed: thedart-cliintegration jobs (dart2js and dart2wasm).connect to app and switch tabsconnects DevTools to a real app and visits every screen, including Memory, which initializesMemoryController. It runs against a Flutter app, a Flutter web app, and a Dart CLI app.empty_app.dart, started withdart run, so its root library is afile:URI. The Flutter apps havepackage:root libraries, sopackage!never failed for them.FakeServiceManager, whoserootInfoNow()always returnedpackage:myPackage/myPackage.dart. No test below the integration level ever had a non-package:root library.Changes
devtools_app_shared:RootInfo.packageisnullunless the root library is apackage:URI.RootInfonow has doc comments, andCHANGELOG.mdstates the old and new values. BecauseRootInfois exported frompackage:devtools_app_shared/service.dart, this is observable by extension authors, which is the point of Accessing the package details seems to return invalid data. #6734.devtools_app:MemoryController,LiveClassSampler, the profile table, and both diff tables userootPackagePrefix.devtools_test:FakeServiceManager,FakeServiceConnectionManager, andMemoryDefaultScene.setUpaccept an optionalrootInfo, so a test can simulate afile:root. This is deliberately separate fromrootLibrary: twoInspectorPreferencesControllertests build a storage key from the fake's fixedpackage:myPackageroot, and makingrootLibraryflow intorootInfoNow()broke them.Tests
isolate_state_test.dart:RootInfoforpackage:,file:,dart:, andnulllibraries.memory_controller_test.dart: the controller's root package forpackage:andfile:roots. Thefile:case fails with the same null check failure as CI on the first version of this PR, and passes now.memory_utils_test.dart:rootPackagePrefixfor several root libraries, and that classes of afile:root library are classified as project classes.The two
dart-cliintegration jobs that failed on the first version pass again. No release note is needed: the only user-visible change is for extension authors, and it is in thedevtools_app_sharedCHANGELOG (release-notes-not-required).