Could someone on AWT team approve the splashscreen changes? -DrD-
> Approved. > > -phil. > > On 5/15/2014 9:31 AM, David DeHaven wrote: >> Ping! >> >> Does this look OK? >> >> I've also filed an issue against JavaFX: >> https://javafx-jira.kenai.com/browse/RT-37125 >> >> -DrD- >> >>>>>> I tried not modifying libpng but still ended up with lingering >>>>>> references to longjmp in pngread.o, despite libpng having >>>>>> png_ptr->longjmp_fn (bug in libpng?). pngread.c calls setjmp to set a >>>>>> default location to jump to in case the caller doesn't call setjmp, so >>>>>> if we continue down this path something in libpng must be modified. The >>>>>> only other option is to create our own setjmp.h and order it before >>>>>> /usr/include/setjmp.h, which seems dubious at best. >>>>>> >>>>>> I'm curious if the libpng changes are even needed since it's only used >>>>>> for splashscreen, which happens very early in the launch process. Also >>>>>> note that we didn't originally even call png_set_longjmp_fn, so any >>>>>> error should have resulted in an abort() instead of a call to longjmp... >>>>>> it appears we could retain the functionality we have today and #undef >>>>>> PNG_SETJMP_SUPPORTED (pngconf.h?). That would put the onus on developers >>>>>> to make sure their pngs don't have errors in them, or libsplashscreen >>>>>> will abort()... >>>>>> >>>>>> >>>>> That's an interesting question and the answer might extend to the >>>>> splashscreen changes too. >>>>> Its platform specific code and on MAC, the thread is created using >>>>> pthreads directly and that >>>>> thread goes away once splashscreen is done. But its running at the same >>>>> time as the VM >>>>> is booting up and creating threads and setting their signal masks. So I >>>>> don't think you can >>>>> guarantee that it won't mess up the masks on the JRE threads if the PNG >>>>> is bad. And I'm >>>>> also not sure you want to remove error handling from the library either. >>>>> So a HIGHLY VISIBLE DO NOT REMOVE comment might be the best you can do >>>>> here. >>>> I have a better idea: >>>> >>>> png_default_error is the only place where png_longjmp is called. We could >>>> call png_set_error_fn to set up our own error handler (for Mac only), >>>> compile with PNG_SETJMP_SUPPORTED unset so it doesn't pull in >>>> setjmp/longjmp and our own implementation of the error handler would call >>>> _longjmp, which would jump back to where we call setjmp currently. >>> Ok, I figured out what's going on. It's not quite intuitive... >>> >>> png_jmpbuf is a macro defined in png.h, this calls png_set_longjmp_fn with >>> longjmp, which is why I was seeing references to longjmp in the object >>> file. That's what was throwing me off as it seems like it should only be >>> getting the jmp_buf ptr stored in the png_ptr. I guess the intention was >>> that setjmp/longjmp was optional, if you don't call setjmp then it just >>> abort()s. >>> >>> >>> I changed splashscreen_png.c to: >>> #ifdef __APPLE__ >>> if (_setjmp(png_set_longjmp_fn(png_ptr, _longjmp, sizeof(jmp_buf)))) { >>> #else >>> if (setjmp(png_jmpbuf(png_ptr))) { >>> #endif >>> >>> and it calls _longjmp instead. I verified this works by changing the macro >>> to set png_longjmp to exit() and without the above change it does indeed >>> exit prematurely with a bad png, with the change it reports the error but >>> continues to load the application as would be expected. >>> >>> pngread.o still has a symbol table entry for _longjmp instead of __longjmp, >>> but it's benign since we're ultimately forcing it to use the correct >>> function. So I've left libpng completely unchanged. >>> >>> >>> With the change and using a bad png for splashscreen, I was able to get a >>> stack trace once the application was running. Without the change to >>> splashscreen_png.c, jstack was unable to connect to the process. So >>> splashscreen absolutely can interfere with the signal handling. >>> >>> >>> Updated webrev: >>> http://cr.openjdk.java.net/~ddehaven/8026385/jdk.1/ >>> >>> I can look into writing a regression test for this. It might not be trivial >>> though since we're dealing with signal handlers, and if timing is a factor >>> the test may not be reliable. >>> >>> -DrD- >>> >
