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

[Marco Dinacci]

Hi,

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

yes this was a mistake.

> 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?

ok, I don't know about the future plans. I think the best then is just
to revert NSApplicationAWT.m entirely, the bug fix is really in
QueuingApplicationDelegate.m anyway. By entirely I also mean removing
the [applicationDelegate retain] that I added in the first patch as I
now understand that there's no need to keep it (at least for now) once
all the events are processed.

> 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?)

done

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

fixed.

The usual convention in Cocoa is to do an if(self) instead of
if(!self) so I got confused.

> 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.

I don't have a strong opinion either, I think it doesn't hurt leaving
it like this because as you say it will be dealloced anyway.

Attached the new patch, I reverted NSApplicationAWT.m and its header
and implemented the changes you suggested above.

Best,
Marco