debug: use unique stack frame id

[xiphon]

No description provided.

[ramya-rao-a]

Thanks for the PR @xiphon!

@brycekahle Given that you had earlier worked in the stackTraceRequest area, can you please review this PR and try out the changes?

[jhendrixMSFT]

@xiphon I tested this out using the repro provided in the linked issue but it doesn't appear to work, i.e. the value for animal is still dog when stopped after animal is shadowed with the value cat. Am I missing something?

[ramya-rao-a]

Any chance this is related to Variable Shadowing?

[jhendrixMSFT]

It is, I thought the intent of the PR was to address it but perhaps I'm mistaken. I had dug into it a bit, delve sends back all the locals including the shadowed ones. E.g.

"pet": "dog",
"pet": "cat"

We send all of them back to VS Code and it would appear that when there are duplicate entries the first one is displayed and the rest discarded (this is just a guess on my part but fits with my observation).
One idea is to prune out the dupes based on their line number with respect to the current line number (I actually have code for this). Another option would be to display the shadowed variables the same way delve does which might be useful (we could also make this option configurable).

[xiphon]

Yep, seems this PR is not related to #1974. Updated the description.

It still fixes the problem - we have to use unique stack frame identifiers.

Btw, there is another PR attempting to fix the same issue #2208

[segevfiner]

We are not resetting stackFrameHandles ever. This will cause memory consumption to infinitely raise.

[ramya-rao-a]

@segevfiner Are you referring to resetting the handles in between 2 debugging sessions or inside a single debugging session?

[segevfiner]

Both are an issue technically. During one debug session you keep accumlating handles, and handles from a previous session remain after it ends (Doesn't VS Code close the debug adapter though?)

Another point is that changing the thread ID, frame ID might cause VS Code to consider this a new thread which can make it lose focus. I didn't check whether this PR has such an issue or not.

[jhendrixMSFT]

@segevfiner the debug adapter process is recycled between sessions however you're correct that the number of handles will continue to grow.
Taking a step back, can somebody articulate why this fix is needed? Not that I'm pushing back (although we do need to address the handle growth), I don't see a referenced issue so I'm missing some context.

[ramya-rao-a]

@jhendrixMSFT When investigating another issue, @segevfiner came across documentation for the debug adapter protocol that the stackframe id should be unique across all threads. See #2133 (comment)

That's the motivation for @segevfiner's attempt to fix the problem.

I believe @xiphon encountered the same when investigating the variable shadowing problem

Though, in itself, this PR doesnt fix any user facing issue we are aware of, this bug could be the reason for some other issue in the future (or present)

[jhendrixMSFT]

Digging into this a bit more we should be resetting stackFrameHandles and _variableHandles every time the debugger stops as the information is no longer valid and is essentially a memory leak; this is exactly what the node debugger does, see here. I think the best place to do this is in handleReenterDebug() unless there's a better suggestion.

[jhendrixMSFT]

Reset handle collections to avoid memory leak.

[xiphon]

@jhendrixMSFT you are absolutely correct. Once we stop there, previous handles are no longer valid, handleReenterDebug is the right place. Updated.

[OneOfOne]

@xiphon can you rebase against master please.

[xiphon]

@OneOfOne
Rebased