Patch for #7170716, crash in OSXAPP_SetApplicationDelegate

[Mike Swingler]

You don't need to retain blocks when adding them to a collection. The collection implicitly retains them. The blocks will be released once the collection is released.

The root problem may be that the delegate is getting released, but I'd need to review the surrounding code more carefully to determine that.

The other design issue is that access to the queue is going through a raw pointer, whereas as property would be a safer alternative.

Regards,
Mike Swingler
Apple Inc.

On Jun 11, 2012, at 9:17 AM, Marco Dinacci wrote:

> Hi,
> 
> I raised a couple of weeks ago bug #7170716 :
> http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7170716
> 
> The original discussion is here:
> http://mail.openjdk.java.net/pipermail/macosx-port-dev/2012-May/004226.html
> 
> In short, every time a Java application is opened from a registered
> file the JVM crash in OSXAPP_SetApplicationDelegate.
> 
> Today I started looking for a solution and I've found two memory
> management issues in the code:
> 
> The first and the one that was causing the bug is related with blocks.
> In QueuingApplicationDelegate.m there are many methods where you're
> adding blocks to an NSMutableArray but without copying them.
> Those blocks literals are created on the stack so even if they're
> retained once the function exits they no longer exist. Calling [copy]
> will assure that the block will have a reference on the heap. Then
> once the block has been consumed it needs to be released.
> 
> The second issue is that you don't retain the delegate object in
> QueuingApplicationDelegate.m and in NSApplicationAWT.m.
> Even if the pointers are static you should still call [retain] because
> it's not guaranteed that the objects the pointers are pointing to
> won't be deallocated at one point.
> 
> Here below a patch that resolve both issues, I tested it successfully
> with a trivial test case and with my application.
> 
> Best,
> Marco
> 
> diff -r 96dd3a52e76c src/macosx/native/sun/osxapp/NSApplicationAWT.m
> --- a/src/macosx/native/sun/osxapp/NSApplicationAWT.m	Thu May 31
> 16:35:25 2012 +0100
> +++ b/src/macosx/native/sun/osxapp/NSApplicationAWT.m	Mon Jun 11
> 16:49:04 2012 +0100
> @@ -334,17 +334,17 @@ AWT_ASSERT_APPKIT_THREAD;
> }
> 
> @end
> 
> 
> void OSXAPP_SetApplicationDelegate(id <NSApplicationDelegate> delegate)
> {
> AWT_ASSERT_APPKIT_THREAD;
> -    applicationDelegate = delegate;
> +    applicationDelegate = [delegate retain];
> 
>     if (NSApp != nil) {
>         [NSApp setDelegate: applicationDelegate];
> 
>         if (applicationDelegate && qad) {
>             [qad processQueuedEventsWithTargetDelegate: applicationDelegate];
>             qad = nil;
>         }
> diff -r 96dd3a52e76c src/macosx/native/sun/osxapp/QueuingApplicationDelegate.m
> --- a/src/macosx/native/sun/osxapp/QueuingApplicationDelegate.m	Thu
> May 31 16:35:25 2012 +0100
> +++ b/src/macosx/native/sun/osxapp/QueuingApplicationDelegate.m	Mon
> Jun 11 16:49:04 2012 +0100
> @@ -104,110 +104,110 @@ static id <NSApplicationDelegate> realDe
>     self->queue = nil;
> 
>     [super dealloc];
> }
> 
> 
> - (void)_handleOpenURLEvent:(NSAppleEventDescriptor *)openURLEvent
> withReplyEvent:(NSAppleEventDescriptor *)replyEvent
> {
> -    [self->queue addObject:^(){
> +    [self->queue addObject:[^(){
>         [realDelegate _handleOpenURLEvent:openURLEvent
> withReplyEvent:replyEvent];
> -    }];
> +    } copy]];
> }
> 
> - (void)application:(NSApplication *)theApplication
> openFiles:(NSArray *)fileNames
> {
> -    [self->queue addObject:^(){
> +    [self->queue addObject:[^(){
>         [realDelegate application:theApplication openFiles:fileNames];
> -    }];
> +    } copy]];
> }
> 
> - (NSApplicationPrintReply)application:(NSApplication *)application
> printFiles:(NSArray *)fileNames withSettings:(NSDictionary
> *)printSettings showPrintPanels:(BOOL)showPrintPanels
> {
>     if (!fHandlesDocumentTypes) {
>         return NSPrintingCancelled;
>     }
> 
> -    [self->queue addObject:^(){
> +    [self->queue addObject:[^(){
>         [realDelegate application:application printFiles:fileNames
> withSettings:printSettings showPrintPanels:showPrintPanels];
> -    }];
> +    } copy]];
> 
>     // well, a bit premature, but what else can we do?..
>     return NSPrintingSuccess;
> }
> 
> - (void)_willFinishLaunching
> {
> -    QueuingApplicationDelegate * q = self;
> -    [self->queue addObject:^(){
> +    [self->queue addObject:[^(){
>         [[realDelegate class] _willFinishLaunching];
> -    }];
> +    } copy]];
> }
> 
> - (BOOL)applicationShouldHandleReopen:(NSApplication *)theApplication
> hasVisibleWindows:(BOOL)flag
> {
> -    [self->queue addObject:^(){
> +    [self->queue addObject:[^(){
>         [realDelegate applicationShouldHandleReopen:theApplication
> hasVisibleWindows:flag];
> -    }];
> +    } copy]];
>     return YES;
> }
> 
> - (NSApplicationTerminateReply)applicationShouldTerminate:(NSApplication *)app
> {
> -    [self->queue addObject:^(){
> +    [self->queue addObject:[^(){
>         [realDelegate applicationShouldTerminate:app];
> -    }];
> +    } copy]];
>     return NSTerminateLater;
> }
> 
> - (void)_systemWillPowerOff
> {
> -    [self->queue addObject:^(){
> +    [self->queue addObject:[^(){
>         [[realDelegate class] _systemWillPowerOff];
> -    }];
> +    } copy]];
> }
> 
> - (void)_appDidActivate
> {
> -    [self->queue addObject:^(){
> +    [self->queue addObject:[^(){
>         [[realDelegate class] _appDidActivate];
> -    }];
> +    } copy]];
> }
> 
> - (void)_appDidDeactivate
> {
> -    [self->queue addObject:^(){
> +    [self->queue addObject:[^(){
>         [[realDelegate class] _appDidDeactivate];
> -    }];
> +    } copy]];
> }
> 
> - (void)_appDidHide
> {
> -    [self->queue addObject:^(){
> +    [self->queue addObject:[^(){
>         [[realDelegate class] _appDidHide];
> -    }];
> +    } copy]];
> }
> 
> - (void)_appDidUnhide
> {
> -    [self->queue addObject:^(){
> +    [self->queue addObject:[^(){
>         [[realDelegate class] _appDidUnhide];
> -    }];
> +    } copy]];
> }
> 
> - (void)processQueuedEventsWithTargetDelegate:(id
> <NSApplicationDelegate>)delegate
> {
> +    realDelegate = [delegate retain];
> +
>     NSUInteger i;
>     NSUInteger count = [self->queue count];
> 
> -    realDelegate = delegate;
> -
>     for (i = 0; i < count; i++) {
>         void (^event)() = (void (^)())[self->queue objectAtIndex: i];
>         event();
> +        [event release];
>     }
> 
>     [self->queue removeAllObjects];
> }
> 
> @end