8297923: java.awt.ScrollPane broken after multiple scroll up/down #14338

aivanov-jdk · 2023-06-06T16:07:53Z

Problem description

If you grab the thumb of the scroll bar of ScrollPane and drag it slowly and continuously up and down, you'll notice the UI stops rendering correctly: the child component of the scroll pane will render on the left of the frame itself, the inside of the scroll pane will be filled with the background color of its child component.

Root cause

AWT calls the ::SetScrollInfo function on EDT when AWT processes scroll bar tracking events. This Windows API is not thread-safe, calling this function on an incorrect thread leads to leaking GDI objects.

When the process reaches the limit on the number of GDI objects, it cannot create new GDI objects, which results in rendering issues.

Fix

To resolve the problem, I modified the code so that ::SetScrollInfo and ::GetScrollInfo are always called on the toolkit thread, all AWT components are created on the toolkit thread.

An automatic test is provided. The test scrolls the vertical scroll bar up and down. Then the test takes a screenshot. When the bug is reproduced, the robot cannot create new GDI objects to capture the screenshot and it throws OutOfMemoryError.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8297923: java.awt.ScrollPane broken after multiple scroll up/down (Bug - P3)

Reviewers

Harshitha Onkar (@honkar-jdk - Committer)
Phil Race (@prrace - Reviewer)
Sergey Bylokhov (@mrserb - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 14338

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/14338.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-06-06T16:09:26Z

👋 Welcome back aivanov! 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.

aivanov-jdk · 2023-06-06T16:11:38Z

The screenshot of the test case after the problem is reproduced:

If you scroll up or down, the left side of the frame gets filled with the light-green color of the canvas which is the child component inside the scroll pane.

openjdk · 2023-06-06T16:11:54Z

@aivanov-jdk 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 · 2023-06-06T16:16:05Z

Webrevs

00: Full (989d40a0)

prrace · 2023-06-06T18:17:42Z

src/java.desktop/windows/native/libawt/windows/awt_ScrollPane.cpp

@@ -701,7 +701,7 @@ Java_sun_awt_windows_WScrollPanePeer_getOffset(JNIEnv *env, jobject self,
    gos->scrollpane = env->NewGlobalRef(self);
    gos->orient = orient;

-    return static_cast<jint>(reinterpret_cast<INT_PTR>(AwtToolkit::GetInstance().SyncCall(
+    return static_cast<jint>(reinterpret_cast<INT_PTR>(AwtToolkit::GetInstance().InvokeFunction(


So this needs to block until the toolkit thread can process this and return, but since its
directly called from Java (ie we are in a JNI method) I think this is likely fine.

That's right.

In addition to that, WScrollPanePeer.getScrollOffset is unused, so it's never called.

If it is unused can we delete it?

If it is unused can we delete it?

It's possible. Yet I'd rather leave it for another time.

There's also a specific WM_AWT_SET_SCROLL_INFO message to call ::SetScrollInfo. This message is never used, it can also be removed.

prrace · 2023-06-06T18:19:00Z

src/java.desktop/windows/native/libawt/windows/awt_ScrollPane.cpp

@@ -742,7 +742,7 @@ Java_sun_awt_windows_WScrollPanePeer_setScrollPosition(JNIEnv *env,
    ssps->x = x;
    ssps->y = y;

-    AwtToolkit::GetInstance().SyncCall(AwtScrollPane::_SetScrollPos, ssps);
+    AwtToolkit::GetInstance().InvokeFunctionLater(AwtScrollPane::_SetScrollPos, ssps);


Whereas in the _GetOffset case above, you clearly need to wait until the result is returned - I guess
you didn't see a need to block here ? Or the case below ?

I had a similar clarification as to why it was InvokeFunction for _getOffset and InvokeFunctionLater for _SetScrollPos and _SetSpans. The above discussion makes it clear.

No, there's no need to wait, and no result is returned.

Java has updated the position of the child component based on the new position of the scroll bar. Now the position of the non-client scroll bars of the scroll pane window need to be updated.

The _SetSpans recalculates the position and range of the scroll bars if the size of the scroll pane or its child component changes. The changes are already handled on the Java side, these changes need to be reflected in the scroll bars.

I have found no scenario where we need to block EDT.

honkar-jdk

The screenshot of the test case after the problem is reproduced:

If you scroll up or down, the left side of the frame gets filled with the light-green color of the canvas which is the child component inside the scroll pane.

Test works as expected with the fix.
Without the fix, OutOfMemory exception is thrown as described.
When running the test without the fix, I had to introduce a small delay after the frame is maximized again to see the light-green block outside the canvas.

aivanov-jdk · 2023-06-06T18:50:23Z

When running the test without the fix, I had to introduce a small delay after the frame is maximized again to see the light-green block outside the canvas.

You can use the test case attached to JDK-8297923, ScrollPaneTest.java, to play around. Alternatively, you can comment out disposing of the frame in the automatic test.

openjdk · 2023-06-06T21:00:07Z

@aivanov-jdk 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:

8297923: java.awt.ScrollPane broken after multiple scroll up/down

Reviewed-by: honkar, prr, 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 46 new commits pushed to the master branch:

92beb85: 8309474: [IR Framework] Wrong @ForceCompile link in README
6402004: 8256302: releasing oopStorage when deflating allows for faster deleting
02bce0b: 8309532: java/lang/Class/getDeclaredField/FieldSetAccessibleTest should filter modules that depend on JVMCI
4ffc8cc: 8309574: Improve core reflection tests for JEP 445
e3f3ac0: 8309420: com/sun/jdi/StepTest.java fails with virtual thread wrapper
c38abbf: 8309510: com/sun/jdi/RedefineNestmateAttr/TestNestmateAttr.java no longer needs to override startUp() method
c24b0ba: 8309503: Improve javax.lang.model tests for JEP 445
a54f4d4: 8309509: com/sun/jdi/RedefineNestmateAttr/TestNestmateAttr.java fails with virtual test thread factory
33bb64f: 8309614: [BACKOUT] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING
5b147eb: 8308288: Fix xlc17 clang warnings and build errors in hotspot
... and 36 more: https://git.openjdk.org/jdk/compare/ca6f07f9ab70d552061a2715342f1227287f2d94...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.

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

prrace · 2023-06-06T21:00:20Z

I wonder if we have any other leaks due to similar thread mis-usage ?
Seems possible since this one went un-detected for a long time and its
probably one of the easier ones to get to run out of GDI objects.

mrserb · 2023-06-07T04:08:49Z

Probably we should update the Java_sun_awt_windows_WScrollbarPeer_setValues as well? it calls the SetScrollInfo

aivanov-jdk · 2023-06-07T11:01:47Z

Probably we should update the Java_sun_awt_windows_WScrollbarPeer_setValues as well? it calls the SetScrollInfo

You're right. It needs to be updated too. However, I'd rather do it separately.

mrserb · 2023-06-07T20:32:40Z

Probably we should update the Java_sun_awt_windows_WScrollbarPeer_setValues as well? it calls the SetScrollInfo

You're right. It needs to be updated too. However, I'd rather do it separately.

That is up2you, but it looks strange when the patch updates the code which is never executed and skips the code which is executed and has the bug

aivanov-jdk · 2023-06-07T20:51:52Z

Probably we should update the Java_sun_awt_windows_WScrollbarPeer_setValues as well? it calls the SetScrollInfo

You're right. It needs to be updated too. However, I'd rather do it separately.

That is up2you, but it looks strange when the patch updates the code which is never executed and skips the code which is executed and has the bug

It has taken a very long time till the root cause for this bug was identified. I admit I was too focused on the code for ScrollPane only and I overlooked the fact that ::SetScrollInfo is also used in ScrollBar.

As such, the updated code is limited to the native implementation of ScrollPane. Let's leave it this way.

Thank you for finding a similar problem in ScrollBar, it needs fixing too.

aivanov-jdk · 2023-06-07T21:18:20Z

/integrate

openjdk · 2023-06-07T21:18:52Z

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

99749c5: 8309562: [JVMCI] Export symbols used by VirtualThread notifyJvmti intrinsics to JVMCI compilers.
92beb85: 8309474: [IR Framework] Wrong @ForceCompile link in README
6402004: 8256302: releasing oopStorage when deflating allows for faster deleting
02bce0b: 8309532: java/lang/Class/getDeclaredField/FieldSetAccessibleTest should filter modules that depend on JVMCI
4ffc8cc: 8309574: Improve core reflection tests for JEP 445
e3f3ac0: 8309420: com/sun/jdi/StepTest.java fails with virtual thread wrapper
c38abbf: 8309510: com/sun/jdi/RedefineNestmateAttr/TestNestmateAttr.java no longer needs to override startUp() method
c24b0ba: 8309503: Improve javax.lang.model tests for JEP 445
a54f4d4: 8309509: com/sun/jdi/RedefineNestmateAttr/TestNestmateAttr.java fails with virtual test thread factory
33bb64f: 8309614: [BACKOUT] JDK-8307153 JVMTI GetThreadState on carrier should return STATE_WAITING
... and 37 more: https://git.openjdk.org/jdk/compare/ca6f07f9ab70d552061a2715342f1227287f2d94...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-06-07T21:18:59Z

@aivanov-jdk Pushed as commit ea41907.

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

aivanov-jdk · 2023-09-05T12:54:52Z

I have submitted the follow-up bugs.

Probably we should update the Java_sun_awt_windows_WScrollbarPeer_setValues as well? it calls the SetScrollInfo. ⇑

JDK-8315690: Call ::SetScrollInfo on toolkit thread in awt.ScrollBar

In addition to that, WScrollPanePeer.getScrollOffset is unused, so it's never called. ⇑

JDK-8315691: Remove unused WScrollPanePeer.getScrollOffset method

There's also a specific WM_AWT_SET_SCROLL_INFO message to call ::SetScrollInfo. This message is never used, it can also be removed. ⇑

JDK-8315693: Remove WM_AWT_SET_SCROLL_INFO message

aivanov-jdk · 2023-09-05T13:10:58Z

I wonder if we have any other leaks due to similar thread mis-usage ? Seems possible since this one went un-detected for a long time and its probably one of the easier ones to get to run out of GDI objects.

@prrace Potentially, all the usages of SyncCall are at risk.

jdk/src/java.desktop/windows/native/libawt/windows/awt_Toolkit.cpp

Lines 1870 to 1878 in ed2b467

    
           void AwtToolkit::SyncCall(void (*ftn)(void *), void *param) { 
        
               JNIEnv *env = (JNIEnv *)JNU_GetEnv(jvm, JNI_VERSION_1_2); 
        
               if (!IsMainThread()) { 
        
                   CriticalSection::Lock l(GetSyncCS()); 
        
                   (*ftn)(param); 
        
               } else { 
        
                   (*ftn)(param); 
        
               } 
        
           }

It does not even synchronise the threads. So, there could be other leaks or issues with memory visibility.

I think this scenario led to leaks because it also modified the position of the horizontal scroll bar, which started animation provided by visual styles. The animation never got a chance to complete before another update came. The issue wasn't reproducible if there was only vertical scroll bar which was being dragged.

aivanov-jdk added 2 commits June 6, 2023 15:17

8297923: Invoke SetScrollInfo and GetScrollInfo on toolkit thread

3d94fcf

8297923: Automatic test for leaked GDI objects in ScrollPane

989d40a

openjdk bot added the rfr Pull request is ready for review label Jun 6, 2023

openjdk bot added the client client-libs-dev@openjdk.org label Jun 6, 2023

prrace reviewed Jun 6, 2023

View reviewed changes

honkar-jdk reviewed Jun 6, 2023

View reviewed changes

prrace approved these changes Jun 6, 2023

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jun 6, 2023

honkar-jdk approved these changes Jun 6, 2023

View reviewed changes

mrserb approved these changes Jun 7, 2023

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Jun 7, 2023

openjdk bot closed this Jun 7, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 7, 2023

aivanov-jdk deleted the 8297923-scrollPane-fix branch June 8, 2023 18:10

aivanov-jdk mentioned this pull request Sep 7, 2023

8313348: Fix typo in JFormattedTextField: 'it self' #15087

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8297923: java.awt.ScrollPane broken after multiple scroll up/down #14338

8297923: java.awt.ScrollPane broken after multiple scroll up/down #14338

aivanov-jdk commented Jun 6, 2023 •

edited by openjdk bot

bridgekeeper bot commented Jun 6, 2023

aivanov-jdk commented Jun 6, 2023

openjdk bot commented Jun 6, 2023

mlbridge bot commented Jun 6, 2023

prrace Jun 6, 2023

aivanov-jdk Jun 6, 2023

mrserb Jun 7, 2023

aivanov-jdk Jun 7, 2023

prrace Jun 6, 2023

honkar-jdk Jun 6, 2023 •

edited

aivanov-jdk Jun 6, 2023 •

edited

honkar-jdk left a comment •

edited

aivanov-jdk commented Jun 6, 2023

openjdk bot commented Jun 6, 2023 •

edited

prrace commented Jun 6, 2023

mrserb commented Jun 7, 2023 •

edited

aivanov-jdk commented Jun 7, 2023

mrserb commented Jun 7, 2023

aivanov-jdk commented Jun 7, 2023

aivanov-jdk commented Jun 7, 2023

openjdk bot commented Jun 7, 2023

openjdk bot commented Jun 7, 2023

aivanov-jdk commented Sep 5, 2023

aivanov-jdk commented Sep 5, 2023 •

edited

8297923: java.awt.ScrollPane broken after multiple scroll up/down #14338

8297923: java.awt.ScrollPane broken after multiple scroll up/down #14338

Conversation

aivanov-jdk commented Jun 6, 2023 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Jun 6, 2023

aivanov-jdk commented Jun 6, 2023

openjdk bot commented Jun 6, 2023

mlbridge bot commented Jun 6, 2023

Webrevs

prrace Jun 6, 2023

Choose a reason for hiding this comment

aivanov-jdk Jun 6, 2023

Choose a reason for hiding this comment

mrserb Jun 7, 2023

Choose a reason for hiding this comment

aivanov-jdk Jun 7, 2023

Choose a reason for hiding this comment

prrace Jun 6, 2023

Choose a reason for hiding this comment

honkar-jdk Jun 6, 2023 • edited

Choose a reason for hiding this comment

aivanov-jdk Jun 6, 2023 • edited

Choose a reason for hiding this comment

honkar-jdk left a comment • edited

Choose a reason for hiding this comment

aivanov-jdk commented Jun 6, 2023

openjdk bot commented Jun 6, 2023 • edited

prrace commented Jun 6, 2023

mrserb commented Jun 7, 2023 • edited

aivanov-jdk commented Jun 7, 2023

mrserb commented Jun 7, 2023

aivanov-jdk commented Jun 7, 2023

aivanov-jdk commented Jun 7, 2023

openjdk bot commented Jun 7, 2023

openjdk bot commented Jun 7, 2023

aivanov-jdk commented Sep 5, 2023

aivanov-jdk commented Sep 5, 2023 • edited

aivanov-jdk commented Jun 6, 2023 •

edited by openjdk bot

honkar-jdk Jun 6, 2023 •

edited

aivanov-jdk Jun 6, 2023 •

edited

honkar-jdk left a comment •

edited

openjdk bot commented Jun 6, 2023 •

edited

mrserb commented Jun 7, 2023 •

edited

aivanov-jdk commented Sep 5, 2023 •

edited