7001973: java/awt/Graphics2D/CopyAreaOOB.java fails #5491

masyano · 2021-09-13T11:32:37Z

Could you please review the 7001973 fixes?

On Windows, CopyAreaOOB.java fails with a blank window. The cause of this problem is that paint() works one time only. Painting area is not guaranteed when showing a window.

I think this behavior should be removed for purpose of this test (@summary: Verifies that copyArea() works properly). Also, this program uses a Robot, but implements waiting logic by itself. This logic should be replaced to Robot API.

This fix works fine in Windows, so this test can be removed from ProblemList.txt. I don't have a Mac environment, so can someone please confirm that this fix will work on Mac?

Progress

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

Issue

JDK-7001973: java/awt/Graphics2D/CopyAreaOOB.java fails

Reviewers

Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5491

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

Using diff file

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

bridgekeeper · 2021-09-13T11:34:16Z

👋 Welcome back myano! 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 · 2021-09-13T11:36:14Z

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

mlbridge · 2021-09-14T08:00:25Z

Webrevs

01: Full - Incremental (52c5ca79)
00: Full (9b3f57a2)

mrserb · 2021-09-15T04:26:47Z

This test is quite strict and its purpose exactly check that the first paint() will properly render the windows content.
So if the window will paint some garbage and then repaint correct content this test wil catch that.

masyano · 2021-09-16T08:03:17Z

I read JDK-6430601 and JDK-8198613, described at @bug. JDK-6430601 is a background rendering problem in JInternalFrame with OpenGL pipeline. JDK-8198613 is a timeout problem with OpenGL pipeline, as a result, OpenGL pipeline test is removed.
So now, the purpose of this test is to confirm the usual drawing result on default settings, then I think there is no need to strict paint() at once.

mrserb · 2021-09-16T09:53:07Z

So now, the purpose of this test is to confirm the usual drawing result on default settings, then I think there is no need to strict paint() at once.

The test checks that the painting is correct, why it should check that a few times? If the paint method was called and the copyArea completed then why the window can be blank?

masyano · 2021-09-17T07:32:41Z

The test checks only one time for painting "result".

I think the painting sequence depends on rendering pipeline. I collected a log the Java2D and the ComponentPeer, then multiple PaintEvent is recorded with default D3D pipeline (see below). So, paint() should not be restrict one time.

$ /Downloads/jdk-17/bin/java -Dsun.java2d.trace=log,timestamp -Djava.util.logging.config.file=logging.properties CopyAreaOOB
Sep 17, 2021 3:30:57 PM sun.awt.windows.WComponentPeer coalescePaintEvent
FINEST: coalescePaintEvent: PAINT: add: x = 0, y = 0, width = 400, height = 400
Sep 17, 2021 3:30:57 PM sun.awt.windows.WComponentPeer coalescePaintEvent
FINEST: coalescePaintEvent: PAINT: add: x = 0, y = 0, width = 400, height = 400
Sep 17, 2021 3:30:57 PM sun.awt.windows.WComponentPeer coalescePaintEvent
FINEST: coalescePaintEvent: PAINT: add: x = 0, y = 0, width = 0, height = 0
Sep 17, 2021 3:30:57 PM sun.awt.windows.WComponentPeer coalescePaintEvent
FINEST: coalescePaintEvent: PAINT: add: x = 0, y = 0, width = 0, height = 0
Sep 17, 2021 3:30:57 PM sun.awt.windows.WComponentPeer coalescePaintEvent
FINEST: coalescePaintEvent: PAINT: add: x = 0, y = 0, width = 400, height = 400
Sep 17, 2021 3:30:57 PM sun.awt.windows.WComponentPeer coalescePaintEvent
FINEST: coalescePaintEvent: PAINT: add: x = 0, y = 0, width = 400, height = 400
1631860257699: D3DFillRect
1631860257732: D3DFillRect
Sep 17, 2021 3:30:57 PM sun.awt.windows.WComponentPeer coalescePaintEvent
FINEST: coalescePaintEvent: PAINT: add: x = 0, y = 0, width = 400, height = 400
1631860257741: D3DFillRect
1631860257743: D3DFillRect
1631860257750: D3DFillRect
1631860257751: D3DFillRect
1631860257755: D3DFillRect
1631860257757: D3DCopyArea
Sep 17, 2021 3:30:57 PM sun.awt.windows.WComponentPeer coalescePaintEvent
FINEST: coalescePaintEvent: PAINT: add: x = 0, y = 0, width = 400, height = 400
1631860257759: D3DFillRect
1631860257762: D3DFillRect
1631860257763: D3DFillRect
1631860257763: D3DFillRect
1631860257764: D3DFillRect
1631860257764: D3DCopyArea

