Bug 210618 - Missing custom property editors when IDE is launched from NbModuleSuite
Missing custom property editors when IDE is launched from NbModuleSuite
Status: VERIFIED FIXED
Product: platform
Classification: Unclassified
Component: Property Editors
7.2
PC Windows XP
: P3 (vote)
: 7.4
Assigned To: Stanislav Aubrecht
issues@platform
: REGRESSION, T9Y
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-03 13:11 UTC by Jiri Skrivanek
Modified: 2013-06-06 08:18 UTC (History)
8 users (show)

See Also:
Issue Type: DEFECT
:


Attachments
Missing buttons of property editors. (10.68 KB, image/png)
2012-04-03 13:11 UTC, Jiri Skrivanek
Details
screenshot from CssCompletionTest (34.90 KB, image/png)
2012-05-03 07:39 UTC, Vladimir Riha
Details
Thread groups listing. (3.62 KB, text/plain)
2012-05-03 09:53 UTC, Jiri Skrivanek
Details
Use: hg unbundle X.hg to get the real changeset (53.35 KB, application/octet-stream)
2012-08-22 14:28 UTC, Jaroslav Tulach
Details
hg diff --git >X.diff version of previous Hg bundle (70.42 KB, patch)
2012-08-22 14:29 UTC, Jaroslav Tulach
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiri Skrivanek 2012-04-03 13:11:24 UTC
Created attachment 117739 [details]
Missing buttons of property editors.

Custom property editors are missing if IDE is launched from NbModuleSuite. As you can see at screen shot, it is possible only to edit properties inline but "..." buttons are not available. It started to happen in tests on trunk builds. To reproduce:

- open jellytools\test\qa-functional\src\examples\PropertiesTest.java
- run file
- test opens project and invokes property sheet for bundle key
Comment 1 Vladimir Riha 2012-05-03 07:39:51 UTC
Created attachment 119000 [details]
screenshot from CssCompletionTest

I may have a similar problem. When I run CssCompletionTest (in [1]), then some values for css properties (e.g. color) are missing and instead of them there are some bogus items (see the screenshot). If I start NetBeans not from NbModuleSuite it works as it should. To reproduce run the CssCompletionTest and wait for e.g. testValues.css where it happens



[1] /web.core.syntax/test/qa-qunctional/src/org/netbeans/test/syntax/CssCompletionTest.java
Comment 2 Jiri Skrivanek 2012-05-03 09:53:18 UTC
It seems it is some general problem in the way how IDE is launched from NbModuleSuite. Tests started to fail in April. PropertiesTest last time passed on April 7-th and failed on April 16-th (tests didn't run in between). If I debug PropertiesTest, it sometimes works correctly. I found out that there is a different ThreadGroup for each case. In case of failure there is ThreadGroup[name=system], in case of success there is TopThreadGroup[name=IDE Main]. Please, look at attachment.
Comment 3 Jiri Skrivanek 2012-05-03 09:53:57 UTC
Created attachment 119016 [details]
Thread groups listing.
Comment 4 Jesse Glick 2012-05-03 18:24:50 UTC
Reporter should consider using hg bisect to help analysis, since it is not really clear even what component is responsible.

The "OK" thread list shows nothing much happening, as if the IDE is not really running, or is not in GUI mode. The "FAIL" list looks normal.

BTW tests like this should avoid opening a project - unnecessary dependencies (j2seproject), time (scanning). Better to just use getWorkDir() to dump some files in, and then add this dir to the Favorites tab.
Comment 5 Jiri Skrivanek 2012-05-11 08:36:30 UTC
> Reporter should consider using hg bisect to help analysis, since it is not
> really clear even what component is responsible.

The test passes with b05058267e82 which is parent of e06437c73d59:

$ hg bisect --bad
The first bad revision is:
changeset:   218066:e06437c73d59
user:        Jesse Glick <jglick@netbeans.org>
date:        Thu Apr 05 14:59:05 2012 -0400
summary:     #210716 hotfix: reverting project.libraries portion of abcea64f592c.

Actually when Jarda reverted #e06437c73d59 by #4278d973f335, it fails again because of changes in NbInstaller. So, finally if I revert changes in NBInstaller the test passes

hg revert -r 772113ff2fb5 core.startup/src/org/netbeans/core/startup/NbInstaller.java
ant -f core.startup
ant -f jellytools test-qa-functional -Dtest.includes=**/PropertiesTest.class -Dnbjdk.home=D:/jdk1.7.0

It is also strange that the test always fails with JDK 6.

> BTW tests like this should avoid opening a project - unnecessary
> dependencies (j2seproject), time (scanning). Better to just use 
> getWorkDir() to dump some files in, and then add this dir to the 
> Favorites tab.

This test is executed within a suite, so project is opened just once at the beginning of the suite.
Comment 6 Jesse Glick 2012-05-11 17:19:51 UTC
Seems like CoreBridgeImpl.registerPropertyEditors is not getting called at the right time? Or some @OnStart process is (as of 4278d973f335) running later and somehow clobbering the core property editor search path? Or due to a ModuleInstall, which is now run in a different order as an incidental side effect?
Comment 7 Jaroslav Tulach 2012-05-14 09:23:14 UTC
The String property editor is really "org.netbeans.beaninfo.editors.StringEditor", but in the test it is loaded from application classloader, not from a module. As a result the check

editor instanceof ExPropertyEditor 

yields false and the custom editor is disabled.
Comment 8 Jaroslav Tulach 2012-05-14 11:50:44 UTC
It seems to me this is a problem of the JDK. When I run on

$ java -version
java version "1.6.0_24"
OpenJDK Runtime Environment (IcedTea6 1.11.1) (6b24-1.11.1-4ubuntu2)
OpenJDK Server VM (build 20.0-b12, mixed mode)

then the proper editor class is loaded (coming from Thread.currentThread.contextClassLoader). While when I run the test on 

$ /usr/local/lib/jdk1.6.0/bin/java -version
java version "1.6.0_30"
Java(TM) SE Runtime Environment (build 1.6.0_30-b12)
Java HotSpot(TM) Server VM (build 20.5-b03, mixed mode)

I get wrong editor class from ClassLoader.getSystemClassLoader(). I contribute this to the fact that the older JDK is using (in ClassFinder.findClass):

            ClassLoader loader = Thread.currentThread().getContextClassLoader();
            if ( loader == null ) {
                // can be null in IE (see 6204697)
                loader = ClassLoader.getSystemClassLoader();
            }
            if ( loader != null ) {
                return Class.forName( name, false, loader );
            }

while the JDK u30 is using the application class loader first (from Introspector.instantiate):

	// Now try the system classloader.
	try {
	    cl = ClassLoader.getSystemClassLoader();
	    if (cl != null) {
	        Class cls = cl.loadClass(className);
		return cls.newInstance();
	    }
        } catch (Exception ex) {
	    // We're not allowed to access the system class loader or
	    // the class creation failed.
	    // Drop through.
	}

	// Use the classloader from the current Thread.
	cl = Thread.currentThread().getContextClassLoader();
	Class cls = cl.loadClass(className);
	return cls.newInstance();

I consider this change a regression in the JDK.
Comment 9 Jaroslav Tulach 2012-05-14 12:03:56 UTC
(In reply to comment #5)
> > Reporter should consider using hg bisect to help analysis, since it is not
> > really clear even what component is responsible.
> 
> The test passes with b05058267e82 

The test passes OK when executed on 1.6.0_u24 @ b05058267e82, but it fails on 1.6.0_u30 @ b05058267e82 on my computer. Again, this leads me to conclusion that this is a regression in JDK.
Comment 10 Jiri Skrivanek 2012-05-15 07:37:19 UTC
If it is JDK bug then please file it but until it is fixed we have to be able to run our tests without such problems. Still there is a question why custom editors are initialized properly when IDE is launched from regular launcher and not when IDE starts using NbModuleSuite. Please, fix this discrepancy.
Comment 11 Jaroslav Tulach 2012-05-25 08:38:30 UTC
When running unit tests in NbModuleSuite, each class is available twice. Once on application classpath once loaded by module system. The regression in new JDK causes the class from classpath to be loaded. There is no workaround, this has to be fixed on the JDK side.

The question to ask is why the JDK team has not discovered this regression by running NetBeans binary test distribution sooner than publishing the JDK to public?
Comment 12 Jiri Skrivanek 2012-05-25 09:42:42 UTC
(In reply to comment #11)
> The question to ask is why the JDK team has not discovered this regression by
> running NetBeans binary test distribution sooner than publishing the JDK to
> public?

Probably there is no suitable test case in the stable test distribution which we provide to JDK team. It is only a selection of all tests and most of qa-functinal tests is excluded.
Comment 13 Jaroslav Tulach 2012-05-25 14:20:08 UTC
I've just verified that the PropertiesTest passes OK on jdk1.7.0_04 - hopefully this is really regression only in jdk1.6.0_30 and will not be part of JDK releases in the future.
Comment 14 Jiri Skrivanek 2012-05-28 07:57:34 UTC
It passes because I have recently added workaround to JellyTestCase (f5074892aa4b and 303b5b79ecb2). It is not perfect because custom editors registered by modules are ignored but my tests pass. To reproduce faulty behaviour revert to parent changeset

hg rev jellytools.platform/src/org/netbeans/jellytools/JellyTestCase.java -r 640f9240be68

It is still an issue for me because as I said we should execute our tests in almost the same IDE as run from launcher.
Comment 15 Antonin Nebuzelsky 2012-05-28 10:59:12 UTC
> I consider this change a regression in the JDK.

Actually not a regression in JDK 6uX. The code above was all the same in all updates of JDK 6 since at least 6u10.

The version 1.6.0_24 referred above as the correct code for our purpose is Open JDK 6, which is actually based on Java 7 codebase (minus Java 7 features).

This explains the difference.

Java 8 and Java 7 codebase (and OpenJDK 6 codebase) contain the code considered correct in the comments above.

Java 6uX contains the different code, but for such a long time it cannot be considered a regression.

> It passes because I have recently added workaround to JellyTestCase

Decreasing the priority and passing back to Jirka for more evaluation.
Comment 16 Jiri Skrivanek 2012-05-28 11:39:58 UTC
As I wrote in comment 14 it works for me only with workaround in JellyTestCase. If I revert it, it still doesn't work with JKD7.

Java; VM; Vendor = 1.7.0_04; Java HotSpot(TM) 64-Bit Server VM 23.0-b21; Oracle Corporation
Runtime = Java(TM) SE Runtime Environment 1.7.0_04-b20

hg rev jellytools.platform/src/org/netbeans/jellytools/JellyTestCase.java -r
640f9240be68
ant -f jellytools.platform
ant -f jellytools test-qa-functional -Dtest.includes=**/CustomPropertiesTest.class -Dnbjdk.home=D:/jdk1.7.0

I still believe it is possible to somehow fix it either in NbModuleSuite or somewhere in core. It worked before e06437c73d59.
Comment 17 Jaroslav Tulach 2012-05-29 20:16:44 UTC
It is nice you give reference to changeset 640f9240be68, but my hope that it will give me some insight into what your code change is has vanished after seeing the enormous diff of changes all over the place and reading the second part of your commit description: "Removed workaround for JDK bug 4924516. It is fixed in JDK1.5. Also fixed warnings and formatting."

Next time I suggest to separate real code changes from beautification of the code into two changesets.
Comment 18 Jiri Skrivanek 2012-05-30 07:27:53 UTC
Changeset 640f9240be68 was mentioned just like the state of JellyTestCase before applying my workaround. Changesets with workaround are #f5074892aa4b and #303b5b79ecb2.
Comment 19 Jaroslav Tulach 2012-08-22 12:26:40 UTC
Reminds me that JDK7 now registers property editors per ThreadGroup...
Comment 20 Jaroslav Tulach 2012-08-22 14:28:46 UTC
Created attachment 123407 [details]
Use: hg unbundle X.hg to get the real changeset
Comment 21 Jaroslav Tulach 2012-08-22 14:29:46 UTC
Created attachment 123408 [details]
hg diff --git >X.diff version of previous Hg bundle
Comment 22 Jaroslav Tulach 2012-08-22 14:35:43 UTC
I've managed to make the test run almost without errors by making sure the propertyeditor are removed from classpath of the test.

I've introduced new module and put all the editors there. The jellytools & friends must not have a dependency on the module.

Few questions:
- is it OK/desirable to separate editors into own module?

- can we delete hackFileChooser? I thought the whole point of introducing org.openide.filesystems.FileChooserBuilder was to concentrate all necessary hacks inside it. If there is something missing in behavior of FileChooserBuilder, it should be added.

- FileStateEditor should not extend ListImageEditor. Rather an IntEditor should be extended to support images provided via its Env.

- there is some strange contract of HtmlBrowser.FactoryEditor - possibly it should be replaced with a value in NbPreferences.
Comment 23 Jaroslav Tulach 2012-08-22 14:59:56 UTC
(In reply to comment #22)
> - FileStateEditor should not extend ListImageEditor. Rather an IntEditor should
> be extended to support images provided via its Env.

At a second sight it looks like FileStateEditor is the only subclass and user of ListImageEditor. In such case merge these two and move the code into module that contains FileStateEditor.
Comment 24 Jiri Skrivanek 2013-01-08 16:13:05 UTC
After http://hg.netbeans.org/main-silver/rev/b1efa878877e (@PropertyEditorRegistration and @PropertyEditorSearchPath annotations) my last workaround in JellyTestCase stopped to work. I don't know if lastly attached diff by Jarda is still an option.
I think the problem is still the same - property editors are registered in different ThreadGroup than they are later expected. It is because AWT-EventQueue-0 thread is created in the system ThreadGroup instead of in TopThreadGroup(IDE Main). Potential fix is to remove line 193 in CoreBridgeImpl and register property editors "really" in AWT thread:

-        NodeOp.registerPropertyEditors();
         SwingUtilities.invokeLater(new Runnable() {
             @Override
             public void run() {
                 NodeOp.registerPropertyEditors();
             }
         });

Until it is fixed this way or a different way I will commit a new workaround in JellyTestCase.
Comment 25 Quality Engineering 2013-01-09 02:26:32 UTC
Integrated into 'main-golden', will be available in build *201301090001* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/2f304f35bc52
User: Jiri Skrivanek <jskrivanek@netbeans.org>
Log: #210618 - register NetBeans property editors again in AWT thread if not properly registered previously.
Comment 26 Stanislav Aubrecht 2013-06-03 08:46:49 UTC
(In reply to comment #24)
> After http://hg.netbeans.org/main-silver/rev/b1efa878877e
> (@PropertyEditorRegistration and @PropertyEditorSearchPath annotations) my last
> workaround in JellyTestCase stopped to work. I don't know if lastly attached
> diff by Jarda is still an option.
> I think the problem is still the same - property editors are registered in
> different ThreadGroup than they are later expected. It is because
> AWT-EventQueue-0 thread is created in the system ThreadGroup instead of in
> TopThreadGroup(IDE Main). Potential fix is to remove line 193 in CoreBridgeImpl
> and register property editors "really" in AWT thread:
> 
> -        NodeOp.registerPropertyEditors();
>          SwingUtilities.invokeLater(new Runnable() {
>              @Override
>              public void run() {
>                  NodeOp.registerPropertyEditors();
>              }
>          });
> 
> Until it is fixed this way or a different way I will commit a new workaround in
> JellyTestCase.

The property editors are registered in both thread groups as of #209598
Comment 27 Quality Engineering 2013-06-05 09:35:49 UTC
Integrated into 'main-golden', will be available in build *201306050626* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/a7dfcf37a3c4
User: Jiri Skrivanek <jskrivanek@netbeans.org>
Log: #210618 - removed workaround.


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