fix(android): avoid send event when has no listener

[HatCloud]

Overview

This commit just make sure that module will not read MAC info before add listener for it.
Everytime createConnectivityEventMap method called when send event, it will get wifiinfo.
It's illegal in China when app read MAC info before user agree the privacy agreement.

Test Plan

[mikehardy]

Interesting - thanks for posting this!

Let me start by saying I think I understand the use case. Similar to advertising restrictions like GDPR in Europe, you must not read the MAC address prior to the user agreeing to something. Stated differently: you need to be in control of when this library starts attempting to read any network information.

If I understand that correctly, then I do not think this change is necessary but I am not certain. If I understand the code control flow of the javascript correctly, this library will not add listeners to native side or fetch state until you call configure, useNetInfo, fetch or addListener, will it? On the native side the AndroidManifest does not register for any broadcast events

However: it looks to me like when the native module loads in NetInfoModule the constructor creates the Receiver objects and during initialize() they register for network events, and then call the method you have modified.

My question is: when is the native module loaded? Is it dynamic and only loaded if the javascript side first references it? Or is that native module loaded (triggering even registration and a connectivity map update) when the app boots no matter what?

This is important because I think the module might be doing a few things wrong:

• it should not even register for broadcasts at all if no one is listening so if native module loads no matter what, android system event registration should be removed from native module construction sequence
• it appears to be adding a native event listener even if none of the state listeners or other function callbacks are added

Basically, your change may work but it seems like a subtle cut-out of the MAC reading, and a future iteration of privacy protection may make it illegal to simply register for listeners before consent, or a future code change here could read MAC prior to sending connectivity change event since it is not explicit why it was being avoided

Basically, this module was built before grabbing network information was considered in any way a privacy concern.

I think it would be better to re-think how it works and make configuration, starting and stopping of the plugin completely explicit, more like it is with GPS services (for example this API) https://github.com/transistorsoft/react-native-background-geolocation#large_blue_diamond-example (with a mandatory ready(<config>) for configuration that prepares things but still does not do a single thing, the a start() and stop() which control all registration on native side and sending.

We may be able do this in a non-semver-major / non-breaking way by implementing it as those new APIs, and having the existing config/fetch/useNetInfo APIs emit a strong warning about privacy then call a new ready()/start() under the covers.

What do you think?

Please understand I know that if this blocks you from releasing your app because it cannot achieve approval this is a show-stopper for you. I highly recommend using patch-package https://github.com/ds300/patch-package until we get something merged for you, so you are not blocked.

[ouabing]

Thanks @mikehardy. I think you are right that we need explicit APIs to init/destroy this lib. But I do think sending event when there's no listener is also something need to optimize. For now we will use patch-package as a workaround, would love to follow the progress of the change you mensioned.

[mikehardy]

I think that's a little bit of a misunderstanding of how it will go. I'm just a volunteer on the internet, and I don't personally care about this use case in the sense that I need it for any work projects. So I'm not going to personally touch the code. That's how open source works - those that need it do it when they need it, and share it, and they later get to take advantage of others doing the same. I assume that's why you're here, because the module is useful from so many people making effort prior.

If you want changes here, it's your turn to make the effort, in other words. I'm here to collaborate with folks and keep the project moving forward by keeping the API reasonably stable and making releases as needed.

I had a really important question for even your smaller change, and I need the answer prior to merging this at all:

If I understand that correctly, then I do not think this change is necessary but I am not certain. If I understand the code control flow of the javascript correctly, this library will not add listeners to native side or fetch state until you call configure, useNetInfo, fetch or addListener, will it? On the native side the AndroidManifest does not register for any broadcast events

However: it looks to me like when the native module loads in NetInfoModule the constructor creates the Receiver objects and during initialize() they register for network events, and then call the method you have modified.

My question is: when is the native module loaded? Is it dynamic and only loaded if the javascript side first references it? Or is that native module loaded (triggering even registration and a connectivity map update) when the app boots no matter what?

---

The follow on of a new API that the module completely quiet until something was asked of it would be follow-on work I'd be happy to merge but again, it's not a use case for my work projects right now, so without a champion taking up the cause, there will be no progress - just a design sketch with no work behind it until someone needs it

