Bug 202681 - Duplicated opened files in the IDE
Duplicated opened files in the IDE
Status: RESOLVED FIXED
Product: platform
Classification: Unclassified
Component: Text
7.0
All All
: P2 (vote)
: 7.2
Assigned To: Jaroslav Tulach
issues@editor
: 71_HR_FIX
Depends on: 200966
Blocks: 213072
  Show dependency treegraph
 
Reported: 2011-09-26 15:16 UTC by Egor Ushakov
Modified: 2012-06-10 05:54 UTC (History)
3 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
log (14.24 KB, text/plain)
2011-11-18 14:34 UTC, Egor Ushakov
Details
proposed patch (1.41 KB, patch)
2012-06-01 11:14 UTC, Vladimir Voskresensky
Details | Diff
Replacing leaking caching with mutual obj<->es references (3.00 KB, patch)
2012-06-07 08:37 UTC, Jaroslav Tulach
Details | Diff
links to tokens from GC (27.61 KB, text/html)
2012-06-08 10:09 UTC, Egor Ushakov
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Egor Ushakov 2011-09-26 15:16:07 UTC
steps to reproduce:
- create welcome sample
- open welcome.cc
- set a line breakpoint in it
- close the IDE and open it again
- double click on the breakpoint in breakpoints view
it will open another copy of welcome.cc
Comment 1 Vladimir Voskresensky 2011-09-26 15:24:30 UTC
Please, pay attention to what's happens in multi-view support.
reverting http://hg.netbeans.org/cnd-main/rev/bb3c22be8a00
helps to resolve this issue as well...
Comment 2 Vladimir Voskresensky 2011-09-26 15:26:37 UTC
I had to push backout
http://hg.netbeans.org/cnd-main/rev/a8b2c5f886fb
Comment 3 Egor Ushakov 2011-09-27 11:56:13 UTC
could be reproduced without breakpoints:
- create welcome
- open welcome.cc
- close the IDE
- double click on welcome.cc in project view
another inctance of welcome.cc is opened
Comment 4 Vladimir Voskresensky 2011-09-28 14:28:51 UTC
so, the issue is still in place
Comment 5 Jaroslav Tulach 2011-10-14 20:48:01 UTC
Yes, things are still broken. allEditors field in CppEditorSupport is empty after deserialization of first editor.
Comment 6 Vladimir Voskresensky 2011-10-16 17:45:35 UTC
Jarda, we have not changed our editor support in 7.1 except registering editor as "Source" for multi views.
Bug is still assigned to you.
Do you expect anything from our team to help?
Comment 7 Jaroslav Tulach 2011-10-17 18:13:15 UTC
I believe the bug will have to be fixed in CND code. However as you will likely need my help I left the issue assigned to me. I plan to deal with it together with my other twenty P2s when the time comes. Of course, if you solve it yourselves sooner, I'll be more than glad.
Comment 8 Egor Ushakov 2011-10-19 16:56:36 UTC
now it can be reproduced only this way on my machine:
- create welcome
- open welcome.cc but have welcome page also opened and not disabled
- close and reopen the IDE
welcome page is activated, welcome.cc is not active, if you double click on welcome.cc in projects view it opens correct file tab, do not do that for now
- close the IDE (welcome.cc was not initialized)
- open the IDE
now double click on welcome.cc opens the second same file
Comment 9 Egor Ushakov 2011-10-19 17:04:18 UTC
also not every time :(
Comment 10 Vladimir Voskresensky 2011-10-19 19:24:07 UTC
Probably related to #203980 as well
Comment 11 Egor Ushakov 2011-10-21 13:11:29 UTC
bug 203980 is fixed but this bug is still there
Comment 12 Jaroslav Tulach 2011-10-25 05:33:46 UTC
I'd start analyzing stacktraces of created components. I'd verify there is only single CloneableEditorSupport for given file. I'd find out why the first component is not registered among "allEditors".
Comment 13 Jaroslav Tulach 2011-11-13 09:54:35 UTC
Works for me even while leaving Start Page open.
Comment 14 Egor Ushakov 2011-11-18 10:06:08 UTC
I can still reproduce it:
- restart the IDE with a file opened in the editor (and start page)
- open the file from the projects tab
it opens another instance of the file.
Comment 15 Vladimir Voskresensky 2011-11-18 10:42:13 UTC
Jarda, what kind of help do you need from our side? Any logger to turn on?
Comment 16 Egor Ushakov 2011-11-18 14:34:10 UTC
Created attachment 113321 [details]
log
Comment 17 Egor Ushakov 2011-11-18 14:36:27 UTC
we've added stack trace into CppEditorSupport and attached log is from the following activity:
- close IDE with welcome.cc opened and Start Page not disabled
- restart IDE and two deserialization exceptions are printed. 
After that clicking on file in Project view opens the second tab.

When only one exception is printed => tab reactivated correctly as expected
Comment 18 Egor Ushakov 2011-11-18 14:39:29 UTC
the difference in two stack traces is that MultiViewCloneableTopComponent.readExternal
calls
CloneableTopComponent.readExternal
first time and
MultiViewPeer.peerReadExternal
- second time.
And each of them causing deserialisation of the CppEditorSupport
Comment 19 Jaroslav Tulach 2011-11-18 16:41:02 UTC
Have not you thought about the problem being at

org.netbeans.modules.cnd.source.CppEditorSupportProvider$CppEditorSupportFactory.convert(CppEditorSupportProvider.java:66)

?
Comment 20 Vladimir Voskresensky 2011-11-18 18:42:03 UTC
(In reply to comment #19)
> Have not you thought about the problem being at
> 
> org.netbeans.modules.cnd.source.CppEditorSupportProvider$CppEditorSupportFactory.convert(CppEditorSupportProvider.java:66)
> 
> ?

Yes, we thought, but it's Lookup's converter which is guaranteed that we don't have to cache instances ourselves
Comment 21 Vladimir Voskresensky 2011-11-18 18:43:20 UTC
More likely there are two instances of Env deserialized due to multiview super and peer deserialization
Comment 22 Jaroslav Tulach 2011-11-19 21:19:58 UTC
Of course, deserialization of Env produces two distinct objects! They are supposed to coordinate on finding the common file. Can you point me somewhere outside of CND where this does not work?
Comment 23 Vladimir Voskresensky 2011-11-21 08:10:21 UTC
(In reply to comment #22)
> Of course, deserialization of Env produces two distinct objects! They are
> supposed to coordinate on finding the common file. Can you point me somewhere
> outside of CND where this does not work?
http://netbeans.org/bugzilla/show_bug.cgi?id=204915
Comment 24 Vladimir Voskresensky 2011-11-21 08:11:08 UTC
(In reply to comment #22)
> Of course, deserialization of Env produces two distinct objects! They are
> supposed to coordinate on finding the common file. Can you point me somewhere
> outside of CND where this does not work?
Jarda, if you need more tracing, just say what to trace to help you analyze the issue
Comment 25 Egor Ushakov 2011-11-23 13:37:50 UTC
fixed (by workaround) in:
http://hg.netbeans.org/cnd-main/rev/01bed0557221
Comment 26 Jaroslav Tulach 2011-11-23 16:12:58 UTC
(In reply to comment #25)
> fixed (by workaround) in:
> http://hg.netbeans.org/cnd-main/rev/01bed0557221

Right, that does not seem like proper fix. CND clearly has some problems during deserialization of its own CloneableEditorSupport.Env implementation. You can have many CloneableEditorSupport.Env objects per the same FileObject, but only one CloneableEditorSupport for it. I'd expect the fix to concentrate on that.
Comment 27 Vladimir Voskresensky 2011-11-24 13:26:40 UTC
fix is reviewed and it is safe workaround to the problem.

I believe it's a problem in platform which incorrectly de-serialize multiview by not holding components read from environment => weak listener releases editor support and it is gc'ed, although TC is created.

When Open is invoked => no editors are found => new TC is opened => two TCs for the same file.

I think it can be the situation for any other editor supports as well if they migrate from CookieSet to Lookups as we did and infrastructure starts using dob.getLookup().lookup(CloneableEditorSupport.class) instead of dob.getCookie(CloneableEditorSupport.class).
Somehow CookieSet's factories holds references longer than Lookup's factories items
Comment 28 Quality Engineering 2011-11-24 16:00:10 UTC
Integrated into 'main-golden', will be available in build *201111240600* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main/rev/01bed0557221
User: Egor Ushakov <gorrus@netbeans.org>
Log: fixed #202681 (Duplicated opened files in the IDE) - implemented workaround
Comment 29 soldatov 2011-11-24 21:01:49 UTC
verified in 201111240600 build (can't reproduce)
Comment 30 Egor Ushakov 2011-11-28 15:13:16 UTC
transplanted into release71:
http://hg.netbeans.org/releases/rev/2984f46e1f02
Comment 31 Quality Engineering 2011-11-29 08:46:01 UTC
Integrated into 'releases'
Changeset: http://hg.netbeans.org/releases/rev/2984f46e1f02
User: Egor Ushakov <gorrus@netbeans.org>
Log: fixed #202681 (Duplicated opened files in the IDE) - implemented workaround
(transplanted from 01bed0557221da4703134794548090cffbf7e7e9)
Comment 32 soldatov 2011-12-01 13:47:31 UTC
Verified in NetBeans IDE 7.1 RC2 (Build 201111302200)
Comment 33 Egor Ushakov 2012-05-29 16:17:27 UTC
Implemented workaround has introduced huge regression in memory consumption and performance in Find Usages, see bug 213072.
Please evaluate possibility to fix it in platform, for more details see comment from Vladimir: http://netbeans.org/bugzilla/show_bug.cgi?id=202681#c27
Comment 34 Vladimir Voskresensky 2012-06-01 11:14:14 UTC
Created attachment 120204 [details]
proposed patch
Comment 35 Vladimir Voskresensky 2012-06-01 11:15:33 UTC
with proposed patch factory's convert is called only once, because reference stay in memory between to function calls in MultiViewCloneableTopComponent:
    public void readExternal (ObjectInput in) throws IOException, ClassNotFoundException {
        super.readExternal(in);
        peer.peerReadExternal(in);
    }
Comment 36 Vladimir Voskresensky 2012-06-01 11:17:01 UTC
the following modification in MultiViewCloneableTopComponent simplifies simulate the problem:
    public void readExternal (ObjectInput in) throws IOException, ClassNotFoundException {
        super.readExternal(in);
        System.gc();
        System.runFinalization();
        System.gc();
        peer.peerReadExternal(in);
    }
Comment 37 Vladimir Voskresensky 2012-06-01 11:17:34 UTC
of course cache in CppEditorSupportProvider should be turned off to see doubled tabs
Comment 38 Jaroslav Tulach 2012-06-01 11:28:25 UTC
OK. I'll try to write a test to simulate the issue.
Comment 39 Vladimir Voskresensky 2012-06-01 11:51:00 UTC
(In reply to comment #38)
> OK. I'll try to write a test to simulate the issue.
Thanks. Just to remind: it is important that 
Env.findCloneableOpenSupport is implemented as:
        @Override
        public CloneableOpenSupport findCloneableOpenSupport() {
            return getDataObject().getLookup().lookup(CppEditorSupport.class);
        }
and in DataObject lookup is lazy based on factory, like in our SourceDataObject
    @Override
    public final synchronized Lookup getLookup() {
        if (myLookup == null) {
            ic = new InstanceContent();
            ic.add(this);
            ic.add(getPrimaryFile());
            ic.add(this, CppEditorSupportProvider.staticFactory);
            myLookup = new AbstractLookup(ic);
        }
        return myLookup;
    }

static class CppEditorSupportFactory implements Convertor<SourceDataObject, CppEditorSupport>
Comment 40 Vladimir Voskresensky 2012-06-01 11:58:06 UTC
(In reply to comment #39)
> and in DataObject lookup is lazy based on factory
well... lazy 
if (myLookup == null) {
is not important, important is that it's based on factory instead of direct instance of CppEditorSupport in lookup
ic.add(this, CppEditorSupportProvider.staticFactory);
Comment 41 Jaroslav Tulach 2012-06-07 08:18:22 UTC
First observation. No surprise you have memory leaks. Using WeakHashMap when values have strong reference to keys (when CppEditorSupport  has reference to DataObject) is not really effective. that is why the fix: 
        // FIXUP for IZ 202681, if we do not keep a reference to it will
        // be released and a new instance will be created due to MultiView peer deserialization
        private final Map<DataObject, CppEditorSupport> cache = new WeakHashMap<DataObject, CppEditorSupport>();
is completely wrong.
Comment 42 Jaroslav Tulach 2012-06-07 08:37:43 UTC
Created attachment 120469 [details]
Replacing leaking caching with mutual obj<->es references
Comment 43 Jaroslav Tulach 2012-06-07 08:42:43 UTC
I have used the System.gc() trick. I commented out the cache in cnd. I've added a printStack to track the number of created instances.

I could see multiple CppES being created for the same data object, but I have not seen duplicated files in the editor.

Anyway I've just attached patch that eliminates the multiple CppES creation problem. Unless I am mistaken it has to fix the duplicated problem as well.
Comment 44 Vladimir Voskresensky 2012-06-07 10:56:32 UTC
(In reply to comment #43)
> I have used the System.gc() trick. I commented out the cache in cnd. I've added
> a printStack to track the number of created instances.
> 
> I could see multiple CppES being created for the same data object, but I have
> not seen duplicated files in the editor.
> 
> Anyway I've just attached patch that eliminates the multiple CppES creation
> problem. Unless I am mistaken it has to fix the duplicated problem as well.
Probably it fixes the issue with tabs, but if I am not mistake it introduces memory leak which is described in issue #213072. Now any holder of data object will keep once queries CES forever which will be never freed.

 I think my fix is safer, it holds reference to COS only by COS-presenter (Listener) and not by DataObject
Comment 45 Jaroslav Tulach 2012-06-07 13:41:40 UTC
I am not sure the change in Listener inner class can really help anything. I would need to understand who holds what right now and what is going to be the difference in the future.

Re. memory leak - the new fix eliminates global leak of all SourceDataObject - of course if there is somebody else holding on all SourceDataObject you are not going to get much. On the other hand all other languages have some reference from the dataobject to its CloneableEditorSupport.
Comment 46 Egor Ushakov 2012-06-07 14:04:04 UTC
unfortunately we hold SourceDataObjects ((
Comment 47 Egor Ushakov 2012-06-08 10:09:01 UTC
Created attachment 120550 [details]
links to tokens from GC
Comment 48 Jaroslav Tulach 2012-06-08 12:21:49 UTC
ergonomics#972ed7b8d6eb

Thanks guys for helping me accept the fact that the problem is in CloneableOpenSupport.
Comment 49 Egor Ushakov 2012-06-08 13:01:19 UTC
Great! Thank you!
Comment 50 Egor Ushakov 2012-06-08 13:04:25 UTC
I reverted the workaround in:
http://hg.netbeans.org/cnd-main/rev/a00a99cafcc8
Comment 51 Quality Engineering 2012-06-10 05:54:28 UTC
Integrated into 'main-golden', will be available in build *201206100001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/972ed7b8d6eb
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #202681: Vladimir's fix for the GC + deserialization problem and my test to verify its correctness.


By use of this website, you agree to the NetBeans Policies and Terms of Use. © 2012, Oracle Corporation and/or its affiliates. Sponsored by Oracle logo