urbanairship / react-native-airship

Airship React Native module
Other
88 stars 62 forks source link

Add Interaction Manager fix for iOS events not firing when transition… #590

Closed syntacticsolutions closed 2 months ago

syntacticsolutions commented 2 months ago

…ing from inactive to foreground

What do these changes do?

When an iOS app is inactive and coming to the foreground it temporarily will drop function calls from the call stack if there is too much happening. In some instances many things are being initialized and the call to dispatchEvents gets dropped because of iOS performance mechanisms. This code change ensures that the events are always dispatched and handled because the code only runs when the JavaScript is ready.

Why are these changes necessary?

The changes are necessary here because it seems like the simplest way to fix the issue.

How did you verify these changes?

We tested it in a react-native project.

Verification Screenshots:

Anything else a reviewer should know?

rlepinski commented 2 months ago

"When an iOS app is inactive and coming to the foreground it temporarily will drop function calls from the call stack if there is too much happening."

Any docs on this behavior anywhere? Would like to know what type of things they drop

rlepinski commented 2 months ago

I assume we probably want to wrap these calls as well? https://github.com/urbanairship/react-native-airship/blob/main/src/AirshipInApp.ts#L16 https://github.com/urbanairship/react-native-airship/blob/main/src/AirshipPush.ts#L115 https://github.com/urbanairship/react-native-airship/blob/main/src/AirshipPush.ts#L230

syntacticsolutions commented 2 months ago

I assume we probably want to wrap these calls as well? https://github.com/urbanairship/react-native-airship/blob/main/src/AirshipInApp.ts#L16 https://github.com/urbanairship/react-native-airship/blob/main/src/AirshipPush.ts#L115 https://github.com/urbanairship/react-native-airship/blob/main/src/AirshipPush.ts#L230

I'm not sure but if they get triggered when the app is being foregrounded from inactive state then possibly.

syntacticsolutions commented 2 months ago

"When an iOS app is inactive and coming to the foreground it temporarily will drop function calls from the call stack if there is too much happening."

Any docs on this behavior anywhere? Would like to know what type of things they drop

https://reactnative.dev/docs/javascript-environment This shows that iOS uses JavaScriptCore which is different than V8. It contains an API for pausing the execution of JavaScript.

I didn't find any direct references for the JavaScript being paused but it seems that the Apple Documentation asks developers to free up memory when they receive warnings.

https://developer.apple.com/documentation/uikit/app_and_environment/managing_your_app_s_life_cycle/responding_to_memory_warnings

That might include pausing the execution of JavaScript in JavaScriptCore.

It seems that this is possible within JavaScriptCore so I would imagine it's being used.

https://stackoverflow.com/questions/21750816/is-there-any-way-stopping-or-pausing-the-execution-of-javascript-in-a-jscontext

rlepinski commented 2 months ago

I assume we probably want to wrap these calls as well? https://github.com/urbanairship/react-native-airship/blob/main/src/AirshipInApp.ts#L16 https://github.com/urbanairship/react-native-airship/blob/main/src/AirshipPush.ts#L115 https://github.com/urbanairship/react-native-airship/blob/main/src/AirshipPush.ts#L230

I'm not sure but if they get triggered when the app is being foregrounded from inactive state then possibly.

All of those are being subscribed during init as a backchannel to our plugin

rlepinski commented 2 months ago

I do not think method calls are getting lost, pausing is not going to lose instructions. I think whats happening is its just delaying that call slightly and that is making it work for you. Any chance you could jump on a call with me and we can remote debug the current version to see whats going on? I don't want to add this to the plugin and find out it only fixes it for you part of the time

syntacticsolutions commented 2 months ago

I do not think method calls are getting lost, pausing is not going to lose instructions. I think whats happening is its just delaying that call slightly and that is making it work for you. Any chance you could jump on a call with me and we can remote debug the current version to see whats going on? I don't want to add this to the plugin and find out it only fixes it for you part of the time

I'm pretty sure it does because If it's not adding instructions to the JavaScript heap to conserve memory then it would stop execution. We even printed the logs and the listener never receives an event unless we add this code. Probably because even though the Native Layer is pushing events. That JavaScript is not ready to receive them because it's not adding function calls to the stack because it's paused.

rlepinski commented 2 months ago

