Created attachment 59627 [details]
Simple NetBeans Platform Application reproducing the problem

Just a note - for debug purposes '>>> Created TestAction instance #{X}' is sent to the System.err when instance of the
action is created, after the application starts you can find the following in logfile:

  >>> Created TestAction instance #1
  >>> Created TestAction instance #2

I guess you do not expect me to fix and integrate this into 6.1, don't you? Otherwise you would have reported this 
issue a week ago...

No I don't, take your time to investigate:o) I have to use 6.0.1 platform for now, anyway.

Dafe, please take this one.

I think I found it in InstanceDataObject - when instance creation is defined in xml layer through instanceCreate
attribute (method call) and there is no instanceClass attribute, infrastructure has no way of finding out the instance
class then to create instance and getClass() from it. However, this created instance is forgotten and creation is
repeated again when instance is needed.
This particular example will be fixable I think, but generally if instanceClass attribute is missing, infrastructure
can't guarantee only one instance - when InstanceDataObject.findFO is called, then we can choose - either FO will not be
found or we have to instantiate instance definition which is without instanceClass attribute.

fixed with test, unfortunately writing the test took me almost two days, as I'm unfamiliar with the IDO and MenuBar code:

http://hg.netbeans.org/core-main/rev/80eea651d4d9
http://hg.netbeans.org/main/rev/80eea651d4d9

Btw: jis, I think using "instanceCreate" methodvalue without "instanceClass" supplied may not be good for performance,
instance is created sooner that it could be.

Reopening, I have to rollback the fix, it caused P1 - 141682

This issue is not fixable - system can't guarantee only one instance of registrations which use "instanceCreate"
attribute but are missing "instanceClass" attribute. System has no other chance but to actually create instance in order
to get class name in this situation. Unfortunately, such instance is created too soon, for example during FolderLookup
creation on some registration folder, and so it doesn't work OK, as whole system expects that instance is really created
when instanceCreate(..) is called.

Conclusion: Add instanceClass when using instanceCreate attribute. Otherwise your object can be created multiple times
(2 times mostly it seems).

I added warning log for such situations:
http://hg.netbeans.org/main/rev/d20eab2d47d8

