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

[Anthony Petrov]

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