Stability idea

[cmsj]

I've been looking through crashes in Sentry and it seems like a lot of them crash deep inside Lua after entering via a C callback.

I'm wondering if this is LuaSkin lifecycle related - eg we're reloading and C callbacks get triggered while we're in a weird state.

I have an idea for how to confirm this hypothesis - @asmagill do you see any issues with setting each module's refTable to REFNIL in the module-level __gc function? If I'm right, that would turn all of the crash reports into pushRef exceptions, and we can guard them all.

[asmagill]

No objections. Not all modules have the meta_gc defined, so expect to have to do so in at least some cases.

Will this make a difference with modules that are loaded by the initial init.lua? Even with clearing refTable, this will be reset as soon as the module is loaded again, even if the callback is in fact a holdover.

[cmsj]

Huh, that's a good point, we may actually be using a refTable that doesn't match the lua_State currently considered prime.

Any ideas for dealing with this more intelligently?

[cmsj]

This all suggests that having the bare refTable variables was a mistake.

@asmagill how about this:

• instead of returning a Lua reference, the LuaSkin registerFoo methods store the refTable in a dictionary with the key being a UUID, and they return the UUID
• modules now have to ask LuaSkin for the refTable value by passing it the UUID
• any mismatch between the two is immediately visible and we can bail out.

The patch for implementing that would be pretty substantial, which makes this hypothesis quite expensive to test, but in theory we'd still be making ourselves more robust even if this doesn't shake out some of the mysterious Lua crashes.

Thoughts?

[asmagill]

The problem I can for-see with this is that independent blocks refer to NSString * and const char * by pointer... wouldn't that mean that changing the value when loading the module after a reload would change the value seen within the independent blocks?

We'd need to test it, but I fear we'd need to change every callback block so that it made a local copy of refTable and that would be... annoying.

What if we kept it an integer, but made it a random number over the entire int range and then dereferenced that against a lookup table within LuaSkin to get the actual regitry index?

Unless all variables within blocks are accessed by reference... is there any definitive documentation on this or do we need to craft some tests?

[cmsj]

You make a good point about pointers working against us here.

I'm pretty sure this is only a problem for situations where C callbacks are jumping into Lua, and they are all conveniently easy to find thanks to the _lua_stackguard_entry() macro, so I'll have a think about how we can, for those modules only, store a copy of the refTable value in a way that won't get neatly rewritten when LuaSkin resets.

Unfortunately there are about 120 of those entrypoints, but the more I think about this, the more sure I am that this is a weakness in our current architecture that's worth addressing.

[cmsj]

So the two of these classes of crash that occur most often are hs.timer and hs.eventtap, both of which create an object which the OS passes to the callbacks with all the extra info they need.

My only real thought here is that we create an NSUUID in LuaSkin which gets refreshed each time Lua is destroyed and recreated, then we take a copy of the UUIDString property and store it in the timer/eventtap callback objects, so now that's a completely separate piece of memory which isn't pointing at anything in LuaSkin or the host module. Then when the callback is fired, we can fetch the current LuaSkin state and compare its UUID to the one we previously saved, and know to just safely do nothing.

So this is looking like switching refTable types to something NSUUID related isn't actually going to buy us anything, leaving the question of whether it's worth landing the "opaque refTable type" patch or not. I kinda think it is, just in case we want to change the type later, and because it doesn't break our ABI. Any objections to that @asmagill ?

[cmsj]

#2699 is a first run at how we might tackle this. My one reservation with this approach is that it's possibly not going to work out for all C->Lua callback sites that use references, because there's no guarantee that all of them will give us the ability to have some of our data passed back to us, but I think mostly they do, so it may be good enough.