macOS memory leak fix

[kerneljake]

This patch fixes a memory leak in macOS Squeezeplay induced by redrawing the rectangles of scrolling text in the Now Playing applet. To reproduce the symptom, set the screensaver to Now Playing with album and track names whose lengths exceed the width of the screen and require scrolling. Let Squeezeplay run for awhile, and you will see the memory usage creep up in Activity Monitor every time the text scrolls. Left running for too long, the app will eventually consume gigabytes of virtual memory.

This doesn’t happen with the Linux build, which helped me narrow it down to Quartz. The root cause is outlined in this stackoverflow article. The leak is avoided by forcing redraws to happen in the main thread.

You can examine the memory usage with the following steps.

# first, add -g to CFLAGS in the Makefiles for Squeezeplay and SDL-1.2 in order to get line number references
make

# start Squeezeplay with memory trace logging, and let it run for awhile
MallocStackLogging=YES  ../build/osx/SqueezePlay.app/Contents/MacOS/SqueezePlay

# extract a memory graph from the running process with the macOS leaks tool
leaks <pid> -outputGraph foo.memgraph

# generate a summary report from the memory graph
malloc_history foo.memgraph -allBySize > /tmp/foo

# pick a line with a lot of repeated mallocs, and generate a trace from it
head -55 /tmp/foo | tail -1 | perl -ne '@lines = split(/[|]/); print join("\n", @lines);'

This will output a trace like the following.

[...]
 0x10feb7c87 (com.logitech.squeezeplay) -[SDLMain applicationDidFinishLaunching:]  SDLMain.m:283
 0x10feb46c7 (com.logitech.squeezeplay) SDL_main  jive.c:562
 0x10ff0e26e (com.logitech.squeezeplay) lua_cpcall
 0x10ff1537a (com.logitech.squeezeplay) luaD_pcall
 0x10ff13e15 (com.logitech.squeezeplay) luaD_rawrunprotected
 0x10ff0e34f (com.logitech.squeezeplay) f_Ccall
 0x10ff14f39 (com.logitech.squeezeplay) luaD_call
 0x10ff14764 (com.logitech.squeezeplay) luaD_precall
 0x10feb4863 (com.logitech.squeezeplay) pmain  jive.c:533
 0x10feb527a (com.logitech.squeezeplay) handle_script  jive.c:433
 0x10feb5410 (com.logitech.squeezeplay) docall  jive.c:396
 0x10ff0e18a (com.logitech.squeezeplay) lua_pcall
 0x10ff1537a (com.logitech.squeezeplay) luaD_pcall
 0x10ff13e15 (com.logitech.squeezeplay) luaD_rawrunprotected
 0x10ff0e1ff (com.logitech.squeezeplay) f_call
 0x10ff14f39 (com.logitech.squeezeplay) luaD_call
 0x10ff14764 (com.logitech.squeezeplay) luaD_precall
 0x10ff38b35 (com.logitech.squeezeplay) ll_require
 0x10ff0e095 (com.logitech.squeezeplay) lua_call
 0x10ff14f50 (com.logitech.squeezeplay) luaD_call
 0x10ff2877d (com.logitech.squeezeplay) luaV_execute
 0x10ff14764 (com.logitech.squeezeplay) luaD_precall
 0x11014fc17 (com.logitech.squeezeplay) jiveL_update_screen  jive_framework.c:0
 0x11015d339 (com.logitech.squeezeplay) jive_surface_flip  jive_surface.c:1270
 0x10fef6ff9 (com.logitech.squeezeplay) SDL_Flip  SDL_video.c:1153
 0x10fef69a6 (com.logitech.squeezeplay) SDL_UpdateRect  SDL_video.c:1036
 0x10fef6d66 (com.logitech.squeezeplay) SDL_UpdateRects  SDL_video.c:1099
 0x10ff021cb (com.logitech.squeezeplay) QZ_UpdateRects  SDL_QuartzVideo.m:1567
 0x7fff29b7e101 (com.apple.AppKit) -[NSView setNeedsDisplay:]
 0x7fff29b7e244 (com.apple.AppKit) -[NSView setNeedsDisplayInRect:]
 0x7fff29b7e4cc (com.apple.AppKit) NSViewSetNeedsDisplayInRect
 0x7fff29c1d763 (com.apple.AppKit) -[NSWindow _setNeedsDisplayInRect:]
 0x7fff29c1db42 (com.apple.AppKit) -[NSDisplayCycleObserver setNeedsDisplay:]
 0x7fff29bb9de7 (com.apple.AppKit) +[NSDisplayCycle currentDisplayCycle]
 0x7fff29bb9fd9 (com.apple.AppKit) -[NSDisplayCycle init]
 0x7fff2e6e98ce (com.apple.Foundation) NSAllocateObject
 0x7fff5393acea (libobjc.A.dylib) class_createInstance
 0x7fff54711612 (libsystem_malloc.dylib) calloc
 0x7fff54710d24 (libsystem_malloc.dylib) malloc_zone_calloc

