Breakpoint system needs a serious overhaul

lukehutch commented 8 months ago

The breakpoint system in Dart needs a serious audit and overhaul. Breakpoints are close to unusable right now.

Among the (many) issues I run into, every time I am trying to debug code:

When you click in the margin of a line in VS Code to set a breakpoint, more often than actually setting a breakpoint on the line, one of the following happens at random (and clicking in the margin of the same line multiple times may do all of these in some random order):
- A breakpoint is set on some other line, above or below where you click. Sometimes that line doesn't even have code on it, it is a comment (the first link below shows this).
- No breakpoint is set at all.
- A breakpoint is listed in the breakpoint dialog at bottom left, but no breakpoint dot is shown on the program. (May be VS Code specific).
- Breakpoints are often unconfirmed (gray open circle) rather than confirmed (red dot). The debugger often doesn't even stop on unconfirmed breakpoints.
- Adding a breakpoint to one line may randomly remove a breakpoint on another line, even several lines away from the line you clicked in the margin of. Sometimes adding one breakpoint will even remove two nearby breakpoints.
Breakpoints either cannot be set at all in lambda functions, or if they are set, they remain unconfirmed, and the debugger won't stop on them.
Clicking the hot reload button in VS Code often causes breakpoints to jump to different lines, or even disappear.

Here's a video of some of the behavior: https://github.com/Dart-Code/Dart-Code/issues/4811

Related, filed by @DanTup : https://github.com/dart-lang/sdk/issues/54100 -- this was closed because it was supposed to have been fixed by https://github.com/dart-lang/sdk/commit/ec30968b54fa33aefa3f1a33ea038e63ec8cfbb3 by @derekxu16, but this did not fix the issue at all.

DanTup commented 8 months ago

@lukehutch are you able to capture a video and log using an SDK that includes ec30968b54fa33aefa3f1a33ea038e63ec8cfbb3 with specific code and steps to reproduce?

lukehutch commented 8 months ago

@DanTup I was only assuming ec30968 is in my latest build, because it was merged in November, and I'm on the pre-release channel of dart-code, and the beta channel of Dart/Flutter. However, how do I check which builds this change made it into?

DanTup commented 8 months ago

If you run flutter --version the output should include your Dart version. The commit above appears to be in 3.3.0-169.0.dev so if the version you have is newer than that, it should include this change. I would expect it to be in Flutter beta by now, although being on the beta channel doesn't necessarily mean you're on the latest beta if you haven't run flutter upgrade in a while.

If your current SDK does not include that fix, then I would try flutter upgrade to see if that gets it for you.

If your current SDK does already include this fix, then a new video/log/repro instructions would help. While it might seem exactly the same, lots of things have changed and a log file from before that fix will be less useful for debugging than one that includes it.

Thanks!

srawlins commented 8 months ago

Tentatively triaging to VM. But let me know if this should be moved to VS Code or... vm_service_client or something...

a-siva commented 8 months ago

//cc @bkonyi

bkonyi commented 8 months ago

We'll need to confirm the SDK version before we can do any investigation into this. I don't doubt there's some edge cases in our breakpoint logic, but it's hard to pinpoint without a small repro case to work with.

Breakpoints are often unconfirmed (gray open circle) rather than confirmed (red dot). The debugger often doesn't even stop on unconfirmed breakpoints.

In VSCode, breakpoints will remain unresolved until the function they're contained in has been executed at least once, triggering the initial compilation of the function. This has always been the case, but VSCode was previously showing all breakpoints as resolved. We have an issue tracking this (https://github.com/Dart-Code/Dart-Code/issues/4734) and an attempt at eagerly compiling functions to force resolve breakpoints in a draft CL somewhere (cc @derekxu16 for the current status).

derekxu16 commented 8 months ago

