Fix exception handling in the prestub worker - take 2 - #112666

Merged
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3
Feb 20, 2025
Merged

Fix exception handling in the prestub worker - take 2#112666
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3

Conversation

@janvorli

Copy link
Copy Markdown
Member

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.

The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.

The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.

On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.
The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.
The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.
On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.
I was hitting intermittent crashes in the unhandled exception test
with GC stress C enabled due to this when GetSSP tried to access the
SSP in the extended part of the context.
@janvorlijanvorli added the area-ExceptionHandling-coreclr only use for closed issues label Feb 18, 2025
@janvorli
janvorli requested a review from jkotasFebruary 18, 2025 22:09
@janvorlijanvorli self-assigned this Feb 18, 2025
CopilotAI review requested due to automatic review settings February 18, 2025 22:09

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (10)
  • src/coreclr/vm/excep.cpp: Language not supported
  • src/coreclr/vm/exceptionhandling.cpp: Language not supported
  • src/coreclr/vm/exceptmacros.h: Language not supported
  • src/coreclr/vm/prestub.cpp: Language not supported
  • src/coreclr/vm/virtualcallstub.cpp: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj: Language not supported
  • src/tests/issues.targets: Language not supported
Comments suppressed due to low confidence (1)

src/tests/Regressions/coreclr/GitHub_76531/test76531.cs:25

  • [nitpick] The error message 'My exception' is unclear. Consider providing a more descriptive message.
=> throw new Exception("My exception");

