Thanks! -DrD-
> The splashscreen changes look fine to me. Approved. > > -- > best regards, > Anthony > > On 5/16/2014 7:18 PM, David DeHaven wrote: >> >> 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- >>>>> >>> >>
