-
Notifications
You must be signed in to change notification settings - Fork 87
Cache js breakpoint ids and reuse them when adding new breakpoints #2721
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
base: main
Are you sure you want to change the base?
Conversation
Internally, there are two different code paths to trigger hot restart: 1. Via connected debugger, triggering `_hotRestart` funciton in `dwds_vm_client.dart`. This codepath removes existing breakpoints and blocks main script execution (by passing `pauseIsolatesOnStart` into `dartHotRestartDwds`) until the connected debugger re-adds breakpoints. 2. Via in-app shortcut in injected UI, which just calls `dartHotRestartDwds` directly. This codepath does not clear breakpoints and does not block main startup. The flow dart-lang#2 is currently doesn't work as intended: 1. Chrome breakpoints aren't cleared, but the attached debugger still gets an isolate start event 2. It tries to set all breakpoints in a new isolate, and sends add breakpoint commands 3. Chrome API returns an error tha the breakpoint exists, and the error is propagated back to DAP server, which sends a set breakpoint error notification to an IDE. 4. The IDE thinks that the breakpoint is "unconfirmed", and hides it from the UI completely. As a result, the user does not see the breakpoint, but it's there, in Chrome debugger state, and the execution stops on it, and there's no way for user to remove it (except for clearing all breakpoints by the restart/reload from an IDE, which clears chrome debugger state). The CL attempts to fix it by caching chrome breakpoint ids by js locations, so that when the debugger adds a breakpoint resolving to the same js location, it gets back the same js breakpoint id.
|
did you have a chance to look at this pr? |
bkonyi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this seems reasonable to me. @Markzipan, can you take a look too?
|
@nshahan has more context about hot restart breakpoints - adding them here |
|
@iinozemtsev I believe this new implementation that blocks running main while the debugger is manipulating breakpoints was added by @srujzs. While adding support for breakpoints after a hot reload he found correctness issues and inconsistencies with the current approach. Do you think it would be possible to utilize the same logic when restarting from the injected UI? I think this would be preferable for multiple reasons:
|
if there are existing tests which wire dev extension, webdev server with dwds, and a dap client, I think adding a test would be relatively straightforward, otherwise I think it's too big task to chew for me at the moment. I'll try to take a look tomorrow if there's an easy way to converge the in-app restart path into dwds_vm_client. |
|
I've sent cl/840666025 internally to demo alternative approach just for quicker prototyping, I can turn that into a real PR if LG. |
Internally, there are two different code paths to trigger hot restart:
_hotRestartfunciton indwds_vm_client.dart. This codepath removes existing breakpoints and blocks main script execution (by passingpauseIsolatesOnStartintodartHotRestartDwds) until the connected debugger re-adds breakpoints.dartHotRestartDwdsdirectly. This codepath does not clear breakpoints and does not block main startup.The second flow with in-app triggered restart is currently doesn't work as intended:
As a result, the user does not see the breakpoint, but it's there, in Chrome debugger state, and the execution stops on it, and there's no way for user to remove it (except for clearing all breakpoints by the restart/reload from an IDE, which clears chrome debugger state).
The CL attempts to fix it by caching chrome breakpoint ids by js locations, so that when the debugger adds a breakpoint resolving to the same js location, it gets back the same js breakpoint id.
Contribution guidelines:
dart format.Many Dart repos have a weekly cadence for reviewing PRs - please allow for some latency before initial review feedback.
Note: The Dart team is trialing Gemini Code Assist. Don't take its comments as final Dart team feedback. Use the suggestions if they're helpful; otherwise, wait for a human reviewer.