Native memory leak caused by BlockLiteral BlockDescriptor memory not being collected after a provided callback function is invoked #20503
Comments
|
This sounds like an interesting issue; I'll try to have a look next week.
Can you provide the source code for the workaround somehow? Maybe in a different branch in your repository? |
|
I can reproduce this. |
rolfbjarne
added
the
performance
…n#20503. We're using two different functions to atomically decrement a reference count, the native `atomic_fetch_sub` and the managed `Interlocked.Decrement`. Unfortunately the return value is not the same: `atomic_fetch_sub` returns the original value before the subtraction, while `Interlocked.Decrement` returns the subtracted value, while our code assumed the functions behaved the same. This resulted in a memory leak, because we'd incorrectly expect `0` to be returned from `atomic_fetch_sub` when the reference count reaches zero, and thus not detect when the descriptor a block should be freed. The fix is to update the expected return value from `atomic_fetch_sub` to be `1` instead of `0`. Fixes xamarin#20503.
|
I found the bug, it turned out to be a simple fix. Thanks for the great test case btw! |
|
Oh, great stuff; just came back here to give my workaround example, which I'm guessing you don't need now. Would this have affected all native callbacks? Or just the one that I'm calling inside itself? |
Blocks can be copied - and this bug only affects those that have been copied. If a block is copied or not is an implementation detail of the function you call, so you can't really know when that will happen. |
|
Ah, yes, I know I've seen through research that the callback in this case is copied into a local field in the native Great, thanks for fixing. |
…n#20503. We're using two different functions to atomically decrement a reference count, the native `atomic_fetch_sub` and the managed `Interlocked.Decrement`. Unfortunately the return value is not the same: `atomic_fetch_sub` returns the original value before the subtraction, while `Interlocked.Decrement` returns the subtracted value, while our code assumed the functions behaved the same. This resulted in a memory leak, because we'd incorrectly expect `0` to be returned from `atomic_fetch_sub` when the reference count reaches zero, and thus not detect when the descriptor a block should be freed. The fix is to update the expected return value from `atomic_fetch_sub` to be `1` instead of `0`. Fixes xamarin#20503.
…al descriptors. Fixes #20503. (#20562) We're using two different functions to atomically decrement a reference count, the native `atomic_fetch_sub` and the managed `Interlocked.Decrement`. Unfortunately the return value is not the same: `atomic_fetch_sub` returns the original value before the subtraction, while `Interlocked.Decrement` returns the subtracted value, while our code assumed the functions behaved the same. This resulted in a memory leak, because we'd incorrectly expect `0` to be returned from `atomic_fetch_sub` when the reference count reaches zero, and thus not detect when the descriptor a block should be freed. The fix is to update the expected return value from `atomic_fetch_sub` to be `1` instead of `0`. Fixes #20503. Backport of #20556. --------- Co-authored-by: Alex Soto <[email protected]>
In my app (NetworkExtension in this case) I call an Objective-C function (
NEPacketTunnelFlow.ReadPackets). This method gets called a lot. Over time, native memory consumption grows steadily, eventually hitting up against the 50MB Network Extension limit.It took me a long time to figure out what was actually leaking memory, but I've narrowed it down to
BlockLiteral, used to pass a callback to the Objective-C methodNEPacketTunnelFlow:runPacketWithCompletionHandler.Inside
BlockLiteral.SetupFunctionPointerBlock,AllocHGlobalis used to allocate native memory to contain the Objective-CBlockDescriptor. In my case it's about 96 bytes each time, but it varies based on the size of the trampoline method signature.The problem is that the
BlockDescriptormemory is not always freed after the resulting callback is invoked, leading to the native memory leaking.I proved this by copying out the trampoline implementation into my own code, and providing a re-usable, pinned BlockLiteral instance to
objc_msgSendinstead of a fresh one each time, and the issue goes away.From what I can tell, after the represented callback is invoked, the ref-count on the BlockLiteral is decremented, and the runtime should free that memory, but it isn't doing so consistently.
Steps to Reproduce
This may be specific to NEPacketTunnelFlow:ReadPackets, only because inside the callback passed to the underlying
:readPacketsWithCompletionHandlertheReadPacketsmethod is invoked again; i.e. within the call stack of the same[UnmanagedCallersOnly]trampoline method, but can't be sure. In general:os_proc_available_memory()to the log. This will decrease gradually and not recover, but the GC TotalMemory will not increase.The issue should be visible in https://github.com/enclave-alistair/dotnet-ios-netextension, but may need some package updates to match latest Xamarin.
Expected Behavior
The memory allocated by BlockLiteral is reliably freed after the provided callback method exits.
Actual Behavior
The memory allocated by BlockLiteral is not consistently freed, leading to a gradual increase in memory usage.
Environment
Version information
Example Project
See https://github.com/enclave-alistair/dotnet-ios-netextension
The text was updated successfully, but these errors were encountered: