Issue 102164

noticed, if "object inspector" is called from writer menu entry "inspect object"
oo will crash:

Microsoft Visual C++ Runtime Library
Runtime Error!
Program: C:\Program Files\OOo-dev 3\program\soffice.bin
This application has requested the Runtime to terminate it in an unusual way.
Please contact the application's support team for more information.

other java extensions are affected too ... 

i tried to remote debug some of my java extension to find the problem
described above - but remote debugging seems not to work anymore with DEV300m48...

java parameter:
-agentlib:jdwp=transport=dt_socket,server=y,address=localhost:8000,suspend=n
should open port 8000 ... this works perfect with oo 3.1 but not with DEV300m48.

"netstat -a" does not show an open port for DEV300m48 ...

i am using jre 1.6_13

added issue http://www.openoffice.org/issues/show_bug.cgi?id=102189
for remote debugging problem

remote debugging seems to work with DEV300m49.

a lot of java extensions will fail to work cause there seems to be a problem 
with the classloader, for example: 

this.xFactory = javax.xml.xpath.XPathFactory.newInstance(); 
will fail with a NullPointerExeception, cause classloader is null.

another example: calling javax.swing.JEditorPane

public static void registerEditorKitForContentType(String type, String 
                          classname, ClassLoader loader) {
  getKitTypeRegistry().put(type, classname);
  getKitLoaderRegistry().put(type, loader);
  getKitRegisty().remove(type);
}
will fail too.

Marcin (see http://www.openoffice.org/servlets/ReadMsg?list=dev&msgNo=24955 for
details) gave me some hints:

adding Thread.currentThread().setContextClassLoader(
getClass().getClassLoader()); will solve the problem temporary

but he mentioned this can be seen only as a hack ;-)

i used JRE 1.6_13 and 1.6_14 for my tests.
a bugfix should work with JRE 1.5 for MAC users too ;-)

btw: there a no problems with oo 3.1 - so i think we have a regression.

Hi,

I just want to mention the beanshell issue 
http://www.openoffice.org/issues/show_bug.cgi?id=89978 from May 2008, which may 
or may not have the same root cause. At least the workaround 
Thread.currentThread().setContextClassLoader(getClass().getClassLoader()) also 
works for issue 89978. See e.g. http://www.oooforum.org/forum/viewtopic.phtml?
t=23763 .
It does not look like there has been any work on 89978 besides setting the 
status on started.

ms777