$ /Downloads/jdk-17/bin/java -Dsun.java2d.trace=log,timestamp -Djava.util.logging.config.file=logging.properties -Duser.language=en -Dsun.java2d.noddraw CopyAreaOOB
Sep 17, 2021 3:32:10 PM sun.awt.windows.WComponentPeer coalescePaintEvent
FINEST: coalescePaintEvent: PAINT: add: x = 0, y = 0, width = 400, height = 400
1631860330225: GDIFillRect
1631860330249: GDIFillRect
1631860330254: GDIFillRect
1631860330254: GDIFillRect
1631860330255: GDICopyArea

mrserb · 2021-09-17T08:15:54Z

Let's check the sequence in the test:

Frame created
Undecorated property is set
peer is created, and native configuration completed
frame is moved to the center of the screen
frame became visible

It is fine to have a few paint events posted for the code above, my question was why the first CopyArea is ignored? If the awt/java2d is not ready yet to draw/repaint the window that events should not be posted, if it is ready then CopyArea should work, no?

masyano · 2021-09-24T11:17:18Z

my question was why the first CopyArea is ignored?

This is a RepaintArea's behavior. clearRect() is called before paint() because shouldClearRectBeforePaint is true for Canvas. Then, if 2nd paint() is called and paint() does nothing, blank window will be drawn.
https://github.com/openjdk/jdk/blob/master/src/java.desktop/share/classes/sun/awt/RepaintArea.java#L235

mrserb · 2021-09-24T19:57:50Z

This is a RepaintArea's behavior. clearRect() is called before paint() because shouldClearRectBeforePaint is true for Canvas. Then, if 2nd paint() is called and paint() does nothing, blank window will be drawn.

So the "sun.awt.noerasebackground" property may fix the problem? Or simple freeze the EDT after g2d.copyArea?

masyano · 2021-09-28T07:15:44Z

So the "sun.awt.noerasebackground" property may fix the problem?

Yes. With -Dsun.awt.noerasebackground=true, original CopyAreaOOB.java is Passed. But it is just a workaround.

paint() is called twice with default settings on Windows, so my suggested fix is more realistic and reasonable than original code with -Dsun.awt.noerasebackground=true.

mrserb · 2021-09-29T00:42:01Z

Yes. With -Dsun.awt.noerasebackground=true, original CopyAreaOOB.java is Passed. But it is just a workaround.

Yes, it is just a workaround that most probably will not work on other platforms. But EDT freeze for some time after copyarea will work.

paint() is called twice with default settings on Windows, so my suggested fix is more realistic and reasonable than original code with -Dsun.awt.noerasebackground=true.

Do you know why we post it twice? It does not seem right, since we repaint all content in the apps like Netbeans/Idea and it is quite a heavyweight operation.

masyano · 2021-09-30T08:40:44Z

Do you know why we post it twice? It does not seem right, since we repaint all content in the apps like Netbeans/Idea and it is quite a heavyweight operation.

Because SurfaceData is judged to be lost when 1st getGraphics() is called on 1st PaintEvent processing.

https://github.com/openjdk/jdk/blob/master/src/java.desktop/windows/classes/sun/java2d/d3d/D3DScreenUpdateManager.java#L478

I traced PaintEvent, then I got following stacktrace.

breakpoint hit: "Thread=AWT-EventQueue-0", java.awt.event.PaintEvent.<init>()、line=106 bci=0

  [1] java.awt.event.PaintEvent.<init> (PaintEvent.java:106)
  [2] sun.awt.PaintEventDispatcher.createPaintEvent (PaintEventDispatcher.java:80)
  [3] sun.awt.windows.WComponentPeer.postPaintIfNecessary (WComponentPeer.java:892)
  [4] sun.awt.windows.WComponentPeer.handlePaint (WComponentPeer.java:886)
  [5] sun.java2d.d3d.D3DScreenUpdateManager.repaintPeerTarget (D3DScreenUpdateManager.java:283)
  [6] sun.java2d.d3d.D3DScreenUpdateManager.validate (D3DScreenUpdateManager.java:492)
  [7] sun.java2d.d3d.D3DScreenUpdateManager.createGraphics (D3DScreenUpdateManager.java:260)
  [8] sun.awt.windows.WComponentPeer.getGraphics (WComponentPeer.java:646)
  [9] java.awt.Component.getGraphics (Component.java:3,128)
  [10] sun.awt.RepaintArea.paint (RepaintArea.java:227)
  [11] sun.awt.windows.WComponentPeer.handleEvent (WComponentPeer.java:371)
  [12] java.awt.Component.dispatchEventImpl (Component.java:5,056)
  [13] java.awt.Container.dispatchEventImpl (Container.java:2,325)
  [14] java.awt.Window.dispatchEventImpl (Window.java:2,775)
  [15] java.awt.Component.dispatchEvent (Component.java:4,827)
  [16] java.awt.EventQueue.dispatchEventImpl (EventQueue.java:772)
  [17] java.awt.EventQueue$4.run (EventQueue.java:721)
  [18] java.awt.EventQueue$4.run (EventQueue.java:715)
  [19] java.security.AccessController.executePrivileged (AccessController.java:753)
  [20] java.security.AccessController.doPrivileged (AccessController.java:391)
  [21] java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege (ProtectionDomain.java:85)
  [22] java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege (ProtectionDomain.java:95)
  [23] java.awt.EventQueue$5.run (EventQueue.java:745)
  [24] java.awt.EventQueue$5.run (EventQueue.java:743)
  [25] java.security.AccessController.executePrivileged (AccessController.java:753)
  [26] java.security.AccessController.doPrivileged (AccessController.java:391)
  [27] java.security.ProtectionDomain$JavaSecurityAccessImpl.doIntersectionPrivilege (ProtectionDomain.java:85)
  [28] java.awt.EventQueue.dispatchEvent (EventQueue.java:742)
  [29] java.awt.EventDispatchThread.pumpOneEventForFilters (EventDispatchThread.java:203)
  [30] java.awt.EventDispatchThread.pumpEventsForFilter (EventDispatchThread.java:124)
  [31] java.awt.EventDispatchThread.pumpEventsForHierarchy (EventDispatchThread.java:113)
  [32] java.awt.EventDispatchThread.pumpEvents (EventDispatchThread.java:109)
  [33] java.awt.EventDispatchThread.pumpEvents (EventDispatchThread.java:101)
  [34] java.awt.EventDispatchThread.run (EventDispatchThread.java:90)
AWT-EventQueue-0[1] > >

At 1st PaintEvent is processing, RepaintArea calls getGraphics(), then D3DScreenUpdateManager finally calls new PaintEvent().
So, 2nd PaintEvent is posted and invoked later on EDT.

mrserb · 2021-10-01T03:00:04Z

At 1st PaintEvent is processing, RepaintArea calls getGraphics(), then D3DScreenUpdateManager finally calls new PaintEvent().
So, 2nd PaintEvent is posted and invoked later on EDT.

It does not look right, isn't it? Every time we show a frame we render its content twice, and the first rendering is always discarded. Probably we should validate the surface before we posting the first paint event and before we show the window.

