Reinitialize NSRunningApplication at EVENT_HANDLER_APPLICATION_LAUNCHED

[donaldguy]

As per #923 (comment) this appears to resolve #920 crashes for me

(all the ones I have experienced so far anyway, there may be more like it)

I'm not asserting this is the correct way to handle this, but its a workaround at least

(Also my literal test case is as xorpse#5 )

[donaldguy]

I tried a more aggressive version where I also deleted

yabai/src/process_manager.c

Line 37 in 8777db4

process->ns_application = workspace_application_create_running_ns_application(process);

but that did segfault elsewhere (though I don't actually notice anything that is/should be looking at process->ns_application prior to EVENT_CALLBACK(EVENT_HANDLER_APPLICATION_LAUNCHED )

[koekeishiya]

I don't get why this would be an issue at all. I'd suggest waiting for a stable Monterey release before doing upstream changes, particularly in cases like these where official APIs suddenly break. Of course there could be some obscure bug in yabai, but I find that unlikely as this code has been running fine for me ever since High Sierra was the latest.

[donaldguy]

So the crash this is squashing is related to the arm64e pointer authentication (https://developer.apple.com/documentation/security/preparing_your_app_to_work_with_pointer_authentication ) - which is being built with on xorpse's fork (and I am using the kernel boot arg to opt into supporting; I admit I am not fully aware why this is desirable – though I note that e.g. the contents of /bin on both Monterey and Big Sur are all x86_64/arm64e universal, not merely x86_64/arm64);

In my original linked comment I was definitely thinking about this in terms of being an initialization issue – like that the NSRunningApplicaiton wasn't valid until later in the launching/reaction lifecycle; but now I'm thinking its probably something more to do with a liability of storing information in C++ structs (as void *s) outside the ObjC runtime's domain

The problem is either:

1. Merely that ObjC doesn't trust the pointer next time it sees them (EDIT: cause it considers them to belong to another thread? cause it considers them a dead reference?): If it is in that way a ~safety violation, but not a correctness problem, its fair that it doesn't belong upstream - or rather what (eventually) belongs upstream is probably something I don't the most understand yet, but talked about in the link above (ptrauth.h - "the ptrauth_strip macro"; to ... remove the signatures from the pointers? / opts out of this functionality?)

And/or (and would be that something like (2) is true but that the "opt out" changes compiler behavior such that it won't be)

2. That the pointer is legit invalid by the time its accessed. My thought in this case would be about where the result of NSRunningApplication runningApplicationWithProcessIdentifier is being allocated.

That this patch works makes me wonder if its maybe its now allocating it directly on the stack of the caller of workspace_application_create_running_ns_application (or maybe in an allocation managed by the ObjC runtime with only that guaranteed lifetime)

If this is the case, it could be the case that either the ObjC behavior has changed (and again, might change back with use of ptrauth macros?)

or it could be if fact that this was ~always theoretically/potentially incorrect—that we are referring with this pointer to memory that happens to still contain the object we care about, but technically is no longer allocated for it; memory (on the heap?) that would be overwritten if enough other NSObjects et al to get allocated? (but that in practice yabai's ObjC surface area is small enough that would never / has never happened?)

---

I don't have ~any objC / Swift / Apple runtime experience – so its totally possible this reasoning is misapplications of understandings from C++ years ago and more recent playing in Rust – or that the ObjC runtime gives some FFI guarantees I'm not aware of that make the void * carry safe ; but it looks risky to me

EDIT: if it is (2) I would think also that maybe the thing we want is something akin either to a rust Pin or a rust Arc - that we ask nicely in workspace_application_create_running_ns_application for the reference in C++ to remain known / un-GC-able to the ObjC side of things (until we do something to reverse the process in workspace_application_destroy_running_ns_application - like very possibly the [application release] thats there )

[donaldguy]

Continuing the above edit, I wonder if what's missing is just a call to [retain] (https://developer.apple.com/documentation/objectivec/1418956-nsobject/1571946-retain) in workspace_application_create_running_ns_application

(either cause that "should" have always been there, or cause something has changed about reference counting under return from a function)

I'll give that a shot

[koekeishiya]

That sounds correct, yes. I wish there was a way to completely disable autoreleasepools, but I haven't found a way to do so.

…

Sent from my iPhone On 28 Jul 2021, at 23:29, Donald Guy ***@***.***> wrote:  Continuing the above edit, I wonder if what's missing is just a call to [retain] (https://developer.apple.com/documentation/objectivec/1418956-nsobject/1571946-retain) in workspace_application_create_running_ns_application (either cause that "should" have always been there, or cause something has changed about reference counting under return from a function) I'll give that a shot — You are receiving this because you commented. Reply to this email directly, view it on GitHub<#967 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABPDZVYL3XC3OACXH6JUDHLT2BZEDANCNFSM5A4RAGJQ>.

[donaldguy]

you:

but I find that unlikely as this code has been running fine for me ever since High Sierra was the latest.

me:

(either cause that "should" have always been there, or cause something has changed about reference counting under return from a function)

For the record, this code re: allocation in its current form dates only to #543 8c9bbbe (which is a pretty short "always" for me to refer to, one when I'm pretty sure we lived in fact in Catalina times; but also already COVID-19 times, so what is/was time anyway)

prior to that it looks like you did a more similar-to-this allocation of the handle uniquely in each callback — beside which you immediately added an observer, which maybe effected the way the lifetime of the object was handled

[donaldguy]

Closing in favor of #973

[donaldguy]

That sounds correct, yes. I wish there was a way to completely disable autoreleasepools, but I haven't found a way to do so.
…

https://clang.llvm.org/docs/AutomaticReferenceCounting.html#general does offer an -fno-objc-arc but maybe that its a bigger hammer than you are asking for, or is incompatible with CoreFoundation or something