8326712: Robot tests fail on XWayland #1490

azvegint · 2024-06-26T11:25:37Z

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, there are subsequent fixes) by using the ScreenCast XDG portal.

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.

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

What are the remaining issues?

https://bugs.openjdk.org/browse/JDK-8335468

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.

https://bugs.openjdk.org/browse/JDK-8335470

Even after solving the #1, the SwingNodeJDialogTest.testNodeRemovalBeforeShow case is still failing.

https://bugs.openjdk.org/browse/JDK-8335469

Internally the ScreenCast session keeps open for 2s.
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 javafx.scene.robot.Robot#getPixelColor() are called within 1 second.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 2 Reviewers)

Issue

JDK-8326712: Robot tests fail on XWayland (Bug - P3)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Joeri Sykora (@tiainen - Author) 🔄 Re-review required (review applies to 3aa56a2c)
Phil Race (@prrace - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1490/head:pull/1490
$ git checkout pull/1490

Update a local copy of the PR:
$ git checkout pull/1490
$ git pull https://git.openjdk.org/jfx.git pull/1490/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1490

View PR using the GUI difftool:
$ git pr show -t 1490

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1490.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-06-26T11:26:08Z

👋 Welcome back azvegint! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-06-26T11:27:16Z

@azvegint This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8326712: Robot tests fail on XWayland

Reviewed-by: kcr, prr, sykora

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 1 new commit pushed to the master branch:

1fb8755: 8198830: BarChart: auto-range of CategoryAxis not working on dynamically setting data

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

modules/javafx.graphics/src/main/native-glass/gtk/fp_pipewire.h

tests/system/src/test/java/test/robot/javafx/embed/swing/SwingNodeJDialogTest.java

modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/screencast/ScreencastHelper.java

prrace · 2024-06-26T20:18:23Z

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

+    private TokenStorage() {}
+
+    private static final String REL_NAME =
+            ".fx/robot/screencast-tokens.properties";


should be .javafx
However I think we can try to READ .awt (and .java) first and if it exists try to use that.
Perhaps WRITE .javafx ?
If we don't understand what is at .awt or .java then we would then READ .javafx and if it is not there or
doesn't work CREATE .javafx

The overall intent is that if a user authorised Java for AWT then we don't need to re-ask them for FX so long as we can parse what is there and it works

Updated to check two locations:

.java/.robot/screencast-tokens.properties - the default, should be used by both OpenJDK and JavaFx

.awt/robot/screencast-tokens.properties - this is currently the only location used by OpenJDK, but it should be changed to the first location after JDK-8335267 is resolved.

If there is a writable file at location #2, we won't try to read or write to location #1.

kevinrushforth

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.

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

modules/javafx.graphics/src/main/native-glass/gtk/fp_pipewire.h

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

kevinrushforth · 2024-06-27T18:51:48Z

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

/reviewers 2 reviewers

openjdk · 2024-06-27T18:52:21Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 2 Reviewers).

kevinrushforth · 2024-06-27T18:58:40Z

@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.

…here is no writable file on the primary path.

azvegint · 2024-06-28T07:49:26Z

modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/screencast/ScreencastHelper.java

+            throw new SecurityException(
+                    "Screen Capture in the selected area was not allowed"
+            );


@kevinrushforth
I think this SecurityException should be removed, as it is not mentioned in the javadoc for the getPixelColor or getScreenCapture. And we also do not want to throw another runtime exception.

If the SecurityException is not thrown, how does the caller of getRGBPixels knows that the result will not be correct because of the missing permission? (as opposed to being incorrect due to a failure)

If the SecurityException is not thrown, how does the caller of getRGBPixels knows that the result will not be correct because of the missing permission? (as opposed to being incorrect due to a failure)

He won't know it. An indirect result of failure can be a black image.
The current documentation documentation specifies only IllegalStateException and NullPointerException.

Since SecurityException is deprecated, we should not throw that.

In a similar situation on macOS (user has not enabled screen capture), we return a blank (all black) image, so I think that's what we would do here.

Updated to return a blank image if permission is not granted.

Since SecurityException is deprecated, we should not throw that.

In a similar situation on macOS (user has not enabled screen capture), we return a blank (all black) image, so I think that's what we would do here.

No, SecurityException is NOT deprecated and there are no plans to deprecate it.

https://download.java.net/java/early_access/jdk23/docs/api/java.base/java/lang/SecurityException.html#%3Cinit%3E()

This kind of platform reason for a "SecurityException" is precisely why it isn't deprecated even though the Java SecurityManager is deprecated.

Thanks for the correction about "SecurityException" not being deprecated.

I still think that for this PR, we should not throw an exception, since we A) don't specify it to do so, and B) don't do that on macOS in a nearly identical situation.

We could consider a future RFE to change the spec (and maybe the behavior).

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

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

mlbridge · 2024-06-28T07:57:51Z

Webrevs

tiainen

The build still works as expected and documented on our CI infrastructure.

tsayao · 2024-06-28T13:54:54Z

Just of out curiosity, please don't mind it too much:

Why the piperwire include files are included? Since the infraestucture should be present for it to work (pipewire is quite new and not present on older linux distributions) a runtime check is needed anyways.

What's the advantage of calling dbus functions directly and not use libportal?

azvegint · 2024-06-28T15:58:46Z

Just of out curiosity, please don't mind it too much:

Why the piperwire include files are included? Since the infraestucture should be present for it to work (pipewire is quite new and not present on older linux distributions) a runtime check is needed anyways.

What's the advantage of calling dbus functions directly and not use libportal?

This is mostly a copy and paste of the OpenJDK code, which should be able to be built on fairly old Linux distributions (consider OpenJDK backports to earlier versions), and we didn't want to add any new external dependencies there.
So with this changeset, we are just bringing tested code with minimal changes to JFX.

kevinrushforth · 2024-07-01T22:16:49Z

The code changes all look good. I've done a fair bit of testing already, and will finish up my testing tomorrow.

azvegint · 2024-07-02T13:09:24Z

tests/system/src/test/java/test/robot/javafx/embed/swing/SwingNodeBase.java

+            // Emulating mouse clicks on XWayland with XTEST does not currently affect the Wayland compositor,
+            // so trying to click on the window title or window body will not bring the window to the front.


This is the same issue we have in OpenJDK, when the emulated mouse click doesn't put one window in front of another.
After this fix and applying a fix for the hang issue, all the SwingNodeJDialogTest tests are passing.

kevinrushforth

Testing is complete. No new issues. I only see the SwingNodeJDialogTest failure.

I did leave one minor naming comment and will reapprove if you make the suggested change.

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

azvegint · 2024-07-03T09:52:07Z

/integrate

openjdk · 2024-07-03T09:53:20Z

Going to push as commit 459b15f.
Since your change was applied there have been 3 commits pushed to the master branch:

5656b80: 8326619: Stage.sizeToScene() on maximized/fullscreen Stage breaks the Window
6c3fb5f: 8289115: Touch events is not dispatched after upgrade to JAVAFX17+
1fb8755: 8198830: BarChart: auto-range of CategoryAxis not working on dynamically setting data

Your commit was automatically rebased without conflicts.

openjdk · 2024-07-03T09:53:26Z

@azvegint Pushed as commit 459b15f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

andy-goryachev-oracle · 2024-07-03T17:45:18Z

modules/javafx.graphics/src/main/java/com/sun/javafx/geom/Dimension.java

+        this.height = height;
+    }
+
+    public boolean equals(Object obj) {


JDK-8335633
Missing @OverRide in Dimension

azvegint added 3 commits June 26, 2024 13:07

Copy files directly from OpenJDK

340adcc

remove "gtk->" prefix

96f5023

initial fix

30b0c2a

azvegint added 8 commits June 26, 2024 15:52

fix warnings

d83d454

add timeout for SwingNodeJDialogTest

e267212

get rid of AWT dependencies(Rectangle, Dimension)

880a833

Change properties prefixes from "fx." to "jfx."

b2fdc5a

Reuse awt tokens

9f4bea6

AWT->JFX naming

76a4f3c

distinguish JFX logs

4a9578c

comment removal

fdb1249

prrace reviewed Jun 27, 2024

View reviewed changes

use "javafx." prefix for the properties

8cb392c

kevinrushforth reviewed Jun 27, 2024

View reviewed changes

azvegint added 4 commits June 28, 2024 06:52

remove HEADLESS check

468c2ba

make @SuppressWarnings("removal") individual

04dd233

Use the secondary path for token storage only if it is writable and t…

ea7448d

…here is no writable file on the primary path.

Merge branch 'master' into xwayland/8326712_robot_tests

3aa56a2

azvegint commented Jun 28, 2024

View reviewed changes

azvegint marked this pull request as ready for review June 28, 2024 07:54

openjdk bot added the rfr Ready for review label Jun 28, 2024

tiainen approved these changes Jun 28, 2024

View reviewed changes

azvegint added 2 commits June 28, 2024 18:05

Revert "remove HEADLESS check"

8ab6ff2

do not throw SecurityException

33fa538

kevinrushforth self-requested a review July 1, 2024 22:15

fix the SwingNodeJDialogTest.testNodeRemovalBeforeShow test failure

2b05125

azvegint commented Jul 2, 2024

View reviewed changes

kevinrushforth approved these changes Jul 2, 2024

View reviewed changes

modules/javafx.graphics/src/main/java/com/sun/glass/ui/gtk/screencast/TokenStorage.java Outdated Show resolved Hide resolved

fix default token path

2c5d425

kevinrushforth approved these changes Jul 2, 2024

View reviewed changes

prrace approved these changes Jul 2, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Jul 2, 2024

openjdk bot added the integrated Pull request has been integrated label Jul 3, 2024

openjdk bot closed this Jul 3, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Jul 3, 2024

andy-goryachev-oracle reviewed Jul 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8326712: Robot tests fail on XWayland #1490

8326712: Robot tests fail on XWayland #1490

azvegint commented Jun 26, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Jun 26, 2024

openjdk bot commented Jun 26, 2024 •

edited

Loading

prrace Jun 26, 2024

azvegint Jun 27, 2024

kevinrushforth left a comment

kevinrushforth commented Jun 27, 2024

openjdk bot commented Jun 27, 2024

kevinrushforth commented Jun 27, 2024

azvegint Jun 28, 2024

johanvos Jun 28, 2024

azvegint Jun 28, 2024

kevinrushforth Jun 28, 2024

azvegint Jun 28, 2024 •

edited

Loading

prrace Jun 28, 2024

kevinrushforth Jun 28, 2024

mlbridge bot commented Jun 28, 2024 •

edited

Loading

tiainen left a comment

tsayao commented Jun 28, 2024

azvegint commented Jun 28, 2024

kevinrushforth commented Jul 1, 2024

azvegint Jul 2, 2024

kevinrushforth left a comment

azvegint commented Jul 3, 2024

openjdk bot commented Jul 3, 2024

openjdk bot commented Jul 3, 2024

andy-goryachev-oracle Jul 3, 2024

		// Emulating mouse clicks on XWayland with XTEST does not currently affect the Wayland compositor,
		// so trying to click on the window title or window body will not bring the window to the front.

8326712: Robot tests fail on XWayland #1490

8326712: Robot tests fail on XWayland #1490

Conversation

azvegint commented Jun 26, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 26, 2024

openjdk bot commented Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth commented Jun 27, 2024

openjdk bot commented Jun 27, 2024

kevinrushforth commented Jun 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azvegint Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlbridge bot commented Jun 28, 2024 • edited Loading

Webrevs

tiainen left a comment

Choose a reason for hiding this comment

tsayao commented Jun 28, 2024

azvegint commented Jun 28, 2024

kevinrushforth commented Jul 1, 2024

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

azvegint commented Jul 3, 2024

openjdk bot commented Jul 3, 2024

openjdk bot commented Jul 3, 2024

Choose a reason for hiding this comment

azvegint commented Jun 26, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Jun 26, 2024 •

edited

Loading

azvegint Jun 28, 2024 •

edited

Loading

mlbridge bot commented Jun 28, 2024 •

edited

Loading