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

Garbage collection of EventSource<T> causes subsequent failure to unsubscribe handlers #842

Closed
Scottj1s opened this issue May 11, 2021 · 3 comments · Fixed by #861 or #910
Closed
Assignees
Labels
bug Something isn't working fixed Issue has been fixed in an upcoming or existing release
Milestone

Comments

@Scottj1s
Copy link
Member

Scottj1s commented May 11, 2021

Reported by an external customer.

WinRT.EventSource has a bug in Subscribe/Unsubscribe, which cache event handlers (by combining delegates) rather than subscribing each one directly to the WinRT event. This allows each handler to be removed without having to track individual event registration tokens. The bug is that if an EventSource is garbage collected, then the next access in C# code to the event source will produce a new RCW. The first RCW will leak its outstanding event handler registrations. Subsequent attempts to unregister a handler on the second RCW will fail on this check in Unsubscribe:

            if (oldEvent is object && _event is null)

This is a common scenario for singleton objects (or long-lived objects) which might access the event source object each time without caching it and preserving the lifetime of the EventSource RCW:

    public SomeObject()
    {
        InitializeComponent();
        Loaded += (object sender, RoutedEventArgs e) =>
        {
            Singleton.Instance().SomeEvent += this.EventHandler;
        };

        Unloaded += (object sender, RoutedEventArgs e) =>
        {
            Singleton.Instance().SomeEvent -= this.EventHandler;
        };
    }

The net effect of the bug is that spurious event handlers are executed.

A workaround exists - cache the RCW to ensure that it does not lose track of its registrations.

@Scottj1s Scottj1s added the bug Something isn't working label May 11, 2021
@angelazhangmsft angelazhangmsft added this to the Release 1.2.7 milestone May 18, 2021
@manodasanW manodasanW added the AssemblyVersion change Requires assembly version change for WinRT.Runtime label Jun 11, 2021
@angelazhangmsft angelazhangmsft added the fixed Issue has been fixed in an upcoming or existing release label Jun 17, 2021
@manodasanW manodasanW reopened this Jun 18, 2021
@manodasanW manodasanW removed the fixed Issue has been fixed in an upcoming or existing release label Jun 18, 2021
@manodasanW
Copy link
Member

manodasanW commented Jun 18, 2021

Re-activating due to change was reverted before release due to WinUI test failures. The assembly version impacting portion of this change was kept and thereby putting it back in should not be assembly version impacting.

@JanRajnoha
Copy link

What is the status of this task?

@manodasanW
Copy link
Member

It turns out after we addressed the test failures, the other issue which we saw resolved by this issue started hitting again. The test failures were a result of a memory leak this fix initially introduced and addressing that led to the other null pointer exception from #840 happening again. But progress is still being made on that issue and we are investigating with WinUI folks if that issue is a result of WinUI still holding onto ICustomProperty instances from pages that have already been cleaned up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed Issue has been fixed in an upcoming or existing release
Projects
None yet
5 participants