Breaking on exception should jump to top frame that has source

[DanTup]

This is slightly related to #25104. The exception peek dialog has been moved to "the top frame that has source" which is great, but the user is still dumped in the frame without source.

Example:

The top two frames in the call stack here (_AssertionError) do not have source and the user ends up looking at a rather useless message.

If you manually navigate to the third stack frame (main.<...>) you'll get the exception popup:

It seems to place the exception popup in the correct place but not to take the user to it when the exception occurs. Jumping directly to where the exception popup would be displayed seems like the ideal thing to do.

[isidorn]

@DanTup I agree, we should jump directly to where the exception popup happend.
However since I do not have repro steps for this on my machine I would ask you to put some breakpoints so we have a better idea what is going on here. An alternative is to provide repro steps so I have this on my machine.

F1 > developer tools > sources
Put a breakpoint here and verify that gets called once the exception is hit. You shuold be able to step into to this piece of code here and there you can check why a stackFrame with no source is chosen. Since the point of that line is to auto reveal only stack frames with sources.

Thanks a lot

[DanTup]

@isidorn Apologies, I didn't include a repro because I thought it was a missing feature not a bug :-)

I put a breakpoint where you suggested and found that stackFrames only has one item in it (the top-most). So I checked stackTraceRequest in my debug adapter, and it seems like the request to it has the levels argument set to 1, which means we only return the top frame. Then this code fires, and then later another request is sent asking for more stack frames (which then renders in the list).

So I guess more stack frames need to be fetched prior to trying to find the first one with sources? Presumably this'll repro with any language, but if you want to repro with Dart (you'll need the Dart extension and a Dart SDK on PATH) just open a folder that has a single .dart file in it that does this:

main() {
  assert(false);
}

(FWIW, while testing that just now, I noticed that sometimes when I switch to the main frame from the callstack, the exception popup doesn't appear - but if I move away to another frame and then back, it does - I'm not sure if there's another bug here or whether it might be something in my debug adapter, but I thought it worth noting in case you've seen it before).

[isidorn]

@DanTup Thanks for the investigation.
I acknowledge the bug. As you already found out the issue here is:

• The top stack frame gets fetched first (due to performance reasons)
• We try to auto focus that stack frame as soon as it is retrieived (we do not wait for the rest of the call stack)
• The rest of the call stack comes back later, however we do not try to autofocus them

The fix would be to try to auto focus the stack fames also when the rest of the call stack comes back as well.
Moving this to On Deck since I do not plan to fix it in December.

[weinand]

I think we should fix this in the Jan milestone.

[weinand]

Instead of focusing twice, couldn't we just delay the loading of the source until we have gotten a frame that has source?

[isidorn]

@DanTup I have pushed a fix for this, please try it out in the insiders after we start updating it again after holidays (in 10 days).

[DanTup]

@isidorn This one, however, doesn't seem to work. If I remove the deemphasize hints, then I'm still breaking on missing source (using the Insiders build from today):

This is how I'm sending the source back:

data.thread.getScript(scriptRef).then((script: VMScript) => {
			if (script.source) {
				response.body = { content: script.source };
			} else {
				response.success = false;
				response.message = "<source not available>";
			}
			this.sendResponse(response);

(this is less urgent for me personally though, since deemphasize works now :-))

[isidorn]

@DanTup thanks for trying it out. Well it seems like in your case the check here
https://github.com/Microsoft/vscode/blob/0d1139653803f7acb831a593f816c1278638011b/src/vs/workbench/parts/debug/electron-browser/debugService.ts#L781
focuses the wrong stack frame for you
Could you please put a breakpoint there and check why he chooses the wrong stack frame for you?

Thank you

[DanTup]

@isidorn I'm a little confused trying to add a breakpoint there, as the debugService file seems completely different (only a few hundred lines) for me:

However if it helps, I made a repro from the mock debug here:

https://github.com/DanTup/vscode-repro-64193
DanTup/vscode-repro-64193@78bc8ed

I changed the code to simulate an exception at the start, changed the stack request to return sourceReferences based on the stack frame index, and added a sourceReference that returned an error for the first frame, and source for the other frames. If you open this and hit F5 to run on a .md file, you'll see it still stops at the top frame:

(I tested in latest insiders)

[isidorn]

@DanTup thanks for the great steps and that you modified mock debug. I really like this process, let's continue doing this.

As for the issue, the problem here is timing.

1. Exception happens
2. We try to auto focus -> we decide to auto focus the first stack frame
3. Because it is focused we try to open and resolve it. Only now we see that the source request returns an error. It is too late since we already auto focused it

As a workaround I suggest that if the adapter know in advance that a source is not available it should return a stackFrame without a source, or with a source.presentationHint === 'deemphasize'

An alternative on the vscode side is that once we do 3. from abaove that we try to refocus, which would visually be a bit ugly. So a better way would be for the adapter to somehow notify vscode that the stackframe is not good as I suggest above.

[DanTup]

I really like this process, let's continue doing this.

Sounds good to me :-)

a stackFrame without a source, or with a source.presentationHint === 'deemphasize'

I didn't realise it was valid to send a stack frame without a source, though I am already using deemphasize so with the fix #65012 my code works fine. I left this issue open just because it seemed like the expected behaviour didn't work quite right. If you want to WontFix this and recommend using deemphasize as a solution, that's not a problem for me 👍

[isidorn]

Yup, I recommend deemphasize and will not fix this. Thanks for understanding.