Hammerspoon: Crash - hs.socket - LuaSkin::pushLuaRef was passed a NOREF/REFNIL ref

[latenitefilms]

See: http://crashes.to/s/a5440b5090d

The problem seems to be somewhere in here:
https://github.com/Hammerspoon/hammerspoon/blob/master/extensions/socket/internal.m#L26

[asmagill]

Without a specific scenario that can reliably trigger this, it's hard to tell, but looking at the code it does appear that it could occur if lua collects the userdata before the callback is invoked...

Taking the connectCallback you reference for example (this also applies to the read and write callback code), notice that the callback is checked against LUA_NOREF at lines 75 and 83; the connectCallback function is invoked and the code which actually performs the lua callback is wrapped with mainThreadDispatch which is a macro defined in socket.h as:

~~~

define mainThreadDispatch(...) dispatch_async(dispatch_get_main_queue(), ^{ @autoreleasepool {__VA_ARGS__;} })

~~~

So, the code doesn't necessarily execute immediately... there is a tiny window in which the userdata could get cleaned up and the reference become stale.

The simplest fix is to just wrap everything within mainThreadDispatch with the standard if callbackRef != LUA_NOREF stuff (and optionally remove it from the other places as redundant, though technically leaving it in would make things the tiniest fraction of a millisecond faster when there truly is no callback function defined)

This fix should be applied to the read and write callbacks as well...

[latenitefilms]

Legend, thank you! Awesome to see you back on GitHub!

I think this has happened to me a few times when I've updated some code, and Hammerspoon/CommandPost has automatically restarted due to "configuration reloading" code, so your explanation would definitely make sense as to why it's crashing.

Are you proposing that the fix is simply something like this?

static void connectCallback(HSAsyncTcpSocket *asyncSocket) {
    mainThreadDispatch(
        if refTable != LUA_NOREF {
            LuaSkin *skin = [LuaSkin shared];
            _lua_stackguard_entry(skin.L);
            [skin pushLuaRef:refTable ref:asyncSocket.connectCallback];
            asyncSocket.connectCallback = [skin luaUnref:refTable ref:asyncSocket.connectCallback];
            [skin protectedCallAndError:@"hs.socket:connect callback" nargs:0 nresults:0];
            _lua_stackguard_exit(skin.L);
        }
    );
}

[asmagill]

In this specific case, it's asyncSocket.connectCallback that should be checked... and likewise asyncSocket.readCallback and asyncSocket.writeCallback for their sections.

And of course, you'll need the parenthesis around the test clause of the if statement, I was just typing psuedo-code above.

FWIW, refTable doesn't get reset during garbage collection, even if the entire LuaState is reset. Sometimes I think it would be nice if it did, but this is usually a static value unique to each module's internal.m file, so impossible (in theory -- it's actually possible but requires ugly code that is frowned upon) for outside code (e.g. the Lua state machine) to modify; and since it always gets reset when the module's init function is run when first loaded into a new Lua state, it's not been an issue except in my experiments with threading Hammerspoon.

[asmagill]

Re my return, I want to try and get at least a couple of nights a week in on Hammerspoon... no promises yet, there are still a lot of things up in the air at the moment for me and my family in the next few months, but I've missed digging into my Mac and making it do things Apple doesn't want me to (Bwahahaha! 😝)