We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 952

Summary: Memory leak in QZ_UpdateRects()
Product: SDL Reporter: Shigekazu Hukui <shigerello_sf>
Component: videoAssignee: Sam Lantinga <slouken>
Status: RESOLVED FIXED QA Contact: Sam Lantinga <slouken>
Severity: major    
Priority: P2 CC: icculus, maarten, rectalogic, shigerello_sf
Version: 1.2.14   
Hardware: All   
OS: Mac OS X (All)   

Description Shigekazu Hukui 2010-02-15 12:07:48 UTC
There is a memory leak in QZ_UpdateRects() (SDL_QuartzVideo.m).

>CGContextRef cgc = (CGContextRef)
>		[[NSGraphicsContext graphicsContextWithWindow: qz_window]
>		 graphicsPort];

"[NSGraphicsContext graphicsContextWithWindow: qz_window]" returns a memory-allocated, one-time-purpose NSGraphicsContext*. But, the returned value never be freed.



Even though it is provisional, but I fixed the problem. The debugged QZ_UpdateRects() is below:


static void QZ_UpdateRects (_THIS, int numRects, SDL_Rect *rects)
{
    if (SDL_VideoSurface->flags & SDL_OPENGLBLIT) {
        QZ_GL_SwapBuffers (this);
    }
    else if ( [ qz_window isMiniaturized ] ) {
		
        /* Do nothing if miniaturized */
    }
    
    else {
        NSGraphicsContext* nsgc = [NSGraphicsContext graphicsContextWithWindow: qz_window];
        CGContextRef cgc = (CGContextRef) [nsgc graphicsPort];
        QZ_DrawResizeIcon (this);
        CGContextFlush (cg_context);
        CGImageRef image = CGBitmapContextCreateImage (cg_context);
        CGRect rectangle = CGRectMake (0,0,[window_view frame].size.width,[window_view frame].size.height);
        
        CGContextDrawImage (cgc, rectangle, image);
        CGImageRelease(image);
        CGContextFlush (cgc);
        CGContextRelease (cgc);
        free(nsgc);
    }
}
Comment 1 Ryan C. Gordon 2010-02-16 07:15:35 UTC
I'm pretty sure you can't use free() on an NSGraphicsContext, even if it happens to work.

Can you try changing that free() call to...

  [nsgc release];

...and see if the memory leak is still present?

(Also, I'm not sure we should be releasing (cgc) either...the docs don't say we should, fwiw.)

--ryan.
Comment 2 Ryan C. Gordon 2010-02-16 07:20:38 UTC
While I'm thinking about it, maybe we should be doing "CGContextRef cgc = [[NSGraphicsContext currentContext] graphicsPort];" and not allocating anything at all.

--ryan.
Comment 3 Shigekazu Hukui 2010-02-17 05:28:05 UTC
I replaced free(nsgc) with [nsgc release]. It caused "double free" runtime error. It seems CGContextRelease(cgc) and [nsgc release] don't get along.

