[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,

Thanks for the updated patch. A webrev with your latest fix is published at:

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

Some comments:

1. I think the changes to the NSApplicationAWT.h are unnecessary.

2. I'd recommend to not remove the if (applicationDelegate) check in the 
NSApplicationAWT.m since in the future we may want to change the logic 
in awt.m, in which case a real delegate may be set before 
finishLaunching is executed. The check doesn't hurt, does it?

3. The realDelegate as a property looks fine. Since we use the self. (or 
self->) notation to access ivars, please update the QAD's callback 
methods (those that create the blocks to be queued), and use this 
notation to access the realDelegate property. (I'm wondering though, 
won't the compiler treat 'self' as a ref to the block instead of the 
object there?)

src/macosx/native/sun/osxapp/QueuingApplicationDelegate.m:
>   61     if (!self) {
>   62         self.realDelegate = nil;
>   63         return self;
>   64     }

I think it's not safe to assign a value to an ivar when the self 
reference is nil. Let's just remove the line 62 - the object itself 
couldn't be initialized, and neither could its member property, I guess. 
There's no reason to nullify it here.

Also, in theory we could nullify the property at the end of 
processQueuedEventsWithTargetDelegate since we don't need to retain the 
object after we've flushed all the queued events. I don't have a strong 
opinion on this though, since the property will be nil'ed in the dealloc 
anyway.

--
best regards,
Anthony

On 6/13/2012 7:32 PM, Marco Dinacci wrote:
> Hi Anthony,
> 
> thanks for looking at the patch.
> 
>> 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?
> 
> I took a better look at the code, including awt.m, to understand how
> and when OSXAPP_SetApplicationDelegate is called.
> It seems to me this function is called only once and always *after*
> finishLaunching().
> 
> *If* this is correct, then this check in finishLaunching() will be
> always false as applicationDelegate is certainly nil at this point:
> 
> if (applicationDelegate) {
>         [self setDelegate:applicationDelegate];
> } else {
>         qad = [QueuingApplicationDelegate sharedDelegate];
>         [self setDelegate:qad];
> }
> 
> and the code could be simplified to:
> 
> qad = [QueuingApplicationDelegate sharedDelegate];
> [self setDelegate:qad];
> 
> In this case, the applicationDelegate global variable could be
> eliminated altogether and the code in OSXAPP_SetApplicationDelegate
> simplified:
> 
> void OSXAPP_SetApplicationDelegate(id <NSApplicationDelegate> delegate)
> {
> AWT_ASSERT_APPKIT_THREAD;
> 
>     if (NSApp != nil) {
>         [NSApp setDelegate: delegate];
> 
>         if (delegate && qad) {
>             [qad processQueuedEventsWithTargetDelegate: delegate];
>             qad = nil;
>         }
>     }
> }
> 
> As you say NSApp will retain the delegate and qad should guaranteed to
> be non-nil as it's been initialised in finishLaunching.
> 
> Regarding the retain in processQueuedEventsWithTargetDelegate, I
> believe it's correct but you're right that it's not been released
> afterwards.
> The problem is that it's a global variable, is there a particular reason why ?
> I think it's better to convert it to a property, in that case it can
> be released safely in dealloc.
> 
> I just tested with both these changes and with both my apps and I have
> no crash.
> I'm attaching a new patch, it will probably be stripped out by the ML
> software but hopefully you should get it.
> 
> Best,
> Marco