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

[Mike Swingler]

This looks good. I'd also suggest making it an atomic property, and also converting the queue to be a property as well. Using -> notation to directly pick at Obj-C ivars across threads is generally considered bad form.

Regards,
Mike Swingler
Apple Inc.

On Jun 18, 2012, at 7:42 AM, Anthony Petrov wrote:

> 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