Created attachment 113355 [details]
stacktrace

I've bumped the priority to P2 since this kills jVi.

I'll download latest and sources and see what I can find out.

Looking at the exception reports it interesting that they started showing up on Mac-OSX on Nov 7. But didn't show up on Linux until Nov 21

jVi puts over 60 items in "Editors/Actions". Last march I started doing it programatically instead of with layer.xml.

During the execution of statement
                InstanceDataObject.create(df, name + ".instance",
                                              action, null);
in the following code the exception occurs.


@ServiceProvider (service=FileSystem.class)
public class KeyActionsFS extends MultiFileSystem
{
    private static final String MY_ROOT = "Editors/Actions";

    private static KeyActionsFS INSTANCE;

    @SuppressWarnings("LeakingThisInConstructor")
    public KeyActionsFS() {
        //will be created on startup, exactly once
        assert INSTANCE == null;
        INSTANCE = this;
        // setPropagateMasks(true); // in case you want to use *_hidden masks
    }

    public static void injectKeyActionsLayer() {
        if (INSTANCE.getDelegates().length == 0) {
            try {
                FileSystem fs = FileUtil.createMemoryFileSystem();
                FileObject dir = FileUtil.createFolder(fs.getRoot(), MY_ROOT);
                DataFolder df = DataFolder.findFolder(dir);

                for(Action action : KeyBinding.getActionsList()) {
                    String name = (String)action.getValue(Action.NAME);
                    InstanceDataObject.create(df, name + ".instance",
                                              action, null);
                }

                INSTANCE.setDelegates(fs);
            } catch(IOException ex) {
                Logger.getLogger(KeyActionsFS.class.getName()).
                        log(Level.SEVERE, null, ex);
            }
        }
    }
}

Jesse should this be assigned to Data Systems?
Thank you

Immediate bug - robustness - is clearly in openide.loaders. Is there a minimal self-contained test case?

I think the real problem here is in faulty module code; InstanceDataObject.create may only be called on a folder in the system filesystem (it ought to immediately throw an IllegalArgumentException if this is not true), since a *.xml file on any other filesystem is going to be treated as a plain old XML file by org.netbeans.modules.xml.XMLDataLoader.

Anyway the quoted module code would behave badly even in the absence of XMLDataLoader - you would not want to serialize an Action and then deserialize it! InstanceDataObject.create is useless for this purpose. On a memory filesystem you can simply use dir.createData("whatever.instance").setAttribute("instanceCreate", theActualInstance) and that object reference will be held as is.

Only applies to a single module not in the standard distro, root cause is badly written module code, NB bug is just in failing to throw a more informative exception earlier => P4.

(In reply to comment #6)

> I think the real problem here is in faulty module code;

Could be. Though the code's been working since March and started failing a few days ago on linux.

I will do:
    dir.createData(name + ".instance")
       .setAttribute("instanceCreate", action)
in the loop and see how it goes

> NB bug is just in failing to throw a more informative
> exception earlier => P4.

About 8 months "earlier" would have been good ;-)



> Immediate bug - robustness - is clearly in openide.loaders. Is there a minimal
> self-contained test case?

If someone can point to a unit test in the right test directory that could serve as a template, I could do one, but I'm not sure I see the usefulness since:

    > InstanceDataObject.create may only be called on a folder in the
    > system filesystem (it ought to immediately throw an 
    > IllegalArgumentException if this is not true)

But, assuming I get something that works, I wouldn't mind submitting a unit test with the new code so it doesn't break again.

