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

[Martin Buchholz]

Hi font folk,

I just discovered this thread.

At Google, we've been going down the same road.
We had the same crash in jdk7, but we had thought that the changes in jdk8
had fixed it:

changeset:   8197:31b8d4931a09
user:        prr
date:        2013-09-27 13:06 -0700
8020190: Fatal: Bug in native code: jfieldID must match object
Reviewed-by: jgodinez, vadim

but we again saw crashes in jdk8.

I also spent some time staring at CloseTTFontFileFunc and wondering whether
it had any purpose at all other than to cause non-reproducible crashes.

Sorry for not being able to provide a clean reproduction recipe (seems Red
Hatters have the same problem)

We're going to disable some of that code in jdk8 by simply adding to
CloseTTFontFileFunc :

    if (scalerInfo->font2D == NULL)
        return;

but 2d maintainers need to fix this properly.


On Tue, Apr 7, 2015 at 1:50 AM, Andrew Dinn <adinn at redhat.com> wrote:

> Could someone who is working on the 2d-dev codebase please respond to
> James' request?
>
> In summary: the current TTFont code (n.b. this includes the fix needed
> to correct the original erroneous argument passing) is still causing
> crashes in product deployments because the weak reference to the font
> may be nulled before the stream close function tries to dereference it.
>
> This really needs addressing, preferably by removing the /redundant/
> stream close function.
>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in UK and Wales under Company Registration No. 3798903
> Directors: Michael Cunningham (USA), Matt Parson (USA), Charlie Peters
> (USA), Michael O'Neill (Ireland)
>
>
> On 02/04/15 06:36, James Livingston wrote:
> > 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?
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/2d-dev/attachments/20150427/71d0acc3/attachment.html>