Protobuf memory leaks - SINGULAR_SETTER_IMP

[sarsonj]

Hi,

we finally decided to update j2objc from very old version 2.8 to current master. During our profiling tests we realised, that after update, there is many memory leaks related to protobuffers. Because we are using protobuffers for inter device communication and we are sending relative big amount of data through them, this is major issue for us.

I went backwards through all changes in protobuf runtime, until I found change, that was implemented in commit 9e9614f . Specifically, the macro:

#define SINGULAR_SETTER_IMP(NAME) \
  static void SingularSet##NAME(id msg, TYPE_##NAME value, size_t offset, CGPHasLocator hasLoc) { \
    TYPE_##NAME *ptr = FIELD_PTR(TYPE_##NAME, msg, offset); \
    ClearPreviousOneof(msg, hasLoc, (uintptr_t)ptr); \
    TYPE_ASSIGN_##NAME(*ptr, value); \
    SetHas(msg, hasLoc); \
  }

that was changed in this commit to:

#define SINGULAR_SETTER_IMP(NAME) \
  static void SingularSet##NAME(id msg, TYPE_##NAME value, size_t offset, CGPHasLocator hasLoc) { \
    TYPE_##NAME *ptr = FIELD_PTR(TYPE_##NAME, msg, offset); \
    ClearPreviousOneof(msg, hasLoc, (uintptr_t)ptr); \
    TYPE_RETAINED_ASSIGN_##NAME(*ptr, value); \
    SetHas(msg, hasLoc); \
  }

seems to causing the memory leaks.

When I changed TYPE_RETAINED_ASSIGN back to TYPE_ASSIGN, memory leaks disappeared.

The screenshot from profiler:

[tomball]

Since you have a reproducible problem here, would you please try editing TYPE_RETAINED_ASSIGN_Retainable and rerun your test:

< #define TYPE_RETAINED_ASSIGN_Retainable(assignee, value) assignee = RETAIN_(value)
---
> #define TYPE_RETAINED_ASSIGN_Retainable(assignee, value) assignee = RETAIN_AND_AUTORELEASE(value)

Assuming the protobuf code is executed by a thread with an autoreleasepool (which all j2objc-generated code must), this should fix the memory leak without changing how the assignee is retained during execution.

[sarsonj]

Thanks,

I tried this change, but it generates EX_BAD_ACCESS during the run of the app. I am not sure where exactly, because it crash in non related j2objc code. But when returned code back or to that TYPE_ASSIGN_ variant, it works without crashes.

[tomball]

Right, the autorelease was a bad suggestion, because after looking at the issue more carefully, that commit fixes a case where a oneof field wasn't retained when it was reset. So if the calling code then releases that new value (as it should, if it no longer owns that object), then the oneof will crash when the oneof value is later fetched.

I think the reason you're having this problem, and Google's huge, heavy proto-using apps aren't, is because you may have fixed this problem previously in a way that doesn't adhere to Apple's rules about object ownership. To replace the oneof value with a new one, the new one initially needs to be owned (retained) by the calling code, then if the calling code no longer directly references that new value, it should be released (better) or autoreleased. Because the oneof now owns the value object, it needs to retain it prior to setting it (which 9e9614f does). The memory leak is due to the calling code not releasing an object it no longer owns.

How are you creating the new value? The pattern should be:

id newValue = [[ValueClass alloc] init];   // Retain count 1
[proto setOneof:newValue];  // Retain count 2: oneof now also owns it
[newValue release];  // Retain count 1, released when oneof is deallocated.

Without that last release, the object will leak. To see how the value object is retained, run the Leaks Profiler in Xcode Instruments, then see a leaked value object's stack trace.

If you instead want this change reverted, you'll need to fork the j2objc/protobuf directory, make whatever changes you want and link that library into your app instead of the one from the j2objc build.

The j2objc protobuf code is in maintenance mode as we're replacing the j2objc protobuf library/plugin with a transpiled Java protolite solution. All of Google's cross-platform apps (Java shared between Android and iOS, and often web) are now required to use the Java Lite Runtime on their Android versions. It's much smaller and faster than the full Java protobuf library as it's optimized for mobile devices, which is why we're moving iOS to also use it.