Comment threadsrc/tests/Regressions/coreclr/GitHub_76531/test76531.cs
if (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc) ||
pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
{
if ((pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jkotas this is the only change w.r.t. the previous PR that was reverted recently. This popping was causing the GC to get lost.

@jkotas

jkotas commented Feb 19, 2025

Copy link
Copy Markdown
Member

The test failure looks related:

Unhandled exception. System.OverflowException: Arithmetic operation resulted in an overflow.
at Microsoft.Win32.OAVariantLib.OAFailed(Int32 hr)
at Microsoft.Win32.OAVariantLib.ChangeType(Object source, Type targetClass, Int16 options, CultureInfo culture)
at System.OleAutBinder.ChangeType(Object value, Type type, CultureInfo cultureInfo)

* The previous set of changes removed popping of ExInfos too. That's not
correct though. But it should be done in a different place than the
ProcessCLRExceptionNew.
* There was a problem (even before the fix) that an exception caught in
DispatchInfo::InvokeMember was reported (via console log and to the
debugger) as unhandled.
This change also adds a new flavor of the unhandled exception test that
throws an unhandled exception on a secondary thread to exercise the
related code path.
@janvorli

Copy link
Copy Markdown
MemberAuthor

@jkotas I have fixed the test failure. There were actually two issues. One was that the exception was reported as unhandled even though it was actually then handled in native runtime code in the DispatchInfo::InvokeMember. The other was that the ExInfo popping was still needed and that lead to the test hang (in an interesting way - the next ExInfo that was pushed by the next portion of the test had accidentally the same address, so the presence of previous stale ExInfo link at the same address created an infinite loop in the m_pPrevNestedInfo chain.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@janvorli@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Fix exception handling in the prestub worker - take 2 - #112666

Merged
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3
Feb 20, 2025
Merged

Fix exception handling in the prestub worker - take 2#112666
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3

Conversation

@janvorli

Copy link
Copy Markdown
Member

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.

The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.

The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.

On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.
The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.
The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.
On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.
I was hitting intermittent crashes in the unhandled exception test
with GC stress C enabled due to this when GetSSP tried to access the
SSP in the extended part of the context.
@janvorlijanvorli added the area-ExceptionHandling-coreclr only use for closed issues label Feb 18, 2025
@janvorli
janvorli requested a review from jkotasFebruary 18, 2025 22:09
@janvorlijanvorli self-assigned this Feb 18, 2025
CopilotAI review requested due to automatic review settings February 18, 2025 22:09

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (10)
  • src/coreclr/vm/excep.cpp: Language not supported
  • src/coreclr/vm/exceptionhandling.cpp: Language not supported
  • src/coreclr/vm/exceptmacros.h: Language not supported
  • src/coreclr/vm/prestub.cpp: Language not supported
  • src/coreclr/vm/virtualcallstub.cpp: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj: Language not supported
  • src/tests/issues.targets: Language not supported
Comments suppressed due to low confidence (1)

src/tests/Regressions/coreclr/GitHub_76531/test76531.cs:25

  • [nitpick] The error message 'My exception' is unclear. Consider providing a more descriptive message.
=> throw new Exception("My exception");

Comment threadsrc/tests/Regressions/coreclr/GitHub_76531/test76531.cs
if (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc) ||
pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
{
if ((pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jkotas this is the only change w.r.t. the previous PR that was reverted recently. This popping was causing the GC to get lost.

@jkotas

jkotas commented Feb 19, 2025

Copy link
Copy Markdown
Member

The test failure looks related:

Unhandled exception. System.OverflowException: Arithmetic operation resulted in an overflow.
at Microsoft.Win32.OAVariantLib.OAFailed(Int32 hr)
at Microsoft.Win32.OAVariantLib.ChangeType(Object source, Type targetClass, Int16 options, CultureInfo culture)
at System.OleAutBinder.ChangeType(Object value, Type type, CultureInfo cultureInfo)

* The previous set of changes removed popping of ExInfos too. That's not
correct though. But it should be done in a different place than the
ProcessCLRExceptionNew.
* There was a problem (even before the fix) that an exception caught in
DispatchInfo::InvokeMember was reported (via console log and to the
debugger) as unhandled.
This change also adds a new flavor of the unhandled exception test that
throws an unhandled exception on a secondary thread to exercise the
related code path.
@janvorli

Copy link
Copy Markdown
MemberAuthor

@jkotas I have fixed the test failure. There were actually two issues. One was that the exception was reported as unhandled even though it was actually then handled in native runtime code in the DispatchInfo::InvokeMember. The other was that the ExInfo popping was still needed and that lead to the test hang (in an interesting way - the next ExInfo that was pushed by the next portion of the test had accidentally the same address, so the presence of previous stale ExInfo link at the same address created an infinite loop in the m_pPrevNestedInfo chain.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@janvorli@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix exception handling in the prestub worker - take 2 - #112666

Merged
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3
Feb 20, 2025
Merged

Fix exception handling in the prestub worker - take 2#112666
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3

Conversation

@janvorli

Copy link
Copy Markdown
Member

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.

The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.

The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.

On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.
The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.
The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.
On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.
I was hitting intermittent crashes in the unhandled exception test
with GC stress C enabled due to this when GetSSP tried to access the
SSP in the extended part of the context.
@janvorlijanvorli added the area-ExceptionHandling-coreclr only use for closed issues label Feb 18, 2025
@janvorli
janvorli requested a review from jkotasFebruary 18, 2025 22:09
@janvorlijanvorli self-assigned this Feb 18, 2025
CopilotAI review requested due to automatic review settings February 18, 2025 22:09

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (10)
  • src/coreclr/vm/excep.cpp: Language not supported
  • src/coreclr/vm/exceptionhandling.cpp: Language not supported
  • src/coreclr/vm/exceptmacros.h: Language not supported
  • src/coreclr/vm/prestub.cpp: Language not supported
  • src/coreclr/vm/virtualcallstub.cpp: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj: Language not supported
  • src/tests/issues.targets: Language not supported
Comments suppressed due to low confidence (1)

src/tests/Regressions/coreclr/GitHub_76531/test76531.cs:25

  • [nitpick] The error message 'My exception' is unclear. Consider providing a more descriptive message.
=> throw new Exception("My exception");

Comment threadsrc/tests/Regressions/coreclr/GitHub_76531/test76531.cs
if (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc) ||
pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
{
if ((pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jkotas this is the only change w.r.t. the previous PR that was reverted recently. This popping was causing the GC to get lost.

@jkotas

jkotas commented Feb 19, 2025

Copy link
Copy Markdown
Member

The test failure looks related:

Unhandled exception. System.OverflowException: Arithmetic operation resulted in an overflow.
at Microsoft.Win32.OAVariantLib.OAFailed(Int32 hr)
at Microsoft.Win32.OAVariantLib.ChangeType(Object source, Type targetClass, Int16 options, CultureInfo culture)
at System.OleAutBinder.ChangeType(Object value, Type type, CultureInfo cultureInfo)

* The previous set of changes removed popping of ExInfos too. That's not
correct though. But it should be done in a different place than the
ProcessCLRExceptionNew.
* There was a problem (even before the fix) that an exception caught in
DispatchInfo::InvokeMember was reported (via console log and to the
debugger) as unhandled.
This change also adds a new flavor of the unhandled exception test that
throws an unhandled exception on a secondary thread to exercise the
related code path.
@janvorli

Copy link
Copy Markdown
MemberAuthor

@jkotas I have fixed the test failure. There were actually two issues. One was that the exception was reported as unhandled even though it was actually then handled in native runtime code in the DispatchInfo::InvokeMember. The other was that the ExInfo popping was still needed and that lead to the test hang (in an interesting way - the next ExInfo that was pushed by the next portion of the test had accidentally the same address, so the presence of previous stale ExInfo link at the same address created an infinite loop in the m_pPrevNestedInfo chain.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@janvorli@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix exception handling in the prestub worker - take 2 - #112666

Merged
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3
Feb 20, 2025
Merged

Fix exception handling in the prestub worker - take 2#112666
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3

Conversation

@janvorli

Copy link
Copy Markdown
Member

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.

The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.

The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.

On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.
The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.
The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.
On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.
I was hitting intermittent crashes in the unhandled exception test
with GC stress C enabled due to this when GetSSP tried to access the
SSP in the extended part of the context.
@janvorlijanvorli added the area-ExceptionHandling-coreclr only use for closed issues label Feb 18, 2025
@janvorli
janvorli requested a review from jkotasFebruary 18, 2025 22:09
@janvorlijanvorli self-assigned this Feb 18, 2025
CopilotAI review requested due to automatic review settings February 18, 2025 22:09

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (10)
  • src/coreclr/vm/excep.cpp: Language not supported
  • src/coreclr/vm/exceptionhandling.cpp: Language not supported
  • src/coreclr/vm/exceptmacros.h: Language not supported
  • src/coreclr/vm/prestub.cpp: Language not supported
  • src/coreclr/vm/virtualcallstub.cpp: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj: Language not supported
  • src/tests/issues.targets: Language not supported
Comments suppressed due to low confidence (1)

src/tests/Regressions/coreclr/GitHub_76531/test76531.cs:25

  • [nitpick] The error message 'My exception' is unclear. Consider providing a more descriptive message.
=> throw new Exception("My exception");

Comment threadsrc/tests/Regressions/coreclr/GitHub_76531/test76531.cs
if (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc) ||
pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
{
if ((pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jkotas this is the only change w.r.t. the previous PR that was reverted recently. This popping was causing the GC to get lost.

@jkotas

jkotas commented Feb 19, 2025

Copy link
Copy Markdown
Member

The test failure looks related:

Unhandled exception. System.OverflowException: Arithmetic operation resulted in an overflow.
at Microsoft.Win32.OAVariantLib.OAFailed(Int32 hr)
at Microsoft.Win32.OAVariantLib.ChangeType(Object source, Type targetClass, Int16 options, CultureInfo culture)
at System.OleAutBinder.ChangeType(Object value, Type type, CultureInfo cultureInfo)

* The previous set of changes removed popping of ExInfos too. That's not
correct though. But it should be done in a different place than the
ProcessCLRExceptionNew.
* There was a problem (even before the fix) that an exception caught in
DispatchInfo::InvokeMember was reported (via console log and to the
debugger) as unhandled.
This change also adds a new flavor of the unhandled exception test that
throws an unhandled exception on a secondary thread to exercise the
related code path.
@janvorli

Copy link
Copy Markdown
MemberAuthor

@jkotas I have fixed the test failure. There were actually two issues. One was that the exception was reported as unhandled even though it was actually then handled in native runtime code in the DispatchInfo::InvokeMember. The other was that the ExInfo popping was still needed and that lead to the test hang (in an interesting way - the next ExInfo that was pushed by the next portion of the test had accidentally the same address, so the presence of previous stale ExInfo link at the same address created an infinite loop in the m_pPrevNestedInfo chain.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@janvorli@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Fix exception handling in the prestub worker - take 2 - #112666

Merged
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3
Feb 20, 2025
Merged

Fix exception handling in the prestub worker - take 2#112666
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3

Conversation

@janvorli

Copy link
Copy Markdown
Member

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.

The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.

The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.

On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.
The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.
The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.
On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.
I was hitting intermittent crashes in the unhandled exception test
with GC stress C enabled due to this when GetSSP tried to access the
SSP in the extended part of the context.
@janvorlijanvorli added the area-ExceptionHandling-coreclr only use for closed issues label Feb 18, 2025
@janvorli
janvorli requested a review from jkotasFebruary 18, 2025 22:09
@janvorlijanvorli self-assigned this Feb 18, 2025
CopilotAI review requested due to automatic review settings February 18, 2025 22:09

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (10)
  • src/coreclr/vm/excep.cpp: Language not supported
  • src/coreclr/vm/exceptionhandling.cpp: Language not supported
  • src/coreclr/vm/exceptmacros.h: Language not supported
  • src/coreclr/vm/prestub.cpp: Language not supported
  • src/coreclr/vm/virtualcallstub.cpp: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj: Language not supported
  • src/tests/issues.targets: Language not supported
Comments suppressed due to low confidence (1)

src/tests/Regressions/coreclr/GitHub_76531/test76531.cs:25

  • [nitpick] The error message 'My exception' is unclear. Consider providing a more descriptive message.
=> throw new Exception("My exception");

Comment threadsrc/tests/Regressions/coreclr/GitHub_76531/test76531.cs
if (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc) ||
pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
{
if ((pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jkotas this is the only change w.r.t. the previous PR that was reverted recently. This popping was causing the GC to get lost.

@jkotas

jkotas commented Feb 19, 2025

Copy link
Copy Markdown
Member

The test failure looks related:

Unhandled exception. System.OverflowException: Arithmetic operation resulted in an overflow.
at Microsoft.Win32.OAVariantLib.OAFailed(Int32 hr)
at Microsoft.Win32.OAVariantLib.ChangeType(Object source, Type targetClass, Int16 options, CultureInfo culture)
at System.OleAutBinder.ChangeType(Object value, Type type, CultureInfo cultureInfo)

* The previous set of changes removed popping of ExInfos too. That's not
correct though. But it should be done in a different place than the
ProcessCLRExceptionNew.
* There was a problem (even before the fix) that an exception caught in
DispatchInfo::InvokeMember was reported (via console log and to the
debugger) as unhandled.
This change also adds a new flavor of the unhandled exception test that
throws an unhandled exception on a secondary thread to exercise the
related code path.
@janvorli

Copy link
Copy Markdown
MemberAuthor

@jkotas I have fixed the test failure. There were actually two issues. One was that the exception was reported as unhandled even though it was actually then handled in native runtime code in the DispatchInfo::InvokeMember. The other was that the ExInfo popping was still needed and that lead to the test hang (in an interesting way - the next ExInfo that was pushed by the next portion of the test had accidentally the same address, so the presence of previous stale ExInfo link at the same address created an infinite loop in the m_pPrevNestedInfo chain.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@janvorli@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix exception handling in the prestub worker - take 2 - #112666

Merged
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3
Feb 20, 2025
Merged

Fix exception handling in the prestub worker - take 2#112666
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3

Conversation

@janvorli

Copy link
Copy Markdown
Member

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.

The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.

The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.

On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.
The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.
The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.
On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.
I was hitting intermittent crashes in the unhandled exception test
with GC stress C enabled due to this when GetSSP tried to access the
SSP in the extended part of the context.
@janvorlijanvorli added the area-ExceptionHandling-coreclr only use for closed issues label Feb 18, 2025
@janvorli
janvorli requested a review from jkotasFebruary 18, 2025 22:09
@janvorlijanvorli self-assigned this Feb 18, 2025
CopilotAI review requested due to automatic review settings February 18, 2025 22:09

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (10)
  • src/coreclr/vm/excep.cpp: Language not supported
  • src/coreclr/vm/exceptionhandling.cpp: Language not supported
  • src/coreclr/vm/exceptmacros.h: Language not supported
  • src/coreclr/vm/prestub.cpp: Language not supported
  • src/coreclr/vm/virtualcallstub.cpp: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj: Language not supported
  • src/tests/issues.targets: Language not supported
Comments suppressed due to low confidence (1)

src/tests/Regressions/coreclr/GitHub_76531/test76531.cs:25

  • [nitpick] The error message 'My exception' is unclear. Consider providing a more descriptive message.
=> throw new Exception("My exception");

Comment threadsrc/tests/Regressions/coreclr/GitHub_76531/test76531.cs
if (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc) ||
pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
{
if ((pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jkotas this is the only change w.r.t. the previous PR that was reverted recently. This popping was causing the GC to get lost.

@jkotas

jkotas commented Feb 19, 2025

Copy link
Copy Markdown
Member

The test failure looks related:

Unhandled exception. System.OverflowException: Arithmetic operation resulted in an overflow.
at Microsoft.Win32.OAVariantLib.OAFailed(Int32 hr)
at Microsoft.Win32.OAVariantLib.ChangeType(Object source, Type targetClass, Int16 options, CultureInfo culture)
at System.OleAutBinder.ChangeType(Object value, Type type, CultureInfo cultureInfo)

* The previous set of changes removed popping of ExInfos too. That's not
correct though. But it should be done in a different place than the
ProcessCLRExceptionNew.
* There was a problem (even before the fix) that an exception caught in
DispatchInfo::InvokeMember was reported (via console log and to the
debugger) as unhandled.
This change also adds a new flavor of the unhandled exception test that
throws an unhandled exception on a secondary thread to exercise the
related code path.
@janvorli

Copy link
Copy Markdown
MemberAuthor

@jkotas I have fixed the test failure. There were actually two issues. One was that the exception was reported as unhandled even though it was actually then handled in native runtime code in the DispatchInfo::InvokeMember. The other was that the ExInfo popping was still needed and that lead to the test hang (in an interesting way - the next ExInfo that was pushed by the next portion of the test had accidentally the same address, so the presence of previous stale ExInfo link at the same address created an infinite loop in the m_pPrevNestedInfo chain.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@janvorli@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Fix exception handling in the prestub worker - take 2 - #112666

Merged
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3
Feb 20, 2025
Merged

Fix exception handling in the prestub worker - take 2#112666
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3

Conversation

@janvorli

Copy link
Copy Markdown
Member

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.

The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.

The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.

On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.
The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.
The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.
On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.
I was hitting intermittent crashes in the unhandled exception test
with GC stress C enabled due to this when GetSSP tried to access the
SSP in the extended part of the context.
@janvorlijanvorli added the area-ExceptionHandling-coreclr only use for closed issues label Feb 18, 2025
@janvorli
janvorli requested a review from jkotasFebruary 18, 2025 22:09
@janvorlijanvorli self-assigned this Feb 18, 2025
CopilotAI review requested due to automatic review settings February 18, 2025 22:09

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (10)
  • src/coreclr/vm/excep.cpp: Language not supported
  • src/coreclr/vm/exceptionhandling.cpp: Language not supported
  • src/coreclr/vm/exceptmacros.h: Language not supported
  • src/coreclr/vm/prestub.cpp: Language not supported
  • src/coreclr/vm/virtualcallstub.cpp: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj: Language not supported
  • src/tests/issues.targets: Language not supported
Comments suppressed due to low confidence (1)

src/tests/Regressions/coreclr/GitHub_76531/test76531.cs:25

  • [nitpick] The error message 'My exception' is unclear. Consider providing a more descriptive message.
=> throw new Exception("My exception");

Comment threadsrc/tests/Regressions/coreclr/GitHub_76531/test76531.cs
if (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc) ||
pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
{
if ((pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jkotas this is the only change w.r.t. the previous PR that was reverted recently. This popping was causing the GC to get lost.

@jkotas

jkotas commented Feb 19, 2025

Copy link
Copy Markdown
Member

The test failure looks related:

Unhandled exception. System.OverflowException: Arithmetic operation resulted in an overflow.
at Microsoft.Win32.OAVariantLib.OAFailed(Int32 hr)
at Microsoft.Win32.OAVariantLib.ChangeType(Object source, Type targetClass, Int16 options, CultureInfo culture)
at System.OleAutBinder.ChangeType(Object value, Type type, CultureInfo cultureInfo)

* The previous set of changes removed popping of ExInfos too. That's not
correct though. But it should be done in a different place than the
ProcessCLRExceptionNew.
* There was a problem (even before the fix) that an exception caught in
DispatchInfo::InvokeMember was reported (via console log and to the
debugger) as unhandled.
This change also adds a new flavor of the unhandled exception test that
throws an unhandled exception on a secondary thread to exercise the
related code path.
@janvorli

Copy link
Copy Markdown
MemberAuthor

@jkotas I have fixed the test failure. There were actually two issues. One was that the exception was reported as unhandled even though it was actually then handled in native runtime code in the DispatchInfo::InvokeMember. The other was that the ExInfo popping was still needed and that lead to the test hang (in an interesting way - the next ExInfo that was pushed by the next portion of the test had accidentally the same address, so the presence of previous stale ExInfo link at the same address created an infinite loop in the m_pPrevNestedInfo chain.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@janvorli@jkotas
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Fix exception handling in the prestub worker - take 2 - #112666

Merged
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3
Feb 20, 2025
Merged

Fix exception handling in the prestub worker - take 2#112666
janvorli merged 5 commits into
dotnet:mainfrom
janvorli:fix-exception-from-class-init-hang-3

Conversation

@janvorli

Copy link
Copy Markdown
Member

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.

The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.

The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.

On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.

There is a bug in the new exception handling when ThePreStub is called
from CallDescrWorkerInternal and the exception is propagated through
that. One of such cases is when a managed class is being initialized
during JITting and the constructor is in an assembly that's not found.
The bug results in skipping all the native frames upto a managed frame
that called that native chain that lead to the exception. In the
specific case I've mentioned, a lock in the native code is left in
locked state. That later leads to a hang. This was case was observed
with Roslyn invoking an analyzer where one of the dependencies was
missing.
The fix is to ensure that when ThePreStub is called by
CallDescrWorkerInternal, the exception is not caught in the
PreStubWorker. It is left flowing into the native calling code instead.
The same treatment is applied to ExternalMethodFixupWorker and
VSD_ResolveWorker too.
On Windows, we also need to prevent the ProcessCLRException invocation
to call into the managed exception handling code.
I was hitting intermittent crashes in the unhandled exception test
with GC stress C enabled due to this when GetSSP tried to access the
SSP in the extended part of the context.
@janvorlijanvorli added the area-ExceptionHandling-coreclr only use for closed issues label Feb 18, 2025
@janvorli
janvorli requested a review from jkotasFebruary 18, 2025 22:09
@janvorlijanvorli self-assigned this Feb 18, 2025
CopilotAI review requested due to automatic review settings February 18, 2025 22:09

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 12 changed files in this pull request and generated 1 comment.

Files not reviewed (10)
  • src/coreclr/vm/excep.cpp: Language not supported
  • src/coreclr/vm/exceptionhandling.cpp: Language not supported
  • src/coreclr/vm/exceptmacros.h: Language not supported
  • src/coreclr/vm/prestub.cpp: Language not supported
  • src/coreclr/vm/virtualcallstub.cpp: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/dependencytodelete.csproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.il: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/tailcallinvoker.ilproj: Language not supported
  • src/tests/Regressions/coreclr/GitHub_76531/test76531.csproj: Language not supported
  • src/tests/issues.targets: Language not supported
Comments suppressed due to low confidence (1)

src/tests/Regressions/coreclr/GitHub_76531/test76531.cs:25

  • [nitpick] The error message 'My exception' is unclear. Consider providing a more descriptive message.
=> throw new Exception("My exception");

Comment threadsrc/tests/Regressions/coreclr/GitHub_76531/test76531.cs
if (!ExecutionManager::IsManagedCode((PCODE)pDispatcherContext->ControlPc) ||
pThread->HasThreadStateNC(Thread::TSNC_ProcessedUnhandledException))
{
if ((pExceptionRecord->ExceptionFlags & EXCEPTION_UNWINDING))

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@jkotas this is the only change w.r.t. the previous PR that was reverted recently. This popping was causing the GC to get lost.

@jkotas

jkotas commented Feb 19, 2025

Copy link
Copy Markdown
Member

The test failure looks related:

Unhandled exception. System.OverflowException: Arithmetic operation resulted in an overflow.
at Microsoft.Win32.OAVariantLib.OAFailed(Int32 hr)
at Microsoft.Win32.OAVariantLib.ChangeType(Object source, Type targetClass, Int16 options, CultureInfo culture)
at System.OleAutBinder.ChangeType(Object value, Type type, CultureInfo cultureInfo)

* The previous set of changes removed popping of ExInfos too. That's not
correct though. But it should be done in a different place than the
ProcessCLRExceptionNew.
* There was a problem (even before the fix) that an exception caught in
DispatchInfo::InvokeMember was reported (via console log and to the
debugger) as unhandled.
This change also adds a new flavor of the unhandled exception test that
throws an unhandled exception on a secondary thread to exercise the
related code path.
@janvorli

Copy link
Copy Markdown
MemberAuthor

@jkotas I have fixed the test failure. There were actually two issues. One was that the exception was reported as unhandled even though it was actually then handled in native runtime code in the DispatchInfo::InvokeMember. The other was that the ExInfo popping was still needed and that lead to the test hang (in an interesting way - the next ExInfo that was pushed by the next portion of the test had accidentally the same address, so the presence of previous stale ExInfo link at the same address created an infinite loop in the m_pPrevNestedInfo chain.

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-ExceptionHandling-coreclronly use for closed issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@janvorli@jkotas