Skip to content

Fix PSES crash on debug start when function breakpoint defined #624

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

Merged
merged 2 commits into from
Feb 20, 2018

Conversation

rkeithhill
Copy link
Contributor

Fixex PowerShell/vscode-powershell#1159

When VSCode passes us a breakpoint to set, we normally set
a flag to indicate "setBreakpointInProgress" so that when
the DebugService_BreakpointUpdated event is fired, we can
tell that we initiated it instead of the user using Set-PSBreakpoint
to set a breakpoint. Well, the code that handled function
breakpoints msgs sent by VSCode was not setting that flag.

Also, when the user does use Set-PSBreakpoint -Command
there is no debug protocol event for function breakpoints
so we need to ignore this type of breakpoint set by the user
until the debug protocol support it.
See https://github.com/Microsoft/vscode-debugadapter-node/issues/157

Fixex PowerShell/vscode-powershell#1159

When VSCode passes us a breakpoint to set, we normally set
a flag to indicate "setBreakpointInProgress" so that when
the DebugService_BreakpointUpdated event is fired, we can
tell that we initiated it instead of the user using Set-PSBreakpoint
to set a breakpoint.   Well, the code that handled function
breakpoints msgs sent by VSCode was not setting that flag.

Also, when the user does use Set-PSBreakpoint -Command
there is no debug protocol event for function breakpoints
so we need to ignore this type of breakpoint set by the user
until the debug protocol support it.  See https://github.com/Microsoft/vscode-debugadapter-node/issues/157
@rkeithhill
Copy link
Contributor Author

Hmm, all the Microsoft.PowerShell.EditorServices.Test.dll tests pass on my machine:

=== TEST EXECUTION SUMMARY ===
   Microsoft.PowerShell.EditorServices.Test  Total: 93, Errors: 0, Failed: 0, Skipped: 3, Time: 12.497s
~\GitHub\rkeithhill\PowerShellEditorServices\test\PowerShellEditorServices.Test

@TylerLeonhardt
Copy link
Member

AppVeyor likes to throw a fit every once in a while. We're good now :)

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks, Keith!


// try
// {
// // Set exception breakpoints in DebugService
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we need this commented out code? If so, can you supply a reason in the comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping we will eventually get support for breaking on unhandled exceptions (maybe even first thrown). When we do, I don't want to have the same mistake of not setting the setBreakpointInProgress flag. That was the one of the causes of the crash. I can add a comment along those lines.

@rkeithhill
Copy link
Contributor Author

@tylerl0706 Can you kick the appveyor build?

@TylerLeonhardt
Copy link
Member

kicked ⚽

Copy link
Contributor

@daviwil daviwil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks Keith!

@rkeithhill rkeithhill merged commit 5eb586e into master Feb 20, 2018
@rkeithhill rkeithhill deleted the rkeithhill/fix-funcbkpt-crash branch February 20, 2018 17:12
@rkeithhill rkeithhill added this to the 1.6.0 milestone Feb 20, 2018
TylerLeonhardt pushed a commit to TylerLeonhardt/PowerShellEditorServices that referenced this pull request Feb 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants