Review request for 7129420: [macosx] SplashScreen.getSplashScreen() returns null

[Mike Swingler]

Looks lovely.

Regards,
Mike Swingler
Apple Inc.

On Jan 26, 2012, at 6:06 AM, Anthony Petrov wrote:

> Would at least one more person review the fix please? Thanks in advance!
> 
> The link to the webrev for your convenience:
> http://cr.openjdk.java.net/~anthony/x-8-getSplashScreenNull.2/
> 
> Please see the quote below for more details if needed.
> 
> --
> best regards,
> Anthony
> 
> On 01/25/12 18:56, Anthony Petrov wrote:
>> On 1/25/2012 6:52 PM, Kumar Srinivasan wrote:
>>> Approved.
>> 
>> Thanks!
>> 
>> 
>>> Also I take it now the splash screen should work from sdk and jre
>>> images directory, it did not work before.
>>> 
>>> Is someone going to fix those SplashScreen regression tests ?
>>> Just curious.
>> 
>> The tests should now run fine regardless of what image is used to run them.
>> 
>> --
>> best regards,
>> Anthony
>> 
>>> 
>>> Thanks
>>> Kumar
>>> 
>>>> Hi Kumar,
>>>> 
>>>> You're right, this additional check and a comment make sense. I've
>>>> updated the webrev at:
>>>> 
>>>> http://cr.openjdk.java.net/~anthony/x-8-getSplashScreenNull.2/
>>>> 
>>>> Please review.
>>>> 
>>>> --
>>>> best regards,
>>>> Anthony
>>>> 
>>>> On 1/25/2012 5:51 PM, Kumar Srinivasan wrote:
>>>>> Hi Anthony,
>>>>> 
>>>>> Generally looks good, however few more comments.
>>>>> 
>>>>> Shouldn't we check for negative returns from snprintf ?
>>>>> 
>>>>> The snprintf(3) contract indicates on openbsd that it will return <0
>>>>> values
>>>>> for errors, in which case, we could be feeding an unitialized
>>>>> splashPath to dlopen, perhaps we should initialize it as well ?
>>>>> 
>>>>> Also appreciate including your comments below in the platform
>>>>> source(s) if applicable, explaining why we don't check the return
>>>>> value for dlopen, will be good. This is in case someone decides
>>>>> to to be meticulous and adds a check.
>>>>> 
>>>>> Thanks
>>>>> 
>>>>> Kumar
>>>>> [PS: cc'ing Joe and David Holmes on launcher fixes, they are
>>>>> usually interested in launcher changes]
>>>>> 
>>>>>> Hi Kumar,
>>>>>> 
>>>>>> Thanks for refactoring the Java launcher code! I've just re-based
>>>>>> my fix to the new structure of the Mac OS X launcher sources, here
>>>>>> it is:
>>>>>> 
>>>>>> http://cr.openjdk.java.net/~anthony/x-8-getSplashScreenNull.1/
>>>>>> 
>>>>>> I've applied all the suggestions listed below. Thanks for the review!
>>>>>> 
>>>>>> Regarding your question about checking for dlopen() status - yes,
>>>>>> we're OK if the splash screen library is unable to load. E.g.
>>>>>> Embedded folks often remove some unnecessary parts from a JRE image
>>>>>> to reduce its size. If they remove the splash screen dynamic
>>>>>> library we'll just silently ignore the error and continue w/o
>>>>>> showing the splash screen - not that it's much needed for embedded
>>>>>> applications anyway.
>>>>>> 
>>>>>> The path lengths, however, are true error conditions, that's why I
>>>>>> JLI_ReportErrorMessage() them with my fix.
>>>>>> 
>>>>>> --
>>>>>> best regards,
>>>>>> Anthony
>>>>>> 
>>>>>> On 1/20/2012 9:48 PM, Kumar Srinivasan wrote:
>>>>>>> Hi Anthony,
>>>>>>> 
>>>>>>> Do you want to wait until I push the launcher changes which
>>>>>>> are under review ?
>>>>>>> 
>>>>>>> In the launcher code we tend to use the JLI prefixed posix
>>>>>>> calls, ex: JLI_Snprintf. vs snprintf
>>>>>>> 
>>>>>>> The error messages are defined in emessages.h, this is so
>>>>>>> that we can L10N these messages in the future, if such a
>>>>>>> requirement arises. I think you can use JRE_ERROR11
>>>>>>> 
>>>>>>> Personally I would create two more buffers to avoid errors
>>>>>>> and make it safer, these are a cause of some grief when
>>>>>>> security audits are performed for buffer overruns.
>>>>>>> 
>>>>>>> char tail[PATH_MAX]
>>>>>>> char tmp[PATH_MAX]
>>>>>>> 
>>>>>>> JLI_Snprintf(tail, PATH_MAX, "/lib/%s", SPLASHSCREEN_SO);
>>>>>>> 
>>>>>>> if (JLI_Snprintf(tmp, PATH_MAX, "%s%s", path, tail)) {
>>>>>>> JLI_ReportErrorMessage.......
>>>>>>> }
>>>>>>> 
>>>>>>> also I noticed that there is no check for dlopen, so is it ok for the
>>>>>>> splashscreen to fail silently if dlopen fails ?
>>>>>>> 
>>>>>>> Thanks
>>>>>>> Kumar
>>>>>>> 
>>>>>>> 
>>>>>>>> Hello,
>>>>>>>> 
>>>>>>>> Please review a fix for
>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=7129420 at:
>>>>>>>> 
>>>>>>>> http://cr.openjdk.java.net/~anthony/x-8-getSplashScreenNull.0/
>>>>>>>> 
>>>>>>>> We remove our custom mechanism to determine the splash screen
>>>>>>>> dynamic library path and instead start using the GetJREPath() for
>>>>>>>> this purpose. I've verified this fix with an SDK (not JRE!)
>>>>>>>> image, and it works just fine.
>>>>>>>> 
>>>>>>>> --
>>>>>>>> best regards,
>>>>>>>> Anthony
>>>>>>> 
>>>>> 
>>>