made VS not crash on invalid dll reference (BadImageFormatException).…#24452
made VS not crash on invalid dll reference (BadImageFormatException).…#24452heejaechang merged 1 commit intodotnet:dev15.6.xfrom
Conversation
|
@dotnet/roslyn-ide can you take a look? @jinujoseph @Pilchie approval? |
jasonmalinowski
left a comment
There was a problem hiding this comment.
Fine by me. @tmat might want to chime in here too.
|
@heejaechang @jinujoseph Any reason not to take this in 15.6? Not sure where "low risk reliability improvements" are fitting right now? |
There was a problem hiding this comment.
Why not just directly call FatalError.ReportWithoutCrash?
There was a problem hiding this comment.
ah. yes. I can do that.
There was a problem hiding this comment.
Do we want to report some user-visible diagnostic (to the error list e.g.), so that the customer can understand why things don't work and perhaps check their invalid assembly?
There was a problem hiding this comment.
we already did this in other places, and we didn't show anything to users. so unless we want to change that, I will leave that up to @jinujoseph
There was a problem hiding this comment.
Perhaps you can file a follow up issue to do so, to minimize this change. I think there is value in letting users know things are wrong. I am doing this in the EnC implementation now. It helps us prevent "it doesn't work and I don't know what's wrong" kind of bug reports.
There was a problem hiding this comment.
ya , file a new issue to improvise this
I think we should take this fix for 15.6 and minimize the change
There was a problem hiding this comment.
@tmat are you creating UI to show it? or info bar or output window? or diagnostics?
There was a problem hiding this comment.
It's a diagnostic that goes to the Error List.
There was a problem hiding this comment.
I'm not sure we will get anything useful here. I'd rather we displayed user visible diagnostic and let the user report an issue to us where they can share the offending assembly (if they are willing to do so).
There was a problem hiding this comment.
for now, I will remove NFW from it then.
… especially for find all reference cache generation
94d77b6 to
ed7fb6c
Compare
|
rebased to dev15.6.x |
|
@dotnet/roslyn-infrastructure test failure - Roslyn.VisualStudio.IntegrationTests.VisualBasic.BasicLineCommit.CommitOnSave |
|
retest windows_debug_vs-integration_prtest please |
|
Approved to mergelink |
… especially for find all reference cache generation
Customer scenario
User adds dlls as references and a few minutes later, VS crashes.
Bugs this fixes
https://devdiv.visualstudio.com/DevDiv/_workitems/edit/557859
Workarounds, if any
no workaround
Risk
VS will no longer crash, but symbol information from such dlls won't be available for find all references.
Performance impact
N/A
Is this a regression from a previous update?
No
Root cause analysis
Metadata symbol reader can throw BadImageFormatException when dll is in invalid format. but the find all reference didn't guard itself from all exception points but only cover some of them. leaving some code path open to the exception.
How was the bug found?
user feedback, watson