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

Hot restart and breakpoints #2257

Closed
elliette opened this issue Oct 17, 2023 · 4 comments · Fixed by #2371
Closed

Hot restart and breakpoints #2257

elliette opened this issue Oct 17, 2023 · 4 comments · Fixed by #2371

Comments

@elliette
Copy link
Contributor

elliette commented Oct 17, 2023

I noticed an issue around breakpoints / hot restart when working on Cider debugging (a "phantom" breakpoint was being hit after hot restart, need to do more investigation to figure out how it got into that state).

However, I've noticed we have another hot restart / breakpoint issue in DevTools:

  1. Add a breakpoint (e.g., at line 100)
  2. Edit the file so that the breakpoint line should be moved (e.g., at 2 lines above so the breakpoint should now be at line 102)
  3. Click "hot restart" from DevTools
  4. The breakpoint is re-established at line 100, not line 102

Note: the breakpoint is getting re-added at the previous line here:

await addBreakpoint(
I'm not sure there is any way for DWDS to know what line to re-establish the breakpoint unless the debugger explicitly tells us.

In VSCode, the breakpoints persist in the right location through restarts. Whatever we are doing for VSCode, we should also do for Cider.

@elliette
Copy link
Contributor Author

Note: I have not been able to repro the "phantom breakpoint" outside of that one time.

@elliette
Copy link
Contributor Author

elliette commented Jan 29, 2024

We've gotten an internal report of phantom breakpoints (this time not when hot restarting an app) and I just hit this myself trying to restart another customer's app.

Along with not re-establishing breakpoints on hot restart (the debugger tool, e.g. VS Code, DevTools should instead send a request to re-establish the breakpoints after the restart has completed as described here), I also think we should migrate back from using the setBreakpointsByUrl.

The motivation for using setBreakpointsByUrl was that Chrome would try to re-establish breakpoints across multiple page reloads (see #1076 (comment)), but that seems like it is causing more bugs than actually being helpful, because we can get in a state where Chrome still thinks a breakpoint exists even though the debugging tool does not. The debugging tool should always be the source of truth in this case.

@elliette
Copy link
Contributor Author

There are a few changes that need to be made to resolve this:

  • Remove the logic to re-establish breakpoints after a hot-restart from DWDS
  • Add logic to re-establish breakpoints after a hot-restart to DevTools
  • Implement the VM Service setFlag method for pause_isolates_on_start so that the debugging client can make sure the app is paused while the set breakpoints (this makes sure breakpoints set early in the app's execution path are hit)
  • Update any code runners to respect the pause_isolates_on_start flag value (e.g. in flutter_tools here)
  • Make sure DevTools and any other debugging clients are calling setFlag with pause_isolates_on_start along with requirePermissionsToResume(onPauseReload: true)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant