asmagill / hs._asm.undocumented.touchbar

Touchbar modules for Hammerspoon
3 stars 1 forks source link

Crash - Collection <NSConcretePointerArray: 0x600003cad400> was mutated while being enumerated. #1

Closed latenitefilms closed 2 years ago

latenitefilms commented 4 years ago

@asmagill - As discussed originally here, I'm still seeing that random crash when using hs._asm.undocumented.touchbar.

Unfortunately it's not easily reproducible, and I'm not sure what's causing it. I'm not doing anything specifically with the Touch Bar when it crashes - it's just blank.

Here's what the Touch Bar looks like when it crashes:

Touch Bar Shot 2020-03-19 at 11 51 58 pm

Here's the crash log:

https://gist.github.com/latenitefilms/b4a79c04b20b3667c941c570893fb9f9

Here's our main Touch Bar code:

https://github.com/CommandPost/CommandPost/blob/develop/src/plugins/core/touchbar/manager/init.lua

Will update this issue if I come up with any ideas on how best to reproduce.

latenitefilms commented 4 years ago

@asmagill - Sadly, I'm still seeing this issues in our crash logs:

https://sentry.io/share/issue/64b5c57e23914bb6a3d53682866f67e5/

Any ideas?

latenitefilms commented 4 years ago

I'd still love to try and solve this @asmagill.

Does this Stack Overflow thread help at all? I can see enumerateObjectsUsingBlock is used quite a few times throughout the extension.