masyano · 2021-10-06T10:53:32Z

I think we should fix D3DScreenUpdateManager. I would like to create a new issue for it.

But I think this test should be fixed, because the purpose of this test is to confirm the usual drawing result on default settings, then there is no need to restrict paint() one time.

If the test that verifies paint() isn't called twice is needed, we should create a new test in the new bug id.

mrserb · 2021-10-12T06:27:14Z

You will not be able to write the test which will check that only one paint event is posted, it is not guaranteed.
You can only check that each rendering caused by the paint event will work fine. This test case check that the first event works fine, it can be improved to check all paint events.

So for the test update, you can add a big delay after the rendering, and wait for what will occur on the screen, any ideas why it may not work? You also can create a separate bug to investigate how we can validate the surface before showing the window so we did not post more events than necessary.

masyano · 2021-10-15T11:06:26Z

I understood what you wanted to test.

I think all paint events can be examined by only checking at the end of paint(). So I moved checkRegion into paint() part. It also works fine in case of multi paint events happen when restoring Frame from the iconified status to normal status.

masyano · 2021-10-21T07:42:35Z

@mrserb Could you please reply to the above comments and fix of test?

mrserb · 2021-10-23T19:34:06Z

This looks fine, but should be checked on other platforms Linux/macOS since it can uncover some issues there.

masyano · 2021-10-25T10:52:25Z

@mrserb I have tested on Linux, and it worked fine. But I don't have MacOS environment. Could you test it instead of me?

mrserb · 2021-10-26T04:52:37Z

I'll check it, let me some time.

masyano · 2021-11-01T09:00:10Z

@mrserb Thank you for helping me with the test. How many more days does the test run?

masyano · 2021-11-05T12:20:50Z

@mrserb Could you please reply to the above comment?

masyano · 2021-11-11T08:38:17Z

@mrserb How is the test running? Could you give me a reply?

masyano · 2021-11-16T09:28:02Z

@mrserb Could you tell me how long it will take to run the test?

masyano · 2021-11-22T09:33:43Z

@mrserb Could you please reply to the above comment?

masyano · 2021-12-14T11:46:01Z

@mrserb I have tested on MacOS. The drawing result has no problem, and passed with no Exception. Please review this PR again.

masyano · 2021-12-22T10:58:40Z

@mrserb Could you please review the above comments?

openjdk · 2021-12-27T20:59:41Z

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

7001973: java/awt/Graphics2D/CopyAreaOOB.java fails

Reviewed-by: serb

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 961 new commits pushed to the master branch:

6b906bb: 8279223: Define version in .jcheck/conf
c295e71: 8276700: Improve java.lang.ref.Cleaner javadocs
3a1fca3: 8278146: G1: Rework VM_G1Concurrent VMOp to clearly identify it as pause
2a59ebb: 8279119: src/jdk.hotspot.agent/doc/index.html file contains references to scripts that no longer exist
299022d: 8279225: [arm32] C1 longs comparison operation destroys argument registers
4f607f2: Merge
54b800d: 8271202: C1: assert(false) failed: live_in set of first block must be empty
2945b78: 8279195: Document the -XX:+NeverActAsServerClassMachine flag
6588bed: 8278889: AArch64: [vectorapi] VectorMaskLoadStoreTest.testMaskCast() test fail
7fea103: 8279134: Fix Amazon copyright in various files
... and 951 more: https://git.openjdk.java.net/jdk/compare/322b1301061ba113dc5f7f3710dde2d80a18a14e...master

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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@mrserb) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

masyano · 2022-01-04T00:45:57Z

/integrate

openjdk · 2022-01-04T00:47:04Z

@masyano
Your change (at version 52c5ca7) is now ready to be sponsored by a Committer.

masyano · 2022-01-12T06:43:07Z

@mrserb Could you please commit this fix as a sponsor?

mrserb · 2022-01-15T02:16:36Z

