Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8275843: Random crashes while the UI code is executed #6199

Closed
wants to merge 9 commits into from

Conversation

mrserb
Copy link
Member

@mrserb mrserb commented Nov 1, 2021

Please take a look to one more wagon in this train:
JDK-8176795->JDK-8204931->JDK-8214579->JDK-8227392->Current issue.

The short description:

We have an inconsistent between xrender java code and the x11 window. The java code thinks that all our windows support only two types of color models: IntRgbX11 and IntArgbPreX11, but the x11 window supports more types. If such inconsistency occurs we render some garbage, or just crash since our "blits" will assume the wrong data layout. This issue was reported and fixed before but it was not realized why the rendering was wrong and that it can cause a crash. The fix is similar to the old one.

The long description:

  • Once upon a time this bug was filed JDK-8176795 it was about the rendering of colors to the volatile image, the description can be found here In short: the volatile images in the xrender use intArgbPre color model, it means that the SunGraphics2D object convert the argb value to the argbPre before passing it to the pipeline, and the xrender pipeline convert this value to the argbPre one more time -> double-conversion-> the bug is reproduced. The fix for that bug disabled the second conversion in the xrender. but it does not take into account that this code path is used for rendering to the ARGB X11 window where the shared code does not convert the color, so the next bug was filed.
  • The second bug is JDK-8204931, the description can be found here. In short the bug was about rendering directly to the window and missing color conversion removed previously. The fix for that bug changed the surface type used by the window, so the shared code start to convert the color from ARGB to argbPre, but it does not take into account that we do not control the color model of the window, see how many of them we actually support Also note in the link above that if we do not recognize the type we do not fallback to something but throw InvalidPipeException, this is because the blits(and other primitives) may use this type to access the data directly. So the next bug was reported:
  • This is the third issue JDK-8214579. The fix for that issue tried to roll back the previous one but does not take into account that the conversion removed by the first issue should be rollback as well, so the last bug was filed.
  • The "last" issue is JDK-8227392 rollback the previous one. So the bug reported in the third report come back.

Fix description

The fix for the current issue contains a few parts:

  • Rollback of the JDK-8204931 so we will use the proper surface data for the window.
  • Return conversion in the xrender pipeline removed by the JDK-8176795 So we will convert it when we render to the volatile image(argbPre) and to the window(argb). We need to convert in both cases since the xrender API which we use expects the argbPre colors.
  • Start to use the eargb value( instead of pixel) stored in the SunGraphics2D so we did not get the double conversion.

Some tests are updated and one more is added.

Note

I have checked the performance implication of this change and it looks like after this change the sequence window.setvisible(true)/window.dispose() will work 30% faster, this may affect the tests which does not wait enough time to make the window visible. Probably this is the reason why some of our tests became more stable at some point in jdk11.

I have tested this by the jck and jdk_desktop tests. But any additional verification/testing is welcome.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8275843: Random crashes while the UI code is executed

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/6199/head:pull/6199
$ git checkout pull/6199

Update a local copy of the PR:
$ git checkout pull/6199
$ git pull https://git.openjdk.java.net/jdk pull/6199/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 6199

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/6199.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Nov 1, 2021

👋 Welcome back serb! 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
Copy link

openjdk bot commented Nov 1, 2021

@mrserb The following label will be automatically applied to this pull request:

  • client

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the client client-libs-dev@openjdk.org label Nov 1, 2021
@mrserb mrserb marked this pull request as ready for review November 4, 2021 00:40
@openjdk openjdk bot added the rfr Pull request is ready for review label Nov 4, 2021
@mlbridge
Copy link

mlbridge bot commented Nov 4, 2021

Webrevs

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 2, 2021

@mrserb This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@mrserb
Copy link
Member Author

mrserb commented Dec 3, 2021

Any volunteers to review?

@bourgesl
Copy link
Contributor

bourgesl commented Dec 3, 2021

Looks good to me, not an official reviewer

@victordyakov
Copy link

@azvegint please review

@@ -396,8 +397,7 @@ public boolean copyArea(SunGraphics2D sg2d, int x, int y, int w, int h,
* Returns the XRender SurfaceType which is able to fullfill the specified
* transparency requirement.
*/
public static SurfaceType getSurfaceType(XRGraphicsConfig gc,
int transparency) {
public static SurfaceType getPixmapSurfaceType(int transparency) {
SurfaceType sType = null;

switch (transparency) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you are touching this, you might want to fix indent for cases in this switch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can change it, but looks like all switches in the "sun.java2d.xr" use this code style.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it is fine as it is.

@azvegint
Copy link
Member

I can confirm that testing looks good, however I didn't manage to get is crashed even once before the fix even with updated tests. (on personal, VM or multiple CI runs)

How often it fails for you and on what config?

@mrserb
Copy link
Member Author

mrserb commented Dec 13, 2021

How often it fails for you and on what config?

I think the easiest way to reproduce the crash is to force the x11 to use some completely different format, like this:
Xvfb :1 -screen 0 1920x1080x16

Note the usage of 16 bits depth. The vnc servers could be used as well.

In such config the new DrawCustomColorModel.java will crash most of the time(depends on timings)

@azvegint
Copy link
Member

I think the easiest way to reproduce the crash is to force the x11 to use some completely different format, like this: Xvfb :1 -screen 0 1920x1080x16

Yes, this one crashes for me almost instantly.

@@ -396,8 +397,7 @@ public boolean copyArea(SunGraphics2D sg2d, int x, int y, int w, int h,
* Returns the XRender SurfaceType which is able to fullfill the specified
* transparency requirement.
*/
public static SurfaceType getSurfaceType(XRGraphicsConfig gc,
int transparency) {
public static SurfaceType getPixmapSurfaceType(int transparency) {
SurfaceType sType = null;

switch (transparency) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it is fine as it is.

@openjdk
Copy link

openjdk bot commented Dec 13, 2021

@mrserb 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:

8275843: Random crashes while the UI code is executed

Reviewed-by: azvegint

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 no new commits pushed to the master branch. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you prefer to avoid any potential 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.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Dec 13, 2021
@mrserb
Copy link
Member Author

mrserb commented Dec 14, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Dec 14, 2021

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

  • 3f91948: 8278791: Rename ClassLoaderData::holder_phantom
  • 7761a3e: 8278761: Parallel: Remove unused PSOldPromotionLAB constructor

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Dec 14, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels Dec 14, 2021
@mrserb mrserb deleted the alpha branch December 14, 2021 18:04
@openjdk openjdk bot removed the rfr Pull request is ready for review label Dec 14, 2021
@openjdk
Copy link

openjdk bot commented Dec 14, 2021

@mrserb Pushed as commit a9c1acb.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client client-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants