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

Mike Swingler swingler at apple.com
Thu Jan 26 10:09:35 PST 2012

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 JLIReportErrorMessage() 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: JLISnprintf. 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 JREERROR11 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[PATHMAX] char tmp[PATHMAX] JLISnprintf(tail, PATHMAX, "/lib/%s", SPLASHSCREENSO); if (JLISnprintf(tmp, PATHMAX, "%s%s", path, tail)) { JLIReportErrorMessage....... } 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/viewbug.do?bugid=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

More information about the macosx-port-dev mailing list