Fix issues with proxying PoolableDrawables

[ThereGoesMySanity]

Currently, if you attempt to create a proxy for a PoolableDrawable, you run into issues when the Drawable is reused. The proxy is disposed when the original is returned to the pool, but the reference in the original is never cleared so HasProxy will still be true despite proxy pointing to a disposed object that no longer renders.

To fix this, I've overridden OnDisposed in PoolableDrawable to clear the reference to the original when it is disposed.

I've also added a test case to TestSceneDrawablePool that fails on origin/master and passes with the changes from this PR.

[bdach]

First, I want to zoom in on one claim here, namely

The proxy is disposed when the original is returned to the pool

This may be true if the original gets returned to the pool because of the original's lifetime expiring and

osu-framework/osu.Framework/Graphics/Drawable_ProxyDrawable.cs

Lines 51 to 57 in e01b3cc

	public override bool RemoveWhenNotAlive => Original.RemoveWhenNotAlive;
	protected internal override bool ShouldBeAlive => Original.ShouldBeAlive;
	public override double LifetimeStart => Original.LifetimeStart;
	public override double LifetimeEnd => Original.LifetimeEnd;

but there are a whole load of scenarios where that doesn't need to be the case.

In fact, a crude modification of the test case added here, designed to stop relying on lifetime, exposes that fact:

diff --git a/osu.Framework.Tests/Visual/Drawables/TestSceneDrawablePool.cs b/osu.Framework.Tests/Visual/Drawables/TestSceneDrawablePool.cs
index aeb3b1ccb..9dc5cda0f 100644
--- a/osu.Framework.Tests/Visual/Drawables/TestSceneDrawablePool.cs
+++ b/osu.Framework.Tests/Visual/Drawables/TestSceneDrawablePool.cs
@@ -320,6 +320,7 @@ public void TestUsePoolableDrawableWithProxy()
 
             AddRepeatStep("get new pooled drawable with proxy", () =>
             {
+                Clear(false);
                 var drawable = consumeDrawable();
                 Add(drawable.CreateProxy());
             }, 50);
@@ -438,8 +439,6 @@ protected override void PrepareForUse()
                 this.FadeOutFromOne(fadeTime);
                 this.RotateTo(0).RotateTo(80, fadeTime);
 
-                Expire();
-
                 PreparedCount++;
             }
 

So even If this change were correct, it's not enough for universal general safety. For more complex usages, consumers will still have to store a reference to the proxy they created and add manual logic ensuring that it is removed and disposed immediately when its pooled original is being returned to the pool.

Secondly, the other issue is that the timing of invocation of disposal is essentially indeterminate because of its asynchronous nature. So while it may appear to work here, I don't think there's any guarantee that it will actually work reliably, even in the one case of lifetime expiration that this is appearing to target. In fact if you induce enough disposal pressure you can probably manufacture a race wherein the disposal of the proxy will not run before the drawable is re-used again, leading to the same failure again.

---

Which is to say, I think if anything is to be done here, then we probably need a primitive that helps users manually, definitively and immediately, get rid of a proxy on their drawable, which is something that I've felt over the years as one of the biggest holes in the whole proxying system.

@smoogipoo mind checking this and seeing if you agree with my assessment here?

[ThereGoesMySanity]

I see what you mean with the diff you posted. It seems like there are a lot of ways to put a Drawable in an unrecoverable state by messing with its Proxy so I agree that something like GetRidOfProxy() would be ideal.

[smoogipoo]

I don't disagree with adding a way to remove a proxy. I worry a little about the song-and-dance of actually doing that, but maybe it's only a matter of "detaching" the ProxyDrawable from the source and (potentially - maybe this can be left up to the user) expiring it.