[release/8.0-staging] Fix generation of minidump #115739
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ilVersion.GetRejitState(); | ||
| ilVersion.GetIL(); | ||
| } | ||
| EX_CATCH_RETHROW_ONLY_COR_E_OPERATIONCANCELLED |
There was a problem hiding this comment.
Why is the exception that this is catching thrown in the first place?
Is this a proper fix or just a quick workaround?
There was a problem hiding this comment.
It's a quick/low-risk workaround. I agree we should do a more full version of the fix in main addressing the reason for the crash.
There was a problem hiding this comment.
It's a quick/low-risk workaround. I agree we should do a more full version of the fix in main addressing the reason for the crash.
There was a problem hiding this comment.
Have we figured out why it is crashing? If we have not figured out why it is crashing, what's our confidence level that this workaround is going to solve the issue reliably and that it won't just break again later when the crash dump is opened in the debugger?
(I am fine with this workaround if we understand the root cause of the crash and determined that this workaround is the right way to solve it in servicing.)
There was a problem hiding this comment.
@jkotas I apologize for the delayed response! The crash was triggered by attempting to read the IL off an InlinedCallFrame pointing to an NDirectMethodDesc. At dump collection time, we enumerate all of the threads, and all of the stacks. While walking the frames, we encounted an InlinedCallFrame and called MethodDesc::EnumMemoryRegions on it. This in turn called ILCodeVersion::GetIL, which called MethodDesc::GetILHeader which attempted to get the IL from the module, causing an access violation due to an invalid pointer. @thaystg verified that avoiding reading the IL on NDirect methoddescs avoids the exception - candidate .net 10 change here - #116391. We decided to leave the try/catch in place but also guard the call to retrieve the IL. If the commit in main looks good, should we back port that to this servicing PR or keep the current PR as-is? The updated fix was verified locally.
There was a problem hiding this comment.
candidate .net 10 change here - #116391.
I think the change is not quite right: #116391 (comment)
should we back port that to this servicing PR or keep the current PR as-is?
The current PR is fine as-is. Thanks!
jeffschwMSFT
added
Servicing-consider
|
Tagging subscribers to this area: @tommcdon |
rbhanda
added
Servicing-approved
Backport of #115562 to release/8.0-staging
/cc @thaystg
Customer Impact
Some mini dumps generated by the runtime are missing information and the managed callstack can't be inspected.
Regression
[If yes, specify when the regression was introduced. Provide the PR or commit if known.]
Testing
We got a full dump and generated a mini dump from it and with the fix the mini dump is with the managed callstack information correctly.
Risk
Low risk, only added another try catch as there are many others in the code.