8365180: Remove sun.awt.windows.WInputMethod.finalize() #26706
Conversation
|
👋 Welcome back prr! A progress list of the required criteria for merging this PR into |
|
@prrace 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 158 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. ➡️ To integrate this PR with the above commit message to the |
openjdk
bot
changed the title
openjdk
bot
added
client
Webrevs
|
| // Due to a memory management problem in Windows 98, we should retain | ||
| // the native input context until this object is finalized. So do | ||
| // nothing here. | ||
| disposerRecord.dispose(); |
There was a problem hiding this comment.
The native dispose will call ImmDestroyContext which is specified as:
However, before calling ImmDestroyContext, the application must remove the input context from any association with windows in the thread by using the ImmAssociateContext function.
I did not check the code, but maybe we are calling ImmAssociateContext in the wrong moment and have to postpone this "dispose" until all the object is really unused?
There was a problem hiding this comment.
This seems to be done by a native method WInputMethod.disableNativeIME(..) - which sets the IME for the window to NULL. The native method is called from various places in WInputMethod.
I can't see that dispose or finalize would do anything to ensure that is called, or introduce any timing issues either way. So if there's a problem it is un-related.
There was a problem hiding this comment.
I can't see that dispose or finalize would do anything to ensure that is called, or introduce any timing issues either way. So if there's a problem it is un-related.
Maybe disableNativeIME is called when the top level Window is disposed, and after that there are no more references to WInputMethod? In that case, it is safe to call it from finalize(), but it might not be as safe to call it from dispose(), which mighe be called before "WInputMethod.disableNativeIME"?
There was a problem hiding this comment.
Maybe disableNativeIME is called when the top level Window is disposed,
No it is not.
Nothing calls disableNativeIME when the window is disposed. I tried this with the disposer removed and finalize restored just to be sure.
Also it needs to be called 'early' but not 'too early' during WIndow.dispose()
If the input method is active when you try to dispose it via APIs like disableNativeIM() then an exception is thrown.
If you call it too late - after removeNotify() then it will not be sent because the peer is now nulled out.
src/java.desktop/windows/native/libawt/windows/awt_Toolkit.cpp
@@ -1082,6 +1082,7 @@ LRESULT CALLBACK AwtToolkit::WndProc(HWND hWnd, UINT message,
AwtComponent* comp = (AwtComponent*)JNI_GET_PDATA(peer);
if (comp != NULL)
{
comp->SetInputMethod(self, useNativeCompWindow);
comp->ImmAssociateContext((HIMC)((intptr_t)context));
}
Since we never call it, then this migration to disposer changes nothing.
If we do call it I think it should be called in WInputMethod.removeNotify() which will clearly be before dispose().
I will add that and we'll be better off then we were before this change.
I also found that if the window has focus when it is dispose()'d that it will receive a focus lost event which ironically creates a new WInputMethod because Window.dispose() had nulled out the existing one
Either don't null out the disposed one, or do something else to avoid re-creation, or decide that it is harmless .
For now I will leave that as it seems to be harmless, pre-existing and un-related to this change, although a little wasteful. It also would be more than a windows-only change.
|
/integrate |
|
Going to push as commit 8e44856.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
Remove finalize() from WInputMethod.java - it is used to free a native id.
Also the reason dispose() didn't free it seems no longer relevant.
Although I did see (when instrumenting) that dispose() was called when I disposed() the Frame referencing the IM,
I don't know if I can be sure that is always done. So safest to add the Disposer in case it isn't.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26706/head:pull/26706$ git checkout pull/26706Update a local copy of the PR:
$ git checkout pull/26706$ git pull https://git.openjdk.org/jdk.git pull/26706/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26706View PR using the GUI difftool:
$ git pr show -t 26706Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26706.diff
Using Webrev
Link to Webrev Comment