[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 et al.,

I've published your latest patch here:

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

The fix looks just fine to me. Thank you for fixing the issue.

Could anyone else on this mailing list review it please?

--
best regards,
Anthony

On 06/16/12 12:51, Marco Dinacci wrote:
> 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