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

Android_JNI_FileClose local reference bug #549

Closed
SDLBugzilla opened this issue Feb 10, 2021 · 0 comments
Closed

Android_JNI_FileClose local reference bug #549

SDLBugzilla opened this issue Feb 10, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: HG 2.0
Reported for operating system, platform: Android (All), ARM

Comments on the original bug report:

On 2012-02-11 07:05:53 +0000, Gabriel Jacobo wrote:

Created attachment 819
Android_JNI_FileClose patch

As reported here: http://forums.libsdl.org/viewtopic.php?p=31940

Fix is attached.

On 2012-02-11 13:19:39 +0000, Tim Angus wrote:

Can you pinpoint where the local reference(s) are being created/leaked? I can't see it. This patch may fix the problem, but I think it's important to understand the cause in case leaks also exist elsewhere. Well done on narrowing it down so far though.

On 2012-02-11 14:14:40 +0000, Tim Angus wrote:

Hmm...

diff -r 03a7ceb3487b src/core/android/SDL_android.cpp
--- a/src/core/android/SDL_android.cpp Tue Feb 07 19:34:24 2012 -0500
+++ b/src/core/android/SDL_android.cpp Sat Feb 11 22:12:23 2012 +0000
@@ -559,6 +559,7 @@
jmethodID mid = mEnv->GetMethodID(mEnv->GetObjectClass(inputStream),
"close", "()V");
mEnv->CallVoidMethod(inputStream, mid);

  •    mEnv->DeleteLocalRef(jmethodID);
       mEnv->DeleteGlobalRef((jobject)ctx->hidden.androidio.inputStreamRef);
       mEnv->DeleteGlobalRef((jobject)ctx->hidden.androidio.readableByteChannelRef);
       if (Android_JNI_ExceptionOccurred()) {
    

I'm not in a position to test this until next week, it would be interesting to see if it has the same effect as the pop/push patch.

On 2012-02-11 14:20:30 +0000, Tim Angus wrote:

Actually, if that is the cause, the results of GetMethodID in the exception handling function would need to be deleted too. If this is the case it would mean that the read function is leaking references also.

On 2012-02-11 14:33:02 +0000, Gabriel Jacobo wrote:

I think that, while it is an interesting exercise, it's not really vital knowing which local reference we are forgetting to release. My vote is to enclose every one of these functions in a Push/Pop local frame, as this type of bugs are hard to detect and I think there's no unit tests for RWops in SDL Android...also I don't know of any downside of doing it like this...
It seems to me it's also a good defensive mechanism for potential future modifications by other programmers that haven't gone through this issues already.

On 2012-02-11 14:34:52 +0000, Gabriel Jacobo wrote:

BTW, at some point while debugging this I did a test by releasing mid, but I don't think that was the cause.

On 2012-02-11 15:46:26 +0000, Tim Angus wrote:

The disadvantages of using push/pop are that it's an inefficient use of local refs, which could potentially lead to exhausting them prematurely. If you manage refs manually, you only ever take exactly what you need... except when you're leaking them of course :). The other thing is that for functions that have mutiple return statements, each must pop the frame. In other words, the control flow is potentially more complicated.

Having said that, you're probably right, but push/pop should be added to all functions (including ones not related to file IO) and any DeleteLocalRef calls removed. By the way, in your existing patch the allocatedLocalFrame variable is unnecessary; it'll always be true when it is used in the if statement.

If what you say about GetMethodID not being the leak is true, I'm pretty mystified where the leak is coming from.

On 2012-02-11 17:52:35 +0000, Gabriel Jacobo wrote:

Hey Tim, this is the true fix:

jobject inputStream = (jobject)ctx->hidden.androidio.inputStream;
// inputStream.close();
jclass inputStreamClass = mEnv->GetObjectClass(inputStream);
jmethodID mid = mEnv->GetMethodID(inputStreamClass,
"close", "()V");
mEnv->CallVoidMethod(inputStream, mid);
mEnv->DeleteGlobalRef((jobject)ctx->hidden.androidio.inputStreamRef);
mEnv->DeleteGlobalRef((jobject)ctx->hidden.androidio.readableByteChannelRef);
mEnv->DeleteLocalRef((jobject)inputStreamClass);

It turns out mEnv->GetObjectClass(inputStream) creates a local ref.

On 2012-02-11 18:44:43 +0000, Sam Lantinga wrote:

Created attachment 820
Patch to automatically manage local references

Here's a completely untested patch to help automate local reference management.

On 2012-02-12 03:29:19 +0000, Tim Angus wrote:

(In reply to comment # 7)

It turns out mEnv->GetObjectClass(inputStream) creates a local ref.

Bah, how did I miss that? Too much beer.

Sam your patch looks ideal. It's easy to forget this is a C++ file.

On 2012-02-12 15:41:12 +0000, Gabriel Jacobo wrote:

The patch compiles fine if you #include SDL_assert.h in core/android/SDL_android.h (or somewhere else...the config file perhaps?) and it solves the issue perfectly.
I must say it's a very elegant solution and perhaps we should add it to the other JNI functions, like the one that handles Exceptions and the audio ones as well.
+1 from me

On 2012-02-12 17:58:03 +0000, Sam Lantinga wrote:

Okay, thanks!
http://hg.libsdl.org/SDL/rev/1893d507ba42

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant