-
Notifications
You must be signed in to change notification settings - Fork 886
Fix flaky clipboard on Windows #8361
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
Conversation
platform/o.n.agent/src/org/netbeans/agent/TransferHandlerActionsNonFinalTransformer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After an initial round of review:
Some comments inline, adding some general here:
- previous attempt to add a Java agent to NetBeans (which, among other things, did roughly what
TransferHandlerActionsNonFinalTransformer.java
is doing) was shut down, on the principle that agents are too complex. I don't object to the agent here, but I would like to hear what changed in the past 5-6 months so that an agent is OK now, and wasn't 5-6 months ago. - I don't particularly like that this is bound to ASM only. This means the IDE won't run as well on newer JDKs, until ASM is upgraded. Given we have ClassFile API now, I suspect there should be a ClassFile backend for the transformers. Whether there should be an ASM backend is (to me, personally), less important. I.e. I think it is important to run well with minimal maintenance on newer JDKs.
I am requesting changes here - that applies specifically to the license of the jar with the shaded ASM.
platform/o.n.agent/build.xml
Outdated
<import file="../../nbbuild/templates/projectized.xml"/> | ||
<target name="-post-jar"> | ||
<jar destfile="${cluster}/${module.jar}" update="true"> | ||
<zipfileset src="../libs.asm/external/asm-commons-9.7.1.jar"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shading is generally frown upon, although may be more or less necessary here.
But - the LICENSE
(and, generally NOTICE
) file in the resulting jar that includes multiple libraries need to include licenses (and, when appropriate notices) for all the libraries. I may be missing something here, but this does not seem to include the ASM license in the resulting jar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, that the shading is ugly, but the alternative would be to add the ASM too the boot classloader and this would mean further fiddling with the command line arguments.
The LICENSE problem was a problem and I added the required notice modification.
return Logger.getLogger(NetBeansAgent.class.getName()); | ||
} | ||
|
||
public static void premain(String arg, Instrumentation instrumentation) throws IOException, URISyntaxException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be wrong, but one of the things I don't like here is that:
a) the transfomers run permanently, for all classes, forever. The transformers transform only one class, it seems wasteful to keep them running (and, potentially, complaining, see below) forever. Even if the transformers "don't do anything" after transforming the classes. I think I would suggest to either 1) uninstall the transformer after it transforms the class it is interested in, and/or 2) load the proactively class here or soon after start (and uninstall the transformer), so that the transformers run for least amount of classes.
b) the transformers are installed even if disabled. If a transformer is disabled, it should not be installed/add
ed at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overhead is an if and and string comparison. I have some hope, that if this gets as hot as you imply, hotspot will optimize this, so that the unlikely matching path is mostly optimized away.
Anyway, modified the only currently retained transformer to remove itself after it is done and only be registered if they are not disabled.
platform/o.n.agent/manifest.mf
Outdated
OpenIDE-Module-Specification-Version: 1.0 | ||
Premain-Class: org.netbeans.agent.NetBeansAgent | ||
Can-Redefine-Classes: true | ||
Can-Retransform-Classes: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can-Retransform-Classes
means the agent itself is requesting ability to retransform classes. Does this agent really initiate retransforming classes? If not, then I think this ability should be removed from the manifest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current state neither redefines, nor retransforms, so removed both.
// We assume, that we are loaded early enough before AWT is initialzed, | ||
// so we don't expect to need to retransform | ||
if (classBeingRedefined != null) { | ||
getLog().log(Level.WARNING, "Attempted retransform for: {0} (Ignored)", new Object[]{className}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand this at all, sorry. What is the point? As I look at the patch, the agent is not initiating retransforming of classes. It does not even check if javax.swing.TransferHandler
was already loaded before the transformer was installed.
If some other agent will request some retransformations, then this transformer will start to complain (I think, at least!), but I don't understand why.
This whole if seems weird to me, unnecessary, and maybe harmful.
Dtto for the other transformer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed (codepath can't be hit as the agent declares itself as non retransforming and the registration with instrumentation declares the same).
then | ||
jargs_without_clusters="$jargs -javaagent:${plathome}/agent/org-netbeans-agent.jar"; | ||
else | ||
jargs_without_clusters="$jargs"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that when the agent is excluded from the application, then nothing happens - definitely a +1 for that.
case "cutAction": | ||
case "copyAction": | ||
case "pasteAction": | ||
return super.visitField(access & (~ Opcodes.ACC_FINAL), name, descriptor, signature, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't like this. Removing the final
modifier will at least make sure that deep reflection will work on the field, but it is still adding (new) deep reflection against JDK class, which not very good. And removing the final
modifier also feels like a highly suspicious thing to me.
Given we have a transforming agent, is there a reason why not change TransferAction.getClipboard()
to do whatever is needed? Or something else, that would be less problematic than using deep reflection on a field from the JDK, and removing the final
modifier to make the reflection work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the Transformer, so does not apply anymore.
+1 to the Windows launcher changes for JAVA_HOME and PATH. Another change that would be good to consider (which is relevant to this) is allowing relative paths in the conf. eg. the shell launcher sources the conf file so can handle things like -0.9 to the agent as is ...
Well, what was discussed was an optional (ideally opt-in), lightweight and temporary solution via patch or agent for the Windows clipboard issue. That optionality has to include in my opinion configuration via the conf file and cli (shortcut) args. I agree with Jan. This feels far more akin to revisiting the agent discussion from 5-6 months ago. If that discussion needs reopening, then let's reopen it. But bear in mind that given revisions and launcher vote required here, this will probably not land in master until a week before branch (if we stick to the date). I also share concerns about the dependence on ASM. -1 to registering NBClipboard by default or without easy override. Its removal fixed as much as it broke. My suggestion if taking the agent route was to bring back as an option for regression issues and testing, not to bring it back permanently and therefore embed the need for an agent in the IDE. It was suggested this Windows issue only occurs with flavor listeners registered. I would still like anyone who can reproduce the clipboard issue (my Windows machine stubbornly refuses to have the bug!) to check whether the removal of flavor listener registration does stop this issue occurring. Because it might be that we can work around the issue that way for now, which would potentially be a lot simpler! |
I would also like to see this reduced to the clipboard bug fix. I agreed to not get in the way of temporary agent proposals to resolve the issue until it can be hopefully fixed in the JDK. #7051 (reply in thread) Regarding ASM: This PR here will log a exception on JDK 25 and NB will continue to launch. I think this is good enough for a temporary agent driven patch. (speaking only for WClipboard patch). I have a ClassFile API version here - but I wrote this mostly as learning experience. This would require a) build on JDK 24 + multi release jar, or b) release agent separately. For temporary solution it is likely a bit too much overhead. I haven't looked at the code gen bits yet. But it would be good to proof via javap that the generated code is identical to javac emitted code when fed with the patched source file - i think this is probably the fastest way to ensure correctness (see linked repo where I tried that approach) |
16b9958
to
3ada442
Compare
3ada442
to
a4161ca
Compare
I pushed an update the reduces the scope of the agent, fixes issues not caught on the first try and addresses review concerns. For the general comments:
I think I never ruled out an agent, so I can't answer this. What is a reality is, that I did not realize that #7928 and #8169 would lead to this. I learned about copy/paste history from a recent comment by @neilcsmith-net (I think) and thus this lead me down the rabbit hole. I also don't think, that the agent code is hard, it is the classfile manipulation that is. What makes it further hard is, that this (
You can disable the code via config (see the system properties) and remove the module. If you insist, I can add a "--disable-nb-agent" option to the launchers.
As said above, maybe not all problems were on the table at time of discussion.
It is ASM or manully coding the class modifications. I'm willing to see if the Class-File API can be sensibly invoked via reflection and implement as corresponding code path, but the current state of discussion does not motivate me to waste energy on this. If we agree to get this in, I'll do it.
I don't remember where I got that that from, but I think I remember, you asking for it. Anyway, I'm not tied to this, not sure, why the security manager approach was more acceptable or how you intent to solve it differently.
What do you have in mind? I now have a virtual machine that can reliably reproduce the problem. I still don't know what changed in that machine, but I used it to verify the change (problem visible without agent, problem gone with it). |
Thanks for the detailed responses.
The system property disables the transformation, not the agent? Module removal requires the IDE location to be modifiable, which is not always the case. I think we need the ability for people to switch this on or off easily in case of issues, either with it or to rule it out. I'm also not sure yet that we want it enabled by default in the cross-platform zip. Launcher updates are something of a PITA, so we should consider how the launcher changes can cover various eventualities. Ideally in my opinion we'd have something like an Default in the launcher should be off in my opinion, and controlled by the default options. Could be other way around. Decision on enabling by default in the zip should happen after we've had some rc testing, but would be done via default options.
Yes, I suggested it as an option for testing, because we know it has side effects. The security manager approach was not more acceptable. When we talked about its removal, there was various discussion about the clipboard. What was interesting is that removing it fixed other things, like copy&paste in the file trees.
When xxDark mentioned the JDK issue he said it was only triggered with registered flavor listeners. We're not using them in many places https://github.com/search?q=repo%3Aapache%2Fnetbeans+flavorlistener&type=code I think registering just in |
IMO: ASM is fine for this usecase since:
Bytecode transformations which are meant to be maintained permanently, should use the classfile API IMO. Beside it being a quite nice API, it being part of the JDK solves all version related compatibility problems. I hope we can avoid those agents though. |
As mentioned on the mailing list, I wouldn't touch the launchers at all and put the The windows launcher improvements could be split into a separate PR - not sure when the branching happens, getting them in for NB 26 might be close. |
@mbien it was a deliberate suggestion because I don't think there's any other way to handle a path relative to the base dir here? If I've missed something and I'm wrong with that, then I agree with you. Otherwise, we'd need a launcher update for this as well as JDK env vars. Not much time to get one in before branch now! Assuming on time, will be middle of next week. |
I don't know where you want to go, so this is what happened to the launcher in the meantime: https://github.com/matthiasblaesing/netbeans/commits/enhance_launcher/ |
@matthiasblaesing sounds good to me. (typo in commit msg "substitution") |
@matthiasblaesing if that basedir option can handle spaces in paths in JDK options without getting in to quoting hell, that would be a great addition for this and other things! I'd considered something similar myself a while back, but was concerned about handling for paths with whitespace. EDIT: the other concern might be alternative path separators on different OS though? |
1af6b9c
to
551bb04
Compare
Pushed another update:
|
Many thanks @matthiasblaesing !
I do not understand this one. Firstly, it seems to be in Putting do not merge on this as and until we've been through the launcher release process. |
Yeah, had to move from
The platform launcher will not get the agent. The platform launcher has no configuration file, so there is no basis to add it. I had it in the platform, because I envision a serious integration of |
Huh? The platform uses the same |
551bb04
to
8413480
Compare
8413480
to
4783f6d
Compare
It was observed, that clipboard on windows sometimes reacts flaky. Investigation hints that missing synchronization in the AWT implementation/integration. Testers reported, that the changes implemented here: - the change handling from the system clipboard is dispatched into the event loop and - the change handling is modified to be called synchronized It is expected, that fixing this upstream in the JDK will take longer and so runtime patching must be considered. Runtime patching in this case is accomplished using the javaagent mechanism. The IDE launcher is modified to load an agent, that will intercept loading classes and modify WClipboard while it is being loaded. The transformer is written, so that it will only transform the target class and will act transparantly for all other classes.
4783f6d
to
9b6f8a0
Compare
Launcher release This survived a smoketest on Windows and Linux with spaces in directory name. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agent integration code looks good to me. Haven't really looked at the emitted bytecode (or platform integration).
with JDK-8353950 filed and the fix available this all will be hopefully only needed temporary
Removed the There's a minor formatting issue in clusters file, and I would still prefer this was in the platform cluster, but not reason not to merge now. Can be addressed later if decided. |
to elaborate a bit: since we essentially need static bytecode injection for the purpose of patching (to swap out a method essentially), verification would be conceptually easy, since all it would have to proof is that the emitted bytecode matches javac bytecode. I tested this here (mail). But since this is hopefully only needed until the JDK is patched and the agent would also disable itself on failure (+ option to edit the launch config), I think this is good to go. (thats why I approved - there is also no time left for changes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks OK to me.
Seems fixed in nb 26-rc1, after hours work ... |
It was observed, that copying from NetBeans on Windows sometimes was flaky. Flaky in this case means, that the copy operation was executed in NetBeans, but the content of the system clipboard was not updated.
Based on the discussion in #7051 this is causes by an implementation problem inside the JDK. While an upstream fix is preferred, the problem is a very real problem and an alternative was found in changing the JDK classes at runtime using an javaagent.
Closes: #3962