Fix/420 iOS Memory Leak

[alburdette619]

Overview

See #420. My project was also getting reports of OOM errors on Sentry & profiling I found the same leak reports using Instruments on CNCopyCurrentNetworkInfo through RNCNetinfo. This was caused because my app did not fulfill one of the requirements for using CNCopyCurrentNetworkInfo which are mentioned in the README but only under the description of the SSID prop which the majority of users probably aren't reading & even if they are a link to an Apple docs page will probably not help most RN devs at all.

Note: If this is approved or whatever the final solution is here I'll update docs before merge.

This fix handles what is probably the most common requirement that any RN app would for CNCopyCurrentNetworkInfo which is to ensure location permissions are approved by the user. For our project this fixed the leak entirely.

Issues wit this approach:

• Doesn't include the three other, admittedly much more edge case, ways to supposedly satisfy CNCopyCurrentNetworkInfo. See the Discussion section here
• If an app using this solution is on WIFI on initial startup and approves location permissions, there is no mechanism for the callback to respond & refresh the data to get the SSID. An app restart or network state change is required.
• CNCopyCurrentNetworkInfo is deprecated for iOS 13+ and this the methods that use CNCopyCurrentNetworkInfo should be overridden based on iOS version to use fetchCurrentWithCompletionHandler.

Test Plan

1. I profiled my app on initial install/startup with Instruments Leaks tool to get the same results from Memory leak on iOS #420 where it can be easily seen that CNCopyCurrentNetworkInfo & RNCNetinfo are the source of the leaks.

2. Did tons of research to attempt to figure this out. I didn't read enough on the Apple docs page as most probably don't 😅
3. Finally continued using my app, gave location permissions, & continued to see that I couldn't repro?!
4. More research & uninstall/re-test confirmed and then I finally read enough on the docs page & searched this library to find it in the SSID description.
5. With this change, replication is no longer possible as the SSID will not be retrieved without location permissions being approved.

Note: I cannot seem to repro this at all on the example app but no clue why currently.

[alburdette619]

Another note, after following the Contibutor guide all tests passed except I could not get the e2e test to run. The build would always fail with Implicit declaration of function 'CACurrentMediaTime' is invalid in C99 🤷

[mikehardy]

Lots of new symbols used, I checked availability:
https://developer.apple.com/documentation/corelocation/clauthorizationstatus/kclauthorizationstatusauthorizedalways - since iOS8 (and similar for catalyst etc)
https://developer.apple.com/documentation/corelocation/clauthorizationstatus/kclauthorizationstatusauthorizedwheninuse - similar, iOS8+
CLLocationManager.locationServicesEnabled - ios4+ (no problem there!)

https://developer.apple.com/documentation/corelocation/cllocationmanager/1423523-authorizationstatus?language=objc - oh no, deprecated in iOS14.

It appears the non-deprecated method is to create a CLLocationManager, and implement locationManagerDidChangeAuthorization, with that DidChange method documented to be called when you create a CLLocationManager with the instance, that you can then check for authorization status. That's all new as of iOS14 meaning that since we support down to iOS9 if I recall correctly, we must have your implementation here and the new "are we on ios14+?" implementation as well (which may imply maintaining some local state as well, always hard to get correct), which is really messy.

I think leaving a comment that the deprecation is noted but is new enough and difficult enough to fix that it's left for future is okay for now. Keeps this fix focused.

The compile problem you mentioned is unexpected though. Need to figure out if that is specific to your environment, and/or introduced with these changes or was pre-existing

[alburdette619]

Lots of new symbols used, I checked availability...

Making sure I understand.

• We're all good on compatibility? Haven't had to deal with such a broad array of platforms before.
• TODO: I need to make a comment about the depreciation described above & otherwise the code is good?
• TODO: Check to see if I missed an install step or something, etc to see if I can get e2e to run. I checked it on the master branch & got the same so definitely could be me.

[mikehardy]

Yep - nailed it I think. Just some more detail in case useful:

• We're all good on compatibility? Haven't had to deal with such a broad array of platforms before.