/sponsor

openjdk · 2022-01-15T02:17:59Z

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

9b0f689: 8279947: Remove two redundant gvn.transform calls in Parse::do_one_bytecode()
eab4e6d: 8280045: ProblemList 2 AppCDS tests until JDK-8279970 is fixed
0d1a97f: 8279064: New options for ktab to provide non-default salt
c359c35: 8280002: jmap -histo may leak stream
fb8fdc0: 8279990: (fs) Awkward verbiage in description of Files.createTempFile(Path,String,String,FileAttribute)
d9dd485: 8280019: Remove unused code from metaspace
cf283e2: 8279570: IGV: Add source/destination property for load and store nodes with an associated field
f180530: 8279918: Fix various doc typos
ac98b22: 8280028: [BACKOUT] Parallel: More precise boundary in ObjectStartArray::object_starts_in_range
61b8944: 8278851: Correct signer logic for jars signed with multiple digestalgs
... and 1086 more: https://git.openjdk.java.net/jdk/compare/322b1301061ba113dc5f7f3710dde2d80a18a14e...master

Your commit was automatically rebased without conflicts.

openjdk · 2022-01-15T02:18:14Z

@mrserb @masyano Pushed as commit 22b7295.

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

7001973: java/awt/Graphics2D/CopyAreaOOB.java fails

6304666

openjdk bot added the client client-libs-dev@openjdk.org label Sep 13, 2021

7001973: java/awt/Graphics2D/CopyAreaOOB.java fails

9b3f57a

openjdk bot added the rfr Pull request is ready for review label Sep 14, 2021

Yano, Masanori added 2 commits October 15, 2021 18:41

Merge branch 'master' of https://github.com/masyano/jdk into 7001973

d5a01ec

7001973: java/awt/Graphics2D/CopyAreaOOB.java fails

52c5ca7

mrserb approved these changes Dec 27, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 27, 2021

openjdk bot added the sponsor Pull request is ready to be sponsored label Jan 4, 2022

openjdk bot added the integrated Pull request has been integrated label Jan 15, 2022

openjdk bot closed this Jan 15, 2022

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Jan 15, 2022

7001973: java/awt/Graphics2D/CopyAreaOOB.java fails #5491

7001973: java/awt/Graphics2D/CopyAreaOOB.java fails #5491

Conversation

masyano commented Sep 13, 2021 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Sep 13, 2021

openjdk bot commented Sep 13, 2021

mlbridge bot commented Sep 14, 2021 • edited Loading

Webrevs

mrserb commented Sep 15, 2021

masyano commented Sep 16, 2021

mrserb commented Sep 16, 2021

masyano commented Sep 17, 2021

mrserb commented Sep 17, 2021

masyano commented Sep 24, 2021

mrserb commented Sep 24, 2021

masyano commented Sep 28, 2021

mrserb commented Sep 29, 2021

masyano commented Sep 30, 2021

mrserb commented Oct 1, 2021

masyano commented Oct 6, 2021

mrserb commented Oct 12, 2021

masyano commented Oct 15, 2021

masyano commented Oct 21, 2021

mrserb commented Oct 23, 2021

masyano commented Oct 25, 2021

mrserb commented Oct 26, 2021

masyano commented Nov 1, 2021

masyano commented Nov 5, 2021

masyano commented Nov 11, 2021

masyano commented Nov 16, 2021

masyano commented Nov 22, 2021

masyano commented Dec 14, 2021

masyano commented Dec 22, 2021

openjdk bot commented Dec 27, 2021 • edited Loading

masyano commented Jan 4, 2022

openjdk bot commented Jan 4, 2022

masyano commented Jan 12, 2022

mrserb commented Jan 15, 2022

openjdk bot commented Jan 15, 2022

openjdk bot commented Jan 15, 2022

masyano commented Sep 13, 2021 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 14, 2021 •

edited

Loading

openjdk bot commented Dec 27, 2021 •

edited

Loading