quicksilver / Quicksilver

Quicksilver Project Source
http://qsapp.com
Apache License 2.0
2.72k stars 283 forks source link

Fix for issue #2919: App specific triggers sometimes do not work right after starting the app. #2921

Closed dhoepfl closed 10 months ago

dhoepfl commented 1 year ago

Fixed: Fast succession of app launch event and active app switch event could result in missed active app switch.

The Carbon Process Events for both app launch and active app switch might be delivered before the notification for the launch event is handled. Fixed this by adding the information for newly started processes to the cache right when the event is received.

pjrobertson commented 1 year ago

Thanks @dhoepfl for the PR! I've left one small comment on the code, if you can take a look and comment, that'd be great!

pjrobertson commented 1 year ago

@dhoepfl - can you take a look at this branch and see if it fixes the issue for you?

https://github.com/quicksilver/Quicksilver/tree/processes-race

Thanks!

dhoepfl commented 1 year ago

can you take a look at this branch and see if it fixes the issue for you?

Yes but does not solve it.

Output of my logging breakpoints (all in QSProcessMonitor, prefixed by line number):

76: CB: appLaunched.
299: N: appLaunched
364: reloadProcesses
371: _reloadProcesses: before QSGCDMainSync
381: _reloadProcesses: after QSGCDMainSync
58: CB: appChanged.
67: dict is nil
311: N: appChanged
393: _reloadProcesses: update processes
2022-08-19 21:56:25.445040+0200 Quicksilver[18753:2736643] Reload time: 149.361968 ms

Note that _reloadProcesses updates the processes dictionary after the appChanged event is handled. This is the reason dict is nil in line 67, which in turn prevents the triggers for the frontmost app to become active.

(If you want to make sure this happens, just add a 1s delay to _reloadProcesses)

dhoepfl commented 1 year ago
2022-08-19 21:56:25.445040+0200 Quicksilver[18753:2736643] Reload time: 149.361968 ms

If you wonder what takes so long to reload the list of processes: [NSBundle bundleWithIdentifier:ident] is quite slow. Would it be possible to use [NSBundle bundleWithPath:appPath] instead? That‘s way faster.

dhoepfl commented 1 year ago

What’s the state of this pull request? Is it worth to resolve the conflicts?

pjrobertson commented 10 months ago

Hey @dhoepfl - sorry this message comes a year late. I have just started experiencing this issue myself with launching the Digital Colour Meter. I tried your branch, but it also didn't work.

It seems like a cross-thread memory issue: processes getting updated on one thread/CPU register but not being properly read on another thread. I have tried everything from OSMemoryBarrier(); to atomic ivars to volatile marks, to separate dispatch_queue.

Very strangely, the only thing that actually works is adding an NSLog(); in NDProcess.m.

There's a built version here: https://github.com/quicksilver/Quicksilver/actions/runs/5910807363

You'll need to right click->open a couple of times since it's not signed.

pjrobertson commented 10 months ago

Update: I believe the issue is with the underlying GetNextProcess() which doesn't really seem to be properly thread safe (although the docs say it is) - see here. I have completely removed NDProcess and replaced it with the newer NSRunningApplication.

Please test and confirm if it sovles the issue for you. I will close this PR now as it did not solve the problem for myself.

Sidenote: I have an intel CPU, and think the issue with GetNextProcess() is related to specific hardware. Yuck

dhoepfl commented 10 months ago

Thanks for giving it another try.

Your build works for me (Last Intel MBP 16")

I, too, cannot explain why the NSLog helps (maybe calling [theNextNDProcess name] does some lazy loading?).

pjrobertson commented 10 months ago

Great, thanks for the comments! I will merge into main and try to get a new release out :)