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

[Anthony Petrov]

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