We have an issue tracking this (Dart-Code/Dart-Code#4734) and an attempt at eagerly compiling functions to force resolve breakpoints in a draft CL somewhere (cc @derekxu16 for the current status).

My draft CL for that regresses breakpoint functionality in certain cases (see the failing test on https://dart-review.googlesource.com/c/sdk/+/338740). I can pick it back up later this week.

a-siva commented 8 months ago

@lukehutch for us to take some action on this issue we would need the follwoing

version of Dart being used
source code of the code being debugged with steps that reproduce the problems.

This would help us reproduce the problems and fix the issues.

lukehutch commented 8 months ago

Hi all, sorry it took me a while to respond, I had to port my code to the main channel of flutter (there were a number of dependency issues).

It turns out I was not in fact using a version of the SDK that included the patch from November -- that patch doesn't appear to have been ported to the beta branch yet.

I switched to the main channel, so now I am using the following, which should include the November CL:

Flutter 3.21.0-16.0.pre.11 • channel main • https://github.com/flutter/flutter.git Framework • revision 6807eacb33 (28 minutes ago) • 2024-03-27 18:31:44 -0400 Engine • revision 73c145c9ac Tools • Dart 3.4.0 (build 3.4.0-279.0.dev) • DevTools 2.34.0-dev.12

Here are the issues that I ran into with this build:

(1) Setting a breakpoint on a const line doesn't seem to work, it seems to jump down to the first non-value statement:

https://github.com/dart-lang/sdk/assets/811305/8b1cdd22-e5eb-4a69-be52-638ad66a58bf

(2) But sometimes even non-const lines are skipped when you try to set a breakpoint, and something below the line gets set as the breakpoint instead (see Style2BottomNavBar here):

https://github.com/dart-lang/sdk/assets/811305/bb5a0756-973d-4634-aba1-5e0afc88bcd7

(3) Sometimes not a single line can have a breakpoint confirmed in an entire function, which is very unsettling. Also, sometimes breakpoints are confirmed and then immediately unconfirmed.

https://github.com/dart-lang/sdk/assets/811305/6316724b-8ddb-4974-89cf-696565b7bfd4

(Also when the debugger stops on an unconfirmed breakpoint, the breakpoint hollow dot disappears, but that is probably a Dart-Code issue)

Issues that do seem to be fixed now include:

The debugger does seem to now stop on unconfirmed breakpoints inside lambdas.
Existing breakpoints don't seem to jump around haphazardly anymore when new breakpoints are added nearby.
Other than the behavior listed in points (1) and (2) above, wherein a breakpoint may jump down a few lines, setting a breakpoint no longer causes a breakpoint to be set randomly above the line that was clicked on (before, clicking on a line could cause a breakpoint to be set or unset anywhere within a few lines' distance above or below the line that was clicked on).

a-siva commented 8 months ago

@lukehutch thanks for providing the information.

bkonyi commented 8 months ago

Thanks for the detailed response @lukehutch! The videos are very helpful in understanding what you're experiencing.

For 1), I think this is working as intended since const expressions are evaluated at compile time, so there's no executable code to set a breakpoint on. The debugger tries to find the next closest valid source location for the breakpoint, which would be the next line with executable code.

For 3), this sounds like https://github.com/Dart-Code/Dart-Code/issues/4734. I'm assuming the breakpoints resolve as soon as the function executes for the first time? If so, this is definitely a Dart-Code UX issue, although it is technically the right behavior since grey breakpoints represent unresolved breakpoints as well as invalid breakpoints. Hopefully we'll be able to force functions to compile when breakpoints are set in them, which will result in valid breakpoints being resolved immediately.

As for 2), it looks like there may be some issues with setting breakpoints in closures (maybe single line closures in particular?). That's definitely worth investigating further.

lukehutch commented 8 months ago

For 1), I think this is working as intended since const expressions are evaluated at compile time, so there's no executable code to set a breakpoint on. The debugger tries to find the next closest valid source location for the breakpoint, which would be the next line with executable code.

OK, but I suggest not setting a breakpoint at all in this case. It is unnerving to click in the margin of a line, and have a breakpoint appear 8 lines below it. Please pop up a "Breakpoint cannot be set on constant expressions" tooltip error or something.

For 3), this sounds like Dart-Code/Dart-Code#4734. I'm assuming the breakpoints resolve as soon as the function executes for the first time?

No, in my case the breakpoints never resolve, even when the function is executed, or even when the debugger actually stops on an unresolved breakpoint! As I mentioned in my previous comment, what happens in this case is that the hollow circle indicator actually disappears (in VS Code) while the debugger is stopped on the breakpoint, but neither then nor later does it turn into a red dot.

grey breakpoints represent unresolved breakpoints as well as invalid breakpoints.

Related to my first point above, I really believe that if a breakpoint cannot be set, there should be no marker showing an invalid breakpoint. Also this is confusing if invalid breakpoints and valid-but-not-confirmed breakpoints are shown the same way.

Hopefully we'll be able to force functions to compile when breakpoints are set in them, which will result in valid breakpoints being resolved immediately.

I hope that does happen! I have never used a debugging system, in any IDE, for any language, that does not set (confirmed) breakpoints immediately wherever I ask it to set them, or that has such a thing as an unconfirmed breakpoint. It is a major usability problem for Dart, and you feel like you're constantly walking on eggshells when using the debugger, because you don't even know what the debugger is going to do with breakpoints.

There should only be one type of breakpoint: a confirmed breakpoint, set on the actual line that you clicked on, which the debugger is guaranteed to stop at when executing that function.

As for 2), it looks like there may be some issues with setting breakpoints in closures (maybe single line closures in particular?). That's definitely worth investigating further.

This may be related to another issue I have seen, which I alluded to: in the current main channel, sometimes I can't get the debugger to stop at all at breakpoints in closures, even though it lets me set an (unconfirmed) breakpoint.

DanTup commented 8 months ago

I really believe that if a breakpoint cannot be set, there should be no marker showing an invalid breakpoint

Unfortunately we can't do this in VS Code - but I'm also not sure we should. The grey indicator specifically means "you've put a breakpoint here but it has not been resolved by the runtime". This could be either because it's an invalid location, or it just hasn't been resolved yet (or that it never will, for example you added it to a file that will never be loaded by the runtime).

I have never used a debugging system, in any IDE, for any language, that does not set (confirmed) breakpoints immediately wherever I ask it to set them, or that has such a thing as an unconfirmed breakpoint.

