On Fri, 15 Nov 2024 02:21:57 GMT, Alexander Zvegintsev <azveg...@openjdk.org> wrote:
>> This is the FX counterpart of the >> [openjdk/jdk/pull/22131](https://github.com/openjdk/jdk/pull/22131) / >> `b.`(aka crash prevention part) >> >> It does not crash without the >> [openjdk/jdk/pull/22131](https://github.com/openjdk/jdk/pull/22131) / `a.` >> part. >> >> The crash prevention part is the same for the JDK and JavaFX, except that >> this PR includes a test. >> >> ---- >> >> Internally the ScreenCast session keeps open for 2s (both JDK and JFX, and >> their implementations are almost identical). >> This is to reduce overhead in case of frequent consecutive screen captures. >> >> When we perform a >> [cleanup](https://github.com/openjdk/jdk/blob/db56266ad164b4ecae59451dc0a832097dbfbd8e/src/java.desktop/unix/native/libawt_xawt/awt/screencast_pipewire.c#L91) >> to close the session, we internally call >> [`pw_deinit`](https://docs.pipewire.org/group__pw__pipewire.html#gafa6045cd7391b467af4575c6752d6c4e). >> >> It becomes a problem if these sessions overlap in time, so that the second >> session cleanup crashes when it tries to call pipewire functions without >> initializing the pipewire system by >> [`pw_init`](https://docs.pipewire.org/group__pw__pipewire.html#ga06c879b2d800579f4724109054368d99) >> (please note that `This function can be called multiple times.`). >> >> So the solution is not to call `pw_deinit` because we don't really need it, >> and it needs to be applied to both the JDK and JavaFX. >> >> [jdk >> commit](https://github.com/openjdk/jdk/pull/22131/commits/19956fda202269e92ec70670bc88c8d3c7accf73) >> / [jfx >> commit](https://github.com/openjdk/jfx/pull/1639/commits/dba8a8871a38831d0a0da697a2be41f0c240c8f0) >> >> ---- >> >> The newly introduced test is disabled for now(as part of >> [JDK-8335470](https://bugs.openjdk.org/browse/JDK-8335470)), because it >> requires the fix on both the JavaFX and JDK sides. >> For the same reason >> `tests/system/src/test/java/test/robot/javafx/embed/swing/SwingNodeJDialogTest.java` >> and `tests/system/src/test/java/test/robot/javafx/scene/SRGBTest.java` are >> not enabled back. >> But if the JDK and JavaFX have the fixes, everything works fine. >> Testing in other different scenarios also looks good. > > Alexander Zvegintsev has updated the pull request incrementally with two > additional commits since the last revision: > > - test cleanup > - add missing copyright header I tested this with and without the AWT patch in openjdk/jdk#22131 and all looks good. I left a couple minor comments on the test earlier. I can't reproduce the crash without this fix on my system, but since it does happen on some systems, it does need to be fixed. We should backport this to jfx23u once integrated. @lukostyra Can you be the second reviewer? ------------- Marked as reviewed by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1639#pullrequestreview-2440768655 PR Comment: https://git.openjdk.org/jfx/pull/1639#issuecomment-2480647453
