Re: AWT Dev [9] Review request for 8042465: Applet menus not rendering when browser is full screen on Mac
Hi Petr, Anthony, Thank you for looking at this. I really missed the case pointed out by Petr. If the test app is running in browser instead of IDE or appletviewer, the situation is much worse - the opened popup does not hide at all when we switch to another space. This behavior is caused by usage of NSWindowCollectionBehaviorCanJoinAllSpaces. I used that option since NSWindowCollectionBehaviorFullScreenAuxiliary does not work properly alone, (i.e if browser is in full screen mode and we open the popup first time, it works well; however if we exit full screen and then enter back again and try to open the popup, it will be displayed behind the browsers window). I am not sure, but it is most likely such behavior is caused by popups caching. I need more time for deeper investigation. Thanks, Dmitry On 14/05/2014 14:46, Anthony Petrov wrote: To add to what Petr just said, what is the exact reason to specify the NSWindowCollectionBehaviorCanJoinAllSpaces behavior? I believe that NSWindowCollectionBehaviorFullScreenAuxiliary alone should do the trick, does it not? Petr: we used to build JDK with OS X 10.6 SDK where the 10.7-specific constants are not defined. Hence the reason for (1 8), etc. As long as this fix is not going to be ported to JDK 7u, I think we could use the constant names explicitly (we need to make sure RE builds 8u with 10.7+ SDK though.) -- best regards, Anthony On 5/14/2014 1:22 PM, Petr Pchelko wrote: Hello, Dmitry. With your fix I'm observing the following regression: 1. Run the test app from the bug in IDE or appletviewer. 2. Open the menu 3. Without closing the menu switch to another space using keyboard (Ctrl+Arrow) or touchpad gesture 4. The opened popup will be shown on another space and than will disappear. But it will be visible for enough time to get noticed and annoying. And also, why are you explicitly setting 18 instead of using the name of the constant? Thank you. With best regards. Petr. On 14 мая 2014 г., at 12:54, dmitry markov dmitry.mar...@oracle.com wrote: Hello, Could you review the fix for jdk9, please? bug: https://bugs.openjdk.java.net/browse/JDK-8042465 webrev: http://cr.openjdk.java.net/~dmarkov/8042465/jdk9/webrev.00/ Problem description: On Mac OS X when a browser is in full screen mode, applet's popup is displayed behind the browser's window. Fix: It is necessary to change the collection behavior for the popup windows to make them visible when the browser runs in full screen mode. Thanks, Dmitry
Re: AWT Dev [9] Review request for 8014755: [TEST_BUG] frames didn't closed after execution some awt/dnd/ tests
Hello, Oleg. Looks good. With best regards. Petr. On 14 мая 2014 г., at 19:02, Oleg Pekhovskiy oleg.pekhovs...@oracle.com wrote: Hi Petr, thank you for the review, here are my thoughts: 1. I added new method that way after analyzing the other methods and their usage in jtreg tests. If you look at getExecutionCommand() you could see the similar solution. Indeed my changes don't break the previous usages of ProcessCommunicator. It's also possible to perform bunch run, but of course without the termination ability. Another issue is that we must kill a child VM explicitly if it doesn't exit automatically, but we must do this only after test sequence is passed. jtreg doesn't kill child VMs on timeout, this is the known problem (that's why this JIRA issue exists). 2. I decided to leave it as is, as main classes in these tests are used in two cases: applet itself and the main class for child VM. 3. I found this mistake and added it to the second version of fix. Regards, Oleg On 14.05.2014 14:01, Petr Pchelko wrote: Hello, Oleg. 1. I'm not sure your about your new API in ProcessCommunicator. First of all, now the communicator can't be used twice in a single test to open and then close a bunch of processes. Also, you need to modify the tests themselves. I think it would be better to do the termination implicitly: you can get jtreg property with execution timeout, start a timer thread when you create a process and kill it after the timeout. In case doWaitFor finishes you kill the timer thread. 2. We normally remove .html if it's possible to convert test to a standalone app. 3. NoFormatsCrashTest lacks .java file. With best regards. Petr. On 13 мая 2014 г., at 14:55, Sergey Bylokhov sergey.bylok...@oracle.com wrote: Hi. Oleg. A few notes. - Some tests contains empty/non-rethrow catch blocks. I guess we should rethrow an exception and the test should fail in this case. - Some tests use System.exit() which should be replaced by the exception. On 5/9/14 7:32 PM, Oleg Pekhovskiy wrote: Hi all, please review the fix http://cr.openjdk.java.net/~bagiras/9/8014755.1/ for https://bugs.openjdk.java.net/browse/JDK-8014755 These tests were moved from the closed workspace. The main idea of fix is to force termination of child JVM if it's not exited automatically after the test sequence. Tests accuracy was improved to cover all error cases. destroyProcess() method was added to test.java.awt.regtesthelpers.process.ProcessCommunicator for termination of child JVM when needed. Thanks, Oleg -- Best regards, Sergey.
Re: AWT Dev [9] Review request for 8014755: [TEST_BUG] frames didn't closed after execution some awt/dnd/ tests
Hi, Oleg. Thanks. The fix looks good. On 14.05.2014 18:20, Oleg Pekhovskiy wrote: Hi Sergey, thanks you for comments! Please review the second version of fix here: http://cr.openjdk.java.net/~bagiras/9/8014755.2/ I've added re-throwing for catch blocks and commented System.exit() calls that are used only for child VMs. Thanks, Oleg On 13.05.2014 14:55, Sergey Bylokhov wrote: Hi. Oleg. A few notes. - Some tests contains empty/non-rethrow catch blocks. I guess we should rethrow an exception and the test should fail in this case. - Some tests use System.exit() which should be replaced by the exception. On 5/9/14 7:32 PM, Oleg Pekhovskiy wrote: Hi all, please review the fix http://cr.openjdk.java.net/~bagiras/9/8014755.1/ for https://bugs.openjdk.java.net/browse/JDK-8014755 These tests were moved from the closed workspace. The main idea of fix is to force termination of child JVM if it's not exited automatically after the test sequence. Tests accuracy was improved to cover all error cases. destroyProcess() method was added to test.java.awt.regtesthelpers.process.ProcessCommunicator for termination of child JVM when needed. Thanks, Oleg -- Best regards, Sergey.
Re: AWT Dev [9] Request for Review: 8003900: X11 dependencies should be removed from Mac OS X build.
Can I get another reviewer on this? I need a sponsor to push too.. (Anthony?) -DrD- src/solaris/native/sun/awt/awt.h 113 #if !defined(HEADLESS) defined(XAWT) 114 extern Display *awt_display; /* awt_GraphicsEnv.c */ You forgot to update this XAWT usage. Otherwise the fix looks fine. Whoops! Updated in-place. -DrD-
Re: AWT Dev Request for review: 8026385: [macosx] (awt) setjmp/longjmp changes the process signal mask on OS X
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-
Re: AWT Dev [9] Request for Review: 8003900: X11 dependencies should be removed from Mac OS X build.
Hello, David. The fix looks good to me. Artem was interested in the review, so we may want to wait for his final vote.. I can push the changeset for you. Please ping me when you decide it’s ready to go. Thank you. With best regards. Petr. On May 15, 2014, at 8:23 PM, David DeHaven david.deha...@oracle.com wrote: Can I get another reviewer on this? I need a sponsor to push too.. (Anthony?) -DrD- src/solaris/native/sun/awt/awt.h 113 #if !defined(HEADLESS) defined(XAWT) 114 extern Display *awt_display; /* awt_GraphicsEnv.c */ You forgot to update this XAWT usage. Otherwise the fix looks fine. Whoops! Updated in-place. -DrD-
Re: AWT Dev Request for review: 8026385: [macosx] (awt) setjmp/longjmp changes the process signal mask on OS X
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-
