New blog post: Breaking Retain Cycles with @Weak and Other Techniques

[Lukhnos Liu]

We've made a blog post about breaking retain cycles in your cross-platform code:

http://j2objc.org/blog/2017/01/30/retain-cycles.html

In addition to J2ObjC's @Weak and @WeakOuter annotations, it also covers lambdas as well as a few tools and techniques that we find helpful.

[Daniel Dickison]

Thanks — that is useful and I've shared it with my team. I finally got around to setting up cycle_finder and plugged a few leaks as a result.

Meanwhile, I'm finding many memory leaks (using Instruments) that appear to be related to Gson deserialization. Most of this work happens in the background via android.os.AsyncTask. I see that jre_emul's cycle_whitelist.txt contains these:

FIELD android.os.AsyncTask.mFuture

FIELD android.os.AsyncTask.mWorker

But these fields are initialized as anonymous classes whose methods refer to the parent AsyncTask, and as far as I can tell they are never nulled-out. Wouldn't this lead to a leak with every AsyncTask? I'm wondering if maybe the Gson-related leaks are actually a result of this.

In general, does this mean anonymous classes are almost always off-limits for code that will be translated?

[Keith Stanger]

On Tue, 31 Jan 2017 at 17:41 Daniel Dickison <danield...@gmail.com> wrote:

Thanks — that is useful and I've shared it with my team. I finally got around to setting up cycle_finder and plugged a few leaks as a result.

Meanwhile, I'm finding many memory leaks (using Instruments) that appear to be related to Gson deserialization. Most of this work happens in the background via android.os.AsyncTask. I see that jre_emul's cycle_whitelist.txt contains these:

FIELD android.os.AsyncTask.mFuture

FIELD android.os.AsyncTask.mWorker

But these fields are initialized as anonymous classes whose methods refer to the parent AsyncTask, and as far as I can tell they are never nulled-out. Wouldn't this lead to a leak with every AsyncTask? I'm wondering if maybe the Gson-related leaks are actually a result of this.

You're right about AsyncTask, these do look like reference cycles. It looks as though these fields were whitelisted when AsyncTask was added without giving them a closer look. I think they could use @RetainedWith annotations, unless it can be proven that these children can't outlive the AsyncTask parent, then @WeakOuter can be used. I'll work on a fix for this. In the meantime you can try adding @RetainedWith to both fields. Let me know if that fixes the leaks.

In general, does this mean anonymous classes are almost always off-limits for code that will be translated?

Not necessarily. Anonymous classes and lambdas are still very useful and don't always cause cycles. The more specific pattern to be wary of is when an anonymous class (or lambda or non-static inner) is assigned to a field in its parent class. In this situation, if the child reference is never held by anything but its parent, then a @WeakOuter can easily be applied without any other change.

On Monday, January 30, 2017 at 4:18:01 PM UTC-5, Lukhnos Liu wrote:

We've made a blog post about breaking retain cycles in your cross-platform code:

http://j2objc.org/blog/2017/01/30/retain-cycles.html

In addition to J2ObjC's @Weak and @WeakOuter annotations, it also covers lambdas as well as a few tools and techniques that we find helpful.

--
You received this message because you are subscribed to the Google Groups "j2objc-discuss" group.
To unsubscribe from this group and stop receiving emails from it, send an email to j2objc-discus...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Daniel Dickison]

On Wednesday, February 1, 2017 at 9:56:41 AM UTC-5, Keith Stanger wrote:

On Tue, 31 Jan 2017 at 17:41 Daniel Dickison <danield...@gmail.com> wrote:

Thanks — that is useful and I've shared it with my team. I finally got around to setting up cycle_finder and plugged a few leaks as a result.

Meanwhile, I'm finding many memory leaks (using Instruments) that appear to be related to Gson deserialization. Most of this work happens in the background via android.os.AsyncTask. I see that jre_emul's cycle_whitelist.txt contains these:

FIELD android.os.AsyncTask.mFuture

FIELD android.os.AsyncTask.mWorker

But these fields are initialized as anonymous classes whose methods refer to the parent AsyncTask, and as far as I can tell they are never nulled-out. Wouldn't this lead to a leak with every AsyncTask? I'm wondering if maybe the Gson-related leaks are actually a result of this.

You're right about AsyncTask, these do look like reference cycles. It looks as though these fields were whitelisted when AsyncTask was added without giving them a closer look. I think they could use @RetainedWith annotations, unless it can be proven that these children can't outlive the AsyncTask parent, then @WeakOuter can be used. I'll work on a fix for this. In the meantime you can try adding @RetainedWith to both fields. Let me know if that fixes the leaks.

I tried adding @RetainedWith to the two fields and it seems that causes the AsyncTask to be deallocated too early; I hit a "*** -[BCSPushNotifications_1 finishWithId:]: message sent to deallocated instance" — I'm not sure why blockSelf isn't automatically retained in postResult() but I haven't investigated beyond that for now.

 

In general, does this mean anonymous classes are almost always off-limits for code that will be translated?

Not necessarily. Anonymous classes and lambdas are still very useful and don't always cause cycles. The more specific pattern to be wary of is when an anonymous class (or lambda or non-static inner) is assigned to a field in its parent class. In this situation, if the child reference is never held by anything but its parent, then a @WeakOuter can easily be applied without any other change.

Ah that makes sense. Thanks for the clarification. 

[Keith Stanger]

I plan to apply @RetainedWith to mFuture and apply @WeakOuter to the anonymous class assigned to mWorker. (It needs to be converted to a local class because WeakOuter doesn't support TYPE_USE targets yet)

I don't know of any internal usage of AsyncTask so your verification is valued here. Let me know if you sort out the kinks, or if you need more help.

--

[Daniel Dickison]

I'll be happy to test and verify that fix.

For now I put @RetainedWith on both of those fields and added a manual retain and release on blockSelf in postResult() (retain before the dispatch_async and release inside the block) and this has fixed both the crash and the original memory leaks, so I think your fix will solve the leak as well.

[Keith Stanger]

I don't see any need for the __block annotation which is causing blockSelf to be captured weakly. Does it work for you to simply reference "self" from within the block? I think that would be most appropriate, and avoids the manual memory management. I think the same change should be applied to the block in publishProgress() as well.

[Daniel Dickison]

You're right — referencing self directly works correctly here. (I had completely forgotten the semantics of __block after only a year or so of swift.)

Here's what my branch looks like with the two @RetainedWith's:
https://github.com/google/j2objc/compare/master...bandcampdotcom:AsyncTask-leak

> You received this message because you are subscribed to a topic in the Google Groups "j2objc-discuss" group.
> To unsubscribe from this topic, visit https://groups.google.com/d/topic/j2objc-discuss/rBAK34ybkMw/unsubscribe.
> To unsubscribe from this group and all its topics, send an email to j2objc-discus...@googlegroups.com.

[Keith Stanger]

FYI, the AsyncTask fix is available here: https://github.com/google/j2objc/commit/46939f3d38f9e71a9cfcaf41cbd19f054dc2189e

[Daniel Dickison]

Just tested to confirm that it fixes the leak, and it does. Thanks!