<#desc5> suggests that this is caused by issue 99290, integrated in DEV300m47
(see
<http://www.openoffice.org/servlets/ReadMsg?list=interface-announce&msgNo=1209>). 
When attaching a native thread to an in-process JVM, OOo used to take care to
set that thread's context class loader to the context class loader initially
obtained from the JVM's initial thread.  For performance reasons, this has been
removed, so that it is now JVM-dependent what the context class loader of such a
thread is (it is typically null).  This change was considered safe, as code that
relies on a context class loader but fails to set one explicitly did not work
reliably at least since OOo 2.3, anyway (see
<http://blogs.sun.com/GullFOSS/entry/is_your_java_extension_ready>).

What's the status of this issue? This is affecting our WollMux extension, too
and I have a bad feeling about the posted workaround. If it was about a Thread
I've spawned myself it would be a different matter, but fiddling with the
classloader of the event dispatching thread doesn't sound like a good idea. I'm
sure application code is not supposed to do that.

Created attachment 64773 [details]
Open attachment, allow macros, press button. If you see "Alles in Ordnung" everything is fine. If you see "Exception: java.lang.NullPointerException" things are broken.

The problem seems to occur only if a Java extension is installed. If I run the
attached BeanShell macro on a plain OOo installation, the context classloader of
the EDT is non-null. However if a Java extension is installed, the EDT's context
classloader is null.

@mux2005:

1 "fiddling with the classloader of the event dispatching thread doesn't sound
like a good idea":  If in a thread you call code that has requirements on the
context class loader, you must ensure that those requirements are met (by
setting and re-setting the context class loader).

2 Re the TestJEditorPane.odt attachment:  Interesting to see that creating a
javax.swing.JEditorPane would require a context class loader (I was not aware of
that).  What is the stack trace of the caught NullPointerException?

> If in a thread you call code that has requirements on the context class loader,
> you must ensure that those requirements are met 

Sure, but AFAIK there is no documented requirement that instantiating a
JEditorPane would require a certain context classloader.

>  What is the stack trace of the caught NullPointerException?

java.lang.NullPointerException
java.util.Hashtable.put(Unknown Source)
javax.swing.JEditorPane.registerEditorKitForContentType(Unknown Source)
javax.swing.JEditorPane.registerEditorKitForContentType(Unknown Source)
javax.swing.JEditorPane.loadDefaultKitsIfNecessary(Unknown Source)
javax.swing.JEditorPane.getKitTypeRegistry(Unknown Source)
javax.swing.JEditorPane.createEditorKitForContentType(Unknown Source)
javax.swing.JEditorPane.getEditorKitForContentType(Unknown Source)
javax.swing.JEditorPane.setContentType(Unknown Source)
javax.swing.JEditorPane.<init>(Unknown Source)

@mux2005:  I could reproduce this with Sun's Java 6u13 and the simple program
  public final class EditorTest {
    public static void main(String[] arguments) {
      Thread.currentThread().setContextClassLoader(null);
      new javax.swing.JEditorPane("text/plain", "");
    }
  }
and I too cannot see that JEditorPane states any requirements that would allow
this to fail.  I filed a (Sun-internal, 6882559) issue against javax.swing, and
will keep you updated on any further information.  (For now, it might work to
not explicitly set the default "text/plain" content type for the
JEditorPane---just calling new javax.swing.JEditorPane() instead did not throw,
but I am not an expert in that area.)

Please reconsider the removal of the classloader setting code. The performance
gains seem very dubious. It's unlikely that anything in the OOo context would
spawn 1000s of threads which would be required to make the time savings visible
on a macroscopic scale. Was this change triggered by actual results from
*profiling* a real world use case? How large were the measured performance gains?
If there were no actual measurements to justify the change this seems like a
prime example of premature optimization (which as we all know is the root of all
evil).

Even if there are performance gains in some special situations, this has to be
weighed against the negative consequences. As we can see from this issue you are
buying your performance gains with complete breakage of existing components.
Buying a little speed in a few use cases with complete breakage of widely
deployed packages would be considered very unwise by most people.

The worst thing is that this creates a permanent Sword of Damocles over the
heads of Java component developers. There is no reason to expect that
JEditorPane is the only code making the assumption that the context classloader
is non-null. And there is no reason to expect that even if this bug is fixed in
Swing it will not reoccur at some other place as code gets rewritten and added
to the JRE. The Java execution environment OOo sets up with its null context
classloader is obviously unusual, something not even the Java guys at Sun test
for, not to speak of the many 3rd party libraries written by developers with
even less knowledge of the hairy details of Java. 

And it cannot be emphasized enough that there is no general workaround for this
problem (at least none has been mentioned here so far). There is no way I can
simply set the context classloader of the EDT myself in my component. I've tried
but it only works temporarily but gets reset at some point, which means I would
need to sprinkle my code with classloader-setting code. And I have no way of
knowing in which places it's needed. Swing and the JRE are moving targets. And
if 3rd party libraries trigger this problem, there is no workaround at all,
because I can't change the 3rd party library.

So please back out that change until you can offer a reliable and generally
applicable way for a Java component to ensure a non-null context classloader
that includes 3rd party libraries.

@mux2005:  I can understand your sentiment, to some degree at least.  We have
seen in the past that lots of Java code relied on an appropriate context class
loader being set, but failed to take care to actually set one.  Such code is not
helped by "the framework" ensuring that generally some "arbitrary" context class
loader is set---in some cases, that "arbitrary" context class loader will be
appropriate and the code in question will happen to work by accident (until
conditions change), in other cases the code in question will fail in more or
less obvious ways.  Against that background, the request to strip down "the
framework" to no longer care to set some "arbitrary" context class loader, but
simply rely on the (also "arbitrary") one already set by the JVM itself, seems
reasonable to me---all the broken code is broken, anyway, regardless whether it
happened to work by accident with the previously set "arbitrary" context class
loader.  (And all the better that the broken code will typically fail faster now
when the "arbitrary" context class loader set by the JVM itself is null.)  It is
hard to tell whether the error in JEditorPane is prototypical of many more
similar errors across the JRE code base, or just a single, unlucky one-off.

@oj:  You requested this change, for performance reasons.  Please comment on the
previous comments from mux2005.

I can't find the numbers but when using the OOo Base the db used is written in
Java and every single call (@see JDBC) will run through this code. So you have a
call for every column value in every row of a table and call to check if it was
NULL. For table with 10 columns and 1000 rows you would have at least 31000
calls when taking into account that you have to move the result set as well. And
no one ask any questions about the ~100 methods big database meta data interface
;-) (which tables, types, columns, keys, users, indexes, etc. exists). And which
database has only one table ;-)

@oj: If I understand you correctly, in the use case you describe a new thread is
started for every call. Rather than attempting to shave off time from thread
creation, wouldn't it be better to eliminate this extensive creation of threads
in the first place? After all, even without the context classloader code, thread
creation takes time. That the eliminated code is called for every database
access does not mean that eliminating it will result in a significant speed
increase. It all depends on how much time is wasted elsewhere.

Aside from that point, this seems like it is very specific to the database
backend. Would it not be possible to treat those threads specially and have
their creation path avoid the context classloader setting code, whereas other
threads would still use it?

