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 Pixmap safe to dispose if already disposed #7347
base: master
Are you sure you want to change the base?
Conversation
@@ -391,7 +391,7 @@ public int getHeight () { | |||
|
|||
/** Releases all resources associated with this Pixmap. */ | |||
public void dispose () { | |||
if (disposed) throw new GdxRuntimeException("Pixmap already disposed!"); | |||
if (disposed) Gdx.app.error("Pixmap", "Pixmap already disposed!"); | |||
pixmap.dispose(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); */
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made it too fast, fixed!
Your last commit title is weird :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would go as far as to remove the warning, but lgtm
From my understanding, once a pixmap is disposed, a reference to the pixmap probably shouldn't exist anymore. However, I would be fine with this, if together with this, we would set libgdx/gdx/src/com/badlogic/gdx/graphics/g2d/Gdx2DPixmap.java Lines 170 to 171 in 7bed7d3
This should prevent use after dispose better I think. |
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 |
Yes, I agree, I just wanted to point out, that throwing a exception on double disposale can unveil those bugs.
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 |
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 |
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
That's a bit too much tbh and I don't think it's worth it. What about setting |
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 |
Not possible as it is, I'd need to remove the |
BTW, it's a good illustration when broken encapsulation obstructs refactoring/enhancement |
Pixmap
seems to be the onlyDisposable
class that is not safe todispose()
if already disposed. Unless there's a good reason for this we should treat this scenario more leniently. Issue originally raised in #7334.