RFR: 8335469: [XWayland] crash when an AWT ScreenCast session overlaps with an FX ScreenCast session [v2]

[Kevin Rushforth]

On Fri, 15 Nov 2024 02:21:57 GMT, Alexander Zvegintsev <azvegint at 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

tests/system/src/test/java/test/robot/javafx/embed/swing/LinuxScreencastHangCrashTest.java line 60:

> 58:         Assumptions.assumeTrue(!Util.isOnWayland()); // JDK-8335470
> 59:         Assumptions.assumeTrue(Util.isOnWayland());
> 60:         robot = new Robot();

This initializes the AWT toolkit before the FX toolkit. Have you also tested it the other way around? I'm not sure it matters on Linux (unlike macOS where it does matter), so maybe unimportant.

tests/system/src/test/java/test/robot/javafx/embed/swing/LinuxScreencastHangCrashTest.java line 72:

> 70:     private static void awtPixelOnFxThread() throws InterruptedException {
> 71:         System.out.println("awtPixelOnFxThread");
> 72:         initFX();

Have you considered moving the `initFX()` call to the `init` method (after the call to AWT robot) so you only need it in one place?

tests/system/src/test/java/test/robot/javafx/embed/swing/LinuxScreencastHangCrashTest.java line 143:

> 141:                 DELAY_BEFORE_SESSION_CLOSE + 25
> 142:         ).forEach(delay -> {
> 143:             System.out.println("Testing with delay: " + delay);

You might consider making this a parameterized test with this list of delays as the parameters, if it isn't too hard.

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1639#discussion_r1844245065
PR Review Comment: https://git.openjdk.org/jfx/pull/1639#discussion_r1844245832
PR Review Comment: https://git.openjdk.org/jfx/pull/1639#discussion_r1844265977