[Bug]: Duplicate deactivation of ViewModel, from ViewModelActivator.

[JakenVeina]

Describe the bug 🐞

I encountered this via an ObjectDisposedException coming out of ViewForMixins.HandleViewModelActivation. This was the result of the following sequence of events:

1. A ViewModel within a DynamicData collection is disposed, after being removed from the collection.
2. The ViewModelActivator within that ViewModel is disposed, along with it.
3. A View consuming this ViewModel fires the Unloaded event, on a future dispatcher frame.
4. ViewModelActivator.Deactivate() is called via disposal of a previous call to .Activate()
5. _deactivated.OnNext() is called, after _deactivated has been disposed.

I initially tried to solve this by calling .Deactivate(ignoreRefCount: true) on the activator before disposing it, but this did not end up preventing the deactivated.OnNext() call within .Deactivate(). This is because I actually have 5 View consumers that are activating this ViewModel, and .Deactivate() does not reset _refCount to wipe these out. In other words, it's possible for ViewModelActivator.Deactivated to fire more times than ViewModelActivator.Activated, which is what _refCount seems to be intended to prevent from happening.

https://github.com/reactiveui/ReactiveUI/blob/11a314090295bc6f53038ffe22aab5687a6df94d/src/ReactiveUI/Activation/ViewModelActivator.cs#L93C43-L93C43

Step to reproduce

var activator = new ViewModelActivator();

activator.Activated.Subscribe(_ => Console.WriteLine("Activated"));
activator.Deactivated.Subscribe(_ => Console.WriteLine("Deactivated"));

var activation1 = activator.Activate();
var activation2 = activator.Activate();
var activation3 = activator.Activate();
var activation4 = activator.Activate();

activator.Deactivate(ignoreRefCount: true);

activator.Dispose();

activation1.Dispose();
activation2.Dispose();
activation3.Dispose();
activation4.Dispose();

For this snippet, ObjectDisposedException throws at the activation3.Dispose() call.

If activator.Dispose() is removed, then you can observe two firings of the Deactivated event in the console, despite Activated only firing once.

Reproduction repository

No response

Expected behavior

• A ViewModelActivator that has is inactive should be safe to dispose, so long as no one tries to activate it again.
• Calling ViewModelActivator.Deactivate(ignoreRefCount: true) should render all IDisposables returned by previous calls to .Activate() inert, and disposing those should not trigger an exception.
• It should not be possible for the ViewModelActivator.Deactivated event to fire more times than `ViewModelActivator.Activated.

Screenshots 🖼️

No response

IDE

No response

Operating system

Windows

Version

10 22H2

Device

N/A

ReactiveUI Version

19.4.1

Additional information ℹ️

I would happily submit a PR to fix this, if it would be welcome.

[glennawatson]

Do you need to dispose is the first reaction. Observables often do not.

[JakenVeina]

Strictly speaking, probably not. .Deactivate(ignoreRefCount: true) should force the removal of the sensitive references that could cause a memory leak if left alive.

This still seems like a violation of the class's apparent contract. And to me, there's value in Disposing the activator, in that if something DOES try to activate it again, I get an exception, and a bug in my own code is revealed.

[glennawatson]

Observables aren't honoring that contract anyway. They just adopted the IDisposable to get advantages of some of the mechanics. It's a mistake to think you have to dispose all of them. Depends on ownership of the handle.

[JakenVeina]

I meant more the "contract" regarding the Activated and Deactivated events. That's got nothing to do with disposal mechanics.

Actually, scratch what I said earlier. ViewModelActivator.Deactivate(ignoreRefCount: true) does not clear out all potentially sensitive references, because the two subjects for Activated and Deactivated could have subscribers, and the only way to clear those, currently, is through disposal.

I think an appropriate change would be from...

public void Deactivate(bool ignoreRefCount = false)
{
    if (Interlocked.Decrement(ref _refCount) == 0 || ignoreRefCount)
    {
        Interlocked.Exchange(ref _activationHandle, Disposable.Empty).Dispose();
        _deactivated.OnNext(Unit.Default);
    }
}

...to...

public void Deactivate(bool ignoreRefCount = false)
{
    var wasActive = ignoreRefCount
        ? (Interlocked.Exchange(ref _refCount, 0) != 0)
        : (Interlocked.Decrement(ref _refCount) == 0);

    if(wasActive)
    {
        Interlocked.Exchange(ref _activationHandle, Disposable.Empty).Dispose();
        _deactivated.OnNext(Unit.Default);
    }
}