@syntacticsolutions Are any events firing or just not push received/response? What if you added the listener before calling takeOff?

I just want to make sure its not some other race condition happening to avoid possibility of this not fixing it in all scenarios

syntacticsolutions commented 2 months ago

@syntacticsolutions Are any events firing or just not push received/response? What if you added the listener before calling takeOff?

I just want to make sure its not some other race condition happening to avoid possibility of this not fixing it in all scenarios

I think we figured out a better way to do it. If we add the listeners before calling takeoff it breaks Airship without any errors. Airship just stops receiving events on the RN layer completely.

We're testing the changes right now

rlepinski commented 2 months ago

If we add the listeners before calling takeoff it breaks Airship without any errors. Airship just stops receiving events on the RN layer completely.

The listener being added should not have anything to do with airship being ready or not. You should be able to add the listeners anytime in the app.

So far I have been unable to reproduce with new and old architecture (but did find a compile issue with new, ill post a PR). Ill provide the home screen in the support ticket to avoid sharing any code you provided.

I am seeing a possible race condition though with startObserving and stopObserving in the plugin if they are called too close to each other. The stopObserving is being called whenever I reload the context. I can try to fix that real fast if you want to see if that resolves some of those issues?

syntacticsolutions commented 2 months ago

If we add the listeners before calling takeoff it breaks Airship without any errors. Airship just stops receiving events on the RN layer completely.

The listener being added should not have anything to do with airship being ready or not. You should be able to add the listeners anytime in the app.

So far I have been unable to reproduce with new and old architecture (but did find a compile issue with new, ill post a PR). Ill provide the home screen in the support ticket to avoid sharing any code you provided.

I am seeing a possible race condition though with startObserving and stopObserving in the plugin if they are called too close to each other. The stopObserving is being called whenever I reload the context. I can try to fix that real fast if you want to see if that resolves some of those issues?

It might fix the addListener call before init but I don't think it will fix the issue of init being called when the app is in inactive state.

syntacticsolutions commented 2 months ago

@rlepinski I figured out a way to use react-native's AppState in order to register the listener when it's not in inactive state, but then you also mentioned the other places where listeners are being registered during init so I'm wondering if I just use AppState to wait to call Airship.takeOff or if it's better to update the documentation to wait to call Airship.takeOff until the app is in the active state.

EDIT: It looks like we assign those modules before even calling takeoff so it wouldn't matter if we waited to call takeoff. So either we do it in all three places, make a util that makes it easier to do in all three places, or wait to assign them in the constructor. I'll push the changes so you can tell me what you think.

rlepinski commented 2 months ago

https://github.com/urbanairship/react-native-airship/pull/591

The first time you call takeOff it will cache the config and apply it as soon as the app is initialized on the next run. If you try to change the config, it wont be applied til next run. So it really doesn't matter where you are calling takeOff and it should have no effect on subscribing to event listening. We also store any events until we have a listener to accept them, so where you are registering for the event listener also does not matter, except for pushReceived which you probably want to register outside of a view in order to process it in the background.

I would suggest you call takeOff in the root of your App.tsx file outside of a useEffect and assume its ready everywhere else in your app. That was the intended use so you do not have to worry about the state of Airship. For the example that we include, I usually just call it around here - https://github.com/urbanairship/react-native-airship/blob/main/example/src/App.tsx#L16

rlepinski commented 2 months ago

@syntacticsolutions if you could give https://github.com/urbanairship/react-native-airship/pull/591 a try to see if that resolves the issue?

syntacticsolutions commented 2 months ago

591

The first time you call takeOff it will cache the config and apply it as soon as the app is initialized on the next run. If you try to change the config, it wont be applied til next run. So it really doesn't matter where you are calling takeOff and it should have no effect on subscribing to event listening. We also store any events until we have a listener to accept them, so where you are registering for the event listener also does not matter, except for pushReceived which you probably want to register outside of a view in order to process it in the background.

I would suggest you call takeOff in the root of your App.tsx file outside of a useEffect and assume its ready everywhere else in your app. That was the intended use so you do not have to worry about the state of Airship. For the example that we include, I usually just call it around here - https://github.com/urbanairship/react-native-airship/blob/main/example/src/App.tsx#L16

