Bug 202681

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...

I had to push backout
http://hg.netbeans.org/cnd-main/rev/a8b2c5f886fb

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

so, the issue is still in place

Yes, things are still broken. allEditors field in CppEditorSupport is empty after deserialization of first editor.

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?

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.

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

also not every time :(

Probably related to #203980 as well

bug 203980 is fixed but this bug is still there

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".

Works for me even while leaving Start Page open.

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.

Jarda, what kind of help do you need from our side? Any logger to turn on?

Created attachment 113321 [details]
log

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

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

Have not you thought about the problem being at

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

?

(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

More likely there are two instances of Env deserialized due to multiview super and peer deserialization

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?

(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

(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

fixed (by workaround) in:
http://hg.netbeans.org/cnd-main/rev/01bed0557221

(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.

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

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

verified in 201111240600 build (can't reproduce)

transplanted into release71:
http://hg.netbeans.org/releases/rev/2984f46e1f02

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)

Verified in NetBeans IDE 7.1 RC2 (Build 201111302200)

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

Created attachment 120204 [details]
proposed patch

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);
    }

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);
    }

of course cache in CppEditorSupportProvider should be turned off to see doubled tabs

OK. I'll try to write a test to simulate the issue.

(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>

(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);

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.

Created attachment 120469 [details]
Replacing leaking caching with mutual obj<->es references

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.

(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

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.

unfortunately we hold SourceDataObjects ((

Created attachment 120550 [details]
links to tokens from GC

ergonomics#972ed7b8d6eb

Thanks guys for helping me accept the fact that the problem is in CloneableOpenSupport.

Great! Thank you!

I reverted the workaround in:
http://hg.netbeans.org/cnd-main/rev/a00a99cafcc8

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.