8353950: Clipboard interaction on Windows is unstable #24614
Conversation
For investigation/testing issue JDK-8353950 it is necessary to create a
non-crashing baseline. Testing JDK-8353950 was done with a fastdebug
build of the JDK. The build was modified so that instead of blockingly
showing a messagebox, only CLI output was generated when an assertion
was hit. This is necessary, so that the reproducer runs with higher
concurrency.
The call to MessageBoxA was commented out for this in awt_Debug.cpp.
Reproducer Code
===============
```java
import java.awt.Toolkit;
import java.awt.datatransfer.FlavorEvent;
import java.awt.datatransfer.FlavorListener;
import java.awt.datatransfer.StringSelection;
import java.util.concurrent.atomic.AtomicBoolean;
import javax.swing.SwingUtilities;
public class Clipboardreproducer2 {
private static final System.Logger LOG = System.getLogger(Clipboardreproducer2.class.getName());
public static void main(String[] args) throws Exception {
AtomicBoolean end = new AtomicBoolean(false);
SwingUtilities.invokeLater(() -> {
Toolkit.getDefaultToolkit().getSystemClipboard().addFlavorListener(new FlavorListener() {
@OverRide
public void flavorsChanged(FlavorEvent e) {
LOG.log(System.Logger.Level.INFO, "Flavor Changed: {0} // {1}", e, SwingUtilities.isEventDispatchThread());
}
});
});
Runnable copy = new Runnable() {
@OverRide
public void run() {
try {
Toolkit.getDefaultToolkit().getSystemClipboard().setContents(new StringSelection("Test"), null);
// LOG.log(System.Logger.Level.INFO, "Copy 1");
} catch (IllegalStateException ex) {
LOG.log(System.Logger.Level.ERROR, "Failed to invoke copy (1)", ex);
}
if(! end.get()) {
SwingUtilities.invokeLater(this);
} else {
System.out.println("Exitting");
}
}
};
SwingUtilities.invokeLater(copy);
Thread.sleep(60 * 1000);
LOG.log(System.Logger.Level.INFO, "DONE");
end.set(true);
Thread.sleep(500);
LOG.log(System.Logger.Level.INFO, "END");
System.exit(0);
}
}
```
Crash message
=============
```
#
# A fatal error has been detected by the Java Runtime Environment:
#
# Internal Error (c:\temp\jdk\src\hotspot\share\runtime/jniHandles.inline.hpp:81), pid=3816, tid=5224
# assert(external_guard || result != nullptr) failed: Invalid JNI handle
#
# JRE version: OpenJDK Runtime Environment (25.0) (fastdebug build 25-internal-adhoc.Matthias.jdk)
# Java VM: OpenJDK 64-Bit Server VM (fastdebug 25-internal-adhoc.Matthias.jdk, mixed mode, tiered, compressed oops, compressed class ptrs, g1 gc, windows-amd64)
# Problematic frame:
# V [jvm.dll+0x5160eb] JNIHandles::resolve_impl<0,0>+0x27b
#
# Core dump will be written. Default location: c:\Temp\hs_err_pid3816.mdmp
#
# An error report file with more information is saved as:
# c:\Temp\hs_err_pid3816.log
[16.331s][warning][os] Loading hsdis library failed
#
# If you would like to submit a bug report, please visit:
# https://bugreport.java.com/bugreport/crash.jsp
#
```
The code from JDK-8332271 was also tested, but that did not yield a
visible result. It crashes, but the only data recorded is an entry in
the eventlog of windows.
Analysis
========
Looking through the code in `awt_Clipboard.cpp` there is a possible race
condition. Each call to openClipboard with a non-null caller will reset
the value of AwtClipboard::theCurrentClipboard. That variable holds a
reference to the java peer of the native clipboard.
The race is (line number refer to state in commit
1d7138f):
Thread 1: Application code calls Clipboard#setContents. On windows the
code now flows through WClipboard#openClipboard and will run to
awt_Clipboard.cpp line 145 (this is in the native implementatio of
openClipboard. Just before DeleteGlobalRef.
Thread 2: The windows event loop gets a WM_CLIPBOARDUPDATE message and
calls into AwtClipboard::WmClipboardUpdate (awt_Clipboard.cpp line 60).
The code runs through the non-null check and just before the
`CallVoidMethod` call.
Thread 1: Continues and executes DeleteGlobalRef.
Thread 2: Continues exection with:
`env->CallVoidMethod(theCurrentClipboard, lostSelectionOwnershipMID)`.
But at this point theCurrentClipboard does not hold a valid object
handle anymore and crashes the VM.
Proposed solution
=================
The code already makes assumptions, that there is only one AwtToolkit
(see calls to AwtToolkit::GetInstance). In addition
WToolkit#getSystemClipboard ensures, that only one WClipboard will
be created for this.
Based on this, this change drops the ability to pass in a new Clipboard
on each call to `openClipboard`. Instead the registration of the
java side of the clipboard with the native side is moved to construction
time of the WClipboard object. `openClipboard` is modified, so that it
verifies the assumption asserting that a new owner matches the initial
owner.
The focus of this change is the windows port of the AWT. openClipboard and closeClipboard are only implemented there. The general idea here is, that the section between openClipboard and closeClipboard must be considered a critical section, that must not be accessed concurrently. The fix idea here is to use a ReentrantLock to protect the native side. openClipboard now check if the lock can be locked (tryLock) if that fails, an IllegalStateException is raised. This is defined behavior for the accessors from client code (see for example the documentation for Clipboard#setContents and Clipboard#getContents. The remaining problem is the upcall from the WToolkit. The message loop handler has to react to the WM_CLIPBOARDUPDATE. When that message is received an upcall to WClipboard#handleContentsChanged is invoked. That in turn will make a downcall to the native getClipboardFormats. That method used EnumClipboardFormats to enumerate the formats in the clipboard. The problem is, that EnumClipboardFormats need the clipboard to be opened (as implemented in openClipboard). This is problematic as the upcall from native should not lead to an exception. Even if the exception is caught, it will result in flavor listeners not being called. Since Windows Vista / Windows Server 2008 GetUpdatedClipboardFormats can be used, which does not need an open clipboard and thus would be safe to be called from the upcall. Testing: - fastdebug build does not hit asserts anymore for the reproducer code (see previous commit) - reproducer code for JDK-8332271 does not crash anymore. Code was run for multiple minutes and no crash was observed - tests from test/jdk/java/awt/Clipboard were run and tests succeeded in release and fastdebug configuration
|
👋 Welcome back mblaesing! A progress list of the required criteria for merging this PR into |
|
@matthiasblaesing 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: 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 1014 new commits pushed to the
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 (@DamonGuy, @kumarabhi006) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@matthiasblaesing The following label will be automatically applied to this pull request:
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. |
|
Some further comments:
|
Webrevs
|
| @Override | ||
| public native void closeClipboard(); | ||
| public void closeClipboard() { | ||
| if(clipboardLocked.isLocked()) { |
There was a problem hiding this comment.
| if(clipboardLocked.isLocked()) { | |
| if (clipboardLocked.isLocked()) { |
|
copyright year update is missing |
|
@matthiasblaesing Footnotes |
|
@ExE-Boss Only the author (@matthiasblaesing) is allowed to issue the |
|
/issue add JDK-8332271 |
|
@matthiasblaesing |
|
It would be great to get a review on this. While the issues are "only" P-3 and P-4, the issues leave a very bad image for java on windows. Copy-and-Paste should just work and not fail randomly and maybe even crash the VM. The issue is serious enough for NetBeans, that it will ship a java agent to work around this problem, but that will not implement this change here and is clearly not an option in the long run and thus this needs a real fix. |
|
is it possible to create a jtreg test for this issue? |
|
@mrserb added the reproducer and validated, that it works:
The test was run via: CONF=windows-x86_64-server-release make test TEST="test/jdk/java/awt/Clipboard/ConcurrentClipboardAccessTest.java"
# and
CONF=windows-x86_64-server-fastdebug make test TEST="test/jdk/java/awt/Clipboard/ConcurrentClipboardAccessTest.java" |
|
@mrserb would you mind having another look at this? |
| long[] formats = getClipboardFormats(); | ||
| checkChange(formats); | ||
| } catch (Throwable ex) { | ||
| System.getLogger(WClipboard.class.getName()).log(Level.WARNING, "Failed to process handleContentsChanged", ex); |
There was a problem hiding this comment.
I'm not saying not to necessarily change this, but what are we hoping to see with logging this warning here?
There was a problem hiding this comment.
The idea here is to not silently swallow exceptions. This code is called from AwtToolkit::WndProc, which from my reading will not report exceptions/throwables. Pushing the exception to the system logger gives a chance to detect problems here.
There was a problem hiding this comment.
Sounds reasonable but loggers don't seem to be used in src files as far as I can tell. They are used in tests to help detect issues as you mentioned, and I can find plenty of occurrences of loggers used in the test directory of the open JDK but I don't see any in src. Makes me think there's an alternative way to handle this.
There was a problem hiding this comment.
I dropped the log level to DEBUG that way in normal operation this will not visible (and shut not affect performance). I think this might be a way to address the concern.
| openClipboard(null); | ||
|
|
||
| try { | ||
| openClipboard(null); |
There was a problem hiding this comment.
Can you help me understand why this is better? Seems odd that the try/finally block has closeClipboard in the finally block but the openClipboard was moved out of the try.
There was a problem hiding this comment.
Before the change the call order was inconsistent:
ClipboardTransferable: one call,openClipboardis called before thetry-blockWClipboard: one call withopenClipboardbefore thetry-block, one call withopenClipboardas first statement in thetry-blockSunClipboard: three calls withopenClipboardas first statement inside thetry-block
I unified these so that in all cases openClipboard is called as the last statement before the try-block. The assumption is, that only if openClipboard succeeds (does not raise an exception), calling closeClipboard makes sense.
| /* | ||
| @test | ||
| @bug 8332271 | ||
| @summary tests that concurrent access to the clipboard does not crash the JVM | ||
| @run main ConcurrentClipboardAccessTest | ||
| */ |
There was a problem hiding this comment.
It has been brought to my attention that this test needs to be headful since we're dealing with clipboard behavior.
There was a problem hiding this comment.
You are right, thanks for catching this. Pushed an updated version.
|
@DamonGuy thanks for your check. I updated the PR and added replies. If more changes/informations are required, I'm willing to help. |
| @@ -109,13 +115,32 @@ protected void clearNativeContext() {} | |||
| * @throws IllegalStateException if the clipboard has not been opened | |||
| */ | |||
| @Override | |||
| public native void openClipboard(SunClipboard newOwner) throws IllegalStateException; | |||
| /** | |||
There was a problem hiding this comment.
was this comment accidentally deleted?
There was a problem hiding this comment.
Restored the comment and adjusted the comment on openClipboard to better address the required calling sequence.
| @Override | ||
| public void run() { | ||
| final Clipboard systemClipboard = Toolkit.getDefaultToolkit().getSystemClipboard(); | ||
| while (true) { |
There was a problem hiding this comment.
will this cause the test to run until timeout if the thread isn't interrupted?
There was a problem hiding this comment.
Valid point. Adjusted test to use daemon threads for the concurrent access tests. I reran the tests on the baseline JDK and after the changes to verify the findings from #24614 (comment). Did not notice this as the jtreg harness indeed seems to terminate the JVM and not report that as a failure, tested now also manually (without jtreg).
| @key headful | ||
| @run main ConcurrentClipboardAccessTest | ||
| */ | ||
| import java.awt.*; |
- reduce log level in WClipboard#handleContentsChanged to DEBUG, so that in normal operation it will not be visible - restore comment on WClipboard#closeClipboard - adjust comment on WClipboard#openClipboard - Ensure ConcurrentClipboardAccessTest shutsdown on its own when not run in an environment with an external timeout handling. - Added finishing message to ConcurrentClipboardAccessTest, so that correct termination can be determined visually if run manually
|
@alisenchung @DamonGuy thank you both for comments, pushed an update, that hopefully addresses your concerns. |
There was a problem hiding this comment.
I think this looks fair to me now. But, you still need a Reviewer to look at this. Maybe @kumarabhi006 ?
There was a problem hiding this comment.
Test passed even without the fix on macOS.
Is the fix applicable only on Windows ?
If yes, then I think you should restrict the test to run only on required platforms.
|
@kumarabhi006 my thinking was, that the JVM should not crash on any platform when clipboard is accessed concurrently, but you are right, that the crash is observed windows only. Only that platform (see implementation class Pushed an update adding the test requirement to the test. |
|
/integrate |
|
@matthiasblaesing |
|
@DamonGuy @kumarabhi006 would be one of you kind enough to act as a sponsor for this change? I have an author status, but not a committer status. Before issuing Thank you! |
|
/sponsor |
|
Going to push as commit 92be782.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@sendaoYan @matthiasblaesing Pushed as commit 92be782. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Introduce a lock into WClipboard that protects the code between
openClipboard/closeClipboard invocations.
The native side does not allow to open the clipboard multiple
times or share the opened clipboard between multiple threads.
Remove of need to call openClipboard/closeClipboard from
getClipboardFormats by using the win32 call
GetUpdatedClipboardFormats
Prevent a race-condition by not registering the connection
between java and native side of clipboard multiple time, but
just at construction time.
Progress
Issues
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24614/head:pull/24614$ git checkout pull/24614Update a local copy of the PR:
$ git checkout pull/24614$ git pull https://git.openjdk.org/jdk.git pull/24614/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24614View PR using the GUI difftool:
$ git pr show -t 24614Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24614.diff
Using Webrev
Link to Webrev Comment