microsoft / cppwinrt

C++/WinRT
MIT License
1.61k stars 232 forks source link

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

Closed ChrisGuzak closed 6 months ago

ChrisGuzak commented 6 months ago

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 commented 6 months ago

Looks like the hresult_error case handles this properly:

https://github.com/microsoft/cppwinrt/blob/2511bf7fcb54a64c1e596a950788e2ee30cd1e4c/strings/base_error.h#L217-L221

While the originate path does not:

https://github.com/microsoft/cppwinrt/blob/2511bf7fcb54a64c1e596a950788e2ee30cd1e4c/strings/base_error.h#L315-L318

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 commented 6 months ago

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

jonwis commented 6 months ago

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 commented 6 months ago

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

github-actions[bot] commented 6 months ago

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.