We tried that already but we have a lot of things happening when the app is initialized from closed state which causes it to go into inactive mode and when it's in inactive mode (According to the documentation). iOS does not receive events and it will just drop them.

The example app would work in a small app but we have a very large app with many things happening at launch.

rlepinski commented 2 months ago

go into inactive mode and when it's in inactive mode (According to the documentation). iOS does not receive events and it will just drop them.

That is not the correct documentation to be pulling from. That is saying not receiving user input events. This happens if for instance you pull down the notification shade while the app is up. It has nothing to do with being able to process events from JS.

syntacticsolutions commented 2 months ago

go into inactive mode and when it's in inactive mode (According to the documentation). iOS does not receive events and it will just drop them.

That is not the correct documentation to be pulling from. That is saying not receiving user input events. This happens if for instance you pull down the notification shade while the app is up. It has nothing to do with being able to process events from JS.

According to other parts of the documentation there are other events that don't get processed as well. The documentation says to do minimal work while the app is in the inactive state. Our thought is that it could be a combination of both things because even when the events were fired and we tried to set state it would not pass the state to the state setter.

rlepinski commented 2 months ago

https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/AppState/AppState.js

AppState uses a NativeEventEmitter as well, so its effectively replacing the call you believe is being dropped with an identical call that should be dropped if your theory is correct. I dont think this is the root cause.

rlepinski commented 2 months ago

After looking at the logs your provided, I think it might be an issue with a conflict with another plugin, possibly https://github.com/invertase/react-native-firebase

I see this from our SDK logs that you provided:

Adding implementation for application:didRegisterForRemoteNotificationsWithDeviceToken: class GUL_mobile.AppDelegate
...

And this from another log you provided:

Adding implementation for application:didRegisterForRemoteNotificationsWithDeviceToken: class mobile.AppDelegate
...

On iOS, all push events come to either the app delegate or a notification center delegate. Airship, Firebase, and a few others use method swizzling to intercept the methods. Usually we do swizzling up front, but on the first launch its when takeOff is called, so the first launch order will be different then subsequent launches. The name changes, indicating we are getting different swizzle orders

First could you add this to the root of your app (outside of useEffect) so we can see if events are working at all?

Airship.addListener(EventType.ChannelCreated, (event) => {
  console.log('ChannelCreated:', JSON.stringify(event));
});

Airship.addListener(EventType.PushNotificationStatusChangedStatus, (event) => {
  console.log('PushNotificationStatusChangedStatus:', JSON.stringify(event));
});

Could you then provide verbose logs showing the event not working (not just first render, send push and tap it), and again when it is working?

syntacticsolutions commented 2 months ago

@rlepinski I updated the code to match the final code changes from our project.

rlepinski commented 2 months ago

@syntacticsolutions So the root issue is Airship not being compatible with an app that has multiple bridge being setup. Each bridge will load its own copy of the plugins, but the Airship module assumes a single bridge and sets the event listener on a singleton when things are being observed from the JS side:

-(void)startObserving {
    __weak RTNAirship *weakSelf = self;

    [AirshipReactNative.shared setNotifier:^(NSString *name, NSDictionary<NSString *,id> *body) {
        [weakSelf sendEventWithName:name body:body];
    }];
}

This basically makes it the Airship plugin only use the last bridge that was set on. Not only that, when you refresh the context, the bridge order changes.

I set up two bridges, and when I first loaded Airship would set the listener for the second bridge (the one I believe you want). After a refresh, the order swapped, and Airship will only notify events to the first bridge. Refreshing further still resulted in the same bridge order. I believe on your first run you have a context refresh that is happening causing the bridge order to be swapped.

The second bridge is coming from the creation of this view RCTRootView with the bundle URL. All the JS code, plugin running, etc.. is all being loaded in the bridge so the view or window setup does not matter that much, just the bridge. I believe your second bridge was an accident as is not being used and instead you should reuse the bridge that is being created by the React App Delegate when creating the RCTRootView. This should fix the issues you are seeing without any plugin changes. You can access the bridge with self.bridge in the app delegate.

The changes you have now work because you delay when the listener is being added on the Airship side to only the bridge that is calling takeOff, so startObserving will only be called on that bridge. After removing the second bridge, you should be able to use the normal plugin without any changes.

I am going to look into changes the way our plugin notifies events to be multi bridge compatible if that becomes a real use case or the norm.