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