You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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.
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:
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
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:
On 2012-02-11 13:19:39 +0000, Tim Angus wrote:
On 2012-02-11 14:14:40 +0000, Tim Angus wrote:
On 2012-02-11 14:20:30 +0000, Tim Angus wrote:
On 2012-02-11 14:33:02 +0000, Gabriel Jacobo wrote:
On 2012-02-11 14:34:52 +0000, Gabriel Jacobo wrote:
On 2012-02-11 15:46:26 +0000, Tim Angus wrote:
On 2012-02-11 17:52:35 +0000, Gabriel Jacobo wrote:
On 2012-02-11 18:44:43 +0000, Sam Lantinga wrote:
On 2012-02-12 03:29:19 +0000, Tim Angus wrote:
On 2012-02-12 15:41:12 +0000, Gabriel Jacobo wrote:
On 2012-02-12 17:58:03 +0000, Sam Lantinga wrote:
The text was updated successfully, but these errors were encountered: