This Bugzilla instance is a read-only archive of historic NetBeans bug reports. To report a bug in NetBeans please follow the project's instructions for reporting issues.
See the two attached screenshots of OptimizeIt window showing differences of number of instances for 3 openings and 4 openings of four html files.
Created attachment 21430 [details] 3 times opened the html files
Created attachment 21431 [details] 4 times opened the html files
DataEditorSupport$2 seems to be the important instance leaking. Other leaking instances seem to be connected to it...
Created attachment 21460 [details] DataEditorSupport$2 reference graph
Marku, any progress on this? No more than four days to solve this in 4.1!
I am working on this today...
There is several independent memory leaks after opening / closing an HTML file. Some of the leaks are not HTML file specific and happens likely for every DataObjects (I tried java, JSP, html). They are: org.openide.util.WeakListenerImpl$ListenerReference org.openide.util.WeakListenerImpl$PropertyChange org.openide.util.WeakListenerImpl$ProxyListener org.openide.loaders.DataShadow$DSWeakReference org.openide.loaders.DataShadow$OrigL Certain number of these instancies is created and remains allocated after each file opening/closing. I tried to investigate what is the problem, but I digged into code which I have no clear idea how it works - most of the incoming references are from lookups. I suggest the performance team to investigate this and possibly file a bug against core or similar module. Second group of leaking objects are following classes: org.openide.text.DataEditorSupport$1 org.openide.text.DataEditorSupport$EnvListener org.openide.util.lookup.InstanceContent org.openide.util.lookup.AbstractLookup org.openide.util.lookup.ArrayStorage Certain number of objects of theses classes (~ 3) are created for each HTML file opening /closing. They are not leaking for any other DOs I tried. There is a circullar references path among most of these instancies (I will attach a simplied reference graph to this issue). The code which is IMO responsible for the leak is DataEditorSupport$1 - an anonymous class created in DataEditorSupport.createLookup:636 - property change listener listening on changes of DataObject and updating an instancies lookup. The lookup contains an instance which references the DataObject. So I fixed this by adding a weak property change listener to the dataobject. I have tested the fix and it seems to work correctly - the instancies are not leaking and nothing else is broken. --- fixed in trunk - I would like to kindly ask Jarda and Mila to review the fix and Tonda to verify whether the leak is gone. --- diff is attached. Checking in DataEditorSupport.java; /cvs/openide/loaders/src/org/openide/text/DataEditorSupport.java,v <-- DataEditorSupport.java new revision: 1.21; previous revision: 1.20 done
Created attachment 21644 [details] The diff of the fix.
Created attachment 21645 [details] The references graph of the leak
Created attachment 21646 [details] A screenshot of JProfiler class moditor after opening/closing some HTML files in IDE with the fix
Verified that the leak caused by DataEditorSupport$1 and including the lookups is gone.
forgot to mark as fixed (in trunk).
The diff seems safe to me. The property change listener being added is never removed which is a flaw that your patch seems to fix. I agree with the integration.
"#57565: Adding the missing test to mfukala's commit" Checking in unit/src/org/openide/text/DataEditorSupportTest.java; /cvs/openide/loaders/test/unit/src/org/openide/text/DataEditorSupportTest.java,v <-- DataEditorSupportTest.java new revision: 1.2; previous revision: 1.1
1. I've reproduced the described problem by the just commited test. It fails before Marek's patch and works now. So far so good. 2. Please do not commit to org.openide.text without tests next time. The code is fragile and by checking in not enough justified code we just will not improve the maintanance of the package. So please, next time write the test first. 3. I'd like to question this to be P2 bug. Imho its has much lower priority and as such it does not need to go to release41. But that is just minor comment.
> So please, next time write the test first. Or, just attach the patch, reassign to openide and let the owner check it in with a test - after all, this is really an openide bug. > I'd like to question this to be P2 bug. Imho its has much lower priority > and as such it does not need to go to release41. I agree, especially given the size of the leaked memory. Tondo, what's your opinion?
>> I'd like to question this to be P2 bug. Imho its has much lower priority >> and as such it does not need to go to release41. > I agree, especially given the size of the leaked memory. Tondo, > what's your opinion? Given the size of the leak, I don't insist on putting the fix at this last moment in release41. Feel free to downgrade this to P3.
I have talked offline about the fix with Jarda and Tonda, and both are OK with integrating the fix into release41. Since the fix of the leak is safe, we have test for it, it is verified and reviewed I will integrate the fix to 41.
fixed in release41 branch Checking in DataEditorSupport.java; /cvs/openide/loaders/src/org/openide/text/DataEditorSupport.java,v <-- DataEditorSupport.java new revision: 1.20.4.1; previous revision: 1.20 done
commited Jarda's test for the fix to release41 branch as well Checking in DataEditorSupportTest.java; /cvs/openide/loaders/test/unit/src/org/openide/text/DataEditorSupportTest.java,v <-- DataEditorSupportTest.java new revision: 1.1.24.1; previous revision: 1.1 done
I think we have a problem: org.openide.text.AnnotationProviderTest start to fail randomly (twice from seven runs) yesterday on build 200504141800 and 200504142205. I believe it is related to this fix and should be resolved.
I prefer remove the fix from release41 branch and downgrade the bug to P3 as Tonda suggested.
Jarda will check whether the test it OK and put an update into this issue. IMO the fix is correct, maybe it revealed another bug?! I will revert the commit in release41 if Jarda confirms that the fix caused the test failure.
According to Jarda's findings the fix is really not safe so removed it from release41 branch. The leak is not so horrible so I will also downgrade the issue to P3. Checking in DataEditorSupport.java; /cvs/openide/loaders/src/org/openide/text/DataEditorSupport.java,v <-- DataEditorSupport.java new revision: 1.20.4.2; previous revision: 1.20.4.1 done
"#57565: Additional changes. We managed to allow GC of the Lookup yesterday, today we prevent it to be GCed immediatelly" Checking in loaders/src/org/openide/text/DataEditorSupport.java; /cvs/openide/loaders/src/org/openide/text/DataEditorSupport.java,v <-- DataEditorSupport.java new revision: 1.22; previous revision: 1.21 done Checking in test/unit/src/org/openide/text/AnnotationProviderTest.java; /cvs/openide/test/unit/src/org/openide/text/AnnotationProviderTest.java,v <-- AnnotationProviderTest.java new revision: 1.4; previous revision: 1.3
I'd like to emphatize two things: 1. when doing change in org.openide.text, write a test 2. when doing any change in openide, please run existing tests before commit that way we prevent mistakes like this as the AnnotationProviderTest failure would be (very likely) discovered sooner.
We have new failure and its log.
Opps wrong issue. I am sorry.
v