And as a bandaid for the current issue, would it not be possible to set the
context classloader at least for the non-application-controlled threads such as
the event dispatching thread. I really don't think OOo 3.2 can ship if commonly
used extensions just break. I think people will much rather live with the
database performance they've now had to live with for years.

@mux2005:  "a new thread is started for every call":  No, it is a single
(native) thread that makes multiple JNI calls to Java, one after another.  Each
such call must attach the thread to the JVM (and then detaches it again).

"would it not be possible to set the
context classloader at least for the non-application-controlled threads such as
the event dispatching thread":  No, that would not work or would not solve the
problem.  For one, those threads that now have a null context class loader are
those attached to the JVM through incoming JNI calls and those in turn spawned
from them; so, if there is a problem with the event dispatch thread having a
null context class loader, this would imply the event dispatch thread was
spawned from such a native thread attached to the JVM, so we would be back to
square one.  For another, some scenarios (like
javax.xml.xpath.XPathFactory.newInstance() or new JEditorPane()) need not even
happen in the event dispatch thread.

The only two ways out I see are:
1  Back out the changes to jmvaccess::VirtualMachine::AttachGuard again for now
(i.e., do set a context class loader for attached threads again) and either live
with the performance degradation in Base for now or somehow modify Base so that
it calls the AttachGuard not around individual Java calls, but around larger
quantities of such calls combined (diminishing the negative impact of the
setContextClassLoader call per AttachGuard creation).  Later, once every code
using the context class loader anywhere would be fixed properly, the backed-out
changes could be put back into AttachGuard.
2  Back out the changes to jvmaccess::VirtualMachine::AttachGuard again and
offer an additional interface ("jvmaccess::VirtualMachine::FastAttachGuard")
that does not set the context class loader, and change the Base code to call
FastAttachGuard instead of AttachGuard.
I would prefer 1 over 2:  In an ideal, correct world, we would only need the
fixed version of AttachGuard (that does not set a context class loader). 
Solution 1 could be turned into such a solution once the world is cleaned up, by
just changing the implementation of AttachGuard.  Solution 2, on the other hand,
would have to introduce a new FastAttachGuard as a sort of a hack to the stable
URE API, and could not remove it again once it becomes unnecessary.

I don't think the world will ever be cleaned up. New incorrect uses of the
context classloader comparable to that in JEditorPane() will spring up as fast
as they are fixed in other places. There are many 3rd party Java libraries. How
could we ever verify them all?
I am a very experienced Java developer and I freely admit that I have only a
basic understanding of classloaders. And I do think I am very representative of
Java developers in this regard. It is irrelevant if developers should care more
about classloaders and should take more time to study all of their intricacies.
They don't. And this won't change. Classloaders are and will forever remain one
of the obscure aspects of Java that developers only care about when things
break. It is telling that it is a part of Swing's core code that has brought
this issue to life. If even Sun's developers can't get context classloader use
right, what do you expect?

So I believe that the only safe choice (and IMHO only a safe choice is
acceptable in an important application such as OOo) is to keep the current
AttachGuard as the general case and only add the optimization where it is known
to be safe. And since the normal AttachGuard can simply call the code of the
FastAttachGuard and then add the classloader setup code, there is no redundant
code that would cause a maintenance issue.

@mux2005:  Again, developers that do not understand and do not care about class
loaders and context class loaders get no help when we put the
context-class-loader-setting back in.  In general, an "arbitrary" context class
loader is as wrong as a null one, so their broken code remains broken.

And re "here is no redundant code that would cause a maintenance issue":  The
maintenance issue that is a problem for me is not any redundant code, but the
additional interface at the URE API.

@mux2005: I cannot confirm that there is any problem with JEditorPane. I'm using
it in LanguageTool and I didn't have to set the classloader for it before.
Anyway, for some other code, I set the classloader at the beginning of execution
of the extension and that's all. I'm afraid you might find that under some buggy
platforms (such as Mac OSX with buggy Java 1.5) you will find that your
extension doesn't work without setting the classloader anyway.

@sb: I would think that the classloader set by OOo so far wasn't arbitrary.
Surely it was capable of loading the standard classes from the JRE. What else
could JEditorPane() want a classloader for?

@milek_pl: Have you tested with Java 6? I don't see the problem with Java 5,
either. And I would very much like to set the context classloader at the
beginning of my extension and be done with it, however the affected thread, the
event-dispatching thread, is not under my complete control. Something resets its
context classloader at unknown (to me) times.

Does anyone succeeded to run Object Inspector with OO 3.2?
I tried to recompile it against JDK 1.5 and setting it in OO as default, but it
still crashes as described above.

As described in comments made by sba, the problem seems to be Java-related and
can be avoid by fixing the extension. So this issue is set to "won't fix".

closed

closed