This concept is baked into VS Code and the debug adapter protocol, it's not something specific to Dart. You can add breakpoints before the program is running and so there's no way for them to be resolved. You may also add breakpoints to files of different types, or files that are just not referenced by your program and are therefore never loaded by the runtime. I've seen grey breakpoints (that ultimately turn red) in several languages in VS Code - although it usually happens pretty quick once the session starts.

I think many of the problems here are because the scripts aren't currently compiled up-front, so you see grey breakpoints a lot when they should generally be an exception (or temporary for the short period when you hit Debug and everything being up and running). I think if the work to pre-compile them was completed then some of this confusion would go away.

OK, but I suggest not setting a breakpoint at all in this case. It is unnerving to click in the margin of a line, and have a breakpoint appear 8 lines below it. Please pop up a "Breakpoint cannot be set on constant expressions" tooltip error or something.

I agree that a breakpoint moving 8 lines is odd. I wonder if breakpoints should only snap a maximum of a line or two. If there are no valid locations within that range, maybe they could just remain unresolved and never be hit (this means they would remain in the editor as grey, but I think that is the correct behaviour.. in the debug adapter protocol we can return a "reason" for an unverified breakpoint which VS Code might show in a tooltip). @bkonyi @derekxu16 what are your thoughts on this? If a breakpoint can't be resolved to a location within some small number of lines, consider that an error (something we can listen for in addition to BreakpointResolved events that we can use to mark the breakpoint as invalid).

lukehutch commented 8 months ago

How about modifying the code generator to produce additional code where necessary, so that a breakpoint can always be placed at a reasonable point in execution, wherever a breakpoint is set?

This would give rise to the possibility for Heisenbugs, because compiling with breakpoints set would introduce some new code in some circumstances (e.g. so that the debugger would stop at an appropriate point in execution even on constant expressions), but I think it would be significantly better than the status quo. (And pretty much any debugging technique can introduce Heisenbugs...)

DanTup commented 8 months ago

This would give rise to the possibility for Heisenbugs, because compiling with breakpoints set would introduce some new code in some circumstances

Breakpoints can be set after code is already compiled so I don't think this would work (at least not consistently). But I'm also not sure pausing execution at a synthetic location like that would be a good experience either (for example if you stop on a const constructor call, it may be confusing for Step In to not work because that code was already evaluated earlier).

I'm also not sure anything this dramatic is required here - I think there are a number of things described above that could significantly improve the experience (the main one IMO being compiling scripts earlier when breakpoints are set so there aren't so many grey breakpoints).

lukehutch commented 8 months ago

I came across another issue. In the following video, the breakpoints initially start as unconfirmed, then they become confirmed once the third breakpoint is hit (in this case, newItems is empty). Then when I trigger a query that causes newItems to be non-empty, the debuger stops on the first breakpoint, but then never reaches the second or third breakpoint! It is impossible to stop in the catch or even the finally block in this case. There is no async code between the first and the second/third breakpoints. All three breakpoints are confirmed, and the debugger was even able to stop on the third breakpoint, but only if the for loop never executes because newItems is empty.

https://github.com/dart-lang/sdk/assets/811305/1bb7aee7-f6ac-477a-a561-fa9aa32aaf68

DanTup commented 8 months ago

@lukehutch are you able to provide any code samples that trigger the issue to help debug? Are you testing this on stable of a version with the other fix discussed above?

bkonyi commented 8 months ago

I came across another issue. In the following video, the breakpoints initially start as unconfirmed, then they become confirmed once the third breakpoint is hit (in this case, newItems is empty). Then when I trigger a query that causes newItems to be non-empty, the debuger stops on the first breakpoint, but then never reaches the second or third breakpoint! It is impossible to stop in the catch or even the finally block in this case. There is no async code between the first and the second/third breakpoints. All three breakpoints are confirmed, and the debugger was even able to stop on the third breakpoint, but only if the for loop never executes because newItems is empty.

The behavior shown in the video is definitely unexpected, but I'm unable to reproduce on the main branch with the following example:

void foo() {
  try {
    print('foo'); // Breakpoint
  } catch(e) {
    print('bar'); // Breakpoint
  } finally {
    print('foobar'); // Breakpoint
  }
}

void main(List<String> arguments) {
  foo(); // Breakpoint
}

Either there's an issue on the revision you're using that's been fixed since or there's something with the particular structure of your code that's triggering this behavior. Can you try executing my code sample as part of your project to see if you can reproduce the same breakpoint behavior in your video?

jackmchristie commented 8 months ago

I am having very similarly broken behavior when setting breakpoints

Let me know if I can help in any other way. This problem is very intermittent for me. Reloading VS Code fixes it for a while usually.

Here is a video recorder - I am trying to set a break point on line 35 - every time I click, it does something weird with my breakpoints.

https://github.com/Dart-Code/Dart-Code/assets/11194069/9ecf0e74-f97d-4a60-9a88-1b77bf82f354

Here is the log that I tried to get:

Dart-Code-Log-2024-03-02 13-09-35.txt

Edit: Another example because I saw I'm not supposed to put a breakpoint on a const. Wacky behavior.

https://github.com/dart-lang/sdk/assets/11194069/2d37026b-acaa-427b-8eed-294d7c2a7b07

Dart-Code-Log-2024-03-02 13-35-58.txt