While searching through sample codes of Xcode's Developer Document, I noticed that, within those sample codes, the returned value of "- (void *)graphicsPort" is never released by CGContextRelease().
As for [NSGraphicsContext currentContext], maybe we can try [qz_window graphicsContext] for acquiring NSGraphicsContext (and it doesn't allocate memory for that).



The following code worked, and I checked there is no excessive object allocation or memory leak involved here using a performance utility.
"- (NSGraphicsContext *)graphicsContext" is available since Mac OSX v10.4. Is that okay for SDL policy for Mac OSX supported version?

static void QZ_UpdateRects (_THIS, int numRects, SDL_Rect *rects)
{
	if (SDL_VideoSurface->flags & SDL_OPENGLBLIT) {
		QZ_GL_SwapBuffers (this);
	}
	else if ( [ qz_window isMiniaturized ] ) {
		
		/* Do nothing if miniaturized */
	}
	else {
		CGContextRef cgc = (CGContextRef) [[qz_window graphicsContext] graphicsPort];
		QZ_DrawResizeIcon (this);
		CGContextFlush (cg_context);
		CGImageRef image = CGBitmapContextCreateImage (cg_context);
		CGRect rectangle = CGRectMake (0,0,[window_view frame].size.width,[window_view frame].size.height);
		
		CGContextDrawImage (cgc, rectangle, image);
		CGImageRelease(image);
		CGContextFlush (cgc);
	}
}
Comment 4 Ryan C. Gordon 2010-02-17 06:51:50 UTC
> The following code worked, and I checked there is no excessive object
> allocation or memory leak involved here using a performance utility.
> "- (NSGraphicsContext *)graphicsContext" is available since Mac OSX v10.4. Is
> that okay for SDL policy for Mac OSX supported version?

For 1.3, that should be fine, I think. For 1.2, we should probably use the 10.0-supported "[[NSGraphicsContext currentContext] graphicsPort]" since we only have one window (and thus one current context) anyhow.

--ryan.
Comment 5 Shigekazu Hukui 2010-02-18 13:11:22 UTC
As for "[[NSGraphicsContext currentContext] graphicsPort]", my test program with "+ (void)setCurrentContext:(NSGraphicsContext *)context" invoked outside of SDL library at very early stage of the program disrupted SDL graphics mechanism.
But when I sandwiched "setCurrentContext" with "saveCurrentState" and "restoreCurrentState" in my test program, SDL graphics mechanism just worked well. NSGraphicsContext things are very OpenGL-like, a state machine.


It seems all (or most of) sample codes from Apple developer document using "setCurrentContext" are careful enough to use this save & restore mechanism. But none can predict what will some developer do with "setCurrentContext," anyway this save & restore mechanism is not a "must-do".
Probably in SDL developer document, we should warn SDL-Cocoa developers to use "saveCurrentState" and "restoreCurrentState" if they intend to use "setCurrentContext" alongside SDL library.
Comment 6 Maarten ter Huurne 2010-04-01 03:12:50 UTC
According to the naming conventions for Cocoa, only methods named "alloc*", "new*" or "*Copy" return objects that must be freed by the caller. For other methods, either the object does not have to be freed, or the method itself already called autorelease on it.

http://developer.apple.com/mac/library/documentation/Cocoa/Conceptual/MemoryMgmt/MemoryMgmt.html

I checked the documentation of "NSGraphicsContext graphicsContextWithWindow:" and it does not list any exception to the convention, so I think it is not necessary to free the object it returns.
Comment 7 Andrew Wason 2010-10-27 10:55:51 UTC
NSGraphicsContext graphicsContextWithWindow: returns an autoreleased object, and the object returned from graphicsPort will be valid as long as the NSGraphicsContext has not been released (i.e. until the next NSAutoreleasePool drain).

Currently in 1.2.14, the CGContextRelease (cgc) causes crashes and memory corruption when the application using SDL drains its NSAutoreleasePool.

Using the Zombies XCode Instrument you can see the double free when the NSAutoreleasePool is released, the NSGraphicsContext attempts to release its internal CGContextRef which has already been released by SDL.

First the NSGraphicsContext is allocated autoreleased in QZ_UpdateRects:

   0 CoreFoundation _CFRuntimeCreateInstance
   1 CoreGraphics CGTypeCreateInstanceWithAllocator
   2 CoreGraphics CGContextCreateWithDelegate
   3 CoreGraphics CGWindowContextCreate
   4 AppKit -[NSWindowGraphicsContext _initWithWindowNumber:scaleFactor:]
   5 AppKit +[NSGraphicsContext graphicsContextWithWindow:]
   6 libSDL-1.2.0.dylib QZ_UpdateRects
   7 libSDL-1.2.0.dylib SDL_DisplayYUV_SW
   8 libSDL-1.2.0.dylib SDL_DisplayYUVOverlay
   [...]

Then QZ_UpdateRects calls CGContextRelease:

   0 CoreFoundation _CFRelease
   1 libSDL-1.2.0.dylib QZ_UpdateRects
   2 libSDL-1.2.0.dylib SDL_DisplayYUV_SW
   3 libSDL-1.2.0.dylib SDL_DisplayYUVOverlay
   [...]

Then when the application releases its NSAutoreleasePool, NSGraphicsContext attempts to release the already released CGContextRef:

   0 CoreFoundation ___forwarding___
   1 CoreFoundation _CF_forwarding_prep_0
   2 AppKit -[NSCGSContext _invalidate]
   3 AppKit -[NSCGSContext dealloc]
   4 AppKit -[NSWindowGraphicsContext dealloc]
   5 CoreFoundation _CFAutoreleasePoolPop
   6 Foundation -[NSAutoreleasePool release]
   [...]


So the CGContextRelease (cgc); needs to be removed from QZ_UpdateRects.
Comment 8 Ryan C. Gordon 2011-08-23 13:52:38 UTC
Resolving bug: we got rid of the bogus CGReleaseContext() call in hg changeset 80ae1ac3bdc9, and we stopped allocating the memory at all in hg changeset 8e0dd46ad0e0.

--ryan.