Correct - and yes, luckily the change is just iOS native, on react-native modules that do windows, web, macos, ios and android it gets really painful if you change all platforms or change javascript because you have to re-test all

• TODO: I need to make a comment about the depreciation described above & otherwise the code is good?

Yep - just looking for minimum path towards targeted fix being merged and released. Felt like a comment would do it here

• TODO: Check to see if I missed an install step or something, etc to see if I can get e2e to run. I checked it on the master branch & got the same so definitely could be me.

Yeah - this one is the "who knows what it could take...". If we have github actions that do the compile + test here (and I think we do?) then isolation of difference between github actions virtual runner env and your local env is one strategy?

[alburdette619]

Done

Looking at the codebase, there is no run of either of the e2e scripts in package.json in the GH actions or anywhere else that I see so unless it's done on a Semantic CI script I cannot see it could just be broken. I thought maybe there were special pods for macOS I didn't install or maybe a gem lock, but looks like I'm good. Also use node 14 like the CI so really not sure.

[mikehardy]

LGTM - thanks!

[mikehardy]

If it's never running in our github actions, that's that - there are no secret execution hideouts ;-). Sounds like a separate issue then. Still bears investigation but shouldn't block this longer than a timebox I will give it to try to pull your fork's branch and test integrate it in a project I'm already doing same on for an fbsdk-next release. Give me one day for that timebox and I assume the result will be successful and I'll merge which then triggers a semantic release

[mikehardy]

Did a fresh init of a react-native-macos project then installed your branch and copied over the example, it all works fine ios/android and macos. So nothing regressed except now because the example app does not have location permission, the keys are simply missing on ios12- instead of the previous dummy values.

But, more worrying, for people that do meet the criteria for the non-location edge cases, this will break them.

On balance, fixing OOM crashes for the normal case (no location permission) that most apps have, vs breaking the edge cases seems like a reasonable tradeoff but it's still pretty painful. I'm trying to think of how to fix OOM while still allowing the other edge cases a simple recourse at the javascript layer

What if instead of having the native code conditional check for permission, you added a boolean configuration element "shouldFetchSSID" here https://github.com/react-native-netinfo/react-native-netinfo#configure - then instead of having the module try to get all the cases right, we defer to the client, with a default of false to avoid the OOM, but a very easy way for consumers that meet whatever the criteria is now + in the future to turn it on. Then there's no worries about responding to permission-changed callbacks or whatever, people can do that using other libraries made for that sort of thing then just chain a netinfo.configure call afterwards

Then you'd have OOM protection without truly breaking people - they'd have to maybe change API but they wouldn't then be forced to send in PRs themselves.

What do you think?

[alburdette619]

Yes that actually does sound really good if I understand you. I think that's the best of both worlds and really alleviates some of the issues that this approach has. If you want to un-approve, I can put some time towards this tomorrow I believe.

[mikehardy]

unapproving per discussion of using a config element vs partial implementation of all the (fairly complicated) conditions where SSID/BSSID is actually available (and thus non-leaking)

[alburdette619]

@mikehardy I'm not seeing a good way to get the config or State or what have you in native code. We could modify the getCurrentState exported method, but that wouldn't cover the listener call to currentDictionaryFromUpdateState. Maybe there's something I'm missing...?

[mikehardy]

Sorry for the really long delay on this one - I'm spread pretty thinly across a lot of repos and they all seem to have something big going on right now like a typescript rewrite or something 😅, I really appreciate your patience.

I looked and I see what you mean, I thought configuration was plumbed through to native but it does not appear to be, it appears that it is only javascript layer right now.

The way to get it through would be to take the existing configure method in index.ts, and add a call to this.native.configure for Platform === ios, and pass down the configuration object, then add a new RCT_EXPORT_METHOD in RNCNetInfo.m that consumed it as (as a ReadableMap I believe), looked for that key, and then stored it in a way that it was generally available to the module. then it could be accessed by the method that's leaking to only conditionally attempt to get the wifi info if it was configured on

