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

Memory leaks #2980

Closed
vaibhavTM opened this issue May 2, 2023 · 16 comments
Closed

Memory leaks #2980

vaibhavTM opened this issue May 2, 2023 · 16 comments

Comments

@vaibhavTM
Copy link

vaibhavTM commented May 2, 2023

Platform

iOS

Installed

CocoaPods

Version

8.1.0

Steps to Reproduce

Just run the app

Expected Result

No memory leaks.

Actual Result

Screenshot 2023-05-02 at 2 29 56 PM

Are you willing to submit a PR?

No

@brustolin
Copy link
Contributor

brustolin commented May 2, 2023

Thanks @vaibhavTM for reaching out. We're going to investigate this.

@vaibhavTM
Copy link
Author

Thankyou @brustolin for quick response.

@vaibhavTM
Copy link
Author

@brustolin Could you please confirm these issues exist in your SDK.

@brustolin
Copy link
Contributor

Hello @vaibhavTM.
I experience a few leaks when profiling is enabled (@armcknight can you check this?).
But In the latest version of the SDK I could not produce any of the leaks in your screenshot.

Can you update your Sentry SDK version and let us know please?

@vaibhavTM
Copy link
Author

Okay.

@brustolin
Copy link
Contributor

Out of curiosity I tried the SDK 8.1 and I was able to see the leaks.
So, Im confident that the leaks in the screenshot are fixed.

We need to fixed leaks regarding profiling tho.

@brustolin brustolin assigned armcknight and unassigned brustolin May 11, 2023
@vaibhavTM
Copy link
Author

vaibhavTM commented May 11, 2023

I am not able to update SDK above 8.1 via cocoapods.
my app's minimum iOS version support is iOS 11.0

This screenshot is of terminal when i did not give any SDK version in pod file. It automatically downloaded version 8.1.0
Installed by adding Sentry SDK version in pod file

This screenshot is of terminal when I gave any Sentry SDK version 8.7.0 in pod file.
Installed without providing SDK version number

Please tell me how can I update to latest version using cocoapods

@philipphofmann
Copy link
Member

@vaibhavTM, try run pod repo update and try again, please.

@vaibhavTM
Copy link
Author

@philipphofmann this worked. Now I am at the latest version of SDK (8.7.0)
@brustolin When can I expect fixes of profiling leaks..?

@brustolin
Copy link
Contributor

Profiling is a experimental feature right now, so please use it with caution, my guess is that this leaks will be fixed soon, but I can't give you an ETA.

@armcknight
Copy link
Member

Thanks for writing in @vaibhavTM . I'm looking at the possible leaks from the profiler now. I tried an instruments run with our Swift-iOS test app, and started a manual transaction with profiling and let it go until the profiler timed out. There are some symbols for me to investigate related to profiling:
image

I hope to get updates on this today.

@vaibhavTM
Copy link
Author

@armcknight okay.

@indragiek
Copy link
Member

@armcknight @brustolin I made some progress tracking this down. Basically what appears to be happening is that we're capturing SentryProfilingState (previously just a bunch of arrays/dictionaries, but same idea) in the std::function that we pass to the SamplingProfiler initializer, but that captured state is never deallocated. It's not clear what's holding the reference to it - in the Xcode memory graph it looks like this:

CleanShot 2023-05-15 at 23 45 20@2x

Some memory that we can't identify (represented by the malloc) is holding a ref to it. I still have to dig deeper on this but if you have time to take a look this would be where to focus - I think it might be some weirdness with how C++ holds references to Objective-C objects.

@indragiek
Copy link
Member

#3055 fixes all of these except for 1 leak

indragiek added a commit that referenced this issue May 24, 2023
This fixes most of the memory leaks reported in #2980 

The root of the problem seems to be that `std::thread` does not correctly deallocate parameters that are passed to the newly spawned thread. It's unclear if this is expected behavior or not, but the fix was simple: pass parameters that were being copied (and leaked) by reference.

There's also one more unrelated fix to a crash that I found while debugging the leaks - it turns out `-[NSString stringWithUTF8String:]` can return `nil` for malformed data and we weren't checking for nil.

---------

Co-authored-by: Sentry Github Bot <[email protected]>
@indragiek
Copy link
Member

@vaibhavTM We are cutting a new release with this fix soon.

@brustolin
Copy link
Contributor

brustolin commented May 25, 2023

This was solved in 8.7.3

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

No branches or pull requests

5 participants