(In reply to comment #7)
> the code's been working since March and started failing a few
> days ago on linux.

Sorry, no clue what would have changed recently.

Note that IDO.create would work on any filesystem if the result is a *.instance file - i.e. a SharedClassObject; or if the result is a *.settings file - anything else, possibly needing serialization. Here it seems it is creating a *.xml file. I am not sure what would trigger that; InstanceDataObject.Creator does not seem to ever create *.xml. Or perhaps a *.settings is being created but XMLDataLoader recognizes it for some reason TBD. Yarda probably knows this code better and could say.

> About 8 months "earlier" would have been good

Sure; possibly you are the first person to try doing this, so no one thought to defend against it. (I am just guessing that running IDO.create on a non-system filesystem cannot work, but it may depend on the real explanation of this exception.)

(In reply to comment #8)
> (I am just guessing that running IDO.create on a non-system
> filesystem cannot work,

Isn't that what jVi's been doing sucessfully?

Anyway...

As indicated, I did
    dir.createData(name + ".instance")
       .setAttribute("instanceCreate", action)
and jVi seems to start up now. But...

Now I get 60+ like following, any idea how to get rid of? Would something like
    xxx.setAttribute("instanceOf", "javax.swing.Action")
work, or use a Class object? or...

WARNING [org.openide.loaders.InstanceDataObject]: Instance file MultiFileObject@1de993a[Editors/Actions/ViCtrl-TKey.instance] uses instanceCreate attribute, but doesn't define instanceOf attribute. Please add instanceOf attr to avoid multiple instances creation,see details at http://www.netbeans.org/issues/show_bug.cgi?id=131951

The .settings loader is preferred on system file system. On other systems, it is one of the last ones. Imho, there had to be some change in XML module that started to recognize .settings files during last few months.

I've added fix for issue #206346 (changeset jet-main #9c0ebcc2eecb), adding a fallback MIME resolver for XML files, based on "magic" <?xml header.

(In reply to comment #11)
> I've added fix for issue #206346 (changeset jet-main #9c0ebcc2eecb), adding a
> fallback MIME resolver for XML files, based on "magic" <?xml header.

Actually that's issue #191544

Sorry :-/ wrong clipboard content during copy/paste ... thanks for correction

Thanks Jesse, the code
        FileObject fo = dir.createData(name + ".instance");
        fo.setAttribute("instanceCreate", action);
seems to work.

The check in InstanceDataObject::getClassName that produces the WARNING may be flawed when the FileObject is set up programatically. Doing
        fo.setAttribute("instanceOf", "javax.swing.Action");
does not get rid of the warning. 

But doing
        fo.setAttribute("instanceClass", action.getClass());
or
        fo.setAttribute("instanceClass", Action.class);
does get rid of the warning.

After a quick look at the code, I was lost as to what's going on in InstanceDataObject.java in this case to actually pick up the object.

(In reply to comment #11)
> jet-main #9c0ebcc2eecb [adds] a
> fallback MIME resolver for XML files, based on "magic" <?xml header

Possibly this should be limited so it does not accidentally match *.settings files. (Not sure if the DTD is capable of expressing that, unfortunately; depend on bug #18910 if not.)

That would explain the apparent regression: XMLDataLoader always ignored files on the SFS, but accepted files on other FSs even if injected into the SFS (reporter's situation); but previously *.settings was not matched by anything, whereas now xml-fallback-resolver.xml treats it as text/xml so XMLDL recognizes it. The following fails with the reported CCE:

package org.netbeans.modules.xml;
import java.util.ArrayList;
import junit.framework.Test;
import org.netbeans.junit.NbModuleSuite;
import org.netbeans.junit.NbTestCase;
import org.openide.filesystems.FileUtil;
import org.openide.loaders.DataFolder;
import org.openide.loaders.InstanceDataObject;
public class InstanceDataObjectInXmlTest extends NbTestCase {
    public InstanceDataObjectInXmlTest(String name) {
        super(name);
    }
    public static Test suite() {
        return NbModuleSuite.createConfiguration(InstanceDataObjectInXmlTest.class).gui(false).suite();
    }
    public void testInstanceDataObjectCreate() throws Exception {
        InstanceDataObject.create(DataFolder.findFolder(FileUtil.createMemoryFileSystem().getRoot()), null, new ArrayList<Object>(), null);
    }
}

(In reply to comment #14)
> The check in InstanceDataObject::getClassName that produces the WARNING may be
> flawed when the FileObject is set up programatically.

Quite possible, since probably you are the first to try it.

> Doing
>         fo.setAttribute("instanceOf", "javax.swing.Action");
> does not get rid of the warning. 

Did you try setting this attribute _before_ instanceCreate? Might be a race condition.

>         fo.setAttribute("instanceClass", action.getClass());
> or
>         fo.setAttribute("instanceClass", Action.class);
> does get rid of the warning.

Properly it would be Action.class.getName() (the attr is of type String) but anyway this is wrong; instanceClass is only for cases where instanceCreate is not set.

> I was lost as to what's going on in InstanceDataObject.java

I think the warning is wrong and should be removed. See bug #131951 comment #43 for some background. There is no purpose for instanceOf on an eager memory filesystem, nor (acc. to #44) for XMLFS-defined files (layers), only on some hypothetical injectable filesystem that supports laziness yet is not based on XMLFS. This seems sufficiently unlikely that the warning is more likely to be misleading, as here.

Anyway it is not clear that 2de8bea801db was right; it changed the warning text but not the surrounding logic in getClassName, which continues to check for instanceClass, not instanceOf.

> (In reply to comment #14)
> > Doing
> >         fo.setAttribute("instanceOf", "javax.swing.Action");
> > does not get rid of the warning. 
> 
> Did you try setting this attribute _before_ instanceCreate? Might be a race
> condition.

Doh! Exception *was* when doing 'setAttribute("instanceCreate"' But doing
            fo.setAttribute("instanceOf", "javax.swing.Action");
            fo.setAttribute("instanceCreate", action);
still produces the warning

I'll leave it as
            fo.setAttribute("instanceCreate", action);
            fo.setAttribute("instanceClass", action.getClass().getName());
Since this doesn't produce a warning (they're all same class). And it avoids that messy serialized .setting thing that gets persisted (Ugh). 

If the warning gets removed as Jesse suggests. Then only
            fo.setAttribute("instanceCreate", action);
is needed? I saw the warning last March; avoiding it might be how I ended up inadvertantly creating a .setting, but I don't remember.

OK. Any remaining complain? Can I get a unit test, if so?

(In reply to comment #17)
> Any remaining complain[ts]?

Yes. The test I gave in comment #15 fails whereas I think it should either pass, or (if this scenario is intentionally not supported) throw a meaningful exception earlier. Also as I described in comment #15 the warning is wrong and should be removed.

...so reopening.

ergonomics#3bcf706b3b7b

Integrated into 'main-golden', will be available in build *201204270400* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/3bcf706b3b7b
User: Jaroslav Tulach <jtulach@netbeans.org>
Log: #205336: InstanceDataObject.create should work on non-systemfilesystem filesystems as well. Using setPreferredLoader.