RFR: 8326712: Robot tests fail on XWayland

[Kevin Rushforth]

On Wed, 26 Jun 2024 11:25:37 GMT, Alexander Zvegintsev <azvegint at openjdk.org> wrote:

> Most of the headful test failures on XWayland are due to screen capture is not working.
> 
> Wayland, unlike X11, does not allow arbitrary applications to capture the screen contents directly.
> Instead, screen capture functionality is managed by the compositor, which can enforce stricter controls and permissions, requiring explicit user approval for screen capture operations.
> 
> This issue is already resolved in OpenJDK ([base issue](https://bugs.openjdk.org/browse/JDK-8280982), there are  subsequent fixes) by using the [ScreenCast XDG portal](https://flatpak.github.io/xdg-desktop-portal/docs/doc-org.freedesktop.portal.ScreenCast.html).
> 
> The XDG ScreenCast portal is utilized for capturing screen data as it provides a secure and standardized method for applications to access screen content.
> Additionally, it allows for the reuse of user permissions without requiring repeated confirmations, streamlining the user experience and enhancing convenience.
> 
> 
> <hr>
> 
> So this changeset is a copy of the OpenJDK fixes with the addition of the JavaFX customization. 
> For ease of review, you can skip the changes in the first two commits:
> - First commit is a direct copy of files from OpenJDK
> - Second commit removes the `gtk-` prefix before the `gtk_...` and `g_...` function calls (in AWT all gtk functions are dynamically loaded, we don't need that in JavaFX).
> 
> properties added:
> 
> - `javafx.robot.screenshotMethod`, accepts `gtk`(existing gtk method) and `dbusScreencast`(added by this changeset, used by default for Wayland)
> - `javafx.robot.screenshotDebug` prints debug info if it is set to `true`
> 
> <hr>
> 
> What are the remaining issues?
> 
> 1.
> 
> After applying this fix, system tests will pass except for the `SwingNodeJDialogTest` test.
> 
> This interop test calls `java.awt.Robot#getPixelColor` which internally `gtk->g_main_context_iteration(NULL, TRUE);` causes a blocking of javafx gtk loop, so the test hangs.
> So a change is required on OpenJDK side to fix this issue.
> 
> 2.
> 
> Even after solving the `#1`, the `SwingNodeJDialogTest.testNodeRemovalBeforeShow` case is still failing.
> 
> 3.
> 
> Internally the ScreenCast session keeps open for [2s](https://github.com/openjdk/jdk/blob/d457609f700bbb1fed233f1a04501c995852e5ac/src/java.desktop/unix/classes/sun/awt/screencast/ScreencastHelper.java#L62).
> This is to reduce overhead in case of frequent consecutive screen captures.
> 
> There is a crash when an AWT ScreenCast session overlaps with the FX ScreenCast session.  E.g.  `java.awt.Robot#getPixelColor()` and `jav...

I did a cursory review, just looking at the changes since the first two commits, and it looks fine. Preliminary testing looks good.

I think this is ready to take out of Draft.

We'll want extra pairs of eyes on this one (so at least two "R"eviewers). I'd also like Gluon to verify that it builds on their CI system.

Reviewers: @kevinrushforth @prrace @tiainen

@azvegint When you do take it out of Draft, please write up something in the Description about the changes as a help to those who will be reviewing it.

Also, can you merge in the latest master? In particular, I want to see a GHA run with the `javac -Werror` fix.

modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/GtkRobot.java line 39:

> 37: import java.security.PrivilegedAction;
> 38: 
> 39: @SuppressWarnings("removal")

Maybe this can go on the individual elements that use the security manager? To do this, you might need to create a local "tmp" variable and then assign that to `screenshotMethod`.
(either way, don't worry about the files you copied from AWT)

modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/screencast/TokenStorage.java line 117:

> 115: 
> 116:         // use secondary path only if it already exists and writable
> 117:         Path path = Files.isWritable(secondaryPath)

If both exist and are writable, then wouldn't you want to default to the primary path? If so, then I think the test should be something like `Files.isWritable(secondaryPath) && !Files.isWritable(primaryPath)`.

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

PR Review: https://git.openjdk.org/jfx/pull/1490#pullrequestreview-2146168288
PR Comment: https://git.openjdk.org/jfx/pull/1490#issuecomment-2195459814
PR Comment: https://git.openjdk.org/jfx/pull/1490#issuecomment-2195470151
PR Review Comment: https://git.openjdk.org/jfx/pull/1490#discussion_r1657594947
PR Review Comment: https://git.openjdk.org/jfx/pull/1490#discussion_r1657655951