[OpenJDK 2D-Dev] What is the point of CloseTTFontFileFunc?

[James Livingston]

Hi Phil et al,

On Fri, 2015-01-16 at 09:19 +0000, Andrew Dinn wrote:
> On 15/01/15 22:53, Phil Race wrote:
> > I can only suppose the author thought he was going to pass the
> > platName (which would be the font file path) to some code that would
> > use it to somehow close an open descriptor on that file. But clearly
> > it does nothing of the sort and an up-call to the java font object
> > (like in the read function) would not need that information anyway.
> > So it looks redundant. I don't see how it would relate to the crash.
> > That *may* be caused by some simultaneous use and/or freeing in
> > different threads of files created using Font.createFont(..) and are 
> > now bing released. I've also seen crashes where admins clean out the
> > /tmp folder whilst the JRE is running using font files there. But
> > that would probably not involve the disposer code.
> 
> The crash has been pinned down to some faulty argument passing. However,
> I am glad to hear that the code appears to be redundant. My concern was
> that it might be worse than that since the native code calls
> JNU_ReleaseStringPlatformChars which, potentially***, can release
> memory. Since platName is created by Java code (Using a combination of
> String literals and String+ operations) that would be a /very bad/
> thing. So, I think this callback should probably be removed (as an a
> fortiori argument I'll note that it is, at best, redundant).

I was involved in this issue internally at Red Hat when Andrew raised it
on the list, and I think there are some more problems with
CloseTTFontFileFunc.


We have a report of a crash since font2D is null, and that function does
not check for it:

#5  <signal handler called>
#6  resolve_non_null (env=0x7f5b11efc9d8, obj=0x0, fieldID=0xc2)
at /usr/src/debug/java-1.7.0-openjdk/openjdk/hotspot/src/share/vm/runtime/jniHandles.hpp:202
#7  jni_GetObjectField (env=0x7f5b11efc9d8, obj=0x0, fieldID=0xc2)
at /usr/src/debug/java-1.7.0-openjdk/openjdk/hotspot/src/share/vm/prims/jni.cpp:2640
#8  0x00007f5af6671e31 in CloseTTFontFileFunc (stream=<value optimized
out>) at ../../../src/share/native/sun/font/freetypeScaler.c:161



All the places in FreetypeFontScaler which pass in
the font, other than initialisation, get it by calling
WeakReference.get(). I can't see anything which guarantees that the
font's lifetime is strictly longer than the scaler, and there are
comments in FontScaler which say:

 scalers are disposed when associated Font2D object (e.g.
 TruetypeFont) is garbage collected. That's why this object
 implements DisposerRecord interface directly (as it is not used
 as indicator when it is safe to release native state) and
 that's why we have to use WeakReference to Font internally.


To me, that sounds like the lifetime of the font definitely could be
short than the scaler, so assuming it is non-null in native code is bad.

Since last time no-one could find a reason for CloseTTFontFileFunc doing
what it does, it is probably better to just remove the code rather than
adding null checks. What do you think?

-- 
James "Doc" Livingston <jlivings at redhat.com>
JBoss Support Engineering Group
Red Hat