[8] Review request for 7170716: JVM crash when opening an AWT app from a registered file. [WAS: Re: Patch for #7170716, crash in OSXAPP_SetApplicationDelegate]

[Anthony Petrov]

Hi Marco,

Thank you for investigating the issue and developing a fix. I've 
uploaded a webrev for your patch at:

http://cr.openjdk.java.net/~anthony/8-34-crashInSetApplicationDelegate-7170716.0/

I've got a concern regarding the retain calls on the delegate instance. 
With your fix we retain it for the first time at 
OSXAPP_SetApplicationDelegate(), and then one more time at 
QAD.processQueuedEventsWithTargetDelegate. However, we never release the 
object. Moreover, I believe that the NSApplication setDelegate: call 
also retains it.

I'd think that we can retain it in the beginning of the 
OSXAPP_SetAppDelegate(), and then release it in the end. And we don't 
need a retain call in the processQEWTD. Could you please verify this?

--
best regards,
Anthony

On 6/11/2012 8:17 PM, 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