derekxu16 commented 7 months ago

@jackmchristie, the log that you included indicates that you're on the stable channel of Flutter. @lukehutch mentioned above that on March 27, there were breakpoint fixes that had not yet made it to the beta channel of Flutter. Can you please try switching to the main channel of Flutter and check whether you still see the same problems?

Ssiswent commented 7 months ago

I have been facing the exact same issue for about a year now. The only thing I remember is that after upgrading Dart or VS Code version, this problem start to appear.

Ssiswent commented 7 months ago

@jackmchristie The same as you and the author. Btw, can I ask how you make this running panel here? This is mine:

jackmchristie commented 7 months ago

@jackmchristie, the log that you included indicates that you're on the stable channel of Flutter. @lukehutch mentioned above that on March 27, there were breakpoint fixes that had not yet made it to the beta channel of Flutter. Can you please try switching to the main channel of Flutter and check whether you still see the same problems?

I am unable to create a reproducible simple example on the latest Flutter main. I just updated to 3.19.6 and will let you know if it happens again. I can tell you that it previously happens sporadically even in my examples. Doing a VSCode Reload Window fixes the issues, but I need to re-run my project. It does not happen every time I run my project.

I have been facing the exact same issue for about a year now. The only thing I remember is that after upgrading Dart or VS Code version, this problem start to appear.

I agree that it started happening around a year ago for me.

@jackmchristie The same as you and the author. Btw, can I ask how you make this running panel here?

It's a setting in VS Code.

derekxu16 commented 7 months ago

I just went and determined the first version of Flutter that includes the debugger fixes – it's 3.19.0. This version is on the stable channel, so if possible, can you flutter upgrade, ensure that you have a version >=3.19.0, and then try your examples again? Thanks!

lukehutch commented 7 months ago

@derekxu16 I'm on 3.87.20240402, so I should have the latest fix, and video I posted 3 weeks ago was on the same version.

derekxu16 commented 7 months ago

Yes, I'm aware that there are still debugger bugs on versions of Flutter >=3.19.0. @jackmchristie said "Let me know if I can help in any other way", though. So I'm trying to see if he's able to provide a minimal reproducible example on a version >=3.19.0.

Ssiswent commented 7 months ago

The latest version still has this problem, and it's really serious. The only way to make breakpoints work again is to stop the running app! (shift + F5)

https://github.com/dart-lang/sdk/assets/37449310/fac5f866-0014-4b03-bbd9-bd7b628c9f77

lukehutch commented 7 months ago

That exactly shows the sorts of problems I have been having.

How could the line on which the breakpoint marker is set be chosen nondeterministically/randomly, among many different alternatives, when a line is clicked? That would probably be a good place to start looking into the issue.

DanTup commented 7 months ago

@Ssiswent @lukehutch are either of you able to provide a code sample that can be copy/pasted that reproduces this? And does it always happen (including on first run), or do things start going wrong after modifying code (and possibly hot reloading/hot restarting)?

I've tried a few times to reproduce these issues from videos without luck. If we can get steps to repro, it will be much easier to track down what's happening.

lukehutch commented 7 months ago

@DanTup I have at least a partial repro:

Do flutter create flutter_debugger_issue then open VS Code on that directory.

The generated code is, in part (removing comments):

  @override
  Widget build(BuildContext context) {
    return Scaffold(
      appBar: AppBar(
        backgroundColor: Theme.of(context).colorScheme.inversePrimary,
        title: Text(widget.title),
      ),
      body: Center(
        child: Column(
          mainAxisAlignment: MainAxisAlignment.center,
          children: <Widget>[
            const Text(
              'You have pushed the button this many times:',
            ),
            Text(
              '$_counter',
              style: Theme.of(context).textTheme.headlineMedium,
            ),
          ],
        ),
      ),
      floatingActionButton: FloatingActionButton(
        onPressed: _incrementCounter,
        tooltip: 'Increment',
        child: const Icon(Icons.add),
      ),
    );
  }

Wrap the body in a LayoutBuilder (to create a lambda boundary), and inline _incrementCounter (so that you now have a lambda inside a lambda):

  @override
  Widget build(BuildContext context) {
    return LayoutBuilder(
      builder: (context, constraints) => Scaffold(
        appBar: AppBar(
          backgroundColor: Theme.of(context).colorScheme.inversePrimary,
          title: Text(widget.title),
        ),
        body: Center(
          child: Column(
            mainAxisAlignment: MainAxisAlignment.center,
            children: <Widget>[
              const Text(
                'You have pushed the button this many times:',
              ),
              Text(
                '$_counter',
                style: Theme.of(context).textTheme.headlineMedium,
              ),
            ],
          ),
        ),
        floatingActionButton: FloatingActionButton(
          onPressed: () {
            setState(() {
              _counter++;
            });
          },
          tooltip: 'Increment',
          child: const Icon(Icons.add),
        ),
      ),
    );
  }

This is the behavior I see:

https://github.com/dart-lang/sdk/assets/811305/fda6b6ee-47a6-4621-abed-83122833fd83

The main difference between this behavior and what I see on a much larger project is that with this smaller project, breakpoints are more likely to be confirmed (red), at least if I click in the LayoutBuilder lambda.