Tested on 10.13.6 x86_64 and 13.6 arm64

[kerneljake]

Part 2: You can still induce a memory leak (albeit at a slower rate) by cycling album artwork. Queue up a Random Mix and then cycle through the album art of each song by clicking the Next button, and you'll see the memory increase when the artwork is updated. There are two other places in SDL_QuartzVideo.m where the rectangles are redrawn, and when I patch them all, I can't see any more leaks.

[kerneljake]

Ouch... it looks like this messes up the window when you hide/unhide. It stays messed up until you click on a widget or try to resize the window. Is this the "white on white" problem described in d3b88cffb ? More investigation needed.

[ralph-irving]

This is awesome! I've been aware of the memory leaks in the mac version, it's been that way for a very long time, but I was never able to isolate the root cause.

[kerneljake]

Part 3

Two problems surfaced when moving the Quartz window refresh to the main thread.

1. Minimize/un-minimize on older macOS versions does not always clear the alpha mask, so the window retains a fuchsia hue when un-minimized. That alpha mask was probably used for very old OS X versions in order to make the dock’s genie effect work correctly. It is not needed any more, and removing it eliminates the problem.
2. Startup on newer macOS versions shows an empty grey screen which remains unchanged until you resize the window. What is supposed to happen is the splash.png screen comes first, then a black screen, then the application loads its widgets. After a trip through Mordor, I discovered that you must pump the event loop in order for macOS to update the initial screen in the main thread. This heralds the triumphant return of the “Free your music” splash screen on older macOS versions. On newer macOS, the grey screen refreshes to the black screen, and then the application loads properly. I cannot figure out how make splash.png display properly on newer macOS. The fact it’s the same code running on newer Quartz makes me think that something in Quartz changed, and the SDL-1.2 code cannot handle it. My opinion is that a brief grey screen at startup is a small price to pay to get rid of a major memory leak.

I was hoping this patch would remain 100% in the macOS specific code, but I touched initSDL() in jive_framework.c. Calling SDL_PumpEvents() should be harmless, but it means that other OSes will need to be tested.

I see the memory usage increase when new cover art is loaded in Now Playing; however, now it seems to cap out with an upper bound instead of increasing forever.

There are two unrelated fixes contained here for things I found along the way:

1. compiler warning for delegate type incompatibility in SDL_QuartzVideo.m
2. missing memset in jive_surface.c. It’s been running for years without it, but memset() after calloc() is the right thing to do.

Test cases:

• startup
• hide/unhide
• minimize/un-minimize with scale effect
• minimize/un-minimize with genie effect
• cycle through album art in Now Playing using Random Mix and “next” button

Tested on:

• 10.12.6 (x86_64)
• 10.13.6 (x86_64)
• 13.6 (arm64)
• 14.1.1 (x86_64)

[ralph-irving]

After some research, I've discovered that rather than putting the call to SDL_PumpEvents in process_events, the jive framework provides a facility to call it on a per platform basis. I've attached a patch against the current PR#19 to implement it. I've found it works as expected on both intel and m1 macs. Can you try it out and confirm? I'd like to get these changes into the source tree soon. 19a.patch.txt Thanks.

[kerneljake]

I confirm it works on both chipsets. What advantage does calling it on a per-platform basis yield -- i.e. does Linux now need its own handler, too?

[ralph-irving]

None of the other platforms require the call. So isolating it to macos just removes the chance of the change breaking one of the other supported OS types. Thanks for confirming the change works for you too.

[kerneljake]

Was 19a.patch.txt supposed to be merged as well?

[ralph-irving]

Thanks for catching this. I missed the changes to platform_osx.c from 19a.patch.