Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 8b48e624 authored by Seigo Nonaka's avatar Seigo Nonaka
Browse files

Follow minikin::FontFamily constructor signature change. 

This is 2nd attempt of Id10ca97f6f6f5bbe4999c1ad2736423a204d6e87.
The root caus of crash is due touching null pointer in nUnrefFamily.
After above change, native object may not be created on error case.
In that case, nUnrefFamily can not be called. In addition to this issue
there is also memory leak on error case. This fixes it by introducing
abortCreation method.

Also this contains fix of layout lib breakage done by jgaillard@.
Originally submitted by Ic8872a43993bcb0a157c5e3f0ce423af9b47f606

Here is original commit message of reverted change.

minikin::FontFamily no longer has addFont function, instead it accept
vector of Fonts in its constructor. To follow this signature change,
holding minikin::Font instance in native and build minikin::FontFamily
instance in FontFamily.freeze() method.

Bug: 34042446
Bug: 28119474
Bug: 34378805
Test: hwui test has passed
Test: m layoutlib layoutlib-tests
Change-Id: Ic34ebaa8191273d4c9f49c43124f15a1da5f7b78
parent 78b09a00

core/jni/android/graphics/FontFamily.cpp

+59 −22
Original line number Diff line number Diff line
@@ -39,27 +39,60 @@ 