The breakpoints in the inner lambda, for onPressed, are not confirmed, and are not set on the right line.

If the above doesn't reproduce the issue for you, try a hot restart, then try again.

DanTup commented 7 months ago

@lukehutch which version of Flutter are you using? I'm on latest master but I don't see to be able to reproduce this even after a hot restart. I see the breakpoints grey initially, then turn red (although I'm not certain why the first one turns red as the second one is added):

https://github.com/dart-lang/sdk/assets/1078012/52117464-5610-4a2a-988d-2b07ca2fa4dc

Besides the grey/red issue (which is described in https://github.com/Dart-Code/Dart-Code/issues/4734 and @bkonyi had some ideas about fixing) it seems to be working as I'd expect.

One thing that's not clear from your repro though is whether you modified code before hot reloading. There are more opportunities for things to get out of sync there because VS Code is tracking the state of breakpoints as you edit files and doesn't really know what version of the file is in the VM.. if you think this is related and have specific steps to repro, please let me know!

lukehutch commented 7 months ago

I'm on Flutter 3.22.0-0.3.pre, Dart 3.4.0-282.3.beta, code v1.88.1-1712771932.el8.x86_64, dart-code v3.87.20240423.

I didn't modify the code between running it and hot-reloading.

PS: your video doesn't play for me, for some reason...

PPS: How are you getting a treeview superimposed in the left margin of the code?

DanTup commented 7 months ago

@lukehutch are you able to capture a full log (run Dart: Capture Debugging Logs before you start the app) and attach here (using the newly created test project, so it matches what I'm using)? Maybe from that I can better understand what's happening.

PS: your video doesn't play for me, for some reason...

Odd, works on my Windows PC and my MacBook. Basically I added breakpoints to the same lines as you, and they all remained on those lines, and then I triggered the ones in the callback by clicking the button in the app.

PPS: How are you getting a treeview superimposed in the left margin of the code?

That's the "Flutter UI Guidelines" - https://dartcode.org/releases/v3-1/#preview-flutter-ui-guides

It's still "preview" and not enabled by default because it's quite janky because of limited VS Code APIs (it's drawn as text with ascii characters) - see https://github.com/microsoft/vscode/issues/73780.

lukehutch commented 7 months ago

@DanTup OK, for some reason this is very hard to reproduce now (and I didn't change the code at all...)

I got a repro only one time out of about 20 or 30 attempts, and the issue showed up for only one one breakpoint:

At 1:14 in this video, I clicked on line 61, but the breakpoint was set on line 47:

https://github.com/dart-lang/sdk/assets/811305/07a00705-f851-4674-964c-74d7fb4200c6

Logs:

Dart-Code-Log-2024-03-02 01-19-19.txt

DanTup commented 7 months ago

@lukehutch thank you for the video and log, this is very helpful. I believe I've tracked down at least one cause of these issues. Here's what I see:

00:48: Add a breakpoint to line 47

The VM adds this breakpoint and resolves it. The ID is "breakpoints/1":

[1:20:14] [General] <== [VM] {"jsonrpc":"2.0","result":{"type":"Breakpoint","fixedId":true,"id":"breakpoints/1","enabled":true,"breakpointNumber":1,"resolved":true,"location":{"type":"SourceLocation","script":{"type":"@Script","fixedId":true,"id":"libraries/@18279082/scripts/package%3Aflutter_debugger_issue%2Fmain.dart/18f2de00c3a","uri":"package:flutter_debugger_issue/main.dart","_kind":"kernel"},"tokenPos":1180,"line":47,"column":31}},"id":"13"}

01:04 Remove breakpoint from line 47

This cause DAP to remove the breakpoint "breakpoints/1" from the VM:

[1:20:22] [General] ==> [VM] {"jsonrpc":"2.0","id":"30","method":"removeBreakpoint","params":{"isolateId":"isolates/2872512907539435","breakpointId":"breakpoints/1"}}
[1:20:22] [General] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Debug","event":{"type":"Event","kind":"BreakpointRemoved","isolateGroup":{"type":"@IsolateGroup","id":"isolateGroups/3867994198877240","name":"main.dart","number":"3867994198877240","isSystemIsolateGroup":false},"isolate":{"type":"@Isolate","id":"isolates/2872512907539435","name":"main","number":"2872512907539435","isSystemIsolate":false,"isolateGroupId":"isolateGroups/3867994198877240"},"timestamp":1714461621702,"breakpoint":{"type":"Breakpoint","fixedId":true,"id":"breakpoints/1","enabled":true,"breakpointNumber":1,"resolved":true,"location":{"type":"SourceLocation","script":{"type":"@Script","fixedId":true,"id":"libraries/@17279082/scripts/package%3Aflutter_debugger_issue%2Fmain.dart/18f2de03318","uri":"package:flutter_debugger_issue/main.dart"},"tokenPos":1180,"line":47,"column":31}}}}}
[1:20:22] [General] <== [VM] {"jsonrpc":"2.0","result":{"type":"Success"},"id":"30"}

01:15 Add a breakpoint to line 61

We send the breakpoint to the VM, and it assigns the ID "breakpoints/1":

[1:20:31] [DAP] ==> {"command":"setBreakpoints","arguments":{"source":{"adapterData":{"type":"@Script","id":"libraries/@17279082/scripts/package%3Aflutter_debugger_issue%2Fmain.dart/18f2de03318","fixedId":true,"uri":"package:flutter_debugger_issue/main.dart"},"name":"package:flutter_debugger_issue/main.dart","path":"/tmp/flutter_debugger_issue/lib/main.dart"},"lines":[61],"breakpoints":[{"line":61}],"sourceModified":false},"type":"request","seq":23}
[1:20:31] [General] ==> [VM] {"jsonrpc":"2.0","id":"36","method":"addBreakpointWithScriptUri","params":{"isolateId":"isolates/3553538615064143","scriptUri":"file:///tmp/flutter_debugger_issue/lib/main.dart","line":61}}
[1:20:31] [General] <== [VM] {"jsonrpc":"2.0","result":{"type":"Breakpoint","fixedId":true,"id":"breakpoints/1","enabled":true,"breakpointNumber":1,"resolved":false,"location":{"type":"UnresolvedSourceLocation","script":{"type":"@Script","fixedId":true,"id":"libraries/@17279082/scripts/package%3Aflutter_debugger_issue%2Fmain.dart/18f2de045ba","uri":"package:flutter_debugger_issue/main.dart","_kind":"kernel"},"line":61}},"id":"36"}
[1:20:31] [General] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Debug","event":{"type":"Event","kind":"BreakpointAdded","isolateGroup":{"type":"@IsolateGroup","id":"isolateGroups/739322616014296","name":"main.dart","number":"739322616014296","isSystemIsolateGroup":false},"isolate":{"type":"@Isolate","id":"isolates/3553538615064143","name":"main","number":"3553538615064143","isSystemIsolate":false,"isolateGroupId":"isolateGroups/739322616014296"},"timestamp":1714461631053,"breakpoint":{"type":"Breakpoint","fixedId":true,"id":"breakpoints/1","enabled":true,"breakpointNumber":1,"resolved":false,"location":{"type":"UnresolvedSourceLocation","script":{"type":"@Script","fixedId":true,"id":"libraries/@17279082/scripts/package%3Aflutter_debugger_issue%2Fmain.dart/18f2de045ba","uri":"package:flutter_debugger_issue/main.dart"},"line":61}}}}}

In the debug adapter, we handle the VM reusing breakpoint IDs (because that can be normal if you add multiple breakpoints that resolve to the same location), but because we never cleared the state we kept when we removed the first breakpoint, it appears that we send back the stale information about the first breakpoint with that ID (on line 47):

[1:20:31] [DAP] <== {"seq":3979,"type":"response","body":{"breakpoints":[{"id":100004,"verified":false}]},"command":"setBreakpoints","request_seq":23,"success":true}
[1:20:31] [DAP] <== {"seq":3980,"type":"event","body":{"breakpoint":{"column":31,"id":100004,"line":47,"verified":true},"reason":"changed"},"event":"breakpoint"}

This is hopefully an easy fix - I'll try to get a repro in a test.

FYI @bkonyi @a-siva - this seems like a DAP bug that would cause breakpoints to show up in the wrong locations, so the VM resolving to different locations might not be the issue we thought it was. I'm curious if it's intended that breakpoint IDs are reused like this though? (I will fix on the assumption it is, but I think not reusing them would avoid some class of issues like this where an editor is caching data to try and handle things like BPs that have already been resolved by earlier requests without waiting for an event).

DanTup commented 7 months ago

Here's a small repro. I think the breakpoint ID reuse only happens when it's a new isolate, so the hot restart is important - it causes the new breakpoint to reuse information about the first one.

Reproducing in plain Dart in the IDE might be tricky (because of the isolates and no hot restart), but probably can be done in a test by sending a breakpoint to just one isolate and removing it, then sending a different breakpoint to another.

import 'package:flutter/material.dart';

/// 1. Add a breakpoint on line 17
/// 2. Remove the breakpoint on line 17
/// 3. Hot Restart
/// 4. Add a breakpoint on line 20.

void main() {
  runApp(const MainApp());
}

class MainApp extends StatelessWidget {
  const MainApp({super.key});

  @override
  Widget build(BuildContext context) {
    return const MaterialApp(
      home: Scaffold(
        body: Center(
          child: Text('Hello World!'),
        ),
      ),
    );
  }
}

https://github.com/dart-lang/sdk/assets/1078012/8c231ae6-a9e1-440e-97da-dfee153fc4c1

DanTup commented 7 months ago

@bkonyi @a-siva

I'm curious if it's intended that breakpoint IDs are reused like this though?

Nevermind - with more testing, I have realised that breakpoint IDs are entirely isolate-specific. The reason they seemed to be the same across isolates was just because in my tests isolates were all created and given the same breakpoints at the same time. If I set breakpoints, spawn an isolate, then spawn more - I can see the new isolate is starting from 1 and the assumptions in DAP are all wrong.

bkonyi commented 7 months ago

@bkonyi @a-siva

I'm curious if it's intended that breakpoint IDs are reused like this though?

Nevermind - with more testing, I have realised that breakpoint IDs are entirely isolate-specific. The reason they seemed to be the same across isolates was just because in my tests isolates were all created and given the same breakpoints at the same time. If I set breakpoints, spawn an isolate, then spawn more - I can see the new isolate is starting from 1 and the assumptions in DAP are all wrong.

I can confirm that we don't reuse breakpoint IDs, but IDs are isolate specific and start from 1.

DanTup commented 7 months ago

@bkonyi thanks - got a fix on the way. It also seems that maybe the VM isn't jumping breakpoints as much as we (I) thought - for example all of those on the const constructor lines here do not move, they just don't resolve:

https://github.com/dart-lang/sdk/assets/1078012/86ddd82d-f8b5-497e-be5b-2b40cb94006b

So it's possible that there's nothing to do there (cc @derekxu16 @aam). It would definitely be nice to improve the tooltip for those breakpoints (because I don't think it's obvious why you can't break there, especially if the const is a long way away in code), but I can review the VM responses and file a separate issue about that.

a-siva commented 7 months ago

@DanTup thanks for the updates and follow up on this. If it turns out the issue is in DAP I would also suggest we don't implement the eager resolution of breakpoints and leave it the way it is.

DanTup commented 7 months ago

@a-siva

If it turns out the issue is in DAP I would also suggest we don't implement the eager resolution of breakpoints and leave it the way it is.

I do still think there's a value in that, because right now you can't tell the difference between breakpoints in invalid locations (such as line 18, 19, 20 in my video above around 00:12) and those that just aren't resolved yet. However, I do think the bug I'm fixing is the bigger issue and things will feel much less broken with is fixed and it may be worth re-evaluating how things feel with that fixed.

Once the fix lands and gets to Flutter master (via a DDS release), it would be good to try and get some feedback from people above that have been hitting these issues.

derekxu16 commented 7 months ago

I do still think there's a value in that, because right now you can't tell the difference between breakpoints in invalid locations (such as line 18, 19, 20 in my video above around 00:12) and those that just aren't resolved yet.

We discussed the possibility of using tooltips to differentiate between these two categories of breakpoints, though. Do you think the UX of that solution is worse than that when breakpoints are eagerly resolved?

lukehutch commented 7 months ago

@DanTup I hope that this does improve things, but I think you are exhibiting a different issue: the breakpoint jumps from line 20 to line 17 because the entire expression is const, and the only non-const part of it is the return.

The issues I have seen most reliably occur in lambdas.

DanTup commented 7 months ago

We discussed the possibility of using tooltips to differentiate between these two categories of breakpoints, though. Do you think the UX of that solution is worse than that when breakpoints are eagerly resolved?

Yes, I think we should do that - but I don't think it solves the same issue. The problem eagerly compiling fixes is giving the feedback sooner. When the user first runs their app, ideally they would see all breakpoints turn red, or remain grey with a tooltip like "A breakpoint is not allowed at this location" (or maybe even "in a const expression"!). If we don't eagerly compile then all breakpoints are grey for a longer period and the user can't tell if that's because they're in invalid locations, or just that they haven't caused compilation of that function yet.

If I'm trying to debug something and I've added a breakpoint, I really want to know if that breakpoint was valid or not before I spend time interacting with my app to try and trigger the issue I'm debugging, because if the breakpoint turns out to be invalid, that time may be wasted.

@lukehutch

but I think you are exhibiting a different issue: the breakpoint jumps from line 20 to line 17 because the entire expression is const, and the only non-const part of it is the return.

I don't believe that's the case - I thought it was until I got your log above and repro'd. I had assumed the jump was the VM snapping, but it was the bug described above - we were using the wrong resolution info (from a breakpoint with the same ID in another isolate). For breakpoints on const expressions, they fail to resolve so they remain in the same place and will remain grey (this is always what's happened, the jumping breakpoints is just a "visual" issue in VS Code, because we told it the wrong thing).

That's not to say there aren't other issues - but the issue above is certainly confusing things and making it hard to tell what's happening (and it's the issue your repro was triggering).

Once we get this into Flutter master, it would be great to have you try to reproduce any other issue and capture a log without that bug getting in the way.

lukehutch commented 6 months ago

OK, I have had a truly weird situation come up, which may shed some light on this issue...

I set two breakpoints, one on setChannelViewNewMessageListener and one on unsetChannelViewNewMessageListener, and caused the code to hit one of the breakpoints:

The code view shows that the breakpoint that was stopped on was setChannelViewNewMessageListener in line 87, but the call stack view shows that the code is actually stopped on the breakpoint in unsetChannelViewNewMessageListener -- which is also showing as line 87 in the bubble at the end of the stack frame, even though that line number is not even in that function!

Furthermore, the parent stack frame, dispose, calls unsetChannelViewNewMessageListener, not setChannelViewNewMessageListener, so it is impossible to stop on line 87 given the call stack:

Update: stepping into setChannelViewNewMessageListener shows the wrong line for that function too. It looks like the source is out of sync with the compiled code:

Doing a cold restart of the app doesn't solve the problem. I have to do flutter clean ; flutter build apk then re-run in VS Code to fix this issue. I have run into this problem quite a bit recently where I needed to do a flutter clean to fix weird issues with the source/binary alignment.

This may be a separate issue, or it may be part of the problem with breakpoints not working as they should.

lukehutch commented 6 months ago

@DanTup following on from my previous comment, I believe that the compiled code getting out of sync with the source is playing a role here (maybe separately from the issue you are currently fixing), because flutter clean ; flutter compile apk on the commandline causes the breakpoints to subsequently behave much better in VS Code.

It would be good if anyone else who is seeing this problem could verify this.

lukehutch commented 6 months ago

More evidence that the compiled code is getting out of sync with the source (there are some old stale files being left around that are not being correctly re-generated during an incremental build):

I plugged in a Pixel 7 Pro, hit F5 to launch the app, which incrementally rebuilt and then launched on the phone, and I got the above error. I then plugged in a Pixel 4, hit F5 again to launch the app, and the above error was not triggered on that device (so it's not a problem in the code -- also, there are no analyzer errors shown in VS Code for this app). I plugged the Pixel 7 Pro back in, attached the debugger, and the error was still triggered. I did a hot reload, and the problem went away.

(This sort of thing keeps happening...)

lukehutch commented 6 months ago

I dug further into the stale code issue. My client, after a hot restart, was trying to call and endpoint that didn't even exist anymore -- in fact the endpoint name didn't even exist in my source anymore. Here are the files that contained the stale symbol:

$ grep addEmojiReaction . -r
grep: ./build/app/intermediates/stripped_native_libs/release/stripReleaseDebugSymbols/out/lib/armeabi-v7a/libapp.so: binary file matches
grep: ./build/app/intermediates/stripped_native_libs/release/stripReleaseDebugSymbols/out/lib/arm64-v8a/libapp.so: binary file matches
grep: ./build/app/intermediates/stripped_native_libs/release/stripReleaseDebugSymbols/out/lib/x86_64/libapp.so: binary file matches
grep: ./build/app/intermediates/flutter/debug/flutter_assets/kernel_blob.bin: binary file matches
grep: ./build/app/intermediates/flutter/release/armeabi-v7a/app.so: binary file matches
grep: ./build/app/intermediates/flutter/release/arm64-v8a/app.so: binary file matches
grep: ./build/app/intermediates/flutter/release/x86_64/app.so: binary file matches
grep: ./build/app/intermediates/assets/debug/mergeDebugAssets/flutter_assets/kernel_blob.bin: binary file matches
grep: ./build/app/intermediates/merged_native_libs/release/mergeReleaseNativeLibs/out/lib/armeabi-v7a/libapp.so: binary file matches
grep: ./build/app/intermediates/merged_native_libs/release/mergeReleaseNativeLibs/out/lib/arm64-v8a/libapp.so: binary file matches
grep: ./build/app/intermediates/merged_native_libs/release/mergeReleaseNativeLibs/out/lib/x86_64/libapp.so: binary file matches
grep: ./.dart_tool/flutter_build/954dc83fd2834ac9b92c685a1effff2e/armeabi-v7a/app.so: binary file matches
grep: ./.dart_tool/flutter_build/954dc83fd2834ac9b92c685a1effff2e/arm64-v8a/app.so: binary file matches
grep: ./.dart_tool/flutter_build/954dc83fd2834ac9b92c685a1effff2e/app.dill: binary file matches
grep: ./.dart_tool/flutter_build/954dc83fd2834ac9b92c685a1effff2e/program.dill: binary file matches
grep: ./.dart_tool/flutter_build/954dc83fd2834ac9b92c685a1effff2e/x86_64/app.so: binary file matches
grep: ./.dart_tool/flutter_build/015933dbf8d31ced73a7771dac98a983/app.dill: binary file matches

DanTup commented 6 months ago

When the running code doesn't match what's in the editor, you'll definitely see odd behaviour. Unfortunately right now VS Code / DAP don't have a good way to handle this (besides unconditionally downloading sources from the VM, but that results in multiple copies of source files opening which can be very confusing, particularly in the case where the sources are unmodified). I have some ideas to try out, but I don't know if they'll work without changes to VS Code / DAP.

It's easy to get into this state if you (for example) disable hot-reload-on-save or even just modify a file and don't save - however I get the impression that you're unexpectedly getting into that state so it would be good to get a repro (possibly that's more of a Flutter issue than Dart/debugger, but I'm not too familiar with the compilation side of things - maybe @a-siva can advise on best steps to debug that).

lukehutch commented 6 months ago

@DanTup that's understandable if I edit code and don't hit Save. It's not understandable for code and the generated program to be out of sync if I save the source then hit F5 to run, then do a hot reload without changing anything, and then I end up with a binary running that somehow has symbols in it that haven't existed in my source for a couple of hours (since many saves ago). Something is very busted here, but I don't know if it's in Dart or Dart-Code.

This may be a new issue -- I upgraded VS Code and Dart/Flutter recently -- however, it could also be an old issue that happens to be manifesting itself much more prominently given the recent upgrades, and that could be causing some of the issues where the breakpoints are not stable or do not resolve to the correct line. (Which is why I reported it here...)

I don't have a clue how to make a good repro case for this, but I'll keep watching the situation, and I will report more if I can find any more info.

dart-lang / sdk