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

Make CoreWebView2 handlers robust around WebView2 destruction #8969

Merged
merged 6 commits into from
Oct 20, 2023

Conversation

DmitriyKomin
Copy link
Contributor

In WebView2, we have a race condition when subscribed events on CoreWebView2/CoreWebView2Controller end up getting raised around the time WebView2 is destructed. The events are raised on the UI thread, but are asynchronous and involve MOJO/IPC, so it's possible in the meantime for WebView2 to be destructed leading to a dangling reference in the handlers. This has surfaced recently with a spike of Watson hits in microsoftmahjong crashing via Microsoft.UI.Xaml.dll!ITrackerHandleManager::DeleteTrackerHandle with HR 0xc0000409 (nominally STATUS_STACK_BUFFER_OVERRUN, but in actuality FAIL_FAST_FATAL_APP_EXIT as logged by Watson, more on that below). The fix is to protect access to WebView2 instance inside CWV2 event handler with a weak ref.

In a representative dump, we see Xaml getting notified of an incoming web message when the owning WebView2 object has already been destructed. We are able to dance around the released WebView2 object's memory during much of WebView2::FireWebMessageReceived, but finally crash at the end of the app's WV2.WebMessageReceived invocation, as we try to invoke WebView2->DeleteTrackerHandle to clean up the temporary tracker_ref<TypedEventHandler<…>> 'handler' variable as shown below:

template <typename... A> void operator()(A const & ... args) const
{
    auto handlers = m_handlers;

   if (auto * map = handlers.get())
   {
        for (const auto & pair : *map)
        {
            auto handler = Impl()->unwrap(pair.second);
            handler(args...);  
        } <--- uh-oh, handler going out of scope, watch out for ref_tracker dtor...
    }
}

At the end of the for() loop scope, handler (tracker_ref<TypedEventHandler<…>>) goes out of scope, and tracker_ref's cleanup invokes ITrackerOwner::DeleteTrackerHandler on the owning WebView2 object. It is this access to (released) WV2 via ITrackerOwner that triggers app termination:

~tracker_ref()
    {
        if (m_owner)
        {
            if (!ShouldFallbackToComPointers())
            {
                if (m_handle)
                {
                    MUX_ASSERT(m_owner);
                    m_owner->DeleteTrackerHandle(m_handle);  <--- crash right about here!
                }
            }
…
}

The invocation of ITrackerOwner::DeleteTrackerHandle on the released WV2 leads to immediate termination via CRT abort(), per frame 0 of stack:
[0x0] : ucrtbase!abort + 0x4e [Switch To]

Thanks to insight from Raymond Chen's blog post, 0xc0000409/STATUS_STACK_BUFFER_OVERRUN doesn't necessarily mean there was a stack buffer overrun, in fact most often it doesn't... in this case it maps to FAIL_FAST_FATAL_APP_EXIT, which is indeed associated with abort() -triggered termination.

In addition to addressing this for the identified WebMessageRecieved handler, apply the fix to all of WebView2's CoreWebView2 subscriptions.

@microsoft-github-policy-service microsoft-github-policy-service bot added the needs-triage Issue needs to be triaged by the area owners label Oct 12, 2023
@DmitriyKomin
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DmitriyKomin
Copy link
Contributor Author

    MUX_ASSERT(inputWindowHwnd != m_xamlHostHwnd); // Focused XAML host window cannot be set as input hwnd

/azp run


Refers to: dev/WebView2/WebView2.cpp:515 in 8c74e30. [](commit_id = 8c74e30, deletion_comment = False)

@DmitriyKomin
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DmitriyKomin
Copy link
Contributor Author

DmitriyKomin commented Oct 18, 2023

@ranjeshj, @codendone, @kmahone - still looking for review on this, can one of you take a look? #Closed

Copy link
Contributor

@codendone codendone left a comment

Choose a reason for hiding this comment

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

:shipit:

@kmahone
Copy link
Member

kmahone commented Oct 20, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@DmitriyKomin
Copy link
Contributor Author

Thanks @codendone!


In reply to: 1767443695

@DmitriyKomin DmitriyKomin merged commit 00b8713 into main Oct 20, 2023
32 checks passed
@DmitriyKomin DmitriyKomin deleted the user/dkomin/wv2_safe_cw2_handlers_winui2 branch October 20, 2023 21:41
ojhad pushed a commit that referenced this pull request Nov 2, 2023
* Harden all CoreWebView2 handlers with weak refs to WV2 object

* fix strongThis comparison

* Fix compariosn of strongThis with nextElement (strongThis->try_as<winrt::WebView2> always gives null)

* Disable consistently failing RadioButtons.AccessKeys test

* Update contribution_workflow.md - we no longer use FabricBot

---------

Co-authored-by: Dmitriy Komin <[email protected]>
Co-authored-by: Keith Mahoney <[email protected]>
@bpulliam bpulliam removed the needs-triage Issue needs to be triaged by the area owners label Jan 12, 2024
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 this pull request may close these issues.

4 participants