Skip to content
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

Bug: winrt::hresult_error::originate asserts when using a clr.dll based error info object #1380

Closed
ChrisGuzak opened this issue Dec 21, 2023 · 5 comments · Fixed by #1386
Closed

Comments

@ChrisGuzak
Copy link
Member

Version

C++/WinRT v2.0.220418.1

Summary

In winrt::hresult_error::originate, running in the context of a Microsoft::VisualStudio::CppUnitTestFramework project, where the error info object is provided by clr.dll and it does not implement IRestrictedErrorInfo. the WINRT_VERIFY(info.try_as(m_info)) line fails the assert.

base.h, ~4892

            com_ptr<impl::IErrorInfo> info;
            WINRT_VERIFY_(0, WINRT_IMPL_GetErrorInfo(0, info.put_void()));
            WINRT_VERIFY(info.try_as(m_info));

Is this a bug in the CLR or is the design intended to support this?

Reproducible example

TEST_METHOD(CatchCppWinRtException)
{
    try
    {
        winrt::check_hresult(E_NOT_SET);
    }
    catch (...)
    {
    }
}

Expected behavior

no assert, but I'm not sure if this is bug in the CLR or cppwinrt

Actual behavior

assert, in debug build

Additional comments

No response

@jonwis
Copy link
Member

jonwis commented Dec 22, 2023

Looks like the hresult_error case handles this properly:

WINRT_IMPL_GetErrorInfo(0, info.put_void());
if ((m_info = info.try_as<impl::IRestrictedErrorInfo>()))
{
WINRT_VERIFY_(0, m_info->GetReference(m_debug_reference.put()));

While the originate path does not:

com_ptr<impl::IErrorInfo> info;
WINRT_VERIFY_(0, WINRT_IMPL_GetErrorInfo(0, info.put_void()));
WINRT_VERIFY(info.try_as(m_info));
}

And worse, the originate path is triggered in the "the current info does not implement IRestrictedErrorInfo" case of the hresult_error ctor.

I'd expect that this is a bad assumption on C++/WinRT's part - there's certainly runtimes that will SetErrorInfo(pCustomErrorInfo) that does not implement IRestrictedErrorInfo so we should harden the code with that in mind.

@kennykerr
Copy link
Collaborator

The latter is preceded by a call to RoOriginateLanguageException so that assertion should always hold.

@jonwis
Copy link
Member

jonwis commented Dec 22, 2023

RoOriginateErrorInformation's documentation doesn't seem to say that it resets the error info. The implementation respects RO_ERROR_REPORTING_SUPPRESSSETERRORINFO and RO_ERROR_REPORTING_USESETERRORINFO flags to RoSetErrorReportingFlags ... the logic for when RoOriginate... calls SetErrorInfo appears to be like this:

if (FlagNotSet(RoErrorFlags, RO_ERROR_REPORTING_SUPPRESSSETERRORINFO))
{
    if (FlagSet(RoErrorFlags, RO_ERROR_REPORTING_USESETERRORINFO) || IsDebuggerPresent())
    {
        SetErrorInfo(...);
    }
}

There's other paths through RoOriginateLanguageException that would cause the call to SetErrorInfo to be skipped. I'm inclined to fix up this path against "could not get the extended error info" as that's not really a developer problem as much as an environmental problem.

@ChrisGuzak can you grab a time-travel trace of the test you've got so we can see what's happening?

@kennykerr
Copy link
Collaborator

Adding @manodasanW as I think he originally owned/wrote the origination APIs.

Copy link

github-actions bot commented Jan 6, 2024

This issue is stale because it has been open 10 days with no activity. Remove stale label or comment or this will be closed in 5 days.

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 a pull request may close this issue.

9 participants