{"id":106700,"date":"2022-06-01T07:00:00","date_gmt":"2022-06-01T14:00:00","guid":{"rendered":"https:\/\/devblogs.microsoft.com\/oldnewthing\/?p=106700"},"modified":"2022-06-01T10:01:33","modified_gmt":"2022-06-01T17:01:33","slug":"20220601-00","status":"publish","type":"post","link":"https:\/\/devblogs.microsoft.com\/oldnewthing\/20220601-00\/?p=106700","title":{"rendered":"The case of the COM reference that suddenly went bad in the middle of a coroutine"},"content":{"rendered":"

A customer reported a crash in a coroutine and needed help understanding it.<\/p>\n

Here’s the crash:<\/p>\n

contoso!winrt::impl::consume_Contoso_IWidgetOptions<winrt::Contoso::IWidgetOptions>::Name+0x15:\r\n00007ff8`fa6c77e1 mov     rax,qword ptr [rcx] ds:00000000`00000000=????????????????\r\n<\/pre>\n
And the stack trace:<\/p>\n
contoso!winrt::impl::consume_Contoso_IWidgetOptions<winrt::Contoso::IWidgetOptions>::Name+0x15\r\ncontoso!winrt::Contoso::implementation::Widget::CreateAsync$_ResumeCoro$1+0xfe\r\ncontoso!winrt::Contoso::implementation::Widget::CreateAsync$_InitCoro$2+0x95\r\ncontoso!winrt::Contoso::implementation::Widget::CreateAsync+0x63\r\ncontoso!winrt::Contoso::implementation::Gadget::CreateWidgetAsync+0x7e\r\ncontoso!winrt::impl::produce<winrt::Contoso::implementation::Gadget,winrt::Contoso::IGadget>::CreateWidgetAsync+0x35\r\nlitware!winrt::impl::consume_Contoso_Gadget<winrt::Contoso::Gadget>::CreateWidgetAsync+0x47\r\nlitware!winrt::LitWare::implementation::GadgetViewer::CreateWidgetsAsync$_ResumeCoro$2+0x343\r\nlitware!std::experimental::coroutine_handle<void>::resume+0xc\r\nlitware!std::experimental::coroutine_handle<void>::operator()+0xc\r\nlitware!winrt::impl::resume_background_callback+0x10\r\nntdll!TppSimplepExecuteCallback+0xa3\r\nntdll!TppWorkerThread+0x686\r\nkernel32!BaseThreadInitThunk+0x10\r\nntdll!RtlUserThreadStart+0x2b\r\n<\/pre>\n
This is crashing inside the C++\/WinRT projection, which you can infer because we crashed inside a consume_<\/code> function. The consume_<\/code> functions are provided by the C++\/WinRT library to convert your C++\/WinRT calls into low-level ABI calls. So something happened inside that projection.<\/p>\n
Here’s the consume function up to the point of the crash:<\/p>\n
    push    rbx\r\n    sub     rsp,20h\r\n    mov     rcx,qword ptr [rcx]   ; get the raw pointer from the IWidgetOptions\r\n    and     qword ptr [rsp+30h],0 ; pre-null the output parameter\r\n    mov     rbx,rdx\r\n    mov     rax,qword ptr [rcx]   ; Load the vtable\r\n<\/pre>\n
The problem it seems is that the this<\/code> parameter to IWidgetOptions::Name()<\/code> is a COM wrapper around a null pointer.<\/p>\n
We are called from _InitCoro<\/code>, which runs the initial synchronous portion, so we are still in the synchronous portion of the coroutine. That’s nice, because it means that the caller is still on the stack:<\/p>\n
IAsyncOperation<Widget> Widget::CreateAsync(const WidgetOptions& options)\r\n{\r\n    auto name = options.Name();\r\n<\/pre>\n
Let’s see what we got as the options<\/code>:<\/p>\n
0:018> .frame 3\r\n03 000000bb`dccff630 00007ff8`fa6c7b32     contoso!Widget::CreateAsync+0x63\r\n0:018> dv\r\n     options = 0x000000bb`dccff978\r\n0:018> ?? options\r\nstruct winrt::Contoso::IWidgetOptions * 0x000000bb`dccff978\r\n   +0x000 m_ptr            : (null)\r\n<\/pre>\n
Yes indeed, the options<\/code> is null. That’s why we crash trying to call a method on it.<\/p>\n
The caller of Widget::Create\u00adAsync<\/code> is Gadget::Create\u00adWidget\u00adAsync<\/code>:<\/p>\n
IAsyncOperation<Widget> Gadget::CreateWidgetAsync()\r\n{\r\n    return Widget::CreateAsync(m_options);\r\n}\r\n<\/pre>\n
The customer suspected that this function was incorrectly implemented. Shouldn’t it be this?<\/p>\n
IAsyncOperation<Widget> Gadget::CreateWidgetAsync()\r\n{\r\n    co_return co_await Widget::CreateAsync(m_options);\r\n}\r\n<\/pre>\n
“The immediate return might be losing some necessary coroutine frame lifetime, so that when GadgetViewer::Create\u00adWidgets\u00adAsync<\/code> completes, it’s operating on already-freed memory. However, I’m not confident in this analysis.”<\/p>\n
The function is fine. While it’s true that the common way of producing a IAsync\u00adOperation<Widget><\/code> is to autogenerate one from a coroutine, it is also perfectly legal to just create one by other means and just return it.<\/p>\n
Let’s try to walk back up the stack to find out what the m_options<\/code> were that we thought<\/i> we were passing in.<\/p>\n
0:018> .frame 4\r\n04 000000bb`dccff930 00007ff8`fa6c7a55 contoso!winrt::Contoso::implementation::Gadget::CreateWidgetAsync+0x7e\r\n0:018> dv\r\n           this = <value unavailable>\r\n<\/pre>\n
Rats, the this<\/code> pointer got optimized out. Keep going.<\/p>\n
0:018> .frame 5\r\n05 000000bb`dccff970 00007ff8`be19b2ff contoso!winrt::impl::produce<winrt::Contoso::implementation::Gadget,winrt::Contoso::IGadget>::CreateWidgetAsync+0x35\r\n0:018> dv\r\n           this = <value unavailable>\r\n      operation = 0x000000bb`dccff9d0\r\n\r\n0:018> .frame 6\r\n06 000000bb`dccff9a0 00007ff8`be109963     litware!winrt::impl::consume_Contoso_Gadget<winrt::Contoso::Gadget>::CreateWidgetAsync+0x47\r\n0:018> dv\r\n           this = <value unavailable>\r\n      operation = 0x00000000`00000000\r\n\r\n0:018> .frame 7\r\n07 000000bb`dccff9f0 00007ff8`be0d4b40     litware!winrt::LitWare::implementation::GadgetViewer::CreateWidgetsAsync$_ResumeCoro$2+0x343\r\n0:018> dv\r\n      <coro_frame_ptr> = 0x00000276`15223b00\r\n            strongThis = struct winrt::com_ptr<winrt::LitWare::implementation::GadgetViewer>\r\n<\/pre>\n
We had to chase all the way back to GadgetViewer::Create\u00adWidgets\u00adAsync<\/code> before we got a foothold into the thing that will lead us to the options<\/code>. Now we can start working our way back in.<\/p>\n
The GadgetViewer::Create\u00adWidgets\u00adAsync<\/code> coroutine looks like this:<\/p>\n
IAsyncOperation<IVectorView<MenuItem>> GadgetViewer::CreateWidgetsAsync()\r\n{\r\n    ...\r\n\r\n    const auto strongThis{ get_strong() };\r\n\r\n    co_await winrt::resume_background();\r\n\r\n    std::vector<winrt::Contoso::Widget> widgets;\r\n\r\n    if (const auto widget1 = co_await m_gadget.CreateWidgetAsync())\r\n    {\r\n        ...\r\n<\/pre>\n
Aha, so the outbound call to Create\u00adWidget\u00adAsync<\/code> is made on the GadgetViewer<\/code>‘s m_gadget<\/code>. Follow the call:<\/p>\n
0:018> ?? strongThis\r\nstruct winrt::com_ptr<winrt::Contoso::implementation::GadgetViewer>\r\n   +0x000 m_ptr            : 0x00000276`184121f0 winrt::Contoso::implementation::GadgetViewer\r\n0:018> ??((winrt::Contoso::implementation::GadgetViewer*) 0x00000276`184121f0)->m_gadget\r\nstruct winrt::Contoso::Gadget\r\n   +0x000 m_ptr            : 0x00000276`0f68d4e0 winrt::impl::abi<winrt::Windows::Foundation::IUnknown,void>::type\r\n<\/pre>\n
Okay, we’ve found the m_gadget<\/code>. Now to the options.<\/p>\n
0:018> dt contoso!winrt::Contoso::implementation::Gadget\r\n   +0x010 vtable           : winrt::impl::produce<winrt::Contoso::implementation::Gadget,winrt::Contoso::IGadget>\r\n   +0x000 __VFN_table : Ptr64\r\n   +0x008 m_references     : std::atomic<unsigned __int64>\r\n   +0x018 m_options        : winrt::Contoso::WidgetOptions\r\n   +0x020 m_source         : winrt::Contoso::GadgetSource\r\n   +0x028 m_home           : std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> >\r\n<\/pre>\n
We see that the vtable for the producer of IGadget<\/code> is at offset 0x010<\/code>, so we subtract that amount from our winrt::Contoso::Gadget<\/code> pointer to get a pointer to the implementation.<\/p>\n
0:018> ?? ((contoso!winrt::Contoso::implementation::Gadget*)(0x00000276`0f68d4e0-0x10))\r\nstruct winrt::Contoso::implementation::Gadget * 0x00000276`0f68d4d0\r\n   +0x010 vtable           : winrt::impl::produce<winrt::Contoso::implementation::Gadget,winrt::Contoso::IGadget>\r\n   +0x000 __VFN_table : 0x00007ff8`fa6df2e8\r\n   +0x008 m_references     : std::atomic<unsigned __int64>\r\n   +0x018 m_options        : winrt::Contoso::WidgetOptions\r\n   +0x020 m_source         : winrt::Contoso::GadgetSource\r\n   +0x028 m_home           : std::basic_string<wchar_t,std::char_traits<wchar_t>,std::allocator<wchar_t> >\r\n\r\n0:018> ?? &((contoso!winrt::Contoso::implementation::Gadget*)(0x00000276`0f68d4e0-0x10))->m_options\r\nstruct winrt::Contoso::WidgetOptions * 0x00000276`0f68d4e8\r\n   +0x000 m_ptr            : 0x00000276`152f6270 winrt::impl::abi<winrt::Windows::Foundation::IUnknown,void>::type\r\n<\/pre>\n
The address of this m_options<\/code> is 0x00000276`0f68d4e8<\/code>, which is not the same as the address that was passed to Widget::Create\u00adAsync<\/code>:<\/p>\n
0:018> .frame 3\r\n03 000000bb`dccff630 00007ff8`fa6c7b32     contoso!Widget::CreateAsync+0x63\r\n0:018> dv\r\n     options = 0x000000bb`dccff978\r\n<\/pre>\n
What happened? How did the address of a variable change?<\/p>\n
Studying the code in the Gadget\u00adViewer<\/code> that uses the m_gadget<\/code> member variable, we see that the member variable is used from both the foreground thread as well as from a background thread:<\/p>\n
\/\/ Property setter: Set the \"Gadget\" property of the GadgetViewer\r\nvoid GadgetViewer::Gadget(winrt::Contoso::Gadget const& value)\r\n{\r\n    VerifyUIThread();\r\n\r\n    if (m_gadget != value)\r\n    {\r\n        m_gadget = value;\r\n        ...\r\n    }\r\n}\r\n<\/pre>\n
Recall that one of the principles of debugging somebody else’s code is to  assume that the code is mostly correct<\/a>. The problem is likely in a small detail, an edge case, or a peculiar combination of factors. After all, if the problem was in a common case, they probably wouldn’t have had to ask an outsider for help.<\/p>\n
The Verify\u00adUI\u00adThread<\/code> call tells us that the expectation is that the gadget is changed only from the UI thread. But there is no synchronization to protect access to this variable from multiple threads, even though we are accessing it from a background thread:<\/p>\n
IAsyncOperation<IVectorView<MenuItem>> GadgetViewer::CreateWidgetsAsync()\r\n{\r\n    ...\r\n\r\n    const auto strongThis{ get_strong() };\r\n\r\n    co_await winrt::resume_background(); \/\/ \u2190 hop to background thread<\/span>\r\n\r\n    std::vector<winrt::Contoso::Widget> widgets;\r\n\r\n    if (const auto widget1 = co_await m_gadget.CreateWidgetAsync())\r\n    {\r\n        ...\r\n<\/pre>\n
What may have happened is that while Create\u00adWidgets\u00adAsync<\/code> was using m_gadget<\/code> on the background thread, the foreground thread changed the m_gadget<\/code>, causing the old gadget (and its m_options<\/code>) to be destructed while the background thread was still using it.<\/p>\n
The customer provided some history: As originally written, the Gadget\u00adViewer<\/code> accessed the m_gadget<\/code> only from the foreground thread, but a subsequent change moved some of the work to a background thread, and that introduced concurrency into code that was written on the assumption that there was no concurrency.<\/p>\n
One possible solution is for Gadget\u00adViewer::Create\u00adWidgets\u00adAsync<\/code> to capture the member variables it intends to use (the m_gadget<\/code>, in this case, but possibly other member variables not seen here) before going to the background thread, and operating entirely on the captured variables. It means that when you call Create\u00adWidgets\u00adAsync<\/code>, you get the widgets associated with the gadget that you were viewing at the point you called Create\u00adWidgets\u00adAsync<\/code>, even if you changed the gadget while the Create\u00adWidgets\u00adAsync<\/code> was still working.<\/p>\n","protected":false},"excerpt":{"rendered":"
Is the coroutine really to blame?<\/p>\n","protected":false},"author":1069,"featured_media":111744,"comment_status":"open","ping_status":"closed","sticky":false,"template":"","format":"standard","meta":{"_acf_changed":false,"footnotes":""},"categories":[1],"tags":[25],"class_list":["post-106700","post","type-post","status-publish","format-standard","has-post-thumbnail","hentry","category-oldnewthing","tag-code"],"acf":[],"blog_post_summary":"
Is the coroutine really to blame?<\/p>\n","_links":{"self":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/106700","targetHints":{"allow":["GET"]}}],"collection":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts"}],"about":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/types\/post"}],"author":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/users\/1069"}],"replies":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/comments?post=106700"}],"version-history":[{"count":0,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/posts\/106700\/revisions"}],"wp:featuredmedia":[{"embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media\/111744"}],"wp:attachment":[{"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/media?parent=106700"}],"wp:term":[{"taxonomy":"category","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/categories?post=106700"},{"taxonomy":"post_tag","embeddable":true,"href":"https:\/\/devblogs.microsoft.com\/oldnewthing\/wp-json\/wp\/v2\/tags?post=106700"}],"curies":[{"name":"wp","href":"https:\/\/api.w.org\/{rel}","templated":true}]}}