[ouabing]

Oh @mikehardy sorry for my poor reading and my rush reply. I just misunderstood that you were explaining this commit and rethinking about the API design.

What I found so far is native module's initialize will be called even after removing all the code references 'react-native-netinfo'. I think the native modules are initialized here once CatalystInstance is initialized, which happens before any user actions like agreeing to the privacy policy.

@mikehardy 's suggestion about converting to explicit APIs is a good one to solve this problem but maybe a little bit overwhelming, maybe we can start another proposal thread later to discuss about it, to see if there are enough people need this change. This is what I mean that I want to follow the progress of this potential change.

On the iOS side, I found RNCNetInfo has been optimized to avoid sending events without registered listeners . So I think this PR is still reasonable to at least make things more consistent on both platforms, and avoid calling methods unnecessarily on Android.

Thank you.

[mikehardy]

Okay, thanks a lot for testing my main question:

My question is: when is the native module loaded? Is it dynamic and only loaded if the javascript side first references it? Or is that native module loaded (triggering even registration and a connectivity map update) when the app boots no matter what?

What I found so far is native module's initialize will be called even after removing all the code references 'react-native-netinfo'. I think the native modules are initialized here once CatalystInstance is initialized, which happens before any user actions like agreeing to the privacy policy.

That's the critical result, and it means that we must make at least a minimal change now in order to handle these use cases.

This:

On the iOS side, I found RNCNetInfo has been optimized to avoid sending events without registered listeners . So I think this PR is still reasonable to at least make things more consistent on both platforms, and avoid calling methods unnecessarily on Android.

...is also great additional information and provides great support for the minimal / temporary solution you've proposed

I agree that an API rework is out of scope for this, then. We need at at least a minimal fix now and this contains a minimal fix. No need to slow it down with a larger idea.

I re-reviewed this and I'm really sad to see there is no general infrastructure in the Java native modules for managing observer count. The counts are maintained in javascript on the other side of the bridge. The same is only partially true for the iOS side - your link there combined with a re-review of the react-native core code solved the mystery I had of why #487 caused #492 and needed a revert in #493 - we partially override the superclass listener counting infrastructure there but without managing observer status. We need something on the iOS side more like what you've implemented here.

That is also out of scope for this PR, but as I document these things, then I have the ability to make new issues from them, thus the typing effort ;-).

I'll merge this and get this released. Thanks for the privacy-conscious improvement here @HatCloud I appreciate it.

[mikehardy]

Oh wait - Just realized if you don't also override method onCatalystInstanceDestroy to reset listener state, this won't work across fast-refresh or bundle reloads (like with react-native-code-push). in addition to needing onCatalystInstanceDestroy (which should just toggle hasListener to false I think, for now) in that implementation please note that after react-native 0.65 is the minimum supported version we need to override invalidate (onCatalystInstanceDestroy is deprecated and on the way out, but invalide is only called as of react-native 0.65 I think?)

[HatCloud]

Oh wait - Just realized if you don't also override method onCatalystInstanceDestroy to reset listener state, this won't work across fast-refresh or bundle reloads (like with react-native-code-push). in addition to needing onCatalystInstanceDestroy (which should just toggle hasListener to false I think, for now) in that implementation please note that after react-native 0.65 is the minimum supported version we need to override invalidate (onCatalystInstanceDestroy is deprecated and on the way out, but invalide is only called as of react-native 0.65 I think?)

new commit pushed for this advice

[mikehardy]

Nice! This looks good to me - I can't think of anything else at the moment.
It is subtle to get these things right, thanks for working through it with me

[matt-oakes]

🎉 This PR is included in version 7.1.6 🎉

The release is available on:

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

Your semantic-release bot 📦🚀

[Legion2]

It looks to me that the removeListeners implementation is wrong

[Legion2]

I think count is the number of listeners to remove, so we need to track the number of listener and if the number of listeners is 0 then hasListener should be false

[mikehardy]

nice catch! I peeled it to a new issue for tracking