Fix reloading when using useAnimatedKeyboard

[graszka22]

Summary

Fixes #3786

Why it crashes:

On the main thread CADisplayLink calls updateKeyboardFrame 60 times per second. Calling updateKeyboardFrame calls listener on the JS side. When reloading the runtime gets destroyed on the JavaScript thread. So when those two things happen at the same time (which in this case happens often) we get the crash that we're trying to call a js function on destroyed js runtime.

Why don't you just remove the listeners in NativeReanimatedModule::~NativeReanimatedModule(), we're cleaning up everything here:

I've tried and it appeared to work at first but I still got crashes when running the 1000 listeners example and probably that's why:

So when ~NativeReanimatedModule is being called the runtime related stuff is already deleted and there is a short window of time that the runtime is being deleted and we still call js stuff, or at least that's my theory 🤷‍♀️

So my proposition for now is to listen for RCTBridgeDidInvalidateModulesNotification notification. It's called just before deleting happens. Also I'm using RCTUnsafeExecuteOnMainQueueSync because I want to wait until the listeners are completely deleted on the main thread and then delete js stuff.

A nicer solution?

If you look a bit above [[NSNotificationCenter defaultCenter] postNotificationName:RCTBridgeDidInvalidateModulesNotification line in React code you'll see that React calls invalidate on all modules' data before even posting the notification. Maybe we should clean reanimated stuff here. I haven't explored that though yet and I don't know where is reanimated's module data and what is module data at all and where to put that invalidate function, I just got this idea while posting this PR.

Test plan

Just run AnimatedKeyboardExample and reload the app.
Also the the 1000 listeners example nicely catches other data races.

Tested with changes from #3911 and it works
Also tested on FabricExample and surprisingly it also works.

[tomekzaw]

Using this function may lead to deadlocks in some cases, can't we just run this block asynchronously?

[jwajgelt]

You don't want listeners to be run after clearListeners has been called, that's the root of the issue here.
If you call it asynchronously, after scheduling this block the js runtime will be cleaned up, and if the listeners run before this block is executed, you get a crash.
That said, there may be better way of doing this, like having an atomic boolean invalidated which stops the listeners from being called - might be worth trying out.

[graszka22]

I was thinking of other ways to do this.
The problem with boolean is that at some point before calling the listeners I we have to check if the boolean is set. Something like this:

if(!invalidated) {
   for listener in listeners
      call listener
}

and now what if we invalidate js runtime while calling the listeners? It is still going to crash.
We can try checking the boolean before calling every listener like this:

for listener in listeners
   if(!invalidated)
      call listener

I didn't test it but I think there is still a couple of instructions between checking the boolean and actually calling a listener so theoretically it can still crash.

I think there is no way around the fact that js thread has to wait somehow for the main thread to finish calling listeners.

[jwajgelt]

The check would have to somehow be done on the JS thread, this way you won't be able to invalidate the runtime while running the listener, since operations on the same thread won't be interleaved. That said, if these listeners run on the main thread, why do they use the JS thread runtime?
Or am I misunderstanding and the UI thread runtime is destroyed on the JS thread for some reason?

[graszka22]

According to the xcode debugger it is destroyed on the JS thread. You can even see it on the second screenshot.
Or is it the js runtime destroyed... 🤔

[graszka22]

So the UI thread runtime is destroyed on the JS thread and called on the main thread and we rather can't change that. So I have no other idea than synchronising threads like that.

[jwajgelt]

I'd say, the current approach is ok.
I don't think we expect the reload to come from the main thread, so there shouldn't be any deadlocks here.

Actually, does this listener actually run on the JS thread? I'm not sure where NSNotificationCenter listeners run.

[jwajgelt]

"Regular notification centers deliver notifications on the thread in which the notification was posted. [...] At times, you may require notifications to be delivered on a particular thread that is determined by you instead of the notification center. [...] In these cases, you must capture the notifications as they are delivered on the default thread and redirect them to the appropriate thread."

So I guess they are run from JS thread then 👍.

[jwajgelt]

Works on my machine 👍