namespace android {



static jlong FontFamily_create(JNIEnv* env, jobject clazz, jstring lang, jint variant) {

    if (lang == NULL) {

        return (jlong)new minikin::FontFamily(variant);

    }

struct NativeFamilyBuilder {

    uint32_t langId;

    int variant;

    std::vector<minikin::Font> fonts;

};



static jlong FontFamily_initBuilder(JNIEnv* env, jobject clazz, jstring lang, jint variant) {

    NativeFamilyBuilder* builder = new NativeFamilyBuilder();

    if (lang != nullptr) {

        ScopedUtfChars str(env, lang);

    uint32_t langId = minikin::FontStyle::registerLanguageList(str.c_str());

    return (jlong)new minikin::FontFamily(langId, variant);

        builder->langId = minikin::FontStyle::registerLanguageList(str.c_str());

    } else {

        builder->langId = minikin::FontStyle::registerLanguageList("");

    }

    builder->variant = variant;

    return reinterpret_cast<jlong>(builder);

}



static void FontFamily_unref(JNIEnv* env, jobject clazz, jlong familyPtr) {

static jlong FontFamily_create(jlong builderPtr) {

    if (builderPtr == 0) {

        return 0;

    }

    NativeFamilyBuilder* builder = reinterpret_cast<NativeFamilyBuilder*>(builderPtr);

    minikin::FontFamily* family = new minikin::FontFamily(

            builder->langId, builder->variant, std::move(builder->fonts));

    delete builder;

    return reinterpret_cast<jlong>(family);

}



static void FontFamily_abort(jlong builderPtr) {

    NativeFamilyBuilder* builder = reinterpret_cast<NativeFamilyBuilder*>(builderPtr);

    minikin::Font::clearElementsWithLock(&builder->fonts);

    delete builder;

}



static void FontFamily_unref(jlong familyPtr) {

    minikin::FontFamily* fontFamily = reinterpret_cast<minikin::FontFamily*>(familyPtr);

    fontFamily->Unref();

}



static jboolean addSkTypeface(minikin::FontFamily* family, sk_sp<SkTypeface> face,

        const void* fontData, size_t fontSize, int ttcIndex) {

static void addSkTypeface(jlong builderPtr, sk_sp<SkTypeface> face, const void* fontData,

        size_t fontSize, int ttcIndex) {

    minikin::MinikinFont* minikinFont =

            new MinikinFontSkia(std::move(face), fontData, fontSize, ttcIndex);

    bool result = family->addFont(minikinFont);

    NativeFamilyBuilder* builder = reinterpret_cast<NativeFamilyBuilder*>(builderPtr);

    int weight;

    bool italic;

    if (!minikin::FontFamily::analyzeStyle(minikinFont, &weight, &italic)) {

        ALOGE("analyzeStyle failed. Using default style");

        weight = 400;

        italic = false;

    }

    builder->fonts.push_back(minikin::Font(minikinFont, minikin::FontStyle(weight / 100, italic)));

    minikinFont->Unref();

    return result;

}



static void release_global_ref(const void* /*data*/, void* context) {
@@ -85,7 +118,7 @@ static void release_global_ref(const void* /*data*/, void* context) { 
    }

}



static jboolean FontFamily_addFont(JNIEnv* env, jobject clazz, jlong familyPtr, jobject bytebuf,

static jboolean FontFamily_addFont(JNIEnv* env, jobject clazz, jlong builderPtr, jobject bytebuf,

        jint ttcIndex) {

    NPE_CHECK_RETURN_ZERO(env, bytebuf);

    const void* fontPtr = env->GetDirectBufferAddress(bytebuf);
@@ -112,8 +145,8 @@ static jboolean FontFamily_addFont(JNIEnv* env, jobject clazz, jlong familyPtr, 
        ALOGE("addFont failed to create font");

        return false;

    }

    minikin::FontFamily* fontFamily = reinterpret_cast<minikin::FontFamily*>(familyPtr);

    return addSkTypeface(fontFamily, std::move(face), fontPtr, (size_t)fontSize, ttcIndex);

    addSkTypeface(builderPtr, std::move(face), fontPtr, (size_t)fontSize, ttcIndex);

    return true;

}



static struct {
@@ -126,7 +159,7 @@ static struct { 
    jfieldID mStyleValue;

} gAxisClassInfo;



static jboolean FontFamily_addFontWeightStyle(JNIEnv* env, jobject clazz, jlong familyPtr,

static jboolean FontFamily_addFontWeightStyle(JNIEnv* env, jobject clazz, jlong builderPtr,

        jobject font, jint ttcIndex, jobject listOfAxis, jint weight, jboolean isItalic) {

    NPE_CHECK_RETURN_ZERO(env, font);
@@ -178,10 +211,11 @@ static jboolean FontFamily_addFontWeightStyle(JNIEnv* env, jobject clazz, jlong 
        ALOGE("addFont failed to create font, invalid request");

        return false;

    }

    minikin::FontFamily* fontFamily = reinterpret_cast<minikin::FontFamily*>(familyPtr);

    minikin::MinikinFont* minikinFont =

            new MinikinFontSkia(std::move(face), fontPtr, (size_t)fontSize, ttcIndex);

    fontFamily->addFont(minikinFont, minikin::FontStyle(weight / 100, isItalic));

            new MinikinFontSkia(std::move(face), fontPtr, fontSize, ttcIndex);

    NativeFamilyBuilder* builder = reinterpret_cast<NativeFamilyBuilder*>(builderPtr);

    builder->fonts.push_back(minikin::Font(minikinFont,

            minikin::FontStyle(weight / 100, isItalic)));

    minikinFont->Unref();

    return true;

}
@@ -190,7 +224,7 @@ static void releaseAsset(const void* ptr, void* context) { 
    delete static_cast<Asset*>(context);

}



static jboolean FontFamily_addFontFromAssetManager(JNIEnv* env, jobject, jlong familyPtr,

static jboolean FontFamily_addFontFromAssetManager(JNIEnv* env, jobject, jlong builderPtr,

        jobject jassetMgr, jstring jpath, jint cookie, jboolean isAsset) {

    NPE_CHECK_RETURN_ZERO(env, jassetMgr);

    NPE_CHECK_RETURN_ZERO(env, jpath);
@@ -233,14 +267,17 @@ static jboolean FontFamily_addFontFromAssetManager(JNIEnv* env, jobject, jlong f 
        ALOGE("addFontFromAsset failed to create font %s", str.c_str());

        return false;

    }

    minikin::FontFamily* fontFamily = reinterpret_cast<minikin::FontFamily*>(familyPtr);

    return addSkTypeface(fontFamily, std::move(face), buf, bufSize, /* ttcIndex */ 0);



    addSkTypeface(builderPtr, std::move(face), buf, bufSize, 0 /* ttc index */);

    return true;

}



///////////////////////////////////////////////////////////////////////////////



static const JNINativeMethod gFontFamilyMethods[] = {

    { "nCreateFamily",         "(Ljava/lang/String;I)J", (void*)FontFamily_create },

    { "nInitBuilder",          "(Ljava/lang/String;I)J", (void*)FontFamily_initBuilder },

    { "nCreateFamily",         "(J)J", (void*)FontFamily_create },

    { "nAbort",                "(J)V", (void*)FontFamily_abort },

    { "nUnrefFamily",          "(J)V", (void*)FontFamily_unref },

    { "nAddFont",              "(JLjava/nio/ByteBuffer;I)Z", (void*)FontFamily_addFont },

    { "nAddFontWeightStyle",   "(JLjava/nio/ByteBuffer;ILjava/util/List;IZ)Z",

graphics/java/android/graphics/FontFamily.java

+51 −15
Original line number Diff line number Diff line
@@ -19,6 +19,7 @@ package android.graphics; 
import android.content.res.AssetManager;

import android.text.FontConfig;

import android.util.Log;

import dalvik.annotation.optimization.CriticalNative;



import java.io.FileInputStream;

import java.io.IOException;
@@ -40,11 +41,11 @@ public class FontFamily { 
     */

    public long mNativePtr;



    // Points native font family builder. Must be zero after freezing this family.

    private long mBuilderPtr;



    public FontFamily() {

        mNativePtr = nCreateFamily(null, 0);

        if (mNativePtr == 0) {

            throw new IllegalStateException("error creating native FontFamily");

        }

        mBuilderPtr = nInitBuilder(null, 0);

    }



    public FontFamily(String lang, String variant) {
@@ -54,27 +55,48 @@ public class FontFamily { 
        } else if ("elegant".equals(variant)) {

            varEnum = 2;

        }

        mNativePtr = nCreateFamily(lang, varEnum);

        if (mNativePtr == 0) {

            throw new IllegalStateException("error creating native FontFamily");

        mBuilderPtr = nInitBuilder(lang, varEnum);

    }



    public void freeze() {

        if (mBuilderPtr == 0) {

            throw new IllegalStateException("This FontFamily is already frozen");

        }

        mNativePtr = nCreateFamily(mBuilderPtr);

        mBuilderPtr = 0;

    }



    public void abortCreation() {

        if (mBuilderPtr == 0) {

            throw new IllegalStateException("This FontFamily is already frozen or abandoned");

        }

        nAbort(mBuilderPtr);

        mBuilderPtr = 0;

    }



    @Override

    protected void finalize() throws Throwable {

        try {

            if (mNativePtr != 0) {

                nUnrefFamily(mNativePtr);

            }

            if (mBuilderPtr != 0) {

                nAbort(mBuilderPtr);

            }

        } finally {

            super.finalize();

        }

    }



    public boolean addFont(String path, int ttcIndex) {

        if (mBuilderPtr == 0) {

            throw new IllegalStateException("Unable to call addFont after freezing.");

        }

        try (FileInputStream file = new FileInputStream(path)) {

            FileChannel fileChannel = file.getChannel();

            long fontSize = fileChannel.size();

            ByteBuffer fontBuffer = fileChannel.map(FileChannel.MapMode.READ_ONLY, 0, fontSize);

            return nAddFont(mNativePtr, fontBuffer, ttcIndex);

            return nAddFont(mBuilderPtr, fontBuffer, ttcIndex);

        } catch (IOException e) {

            Log.e(TAG, "Error mapping font file " + path);

            return false;
@@ -83,20 +105,34 @@ public class FontFamily { 


    public boolean addFontWeightStyle(ByteBuffer font, int ttcIndex, List<FontConfig.Axis> axes,

            int weight, boolean style) {

        return nAddFontWeightStyle(mNativePtr, font, ttcIndex, axes, weight, style);

        if (mBuilderPtr == 0) {

            throw new IllegalStateException("Unable to call addFontWeightStyle after freezing.");

        }

        return nAddFontWeightStyle(mBuilderPtr, font, ttcIndex, axes, weight, style);

    }



    public boolean addFontFromAssetManager(AssetManager mgr, String path, int cookie,

            boolean isAsset) {

        return nAddFontFromAssetManager(mNativePtr, mgr, path, cookie, isAsset);

        if (mBuilderPtr == 0) {

            throw new IllegalStateException("Unable to call addFontFromAsset after freezing.");

        }

        return nAddFontFromAssetManager(mBuilderPtr, mgr, path, cookie, isAsset);

    }



    private static native long nCreateFamily(String lang, int variant);

    private static native long nInitBuilder(String lang, int variant);



    @CriticalNative

    private static native long nCreateFamily(long mBuilderPtr);



    @CriticalNative

    private static native void nAbort(long mBuilderPtr);



    @CriticalNative

    private static native void nUnrefFamily(long nativePtr);

    private static native boolean nAddFont(long nativeFamily, ByteBuffer font, int ttcIndex);

    private static native boolean nAddFontWeightStyle(long nativeFamily, ByteBuffer font,

    private static native boolean nAddFont(long builderPtr, ByteBuffer font, int ttcIndex);

    private static native boolean nAddFontWeightStyle(long builderPtr, ByteBuffer font,

            int ttcIndex, List<FontConfig.Axis> listOfAxis,

            int weight, boolean isItalic);

    private static native boolean nAddFontFromAssetManager(long nativeFamily, AssetManager mgr,

    private static native boolean nAddFontFromAssetManager(long builderPtr, AssetManager mgr,

            String path, int cookie, boolean isAsset);

}

graphics/java/android/graphics/Typeface.java

+7 −0
Original line number Diff line number Diff line
@@ -222,10 +222,13 @@ public class Typeface { 


                FontFamily fontFamily = new FontFamily();

                if (fontFamily.addFontFromAssetManager(mgr, path, 0, true /* isAsset */)) {

                    fontFamily.freeze();

                    FontFamily[] families = { fontFamily };

                    typeface = createFromFamiliesWithDefault(families);

                    sDynamicTypefaceCache.put(key, typeface);

                    return typeface;

                } else {

                    fontFamily.abortCreation();

                }

            }

        }
@@ -271,8 +274,11 @@ public class Typeface { 
        if (sFallbackFonts != null) {

            FontFamily fontFamily = new FontFamily();

            if (fontFamily.addFont(path, 0 /* ttcIndex */)) {

                fontFamily.freeze();

                FontFamily[] families = { fontFamily };

                return createFromFamiliesWithDefault(families);

            } else {

                fontFamily.abortCreation();

            }

        }

        throw new RuntimeException("Font not found " + path);
@@ -341,6 +347,7 @@ public class Typeface { 
                Log.e(TAG, "Error creating font " + font.getFontName() + "#" + font.getTtcIndex());

            }

        }

        fontFamily.freeze();

        return fontFamily;

    }

libs/hwui/hwui/Typeface.cpp

+2 −2
Original line number Diff line number Diff line
@@ -130,9 +130,9 @@ void Typeface::setRobotoTypefaceForTest() { 
    sk_sp<SkTypeface> typeface = SkTypeface::MakeFromStream(fontData.release());

    LOG_ALWAYS_FATAL_IF(typeface == nullptr, "Failed to make typeface from %s", kRobotoFont);



    minikin::FontFamily* family = new minikin::FontFamily();

    minikin::MinikinFont* font = new MinikinFontSkia(std::move(typeface), data, st.st_size, 0);

    family->addFont(font);

    minikin::FontFamily* family = new minikin::FontFamily(

                 std::vector<minikin::Font>({ minikin::Font(font, minikin::FontStyle()) }));

    font->Unref();



    std::vector<minikin::FontFamily*> typefaces = { family };

tools/layoutlib/bridge/src/android/graphics/FontFamily_Delegate.java

+16 −8
Original line number Diff line number Diff line
@@ -249,14 +249,17 @@ public class FontFamily_Delegate { 
    // ---- delegate methods ----

    @LayoutlibDelegate

    /*package*/ static boolean addFont(FontFamily thisFontFamily, String path, int ttcIndex) {

        final FontFamily_Delegate delegate = getDelegate(thisFontFamily.mNativePtr);

        if (thisFontFamily.mBuilderPtr == 0) {

            throw new IllegalStateException("Unable to call addFont after freezing.");

        }

        final FontFamily_Delegate delegate = getDelegate(thisFontFamily.mBuilderPtr);

        return delegate != null && delegate.addFont(path, ttcIndex);

    }



    // ---- native methods ----



    @LayoutlibDelegate

    /*package*/ static long nCreateFamily(String lang, int variant) {

    /*package*/ static long nInitBuilder(String lang, int variant) {

        // TODO: support lang. This is required for japanese locale.

        FontFamily_Delegate delegate = new FontFamily_Delegate();

        // variant can be 0, 1 or 2.
@@ -270,6 +273,11 @@ public class FontFamily_Delegate { 
        return sManager.addNewDelegate(delegate);

    }



    @LayoutlibDelegate

    /*package*/ static long nCreateFamily(long builderPtr) {

        return builderPtr;

    }



    @LayoutlibDelegate

    /*package*/ static void nUnrefFamily(long nativePtr) {

        // Removing the java reference for the object doesn't mean that it's freed for garbage
@@ -278,22 +286,22 @@ public class FontFamily_Delegate { 
    }



    @LayoutlibDelegate

    /*package*/ static boolean nAddFont(long nativeFamily, ByteBuffer font, int ttcIndex) {

    /*package*/ static boolean nAddFont(long builderPtr, ByteBuffer font, int ttcIndex) {

        assert false : "The only client of this method has been overriden.";

        return false;

    }



    @LayoutlibDelegate

    /*package*/ static boolean nAddFontWeightStyle(long nativeFamily, ByteBuffer font,

    /*package*/ static boolean nAddFontWeightStyle(long builderPtr, ByteBuffer font,

            int ttcIndex, List<FontConfig.Axis> listOfAxis,

            int weight, boolean isItalic) {

        assert false : "The only client of this method has been overriden.";

        return false;

    }



    static boolean addFont(long nativeFamily, final String path, final int weight,

    static boolean addFont(long builderPtr, final String path, final int weight,

            final boolean isItalic) {

        final FontFamily_Delegate delegate = getDelegate(nativeFamily);

        final FontFamily_Delegate delegate = getDelegate(builderPtr);

        if (delegate != null) {

            if (sFontLocation == null) {

                delegate.mPostInitRunnables.add(() -> delegate.addFont(path, weight, isItalic));
@@ -305,8 +313,8 @@ public class FontFamily_Delegate { 
    }



    @LayoutlibDelegate

    /*package*/ static boolean nAddFontFromAsset(long nativeFamily, AssetManager mgr, String path) {

        FontFamily_Delegate ffd = sManager.getDelegate(nativeFamily);

    /*package*/ static boolean nAddFontFromAsset(long builderPtr, AssetManager mgr, String path) {

        FontFamily_Delegate ffd = sManager.getDelegate(builderPtr);

        if (ffd == null) {

            return false;

        }