Integrated into 'main-golden', available in build *200808010201* on http://bits.netbeans.org/dev/nightly/
Changeset: http://hg.netbeans.org/main/rev/b5daad390520
User: Dafe Simonek <dsimonek@netbeans.org>
Log: backout of 80eea651d4d9 (fix of #131951)

I'll report all the warnings to appropriate components

[reference issue #142550]

This doesn't look right to me. Please do not file issues for modules to add this attribute until the situation has been
properly reviewed. I would recommend #d20eab2d47d8 also be reverted for now.

           <file name="testmodule-TestAction.instance">
                <attr name="instanceCreate" methodvalue="testmodule.TestAction.create"/>
           </file>

is a perfectly good instance registration. Adding an instanceClass attribute should not be necessary, and we have
repeatedly told people in various forums not to add it. In general,

<attr name="instanceClass" stringvalue="C"/>

is just a shortcut for

<!-- if needed --><attr name="instanceOf" stringvalue="C"/>
<attr name="instanceCreate" newvalue="C"/>

There is no guarantee in general that a FileObject attribute will be instantiated only once. The filesystem should cache
the created value so long as it is held in some other way (i.e. weakly cache), and FolderInstance may cache instances
associated with files for a while, but that is all. If a module breaks due to assuming that an object-valued attribute
is created only once, the fault is generally in the module.

Anyway no comment in this issue explains to my satisfaction what the real problem here was. No one should be calling
InstanceCookie methods on an item in Actions/ or Menu/ except MenuFolder, which should ask for IC.instanceCreate anyway.
If this is not the case, I want to see a stack trace.

The original use case was for VisualVM which defines its own folder in layer for contents of its context menus.
InstanceCookie is used here to resolve action instances.

The problem is that VisualVM defines an action which needs to register itself as a listener to another class. This
action needs to have exactly one instance, thus .instance is used in one folder and .shadow at other places of the
layer. Expected behavior - only one action instance is created (otherwise the whole .shadow think doesn't make much
sense for me). Actual behavior - two instances of the action are created, causing ugly side effects (2 listeners being
registered) which are very difficult to debug.

I haven't found any explicit note that more than one instance may be created when using .shadow, so if not fixable, I
think that current logging the warning is the best approach. I would also strongly suggest to describe this behavior
visibly in some docs, showing concrete code examples ensuring that exactly one instance being created (singleton).

Given how much other actions could potentially be also affected (based on warnings logging during IDE startup) I'm also
cc'ing performance as this may noticeable impact startup performance.

I don't believe the issue has to do with use of *.shadow files, which just delegate their InstanceCookie to the original
*.instance file anyway.

I certainly agree it is desirable to not create more than one instance from a *.instance. I do not agree that the system
is required to do so in all situations - just make a best effort. If registering >1 listener from your Action instance
is a real problem rather than just an oddity when debugging (and you have do not use WeakListener, so both listeners
remain active indefinitely), then you should register a single heavyweight listener from static code, and have the
Action instances register lightweight proxy listeners to this static source (or use WeakSet to enable the static
listener to refire changes to all extant instances).

What I am opposed to specifically is the use of instanceClass as a workaround for some underlying bug which I have not
seen adequately described or investigated. (Why is the object being instantiated more than once? Is the first instance
being held strongly by infrastructure code or not? Who was calling InstanceCookie.instanceClass and why?)

Probably instanceClass should be deprecated altogether as its functionality is completely subsumed by instanceCreate.
(Plus instanceOf where this is necessary, which it should not be for action registrations - generally instanceOf should
only be needed for instances in the effectively deprecated Services/ folder.) Furthermore, instanceClass and
instanceCreate should be mutually exclusive.

Since there has been no comment from Dafe so far, I am opening a separate issue #142836 requesting that the console
warning be removed, and that no one be asked to use instanceClass gratuitously. This issue - that sometimes extra
instances are created which ideally would not be - can be left as is or reopened independently.

Jesse, I think we have misunderstanding here. Please read my detailed comments above, while I'll try to describe what we
have:

1) Bug impact
Looking at log info in related 142550, we can see that:
- at least 25 menu actions is instantiated twice during every startup
- at least 3 navigator panels are instantiated twice during every startup
- about 10 other actions in Loaders/text and Editors/text are instantiated twice during every startup
- about 12 option panel controllers are instantiated twice when Tools/Options is invoked for first time
...and probably lots of other things which wasn't caught

I'll try to attach thread dumps of multiple instantiations

2) State of fix
I did what i could and the best thing with which I could came is logging warning and using instanceClass attribute. if
anybody come with better solution, I'm perfectly happy with it and I'll remove warning immediately. But so far, I think
warning is the best what we have. Certainly we shouldn't let the things as they are now - I expected this issue to be P0
release stopper for performance team.

Of course we should not gratuitously reinstantiate instances. The problem is the workaround with instanceClass, which
makes no sense from an API perspective. If I had

<file name="x.instance">
  <attr name="instanceCreate" methodvalue="some.Factory.create"/>
</file>

where

package some;
public class Factory {
  public static Action create() {
    return new Action() {...};
  }
}

should I then change my layer to the following?

<file name="x.instance">
  <attr name="instanceCreate" methodvalue="some.Factory.create"/>
  <attr name="instanceClass" stringvalue="some.Factory$1"/>
</file>

This is clearly not an acceptable long-term workaround.

I have read your earlier comments in this issue but they do not explain what is needed to look for a proper fix:

1. Exactly what happens in the filesystems/datasystems/settings infrastructure (which module? which line of code?) that
causes an object to be instantiated twice.

2. Why adding an instanceClass attribute causes this bug not to appear.

Something like InstanceDataObjectModule38420Test should also be useful in debugging the problem, though this test does
use instanceClass.

Created attachment 66707 [details]
thread dumps of instantiations, which explains what is going on

After off line discussion with jtulach, I'm passing back to him for better fix. Sorry I didn't come with something
better myself...

Note I was not exact with my bug impact estimations - most of our actions somehow survive and are instantiated just
once. It seems that only actions that extends javax.swing.Action purely are affected.

SystemAction's, or indeed any objects assignable to SharedClassObject, would not be affected.

FolderList seems to be calling instanceOf for a reasonable purpose - to find nested Lookup's, before actually creating
normal instances. ActionsList is calling instanceOf just to check for JSeparator's, which it will not create.

Should BinaryFS$AttrImpl.getValue be caching the value, at least during startup? Seems in general that caching of
methodvalue/newvalue/serialvalue attributes is desirable.

InstanceDataObject$Ser.instanceCreate does keep a soft cache of the created object, which is fine. There is an obvious
problem that getClassName does not use this cache even when it exists! (If a full GC happened to intervene, and there
was no extant hard ref, then the object would be recreated - which is OK.)

Right, also see my first fix, http://hg.netbeans.org/main/rev/80eea651d4d9, I tried to fill InstanceDataObject$Ser soft
ref cache in getClassName, but the problem is that instance is created "too soon" and it leads to 141682, and possibly
other similar issues, which I didn't know how to fix.

I can add a special API to XMLFS and BinaryFS to provide special handling of "instanceClass" for newvalue and 
methodvalue. In case of newvalue, it is quite easy. In case of methodvalue, I would use the return type of the 
method - not absolutely correct, but hopefully correct enough.

#80eea651d4d9 looks right to me. What was the problem? "Too soon" for whom exactly?

"Too soon" for org.netbeans.modules.options.colors.FontAndColorsPanelController:82, description of what happened is in
141682. In short, Lookup.forPath("some valid path").lookupApp(Object.class) returned empty result for
FontAndColorsPanelController instance - I wasn't able to exactly find out why.

"Lookup.forPath("some valid path").lookupAll(Object.class) returned empty result" - that sounds more like the real
problem. Perhaps a bug in FolderLookup?

I guess the best fix is to enhance contract of XMLFileSystem.getAttribute. Passing to Jirka to review and apply the 
patch.

Created attachment 67384 [details]
hg out -p of the change that introduces getAttribute("class:attrName") handling

Reviewers speak up, so that Jirka or me can integrate next week.

Jesse is on vacation I think.

DS01: What will happen if clients will code creation method returning not exact type, like this:
public static synchronized Action create() ...
instead of 
public static synchronized CreateOnlyOnceAction create()?

I think info about class will not be precise then - isn't it a bit of problem - I mean that some isAssignable tests may
fail, isn't it right? (I don't know if there are any such places in the system, just theoretically) 

Re. DS01, there is a test with Runnable to demonstrate the behaviour. Hopefully not many things fail due to that, but 
you are right, due to the methodvalue type guessing this is in fact semantically incompatible change.

Ok

VV01: We support C and C++ mime-types and have some actions with constructor
MyAction(boolean cpp) 

we don't want to expose internal Action classes at all, so we have factory methods, but even if we are:
public static MyAction craeteCAction() { return MyAction(false);}
public static MyAction craeteCppAction() { return MyAction(true);}

does it mean that now only one random (the first initialized) action will be instantiated?

Re: VV1, right now your method craeteCAction() is called twice. As well as your craeteCppAction() method. After the 
fix, each of your methods will be called just once.

Using MQ, or otherwise producing a single patch for review (e.g. using hg diff -r ... -r ...), would be more friendly
than forcing reviewers to look over several patches of which the later partially overwrite the former.


[JG01] The mistyped overload of methodValue (in both XMLMapAttr and BinaryFS) is confusing. Better would be to make it
return Method (or null), and move the call to Method.invoke into the getObject method's body.


[JG02] BinaryFS.getType should return Class<?>. Also apichanges should specify that a java.lang.Class object is
returned, not a String class name.

Re:[JG01] There is some logic which handles parameters of method. It needs to be duplicated if we want to separate it.

Re:[JG02] Agree but apichanges doesn't need to be updated because BinaryFS.AttrImpl is not public API.

Otherwise I am going to integrate the patch as it is.

JG01 - it is true that methodValue would in fact need to return a struct of (Method, Object[]). No code duplication is
needed; the current call point would call setAccessible and invoke, and the new call point would ignore the Object[] and
just use Method.getReturnType.


JG02 - sorry for confusion, the "apichanges" I was referring to was openide.filesystems/{apichanges,arch}.xml. BTW this
description of behavior should IMO be moved to the XMLFileSystem class Javadoc, not arch.xml, and apichanges.xml should
just have a brief mention of the change (leave the details to real documentation).

Re: JG01 - OK, I added MethodAndParams to store Method and Object[]. Please, look at it if I get it right.

Re: JG02 - Description moved from arch.xml to XMLFilesystem. If you feel it needs to be more polished, please send me a
patch.

Created attachment 68097 [details]
Updated patch.

JG01 - clearer, thanks. BTW you can delete getParams().


JG02 - the change I originally requested was to note that the value type is Class, not String.

Don't forget to remove the warning in InstanceDataObject, and close issue #142550, issue #142539, issue #142541, issue
#142543, issue #142544, issue #142546, issue #142540, and issue #142836 as INVALID.

The warning shall stay there, imho. It is not printed if the underlaying fs supports the "class:XYZ" semantics, but in 
case someone would use for example custom SFS extensions, it could still be useful to identify ineffective 
registrations.

Almost everyone in practice is going to be using XMLFileSystem (in unit tests) and BinaryFS (in the real app). Any
warning printed in such a case is just confusing and irritating - filling up the log file with bad advice.

In those cases where instanceOf is actually important (i.e. where a correct instanceOf may permit instanceCreate to
never be called at all), then the correct attribute to declare is instanceOf, not instanceClass! A warning is acceptable
in case (1) instanceOf actually matters (though this is perhaps impossible to check from InstanceDataObject, since it
depends on the caller's behavior); (2) instanceOf has not been declared; (3) a strange FileSystem impl is in use which
does not support 'class:instanceCreate'.

> Almost everyone in practice is going to be using XMLFileSystem (in unit tests) and BinaryFS (in the real app). Any
> warning printed in such a case is just confusing and irritating - filling up the log file with bad advice.

The current patch prints no warnings in this cases - because both of these systems support "class:XYZ" extension. So 
please keep the warning there for the other cases that may happen when people inject their own filesystem 
implementations into the systemfilesystem.

Integrated (warning not removed).
http://hg.netbeans.org/core-main/rev/9d54d7b841c3

The warning can stay for such hypothetical cases but it is still wrong - it should ask them to specify instanceOf, not
instanceClass.

Integrated into 'main-golden', available in build *200808251401* on http://bits.netbeans.org/dev/nightly/
Changeset: http://hg.netbeans.org/main/rev/9d54d7b841c3
User: Jiri Skrivanek <jskrivanek@netbeans.org>
Log: #131951 - Supress creating of two instances of a class. If you are interested just in the Class of an attribute, but without creating its instance, use fileObject.getAttribute("class:attrName"). This instructs the XMLFileSystem to scan its XML files for definition of attrName attribute and guess its class.

Integrated into 'main-golden', available in build *200808280201* on http://bits.netbeans.org/dev/nightly/
Changeset: http://hg.netbeans.org/main/rev/2de8bea801db
User: Jiri Skrivanek <jskrivanek@netbeans.org>
Log: #131951 - Warning message should ask for instanceOf instead of instanceClass.

Given this is an incompatible change, shouldn't it be marked as incompatible somewhere? BTW: java.debug is broken due to
this, I will likely need to introduce ModuleInstall to fix it.

In what way is this incompatible? How is java.debug broken? Consider filing a separate issue blocking this one.

Yes, please report the incompatibility and let us know. I guess the incompatible change is in loaders, so please 
create new issue.

From what I know, if a methodvalue references a method which return type is j.l.Object but would return an instance of
String, and someone does lookup(String.class) (over the corresponding folder), the lookup result would contain the
String from the method before this patch, but wouldn't after this patch. Jarda seems to talk about it in desc32.

That's true, but unavoidable I think if the patch is to work at all. The workaround would simply be to declare

<attr name="instanceOf" stringvalue="java.lang.String"/>