-
Notifications
You must be signed in to change notification settings - Fork 675
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
Prevent crash due to NullReferenceException in interactive window component #4589
Conversation
_eval.WriteError(FixNewLines(e.Data)); | ||
try { | ||
_eval.WriteError(FixNewLines(e.Data)); | ||
} catch (Exception ex) when (!ex.IsCriticalException()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what causes NRE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the stack and links I've put in #4513
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, can we check Dispatcher.HasShutdownStarted
in when
condition?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How? With Dispatcher.CurrentDispatcher.HasShutdownStarted
? Docs say that never returns null
, that one is created if there isn't one. Will it have the same values as the dispatcher object in the interactive window:
private Dispatcher Dispatcher => ((FrameworkElement)_uiOnly.TextView).Dispatcher; // Always safe to access the dispatcher.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Always safe to access the dispatcher.
Well, that's actually not true: https://referencesource.microsoft.com/#WindowsBase/Base/System/Windows/Freezable.cs,887
We should be able to use _eval.CurrentWindow.TextView.VisualElement.Dispatcher == null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just saw the exception while I was debugging, I was able verify that _eval.CurrentWindow.TextView.VisualElement.Dispatcher.HasShutdownStarted==true
(and _eval.CurrentWindow.TextView.VisualElement.Dispatcher.hasShutdownFinished==true
)
so I'll catch if shutdown started or dispatcher is null.
…e window sometimes throws.
@@ -290,7 +291,8 @@ Process process | |||
if (!AppendPreConnectionOutput(e)) { | |||
try { | |||
_eval.WriteOutput(FixNewLines(e.Data)); | |||
} catch (Exception ex) when (!ex.IsCriticalException()) { | |||
} catch (Exception ex) when (!ex.IsCriticalException() && | |||
(_eval.CurrentWindow.TextView.VisualElement.Dispatcher?.HasShutdownStarted ?? true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's extract it into separate method.
try { | ||
_eval.WriteError(FixNewLines(e.Data)); | ||
} catch (Exception ex) when (!ex.IsCriticalException() && | ||
(_eval.CurrentWindow.TextView.VisualElement.Dispatcher?.HasShutdownStarted ?? true)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's extract it into method
Fix #4513
Adding the same type of protection we already had in
ProcessExited
handler to theStdErrReceived
andStdOutReceived
handlers.