My GUESS as to what is happening is that maybe I'm triggering :templateItems() too quickly? Currently I'm basically triggering it each time you change applications (as I've got it so that each application essentially has its own Touch Bar).

Thoughts? Anything I can do to try and fault-find?

latenitefilms commented 4 years ago

It also seems to crash a lot of the time when waking my MacBook Pro from sleep - so maybe the Touch Bar takes a little bit longer than expected to "wake up"?

latenitefilms commented 4 years ago

These debug messages may also be relevant?

Screen Shot 2020-08-31 at 11 01 55 pm
asmagill commented 4 years ago

I made a couple of changes to setTemplateItems: in bar.m (https://github.com/asmagill/hs._asm.undocumented.touchbar/blob/master/bar.m#L56-L108) to move the actual setting of the template items to the end of the method after all blocks are completed, and log entry and exit from the method... this appears to be the only place in the touchbar code that utilizes enumerators and can be entered from a non-main thread, but I'm still not convinced that it's the issue, hence the debug crumbs...

I've not built an actual release with this change yet as I suspect you can download the changed file (https://github.com/asmagill/hs._asm.undocumented.touchbar/blob/master/bar.m) and make the changes to your own code base, but if you need me to build it, I can, but it will be at least another couple of hours before I have the time.

If this method does prove to be the culprit, and if the movement of the call to the super object is an insufficient fix, I have an idea for refactoring that might allow us to bypass this entire class method override, but before I put in the time, I want to make sure I'm replacing the correct code...

The other possibility is that its something to do with KVO (the references to NSKeyValue* methods), but we're not enumerating within our KVO method, so it would have to be some other part of the system thats leveraging KVO that's crashing because a block of ours is changing something while KVO is still looping through the registered watchers... and we're back to the only enumeration that is happening outside of our added lua commands -- setTemplateItems:.

At any rate, let me know how this impacts your crashes with this specific issue and if it does continue to crash, whether or not you see the new debug lines near or around the crash time.

latenitefilms commented 4 years ago

Amazing! Thank you! Will build and test out today. HUGELY appreciate all your help and support!

latenitefilms commented 4 years ago

Unfortunately, after building using the latest code in this repo, it's still crashing when waking from sleep.

Here's the Sentry.io entry: https://sentry.io/share/issue/c6b777f8bca94fe49fcd808b7499a35e/

Looking at Sentry.io, it seems to be crashing when CommandPost is in a background lifecycle state.

Unlike Fabric.io, it seems that Sentry.io sadly doesn't tell you what line of code is crashing anymore.

However, looking back at the crash log in my original issue above it does actually seem like it might be KVO related?

Screen Shot 2020-09-01 at 9 58 05 am
asmagill commented 4 years ago

Well 💩.

Ok, well, I may try doing the refactor to use the delegate's touchBar:makeItemForIdentifier: instead of overriding the instance's setTemplateItems: method anyways because I think it will be cleaner, but as I really don't think it will make a difference to this issue, not yet...

Are you using the visibility callback? If not, you could try commenting out the code in bar.m that ties us in to the touchbar's KVO handling completely... I don't see why it would affect anything, but at the moment, its the next thing to check off. I think to remove all of our ties to KVO for the module, you just need to comment out the following lines:

In the mean time, I'll see if I can pair down your usage of the touchbar linked to at https://github.com/CommandPost/CommandPost/blob/develop/src/plugins/core/touchbar/manager/init.lua and see if I can replicate the problem on my machine so I can poke at it more directly, but its going to be sometime tomorrow or Wednesday before I can carve out the time to really play with it much.

latenitefilms commented 4 years ago

Legend, thank you!

I'm not using the visibilityCallback, however, I'll try removing the code and see if that fixes things.

Failing that, my other idea is to try using hs.caffeinate.watcher to create/destroy the Touch Bar on wake/sleep and see if that has any impact.

Will keep you posted. Thanks again!

latenitefilms commented 4 years ago

Victory! Commenting out the visibilityCallback code in bar.m seems to have fixed things.

latenitefilms commented 4 years ago

Actually... I'm not entirely confident that's true. I tried reverting back to the original code, just to triple check, and it's not crashing anymore, and I haven't able to make that crash either.

I am however wondering what the purpose of this line of code is?

[super observeValueForKeyPath:keyPath ofObject:object change:change context:context] ;
latenitefilms commented 4 years ago

This might be related?

https://www.innerexception.com/2009/10/uitableviewcontroller-and.html

asmagill commented 4 years ago

As to why you've suddenly seen a drop in the crash even when reverting the code back, don't know... maybe reboot and try the reverted code again? (Yes, reboot to see if we can bring the crash BACK... never thought in my help desk days I'd ever say that!)

And in regards to https://www.innerexception.com/2009/10/uitableviewcontroller-and.html, it's a different exception in his case, but may be relevant...

To the best of my recollection, my implementation of KVO (which is used in hs.settings in core and a smattering of my "in progress" modules that haven't caused me issues... yet, at least) is based primarily on http://nshipster.com/key-value-observing/ and https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/KeyValueObserving/KeyValueObserving.html. Looking at the registration page of the latter, it should probably be checking to make sure context doesn't match our myKVOContext before invoking the method on the super class, so I'm not sure if a third source was consulted in my initial reviews or if one of my primary sources has since been updated or if I just made a change in my implementation...

And as to whether or not the super class does respond to the selector in question, NSTouchBar (and NSTouchBarItem, and thus item.m might have an issue as well) are supposed to respond to it, as they are direct descendants of NSObject, where the method is implemented... and a check with hs._asm.objc yields the following:

> o = require("hs._asm.objc")
> o.class("NSTouchBar"):respondsToSelector(o.selector("observeValueForKeyPath:ofObject:change:context:"))
true
> o.class("NSTouchBarItem"):respondsToSelector(o.selector("observeValueForKeyPath:ofObject:change:context:"))
true

If you want to try to troubleshoot further before I get a chance to implement your usage on my machine, my thoughts on what to try next are as follows:

(FWIW, you don't implement invoking the super class instance in your hs.midi module, so it might be ok in general now? Apple changes things behind the scenes all the time and if they've made the notification code smarter and haven't told us, the superclass invocation may not even be required anymore... only testing with multiple things checking the same path on multiple instances of a class and its super class, with different contexts could tell us for certain and without direct knowledge of how KVO might be used within the superclass's internal implementation -- usually not knowable by us in any case -- I'm not sure how to design, much less implement, a proper mock up for testing this.)

And maybe do similar in item.m just to be certain.

I'll try and get a similar setup running here tonight so I can also test more directly and see if we can narrow it down further.

While seeing the crashes disappear is "A Good Thing"â„¢, I'd be happier knowing what's causing it in the first place.

asmagill commented 4 years ago

I suppose we could also check that the super class responds to the observeValueForKeyPath:ofObject:change:context: selector and that the specific key can be watched with [super automaticallyNotifiesObserversForKey:key] as well before invoking the super class method, as a general "enhancement" to the KVO template, but in this specific case:

> o.class("NSTouchBar"):respondsToSelector(o.selector("observeValueForKeyPath:ofObject:change:context:"))
true
> o.class("NSTouchBar")("automaticallyNotifiesObserversForKey:", o.class("NSString")("stringWithUTF8String:", "visibility"))
true

So not sure that it would help in this specific situation...

latenitefilms commented 4 years ago

This is the code I'm currently using, and so far I haven't been able to make it crash:

- (void)observeValueForKeyPath:(NSString *)keyPath ofObject:(id)object change:(NSDictionary *)change context:(void *)context {

    [LuaSkin logDebug:[NSString stringWithFormat:@"entered %s:observeValueForKeyPath", USERDATA_TAG]] ;

    if (context == myKVOContext && [keyPath isEqualToString:@"visible"]) {
        if (_visibilityCallbackRef != LUA_NOREF) {
            LuaSkin *skin = [LuaSkin sharedWithState:NULL] ;
            lua_State *L  = [skin L] ;
            // KVO seems to be slow and may not invoke the callback until after gc during a reload
            if ([skin pushLuaRef:refTable ref:_visibilityCallbackRef] != LUA_TNIL) {
                [skin pushNSObject:self] ;
                lua_pushboolean(L, self.visible) ;
                if (![skin protectedCallAndTraceback:2 nresults:0]) {
                    [skin logError:[NSString stringWithFormat:@"%s:visibilityCallback error:%s", USERDATA_TAG, lua_tostring(L, -1)]] ;
                    lua_pop(L, 1) ;
                }
            } else {
                lua_pop(L, 1) ;
            }
        }
    } else if (context != myKVOContext) {
        [LuaSkin logDebug:[NSString stringWithFormat:@"entered %s: contexts don't match", USERDATA_TAG]] ;
        [super observeValueForKeyPath:keyPath ofObject:object change:change context:context] ;
        [LuaSkin logDebug:[NSString stringWithFormat:@"exiting %s: contexts don't match", USERDATA_TAG]] ;
    } else {
        [LuaSkin logDebug:[NSString stringWithFormat:@"%s: contexts do match", USERDATA_TAG]] ;
    }

    [LuaSkin logDebug:[NSString stringWithFormat:@"exiting %s:observeValueForKeyPath", USERDATA_TAG]] ;
}

I'll keep using this code for now, and see if it randomly crashes at any point.

asmagill commented 4 years ago

I'll update the repository to match this so we can keep in sync and finish setting up some regular touchbars for me to run as well and see how it goes.

I know you'd like to see touchbar and touchdevice eventually added to core, but because of issues like these, I want to land 0.9.79 first.

latenitefilms commented 4 years ago

Whilst we're using hs._asm.undocumented.touchdevice in CommandPost, from memory, I think all we're really using it for is to detect the Trackpad, so I haven't tested it enough to know if it's stable enough to put into Hammerspoon's core.

However, we're been using hs._asm.undocumented.touchbar for a few years now, and ignoring this particular crash, it's actually been incredibly stable and reliable. I think this crash has always been there, but it's been so hard to reproduce.

Since Fabric changed to Firebase, and now we've switched to sentry.io in our beta builds - it's a little bit confusing to get a grip on exactly how many active monthly users we have, but last time I updated our website it was sitting at 3300 - so I feel like this is a pretty good number for testing stability.

latenitefilms commented 4 years ago

Unfortunately it crashed again when I woke up my computer from sleep this morning, but the crash logs don't seem very helpful.

Here's the macOS one: https://gist.github.com/latenitefilms/4fb420559a421d80750c68d2bda68821 Here's sentry.io's one: https://sentry.io/share/issue/c6b777f8bca94fe49fcd808b7499a35e/

I'm also not seeing any of the debug breadcrumbs, so I'm not even sure this crash is related to the Touch Bar in this case.

I guess it's possible we have similar issues in other extensions?

asmagill commented 4 years ago

if you're using the hs.settings.watchKey function, it utilizes KVO, so you might try sticking similar fixes into its code... beyond that, I'll have to think some more and see what I can come up with or what I find on my machine over the next couple of days.

edit - it's should have been its... my primary school grammar teacher would be so proud...

latenitefilms commented 4 years ago

Nope, we're not using hs.settings.watchKey. I'll have a search through Hammerspoon's code, and the extensions that aren't included in Hammerspoon and see what I can uncover. Thanks heaps @asmagill !

latenitefilms commented 4 years ago

Another crash waking from sleep.

Interestingly, it seems to create two different sentry.io issues from the single crash.

Also interestingly, the only breadcrumbs seem to be a lot of:

require: hs.timer
require: hs.math
require: hs.host

...not sure why that is? I wonder if @cmsj has been experimenting much with sentry.io?

https://gist.github.com/latenitefilms/01ba7ec8925172e2882c262e06e7f710 https://sentry.io/share/issue/c6b777f8bca94fe49fcd808b7499a35e/ https://sentry.io/share/issue/ee9cce206691471b80c6aab1e37ffc92/

latenitefilms commented 4 years ago

Random thought... I was trying to think what uses hs.timer, hs.math and hs.host - and it occurred to me that hs.coroutineApplicationYield definitely does. This would explain the multiple requires in the sentry.io logs.

Side Note: We should probably tweak it in coresetup so that we only require hs.timer, hs.math and hs.host once?

However, I also wonder if it would also explain the crashes?

We're currently only using hs.coroutineApplicationYield once here:

https://github.com/CommandPost/CommandPost/blob/develop/src/plugins/core/menu/menuaction.lua#L81

This code is getting triggered from an application watcher though, so it's possible coroutines might cause issues if triggered when waking from sleep?

Thoughts @asmagill ?

asmagill commented 4 years ago

Perhaps... and the three modules you mention are used there...

hs.coroutineApplicationYield = function(delay)
    delay = delay or require"hs.math".minFloat

    local thread, isMain = coroutine.running()
    if not isMain then
        local uuid = require"hs.host".uuid()
        resumeTimers[uuid] = require"hs.timer".doAfter(delay, function()
            resumeTimers[uuid] = nil
            local status, msg = coroutine.resume(thread)
            if not status then
                hs.luaSkinLog.ef("hs.coroutineApplicationYield: %s", msg)
            end
        end)
        coroutine.yield()
    else
        error("attempt to yield from outside a coroutine", 2)
    end
end

They are pulled through a require each time, so the console doesn't get spammed, but I forget (because I have disabled it in my setup as I play around with other require wrappers on occasion) that it's still logged internally for every require command, so sticking those in local variables defined outside of the function would prevent that log spamming (though might have made it harder to zero in on this as a possible place to investigate... I'll make the change but probably add a breadcrumb log line to the function that makes it clearer when it is used since it's about to go into core and subtle issues like this may crop up in other combinations.)

The crash is still in a non-main thread, which means it's some external event (even if we set it up through a timer or such) that's triggering it, so I'll look closer at your code to see exactly what CP is doing at that time and see what I can figure out.


On another, slightly related note, I'm going to be trying to merge the outstanding pulls that we want in 0.9.79 this weekend... I still want to hold off on the touchbar and touchdevice modules, though I may see if I can encapsulate the detection code into a function that can be added maybe to hs.host or hs.mouse and worry about the rest later.

latenitefilms commented 4 years ago

FYI - it seems like sentry.io is limited to 100 breadcrumbs. It does look like there's a max_breadcrumbs variable that you can change, but I'm not exactly sure how you do it. If you searching in Hammerspoon's Xcode project for max_breadcrumbs you'll see reference to it. @cmsj might know more?

latenitefilms commented 4 years ago

I've been able to fairly reliably make my laptop crash tonight, by putting it to sleep.

Looking at one of the most recent crash logs:

https://sentry.io/share/issue/ee9cce206691471b80c6aab1e37ffc92/

...I'm still seeing NSFunctionRow, so I'm assuming the crash is Touch Bar related.

As a workaround, I've added this:

    local cafeWatcher = require "hs.caffeinate.watcher"
    mod._sleepWatcher = cafeWatcher.new(function(event)
        if event == cafeWatcher.systemWillSleep then
            log.df("sleeping...")
            mod.stop()
        elseif event == cafeWatcher.systemDidWake then
            log.df("waking...")
            if mod.enabled() then
                doAfter(0.2, function()
                    log.df("restarting touchbar")
                    mod.start()
                    mod.update()
                end)
            end
        end
    end):start()

Essentially it kills the Touch Bar on sleep, and restores it 0.2 seconds after the laptop wakes up.

So far... so good. I've been putting my laptop to sleep several times, and no crashes yet.

Based on this, my GUESS is that it's a Touch Bar related crash - but it's in Apple's code, not ours (which makes sense based on the above crash log). I'm guessing it's because bar:presentModalBar(), just always assumes the Touch Bar is available, but when waking from sleep, maybe sometimes the T2 chip is a bit slower than the Mac, and it takes a little longer to wake up, hence the crash?

I'll keep this hs.caffeinate.watcher in the code, and see if I can make it crash - but so far, with 10 sleeps/wakes - it's all working as expected.

latenitefilms commented 4 years ago

Doh! Nope - that isn't the fix - waking up my laptop this morning, it still crashes on wake.

https://sentry.io/share/issue/ee9cce206691471b80c6aab1e37ffc92/ https://sentry.io/share/issue/c6b777f8bca94fe49fcd808b7499a35e/

I temporarily disabled my code which is using coroutine, which is why you don't see all the hs.timer, hs.math and hs.host require breadcrumbs.

However, looking at sentry.io, if I'm reading it right, it still looks like it's crashing whilst in a "background" state:

Screen Shot 2020-09-05 at 9 19 48 am

Because when I wake my laptop from sleep, it goes to the login screen, where the Touch Bar isn't active - I wonder if this is related?

The hunt continues!

latenitefilms commented 4 years ago

I reached out to Andreas Hegenberg, who makes BetterTouchTool, and is probably one of the most experienced Touch Bar developers out there, and he said:

I'm overriding NSApplication's _crashOnException method 🙈 (not a good solution obviously but I haven't found a better workaround) aaa

However, I also tried increasing the delay to one second after waking from sleep, and so far, I haven't seen any crashes, even after long periods of sleep.

I added in a bunch of debugging code, but I wasn't able to work out at what point the crash was happening.

I'm going to try reducing the timer back to 0.2 and see if it brings back any crashes.

latenitefilms commented 4 years ago

Reducing the timer to 0.2. definitely crashes - it crashed both times I woke my laptop from sleep today.

I'm putting the timer back to 1 to see if it fixes the crashes.

latenitefilms commented 4 years ago

After putting the timer back to 1, it still crashes. Doh! Therefore, I don't think the workaround using hs.caffeinate.watcher to disable the Touch Bar on sleep and then re-enable it on wake will work.

I'll try Andreas's solution - although I'm not sure that will work, given we're seeing a NSGenericException and he's added a workaround for NSNotFound, but maybe I'm misunderstanding something.

latenitefilms commented 4 years ago

@asmagill - Actually... I have no idea how to actually implement Andreas's suggestion anyway, so might need your help to test that method out.

asmagill commented 4 years ago

This is only a partial method... I think what the full one should be is something like:

-(void) _crashOnException:(NSException *)exception {
    if ([exception.debugDescription rangeOfString:@"NSFunctionRow"].location != NSNotFound) {
        NSLog("_crashOnException override: %@", exception.debugDescription) ;
        return ;
    } else {
        [super _crashOnException:exception] ;
    }
}

But to implement it, I think we need to either create our own NSApplication subclass and use it instead of the default subclass (https://forums.macrumors.com/threads/overriding-nsapplication-run-method.470894/post-5315424) or "swizzle" the default implementation (https://nshipster.com/method-swizzling/).

The problem with creating an NSApplication subclass is that we would need to change every occurrence of the use of NSApp or [NSApplication sharedApplication] to [myApp sharedApplication] for this to work... and we use these in extensions which means needing to define the NSApplication subclass in such a way that it can be recognized by shared libraries, which usually requires linking them against the Hammerspoon application directly or through a Framework like LuaSkin...

The issue with "swizzling" is that it's a little tricky to do right. i've done it a couple of times with my experiments with building a threaded LuaSkin, and with the coroutine shim I wrote a while back as an interim before the coroutine fixes were added to the master repo, but since we need to actually keep the original implementation around (for the [super _crash...] line, so I'll need to give it a little thought and get back to you...

edit - forgot one of the tildes for program block... must remember to preview before submitting.

latenitefilms commented 4 years ago

I wonder if we could use a @try, @catch somewhere in the code instead? I'm guessing not as the code is triggered via KVO?

Or can we override [NSFunctionRow markActiveFunctionRowsAsDimmed:] somehow?

latenitefilms commented 4 years ago

Header files:

asmagill commented 4 years ago

Override it with what? Do you know what it's supposed to accomplish or how to determine what set of parameters/situations should be checked for to prevent it from crashing? Those are private classes that don't have a counterpart in GnuStep, so short of decompilation, which only gives general clues, I at least don't know what to override them with...

latenitefilms commented 4 years ago

I was thinking basically wrapping [NSFunctionRow markActiveFunctionRowsAsDimmed:] with a @try, @catch but I'm not even sure if that's possible?

Ok, new random idea.

I came across this article: https://indiestack.com/2016/12/touch-bar-crash-protection/

It suggests using defaults write bundleID NSFunctionBarAPIEnabled -bool NO to disable Touch Bar support on your Mac to get around some old (2016) crashes.

I wonder if hs._asm.undocumented.touchbar is conflicting with macOS's built-in Touch Bar support for each app? If so, I'm hoping by disabling Touch Bar support via NSFunctionBarAPIEnabled might resolve any conflicts?

I can confirm that having NSFunctionBarAPIEnabled set to NO doesn't break any functionality, so that's a good start.

I'll now try see if it has any impact on crashes. Fingers and toes crossed!

latenitefilms commented 4 years ago

Good news! Setting NSFunctionBarAPIEnabled to NO seems to be the fix! No crashes thus far, and I've put my machine to sleep several times after varying intervals. Will give it another day, but assuming there's no more crashes, I'll close this issue.

latenitefilms commented 4 years ago

Haha, nope, still seeing crashes. Doh!

latenitefilms commented 4 years ago

My only other idea is to go back and try making hs.caffeinate.watcher destroy hs._asm.undocumented.touchbar when going to sleep (running garbage collection as well) and see if that has any impact. Will give it a try this arvo.

Failing that, "swizzling" sounds a bit too risky, so I wonder how practical it is to sub-class NSApplication? Given all extensions will need to be updated for the next version of Hammerspoon anyway, now seems like a good to to sub-class anyway for future use? Thoughts @asmagill ?

asmagill commented 4 years ago

A quick check of the built in extensions and my external ones show that all but one use of [NSApp sharedApplication] call is within the built-in modules, so lI think we'll be OK with updating them, though I may need to add a header to the files.

If we go that route, though, I'd really rather make it a change for 0.9.80, as we already have a lot of potentially "breaking" (or at least significant) changes going into 0.9.79 and this (so far) appears to only affect one module.

Let me get through the next couple of days of merges so we have a master branch @cmsj can prep for actual release and I'll start looking at the swizzling approach -- it's actually not that hard if approached correctly, and since we'd only be swizzling a method that isn't actually invoked until the App is proceeding to generate a crashing exception anyways, we don't have to worry about another part of the application being caught of guard during the switch -- it can be made part of the touchbar module itself so it's not loaded in until and unless actually required.

latenitefilms commented 4 years ago

Legend, thanks @asmagill - HUGELY appreciate all your help and support.

Apologies for all the posts in this thread - it's mainly just me making notes so I can keep track of things. Please don't feel any pressure or obligation to reply.

Let me know if there's anything I can do to help/test with all the stuff you're doing in 0.9.79.

asmagill commented 4 years ago

And the notes let me know what's already been tried, so don't feel bad about adding them.

I admit that the touchbar hasn't usually been a priority for me, after the initial joy of figuring out a way to create a virtual one, since I don't have a touchbar mac, but mine is showing its age (it's a 2014 model), so I'll likely be getting a new laptop sometime in the next year or so when I can justify the price, and it will be nice to start really using it then with all of the bugs already worked out (fingers crossed!)

asmagill commented 4 years ago

Ok, I've updated bar.m so that it swizzles _crashOnException with our modified version. I can confirm that it is in fact doing the swizzling, but it hasn't crashed on my machine yet, so I can't confirm if it addresses your issue or not. Figure the best test is to put it in your hands. You can use the precompiled version if you like (the one with "swizzle" in the version name) or just replace the bar.m file in your local sources (that's all that has really changed -- the change to the Makefile was concerning yet a new warning Apple/clang has foisted upon us...)

I'm a little uncertain about line 104 in bar.m (https://github.com/asmagill/hs._asm.undocumented.touchbar/blob/master/bar.m#L104) because it uses the built in LuaSkin logger to log a message (I figure we're more likely to see that it's working if something appears there)... I'm hoping that it works fine but because we're deep in the macOS internals at the time and haven't wound ourselves back out yet, I'm not 100% positive that Lua code can be invoked at that juncture... because the class level loggers use dispatch_async, I think it should be OK, but it's possible this may cause a different crash then the one you're seeing... if you still get a crash, but the NSLog line appears in the Console app (or in your crash logs), let me know and I'll remove it...

latenitefilms commented 4 years ago

You're an absolute legend, thank you! I'll download now and see what explodes.

latenitefilms commented 4 years ago

So far I haven't seen any crashes, but I also haven't seen anything matching any:_crashOnException in Console.app, apart from the initial:

BREADCRUMB: (DEBUG) _crashOnException and xxx_hammerspoon_crashOnException swizzled in NSApplication

Previously, it would crash pretty reliably when waking from sleep, so I'm wondering if Console.app actually updates when sleeping/waking?

I'm also not seeing anything in the Hammerspoon/CommandPost Console, but I think that must be because the hs.logger instance for the Touch Bar isn't set to "debug".

I'll leave it to sleep overnight, and see what happens in the morning when I wake it up. If it's still working fine, I might try hs.crash.crash() to see if I can spot anything in the Sentry.io entries.

latenitefilms commented 4 years ago

It didn't crash in the morning, and hasn't crashed at all today, which is great news - but I'm still not seeing any proof in the Console.app, so I triggered hs.crash.crash() to see if I can spot any _crashOnException breadcrumbs in the Sentry.io logs, but can't see anything there either. So it's either working, and just not writing the debug messages, or whatever the secret formula is to cause the crash hasn't been triggered yet.

I'll keep an eye on this over the coming days, and see what happens.

latenitefilms commented 4 years ago

Doh! Still crashing:

https://sentry.io/share/issue/ee9cce206691471b80c6aab1e37ffc92/ https://sentry.io/share/issue/c6b777f8bca94fe49fcd808b7499a35e/

Re-looking at Andreas's suggestion, it seems to be for a different issue anyway, as I doubt *** Collection <NSConcretePointerArray: 0x6000025694a0> was mutated while being enumerated. would be prevented from his workaround anyway.

latenitefilms commented 4 years ago

Ok, so I'm still seeing a lot of these crashes across a more users now, so I really want to try and come up with a fix.

https://sentry.io/share/issue/c6b777f8bca94fe49fcd808b7499a35e/ https://sentry.io/share/issue/b7a65e1ebf1c44068c148f911e08a263/ https://sentry.io/share/issue/64b5c57e23914bb6a3d53682866f67e5/

The only use of NSMutableArray in this extension is for identifiers, which can be found here - but I can't see any obvious issues here.

If you're not seeing the issues on your machine, I'm ASSUMING it may be an issue in our Lua code, as we do use a LOT of hs.timer's and application/window/ax watchers - so maybe we're forcing the Touch Bar to update too often or something?

I'll try and tweak some things in our Lua code and see what happens.

latenitefilms commented 2 years ago

Closing in favour of:

https://github.com/asmagill/hs._asm.undocumented.touchbar/issues/5