Repository navigation
Revert: [devicelab] Clean up Chrome profile directories and trap abort signals in web benchmarks - #192615
Conversation
…t signals in web benchmarks (flutter#192584)" This reverts commit 74435ae.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the web benchmark execution logic by removing external cleanup helpers and signal listeners, inlining the cleanup process into a finally block. The review feedback identifies potential issues in this new cleanup flow, including a possible Chrome process leak if the benchmark fails early, and unhandled exceptions when writing to the stdin of an already terminated process, which could mask original errors. It also suggests avoiding redundant string interpolation in a path.join call.
| } | ||
| } finally { | ||
| unawaited(server.close()); | ||
| chrome?.stop(); |
There was a problem hiding this comment.
If the benchmark fails early (e.g., during startup or before the first request is received), chrome will still be null, but the Chrome process might have been launched and whenChromeIsReady is active. In this scenario, calling chrome?.stop() does nothing, resulting in a leaked Chrome process.
We should check if whenChromeIsReady is active and stop the Chrome instance once it is ready to prevent resource leaks.
if (chrome != null) {
chrome.stop();
} else if (whenChromeIsReady != null) {
unawaited(whenChromeIsReady.then((Chrome c) => c.stop(), onError: (_) {}));
}| flutterRunProcess.stdin.write('q'); | ||
| await flutterRunProcess.stdin.flush(); |
There was a problem hiding this comment.
If the flutterRunProcess has already exited, calling stdin.write or stdin.flush will throw an unhandled exception (such as a SocketException or StdoutException). Since this occurs inside a finally block, it will mask any original exception thrown during the benchmark run, making debugging significantly harder.
Wrapping these operations in a try-catch block ensures that cleanup continues safely even if the process has already terminated.
| flutterRunProcess.stdin.write('q'); | |
| await flutterRunProcess.stdin.flush(); | |
| try { | |
| flutterRunProcess.stdin.write('q'); | |
| await flutterRunProcess.stdin.flush(); | |
| } catch (_) { | |
| // Stdin pipe may already be closed if the process has exited. | |
| } |
|
|
||
| final String dartToolDirectory = path.join(macrobenchmarksDirectory, '.dart_tool'); | ||
| userDataDir = io.Directory( | ||
| final String dartToolDirectory = path.join('$macrobenchmarksDirectory/.dart_tool'); |
There was a problem hiding this comment.
Using string interpolation inside path.join like path.join('$macrobenchmarksDirectory/.dart_tool') is redundant and defeats the purpose of using path.join for platform-independent path construction.
Passing the directory and the subdirectory as separate arguments is cleaner and more idiomatic.
| final String dartToolDirectory = path.join('$macrobenchmarksDirectory/.dart_tool'); | |
| final String dartToolDirectory = path.join(macrobenchmarksDirectory, '.dart_tool'); |
cbracken
added
the
emergency
|
Detected the If you add the The "Merge" button is also unlocked. To bypass presubmits as well as the tree status, press the GitHub "Add to Merge Queue". |
Roll Flutter from 63b9518f16bc to 2553f89be1ef (62 revisions) flutter/flutter@63b9518...2553f89 2026-09-12 [email protected] Roll clang to 07d67299a15ce03b053736e2d31a668ee0576987 (flutter/flutter#192607) 2026-09-12 [email protected] Roll Dart SDK from b24b352c29db to 7863887acbfb (3 revisions) (flutter/flutter#192665) 2026-09-12 [email protected] fix(tree): et format --all (flutter/flutter#192669) 2026-09-12 [email protected] Support activity-alias in Android launcher activity detection (flutter/flutter#187251) 2026-09-12 [email protected] fix: regenerate gradle lockfiles for external_textures integration test (flutter/flutter#192657) 2026-09-12 [email protected] Roll Skia from 51aa02b2c195 to e5e96533b732 (4 revisions) (flutter/flutter#192646) 2026-09-12 [email protected] [WebParagraph] Fix text pixelation, glyph clipping, and snapping offsets to avoid subpixel raster caching (flutter/flutter#191736) 2026-09-12 [email protected] fix(tests): Add ruby dep to Mac windowing_test (flutter/flutter#192656) 2026-09-12 [email protected] Remove some `unawaited_futures` outdated ignores (flutter/flutter#192026) 2026-09-11 [email protected] Bump Dart to 3.13, reformat, re-enable formatting check (flutter/flutter#192649) 2026-09-11 [email protected] [android] Isolate API-34 high contrast listener in AccessibilityBridge behind Api34Impl (flutter/flutter#192502) 2026-09-11 [email protected] Update style guide for Dart 3.13 (flutter/flutter#192639) 2026-09-11 [email protected] ci(bringups): remove bringup from these tests (flutter/flutter#192637) 2026-09-11 [email protected] Unmark new_gallery_windows_impeller__transition_perf as bringup (flutter/flutter#192633) 2026-09-11 [email protected] Unmark new_gallery_windows_skia__transition_perf as bringup (flutter/flutter#192635) 2026-09-11 [email protected] Roll Skia from f446aec4ce9e to 51aa02b2c195 (2 revisions) (flutter/flutter#192631) 2026-09-11 [email protected] fix(ci): mac verify binaries still racing (flutter/flutter#192634) 2026-09-11 [email protected] Roll Packages from 8a35b16 to 9caa77c (8 revisions) (flutter/flutter#192630) 2026-09-11 [email protected] Roll Dart SDK from a76fee2722e0 to b24b352c29db (2 revisions) (flutter/flutter#192627) 2026-09-11 [email protected] [tool] Migrate PackagesCommand and Pub to modular dependency injection (flutter/flutter#192460) 2026-09-11 [email protected] [tool] Migrate Darwin framework commands to modular dependency injection (flutter/flutter#192252) 2026-09-11 [email protected] [tool] Migrate Upgrade, Downgrade, and UpdatePackages commands to modular dependency injection (flutter/flutter#190787) 2026-09-11 [email protected] Roll Skia from b3fa80a0e168 to f446aec4ce9e (55 revisions) (flutter/flutter#192620) 2026-09-11 [email protected] Roll Dart SDK from d20d59cfa664 to a76fee2722e0 (2 revisions) (flutter/flutter#192616) 2026-09-11 [email protected] remove deeplink integration tests from bringup (flutter/flutter#192459) 2026-09-11 [email protected] Revert: [devicelab] Clean up Chrome profile directories and trap abort signals in web benchmarks (flutter/flutter#192615) 2026-09-11 [email protected] Roll Fuchsia Linux SDK from C0j8F-_Iy-OQZ6YyI... to m69nfK-wnHaMTe8P9... (flutter/flutter#192608) 2026-09-11 [email protected] Fix Wasm build cache mismatch in ResidentWebRunner and browser 404 fallbacks (flutter/flutter#190786) 2026-09-11 [email protected] Roll Dart SDK from 3ce3116bc9ec to d20d59cfa664 (1 revision) (flutter/flutter#192588) 2026-09-11 [email protected] [Linux] Keep the frame at the top of a resizing subsurface (flutter/flutter#192408) 2026-09-10 [email protected] [devicelab] Clean up Chrome profile directories and trap abort signals in web benchmarks (flutter/flutter#192584) 2026-09-10 [email protected] [web] Keep ImageSource alive until Picture disposal (flutter/flutter#192516) 2026-09-10 [email protected] Remove the accessibility evaluations feature flag (flutter/flutter#192266) 2026-09-10 [email protected] Promote Windows gradient benchmark tests to non-bringup (flutter/flutter#192581) 2026-09-10 [email protected] Remove commented out export in `text_input.dart` since the dart compiler has the correct behavior now (flutter/flutter#191539) 2026-09-10 [email protected] [flutter_tools] Do not mistake server-initiated requests for responses to client requests in AnalysisServer (flutter/flutter#192546) 2026-09-10 [email protected] Roll Packages from 36e088a to 8a35b16 (12 revisions) (flutter/flutter#192550) 2026-09-10 [email protected] Roll pub packages (flutter/flutter#192517) 2026-09-10 [email protected] feat(web): support --web-content-hash for entrypoint files (flutter/flutter#190153) 2026-09-10 [email protected] [web] Stop iOS auto-zoom by sizing semantic inputs at 16px (flutter/flutter#192328) 2026-09-10 [email protected] Roll Dart SDK from 0d55b7a6a9ee to 3ce3116bc9ec (2 revisions) (flutter/flutter#192534) 2026-09-10 [email protected] Update more enum instances comparison to use equality == since dart has the correct behavior now (flutter/flutter#191788) 2026-09-10 [email protected] Fix specify_nonobvious_local_variable_types lints in luci_resultdb_test.dart (flutter/flutter#192543) 2026-09-10 [email protected] Roll ICU from 8cc91d9b6ab9 to 6ebb40c59477 (1 revision) (flutter/flutter#192527) 2026-09-10 [email protected] [Linux] Make the framebuffer depth/stencil buffer optional (flutter/flutter#192156) 2026-09-10 [email protected] [Linux] Cancel input when events stop being delivered (flutter/flutter#192277) ...
2 participants
Reverts: [devicelab] Clean up Chrome profile directories and trap abort signals in web benchmarks
Initiated by: @cbracken
Reason for reverting:
Original PR Author: @harryterkelsen
Reviewed By: @mdebbar
The original PR description is provided below:
Ensures temporary Chrome user profile directories created during web benchmarks are reliably deleted, and ensures benchmark child processes (Chrome, Shelf, and
flutter run) are cleaned up when benchmarks complete or when interrupted with SIGINT/SIGTERM.Part of #191316
Tip for reviewers: Recommend viewing with "Hide whitespace" enabled (
?w=1), as wrapping the task body intry ... finallyre-indents the inner block.Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.