Make Pixmap safe to dispose if already disposed

[obigu]

Pixmap seems to be the only Disposable class that is not safe to dispose() if already disposed. Unless there's a good reason for this we should treat this scenario more leniently. Issue originally raised in #7334.

[Anton-Samarskyi]

Is it safe to run it twice? It calls the next code inside:
private static native void free (long pixmap); /* gdx2d_free((gdx2d_pixmap*)pixmap); */

[obigu]

Made it too fast, fixed!

[Anton-Samarskyi]

Your last commit title is weird :D

[PokeMMO]

I would go as far as to remove the warning, but lgtm

[Berstanio]

From my understanding, once a pixmap is disposed, a reference to the pixmap probably shouldn't exist anymore.
Doing a double dispose might mean, that after initial disposal, a reference to the Pixmap was still around somewhere.

However, I would be fine with this, if together with this, we would set pixmap to null after disposale and setting "basePtr" to "-1" here after freeing:

libgdx/gdx/src/com/badlogic/gdx/graphics/g2d/Gdx2DPixmap.java

Lines 170 to 171 in 7bed7d3

	free(basePtr);
	}

This should prevent use after dispose better I think.

[obigu]

From my understanding, once a pixmap is disposed, a reference to the pixmap probably shouldn't exist anymore. Doing a double dispose might mean, that after initial disposal, a reference to the Pixmap was still around somewhere.

But for that to occur you don't need a double disposal, a reference might be kept afer a single disposal and the problem you describe would already apply. This is something that can occur with all Disposable assets, if you keep a reference and do stuff with it it will fail. Pixmap already does this check on some of its methods such as getPixels() (it's possible more of those checks are required). I think an exception in those cases is the right approach as there's something wrong in the app code that may be more serious (more than just calling dispose() twice).

[Berstanio]

But for that to occur you don't need a double disposal, a reference might be kept afer a single disposal and the problem you describe would already apply.

Yes, I agree, I just wanted to point out, that throwing a exception on double disposale can unveil those bugs.

Pixmap already does this check on some of its methods such as getPixels() (it's possible more of those checks are required).

Yes, thats what I wanted to say with my second paragraph. I don't see a downside on making Pixmap more lenient against double disposal, if the other use after dispose guards are enforced stronger (by e.g. setting pixmap to null)

[obigu]

Yes, thats what I wanted to say with my second paragraph. I don't see a downside on making Pixmap more lenient against double disposal, if the other use after dispose guards are enforced stronger (by e.g. setting pixmap to null)

Ok, I'd go with the controlled exception instead of the nullpointer crash though, is there any particular method you think we should add checks on in addition to getPixels() to avoid those potential issues?

[Berstanio]

there any particular method you think we should add checks on in addition to getPixels() to avoid those potential issues?

I would say every method, that assumes an undisposed pixmap. However, the list is long, so I thought doing a uncontrolled NPE is good enough.
At first glance, basically setFilter/setBlending, all fill* methods and draw* methods, getPixel* methods and setPixels

[obigu]

I would say every method, that assumes an undisposed pixmap. However, the list is long, so I thought doing a uncontrolled NPE is good enough.

I don't like the NPE approach as users won't know what's going and that's an approach we don't follow afaik on any Disposable class.

At first glance, basically setFilter/setBlending, all fill* methods and draw* methods, getPixel* methods and setPixels

That's a bit too much tbh and I don't think it's worth it.

What about setting basePtr to -1, what would happen if user attempts using an already disposed Pixmapin that case?

[Berstanio]

What about setting basePtr to -1, what would happen if user attempts using an already disposed Pixmapin that case?

Undefined behaviour, but in nearly all cases a segfault. While I think it is better than a use-after-free, I think it shouldn't be the usual thing what happens, when working on a disposed Pixmap. I only suggested it as a fail-safe after the "pixmap is null" guard, to fully prevent use-after-free cases.

[obigu]

What about setting basePtr to -1, what would happen if user attempts using an already disposed Pixmapin that case?

Undefined behaviour, but in nearly all cases a segfault. While I think it is better than a use-after-free, I think it shouldn't be the usual thing what happens, when working on a disposed Pixmap. I only suggested it as a fail-safe after the "pixmap is null" guard, to fully prevent use-after-free cases.

Ok, I guess setting pixmap to null is not that bad as we can always add the disposed check + exception on any methods we want a more informative crash. I'll make the change.

[obigu]

What about setting basePtr to -1, what would happen if user attempts using an already disposed Pixmapin that case?

Undefined behaviour, but in nearly all cases a segfault. While I think it is better than a use-after-free, I think it shouldn't be the usual thing what happens, when working on a disposed Pixmap. I only suggested it as a fail-safe after the "pixmap is null" guard, to fully prevent use-after-free cases.

Ok, I guess setting pixmap to null is not that bad as we can always add the disposed check + exception on any methods we want a more informative crash. I'll make the change.

Not possible as it is, I'd need to remove the final qualifier. I'm leaning to leaving it as it is. Let's see if there's more opinions on this.

[Anton-Samarskyi]

BTW, it's a good illustration when broken encapsulation obstructs refactoring/enhancement