Depending on your comfort with implementing native methods in react-native this is either a really big scope expansion or trivial ;-) - the method itself is easy to add but doing this for the first time was painful if my memory serves. Let me know what you think - if it sounds daunting and me pushing to your branch doesn't bother you I could try to set up just the config plumbing from JS->Obj-C so that configure was hooked up and it logged the value of the proposed configuration element, then from there I think it's back into the area you were handling things great and have the actual use case where you see this

[alburdette619]

@mikehardy no worries, I am also just trying to keep the tab open and check occasionally. When I have some availability I'll get on this. Thanks!

[alburdette619]

There we go, easy peasy. LMK if the web changes, naming, etc look good. but seems to be what we want.

[mikehardy]

Again apologies for the delay, just made it back to the netinfo repo for a sweep here. Your continued patience is really appreciated
This does look great, really nice work on all the things that matter, as far as I can tell.

I did the irritating reviewer thing of proposing some wording changes in the comments but of course that's easy so everyone does that ;-) (https://en.wiktionary.org/wiki/bikeshedding). I have no intention to waste time going back and forth on the trivia though, so please feel free to accept or reject or alter etc in one final pass then whatever your opinion is on the suggestions (accept or reject) I'll merge and this will ship. Thanks again!

[alburdette619]

Oh, haha sorry if you're still in the middle of this, I'm just taking all suggestions on wording. No worries on time again.

[mikehardy]

Nope - I was all done :-), let's go! 🚀

[matt-oakes]

🎉 This PR is included in version 7.1.12 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[cristianoccazinsp]

Hello, just double checking. In order to upgrade to 7.1.12 we need to use shouldFetchWiFiSSID if we were previously getting SSID info? Shouldn't this be a breaking change (and therefore a higher version bump)?

[mikehardy]

@cristianoccazinsp i think you're right on reflection. a very small percentage of library users will have this permission at this point, but breaking is breaking and non-zero is sufficient. Easiest fix is probably to update the changelog with a note on current version that there is an inadvertent break (on my part, I'll note that) for this specific group of people, and label the commit in that PR with that update as breaking then run the release to bump to next major version, with the same note about exactly who is effected etc. I'll do the janitor thing and clean it up - thanks for noting it

For most users at least (since the requirements from Apple to get SSID now appear to be pretty tight, by my read) this is just a fix though, they will no longer leak memory

[cristianoccazinsp]

@mikehardy a note in the changelog should suffice :)

[cristianoccazinsp]

Also, I couldn't fully understand whether or not using shouldFetchWiFiSSID when the user has rejected location access will still continue to cause a memleak. Here's the problem, I could set shouldFetchWiFiSSID: true since I know I'm asking for permission, but there's a chance the permission is removed. Does that mean I will need to constantly update shouldFetchWiFiSSID based on location changes? What happens if I keep it true and location permission is removed? @alburdette619

[mikehardy]

We are still allowing for this to be a "foot gun" with the way the PR is implemented. The logic is that we can either try to determine all the cases where the app may have permission to get the SSID (and inevitably either get them wrong from the start despite really complicated code, or have Apple change the rules and get them wrong via entropy / decay) or we can say "hey, the app developers know best, let them control it".

So yes, the app will have to monitor it's own use cases and set the config as appropriate, managing it over time.

That's a bit of a pain, but it's better than having the code be wrong from the start deep in native code (if we tried to get it perfect and track Apple policy change) or crash by default. Can still crash (via memory leak) if used wrong but at that point it is an opt-in crash

[alburdette619]

And as far as implementation, I would think on grant and on startup of the app would be enough to make the configuration setting work well. I will have to implement this myself and I doubt the cases where the user is on wifi, has granted permissions, and removes them without closing the app will be much of an issue. If they do, it's a manual app restart or system kill in the background from the leak but after that the configuration setting will be set correctly again on next startup.

[mikehardy]

#574 for changelog notice and rationale for preferring to just issue a new major (TL;DR: version numbers are free, no reason not to issue one even for just 1 user if behavior changes)