search

android / codelab-while-in-use-location

Codelab:
https://github.com/googlecodelabs/while-in-use-location
Apache License 2.0
81 stars 51 forks source link

Mention (or fix) locationCallback leak #7

Closed bubenheimer closed 2 years ago

bubenheimer commented 4 years ago

The codelab might mention the longstanding memory leak issue regarding locationCallback, or, even better, bring about a fix; the locationCallback object and any (potentially inadvertently) referenced objects will never actually be garbage collected after calling removeLocationUpdates(). In the case of the codelab it's just the (already eternal) foreground service, but the consequences tend to be more pronounced for other uses:

https://github.com/android/location-samples/issues/26 https://issuetracker.google.com/issues/37126862

codingjeremy commented 3 years ago

Let me look into the status and follow up.

bubenheimer commented 3 years ago

Thanks, much appreciated! I just tried my repro again, leak is still present, with latest versions of all dependencies:

https://github.com/bubenheimer/requestlocationupdatesleak

Adding & removing the LocationCallback 4 times crashes the repro app on Pixel 3 XL Android 11 test device with OutOfMemoryError. (This particular LocationCallback allocates a 50MB buffer for leak repro purposes.)

codingjeremy commented 3 years ago

I will pass this along. Thanks for the extra information + code+

codingjeremy commented 3 years ago

Talked to the team. They are going to look into it after the rush over Android Dev Previews is done. I hope to have an update in mid-April to late-April.

bubenheimer commented 3 years ago

Thank you so much for your efforts. It would be wonderful to get this longstanding problem resolved, depending on the use case an appropriate workaround can be a real architectural pain point. In case I can offer additional assistance in the resolution, please let me know.

bubenheimer commented 3 years ago

It's been a while. Has there been any progress? I've updated the sample to use the latest versions of everything, but the leak situation is unchanged. Thanks.

codingjeremy commented 2 years ago

Sorry for the delay as I moved off location awhile back, but I wanted to follow up when I had more information.

I got an updated from ENG and they completely rewrote the way they store listeners and a bunch of other stuff. They closed the internal bug on this saying it should be resolved.

Do you mind verifying with the latest version if you are still seeing the issue? I can follow up again if not. Thanks again for you patience!

bubenheimer commented 2 years ago

I've updated to latest versions (location 19.0.1) and the problem is unchanged. I tested on API 31 emulator and Samsung S21 with latest updates, which is January 1 update level for Google Play system update & security update. Play Services used to print client & service library versions to the log, but I don't see that anymore (maybe that was just Maps).

codingjeremy commented 2 years ago

Shoot, that isn't good news, thank you so much for testing again.

Let me reach out to them and point to this bug.

codingjeremy commented 2 years ago

Ok, so I have some possibly good news, the rewrite I mentioned hasn't gone live yet. That's my fault for misunderstanding that it was fixed in production.

It goes live in version 20. The engineer who wrote the rewrite checked and wasn't seeing the same issue, i.e., it should be resolved.

However, that does mean we have to wait until 20 is released. Do you mind checking it again on the release to double check? It shouldn't be too long... hopefully maybe in a month or so. I can update this thread when I see it goes out for you to check.

bubenheimer commented 2 years ago

Sounds good, I will check it once I become aware of the new release. Thanks!

codingjeremy commented 2 years ago

Just to follow up, they are still working on the releasing version 20.x.x (with fix) as FYI. Once I see it go out, I will let you know (if you don't see it first).

bubenheimer commented 2 years ago

The release is out and I've verified that the leak indeed looks fixed. It's like 7 christmases combined after this many years of ugly workarounds and much energy spent to get it resolved.

Thank you @codingjeremy for making a difference and bringing about the fix 🙏🙏🙏

codingjeremy commented 2 years ago

Awesome, I'm glad it is resolved! You beat me to it. :)