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

[Andrew Dinn]

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?
>