-
Notifications
You must be signed in to change notification settings - Fork 227
Remove IPropertyChangeListener registration in dispose method of editor #3233
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
Remove IPropertyChangeListener registration in dispose method of editor #3233
Conversation
bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/part/EditorPart.java
Outdated
Show resolved
Hide resolved
|
Habe you checked in git whether this is recent regression, or was it always the case? |
|
EditorPart leak is a very old one, from 28f4ed5 |
iloveeclipse
left a comment
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.
Good catch, however small fixes needed.
Note: it will go into 4.38 and master is not opened yet for that, so we will need to wait a week or two for merging.
bundles/org.eclipse.ui.workbench/eclipseui/org/eclipse/ui/part/MultiPageEditorPart.java
Outdated
Show resolved
Hide resolved
f20d877 to
5adca56
Compare
|
@tobiasmelcher : did you missed this: #3233 (comment) ? |
5adca56 to
99f37b3
Compare
yes, I missed it. [99f37b3] should contain the requested change. |
|
Note: most likely API tools would complain in next release and ask for a minor segment change (dispose() is public added to EditoPart). We will see it first after master is setup for 4.38. @tobiasmelcher : please ping me if I forget about this PR in two weeks and thanks for the bug report & fix. |
Why should API tools complain here? dispose is not added, it is only overridden here, so it was always part of the public API as declared in Still a version bump is required when fist changing a bundle in the release. |
I've just said "most likely". If it doesn't complain, we are fine. |
If it does I would say its a bug that needs be fixed. Just in case... |
99f37b3 to
d76d164
Compare
|
This pull request changes some projects for the first time in this development cycle. An additional commit containing all the necessary changes was pushed to the top of this PR's branch. To obtain these changes (for example if you want to push more changes) either fetch from your fork or apply the git patch. Git patchFurther information are available in Common Build Issues - Missing version increments. |
|
thanks a lot @iloveeclipse for merging the pull request |
The two classes
org.eclipse.ui.part.EditorPartandorg.eclipse.ui.part.MultiPageEditorPartcurrently register a property change listener in their constructors but fail to unregister it in their dispose() methods.This oversight results in a memory leak: even after an editor is closed and no longer in use, the associated memory cannot be garbage collected because the listener remains registered. Over time, this can lead to increased memory consumption and degraded performance, especially in environments where editors are frequently opened and closed.
This change ensures that the property change listeners are properly removed during disposal, allowing the editor instances to be garbage collected as expected.
Here the steps to reproduce the problem scenario:
sample.SampleFormEditorwhich allocates some amount of memory in the constructor.test.sampleform. Open this file and the SampleFormEditor should be opened. The editor itself is empty and does not provide any meaningful content.Here a screenshot:
After applying the changes of this pull request, the reference is gone and the SampleFormEditor instance can be garbage collected after it is closed and no longer in use.