Dart-Code icon indicating copy to clipboard operation
Dart-Code copied to clipboard

Breakpoint not hit on web after a hot reload in latest stable

Open DanTup opened this issue 1 year ago • 10 comments

Not sure where this bug is yet, but on current stable if I:

  • create new Flutter app
  • add a breakpoint in build method
  • run on Chrome device
  • hit breakpoint
  • click resume
  • click hot reload

The breakpoint is again hit during the execution of code after the hot reload.

However, on the latest beta, the breakpoint is hit the first time, but is not hit after the reload. A quick scan of the logs suggest that the VM does not pause during the hot reload.

This is causing the "stops at a breakpoint after each reload (1)" integration test to fail.

(@elliette @bkonyi FYI in case you have any ideas, it might take me a little while to get useful logs that can be compared since there's a ton of noise in them)

DanTup avatar Aug 01 '24 18:08 DanTup

Did you mean hot restart by any chance?

bkonyi avatar Aug 01 '24 18:08 bkonyi

Nope, hot reload :-)

DanTup avatar Aug 01 '24 18:08 DanTup

Wait, I kinda do, because we don't support really hot reload on web. But as far as the IDE knows it's a hot reload 😄 (see https://github.com/Dart-Code/Dart-Code/issues/4991)

(but if I send a Hot Restart from the IDE, all is fine... if I send a Hot Reload, then we don't seem to see get the pause)

DanTup avatar Aug 01 '24 18:08 DanTup

That's interesting that hot reload doesn't work while hot restart does since they should be doing the same thing at this point in time on stable...

bkonyi avatar Aug 01 '24 18:08 bkonyi

I'm not sure what might have regressed here, but here is the tracking bug for initially getting this to work: https://github.com/flutter/devtools/issues/7231

Sending you both a doc with more details as well

elliette avatar Aug 01 '24 19:08 elliette

Hmm, interestingly with a brand new project I see this bug on both stable and beta, but on the Dart-Code test project (which is basically the same but with a little extra code that sets some variables and prints some values), it works on stable but fails on beta. I wonder if there is a race here and the test project has enough code that affects the outcome.

Testing on the version where I see the difference, the logs look like this. For stable (when it works), the VM service traffic when I hot reload looks like this:

########################### Clicking hot reload                                                     <== [VM] {"
[12:15:09 PM] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Debug","event":{"type":"Event","kind":"BreakpointRemoved","timestamp":1111111,"isolate":{"type":"@Isolate","id":"1","number":"1","name":"main()","isSystemIsolate":false,"isolateGroupId":""},"breakpoint":{"type":"Breakpoint","id":"bp/4#49:0","breakpointNumber":733,"enabled":true,"resolved":true,"location":{"type":"SourceLocation","script":{"type":"@Script","id":"4","uri":"package:flutter_hello_world/main.dart"},"tokenPos":1436,"line":49,"column":5}}}}}
[12:15:09 PM] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Isolate","event":{"type":"Event","kind":"IsolateExit","timestamp":1111111,"isolate":{"type":"@Isolate","id":"1","number":"1","name":"main()","isSystemIsolate":false,"isolateGroupId":""}}}}
[12:15:09 PM] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Debug","event":{"type":"Event","kind":"BreakpointAdded","timestamp":1111111,"isolate":{"type":"@Isolate","id":"754","number":"754","name":"main()","isSystemIsolate":false,"isolateGroupId":""},"breakpoint":{"type":"Breakpoint","id":"bp/757#49:0","breakpointNumber":1486,"enabled":true,"resolved":true,"location":{"type":"SourceLocation","script":{"type":"@Script","id":"757","uri":"package:flutter_hello_world/main.dart"},"tokenPos":244044,"line":49,"column":5}}}}}
[12:15:09 PM] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Isolate","event":{"type":"Event","kind":"IsolateStart","timestamp":1111111,"isolate":{"type":"@Isolate","id":"754","number":"754","name":"main()","isSystemIsolate":false,"isolateGroupId":""}}}}
[12:15:09 PM] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Debug","event":{"type":"Event","kind":"Resume","timestamp":1111111,"isolate":{"type":"@Isolate","id":"754","number":"754","name":"main()","isSystemIsolate":false,"isolateGroupId":""}}}}
[12:15:09 PM] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Isolate","event":{"type":"Event","kind":"IsolateRunnable","timestamp":1111111,"isolate":{"type":"@Isolate","id":"754","number":"754","name":"main()","isSystemIsolate":false,"isolateGroupId":""}}}}

Here, a breakpoint is added and then the isolate is resumed. Note: This traffic is all from the VM Service (DWDS), nothing was sent to it. I think this means DWDS is sending the breakpoint and resuming (this is unexpected, but presumably what results in the breakpoint working).

In the broken version (beta), the log looks like this:

########################### Clicking hot reload                                                     <== [VM] {"
[12:15:09 PM] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Debug","event":{"type":"Event","kind":"BreakpointRemoved","timestamp":1111111,"isolate":{"type":"@Isolate","id":"1","number":"1","name":"main()","isSystemIsolate":false,"isolateGroupId":""},"breakpoint":{"type":"Breakpoint","id":"bp/4#49:0","breakpointNumber":737,"enabled":true,"resolved":true,"location":{"type":"SourceLocation","script":{"type":"@Script","id":"4","uri":"package:flutter_hello_world/main.dart"},"tokenPos":1435,"line":49,"column":5}}}}}
[12:15:09 PM] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Isolate","event":{"type":"Event","kind":"IsolateExit","timestamp":1111111,"isolate":{"type":"@Isolate","id":"1","number":"1","name":"main()","isSystemIsolate":false,"isolateGroupId":""}}}}
[12:15:09 PM] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Isolate","event":{"type":"Event","kind":"IsolateStart","timestamp":1111111,"isolate":{"type":"@Isolate","id":"758","number":"758","name":"main()","isSystemIsolate":false,"isolateGroupId":""}}}}
[12:15:09 PM] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Debug","event":{"type":"Event","kind":"Resume","timestamp":1111111,"isolate":{"type":"@Isolate","id":"758","number":"758","name":"main()","isSystemIsolate":false,"isolateGroupId":""}}}}
[12:15:09 PM] <== [VM] {"jsonrpc":"2.0","method":"streamNotify","params":{"streamId":"Isolate","event":{"type":"Event","kind":"IsolateRunnable","timestamp":1111111,"isolate":{"type":"@Isolate","id":"758","number":"758","name":"main()","isSystemIsolate":false,"isolateGroupId":""}}}}

Here, the isolate is resumed, but no breakpoint was added. I think this may be related to the issue @elliette linked above, because we don't think DWDS should be adding the breakpoints (the IDE/DAP should). However, the isolate being resumed before DAP has been able to re-send the breakpoints means there is a race here. The isolate should be starting paused.

Looking back through the complete log, I see this:

[12:16:13 PM] ==> [VM] {"jsonrpc":"2.0","id":"8","method":"requirePermissionToResume","params":{"onPauseStart":true,"onPauseReload":false,"onPauseExit":true}}

This false for reload looks a little suspicious, however it's not clear to me which flag here applies (because the IDE thinks this is hot reload but DWDS is probably doing a hot restart)? Looking in DAP, it appears that we might only ever set the first/last flag and now I'm also wondering if that is correct?

https://github.com/dart-lang/sdk/blob/786742f34cb4a55e6cfbc349a0918c97b54aa43a/pkg/dds/lib/src/dap/adapters/dart.dart#L870

DanTup avatar Aug 02 '24 11:08 DanTup

@bkonyi @elliette is there a clear definition of onPauseStart and onPauseReload somewhere? onPauseReload is false in the log before but it's not clear to me if that's the flag used during a hot reload. If it is, I feel like it should be true so we can re-send breakpoints?

DanTup avatar Aug 20 '24 16:08 DanTup

I was going off the assumption that onPauseStart is for hot-restarts and onPauseReload is for hot-reloads (but I don't know if that's documented anywhere). DWDS ignores onPauseReload because hot-reloads aren't supported for web.

It looks like DWDS 24.0.0 is in the current Flutter beta: https://github.com/flutter/flutter/blob/7c6b7e9ca485f7eaaed913c6bb50f4be6da47e30/packages/flutter_tools/pubspec.yaml#L17

There were some fixes to restarting with breakpoints in DWDS 24.1.0, although I'm not sure they would fix this issue. https://github.com/dart-lang/webdev/releases/tag/dwds-v24.1.0. That version is on Flutter master though https://github.com/flutter/flutter/blob/cdab9d509b79660164791c34fb6e2610c7233ae8/packages/flutter_tools/pubspec.yaml#L17, so it might be good to see if this reproduces there or not. I think regardless it might make sense cherry-picking 24.1.0 into the current Flutter beta, so that it has https://github.com/dart-lang/webdev/pull/2441. FYI @bkonyi

elliette avatar Aug 20 '24 18:08 elliette

I was going off the assumption that onPauseStart is for hot-restarts and onPauseReload is for hot-reloads (but I don't know if that's documented anywhere). DWDS ignores onPauseReload because hot-reloads aren't supported for web.

That makes some sense to me, but isn't DDS handling the pause/resume behaviour, so if it treats it as a hot reload (since that's what the DAP is sending), wouldn't it use the reload flag? (and could that be why it's immediately resuming, since we didn't set that?)

If it's not that, I'm still wondering if we have a bug for non-web here - we're never setting onPauseReload for DAP, but for non-web we'd still want the new pause permission behaviour? I'm wondering if we should be setting all the flags here?

https://github.com/dart-lang/sdk/blob/337ad11aca52aa63b34df2f8b8688775ec65649b/pkg/dds/lib/src/dap/adapters/dart.dart#L870

I re-tested on Flutter master, but no change there, same issue.

DanTup avatar Aug 22 '24 15:08 DanTup

If it's not that, I'm still wondering if we have a bug for non-web here - we're never setting onPauseReload for DAP, but for non-web we'd still want the new pause permission behaviour? I'm wondering if we should be setting all the flags here?

Thinking more, I think this is only an issue if there's another debugger also trying to resume (which won't wait for DAP). So if I'm understanding correctly, I think there is a bug there, but it's probably a minor one few will see (requires two debuggers)?

I also don't think it's related to the issue here for the same reason, because there are not two debuggers involved. I think the main issue here is that during a "hot reload", there is no pause, so the breakpoints race with the application starting to run.

DanTup avatar Aug 22 '24 15:08 DanTup