Close FDs / mmap handles promptly.

Currently we rely on GC to close them.
This may cause system_server crash depending on the timing of GC.

Bug: 187879195
Test: atest UpdatableSystemFontTest
Change-Id: I09ac3f349e5ec100e4164320cbf27977474cc4bb

4 files changed, 169 insertions, 53 deletions

diff --git a/graphics/java/android/graphics/fonts/SystemFonts.java b/graphics/java/android/graphics/fonts/SystemFonts.java
index 255f9e659c36..1d392d2c27b5 100644
--- a/graphics/java/android/graphics/fonts/SystemFonts.java
+++ b/graphics/java/android/graphics/fonts/SystemFonts.java
@@ -262,8 +262,14 @@ public final class SystemFonts {
      */
     @VisibleForTesting
     public static Map<String, FontFamily[]> buildSystemFallback(FontConfig fontConfig) {
+        return buildSystemFallback(fontConfig, new ArrayMap<>());
+    }
+
+    /** @hide */
+    @VisibleForTesting
+    public static Map<String, FontFamily[]> buildSystemFallback(FontConfig fontConfig,
+            ArrayMap<String, ByteBuffer> outBufferCache) {
         final Map<String, FontFamily[]> fallbackMap = new ArrayMap<>();
-        final ArrayMap<String, ByteBuffer> bufferCache = new ArrayMap<>();
         final List<FontConfig.FontFamily> xmlFamilies = fontConfig.getFontFamilies();
 
         final ArrayMap<String, ArrayList<FontFamily>> fallbackListMap = new ArrayMap<>();
@@ -273,7 +279,7 @@ public final class SystemFonts {
             if (familyName == null) {
                 continue;
             }
-            appendNamedFamily(xmlFamily, bufferCache, fallbackListMap);
+            appendNamedFamily(xmlFamily, outBufferCache, fallbackListMap);
         }
 
         // Then, add fallback fonts to the each fallback map.
@@ -282,7 +288,7 @@ public final class SystemFonts {
             // The first family (usually the sans-serif family) is always placed immediately
             // after the primary family in the fallback.
             if (i == 0 || xmlFamily.getName() == null) {
-                pushFamilyToFallback(xmlFamily, fallbackListMap, bufferCache);
+                pushFamilyToFallback(xmlFamily, fallbackListMap, outBufferCache);
             }
         }
 
diff --git a/services/core/java/com/android/server/graphics/fonts/FontManagerService.java b/services/core/java/com/android/server/graphics/fonts/FontManagerService.java
index 26545306903f..dc3dc468f763 100644
--- a/services/core/java/com/android/server/graphics/fonts/FontManagerService.java
+++ b/services/core/java/com/android/server/graphics/fonts/FontManagerService.java
@@ -25,12 +25,14 @@ import android.graphics.fonts.FontFamily;
 import android.graphics.fonts.FontManager;
 import android.graphics.fonts.FontUpdateRequest;
 import android.graphics.fonts.SystemFonts;
+import android.os.ParcelFileDescriptor;
 import android.os.ResultReceiver;
 import android.os.SharedMemory;
 import android.os.ShellCallback;
 import android.system.ErrnoException;
 import android.text.FontConfig;
 import android.util.AndroidException;
+import android.util.ArrayMap;
 import android.util.IndentingPrintWriter;
 import android.util.Slog;
 
@@ -46,6 +48,9 @@ import java.io.File;
 import java.io.FileDescriptor;
 import java.io.IOException;
 import java.io.PrintWriter;
+import java.nio.ByteBuffer;
+import java.nio.DirectByteBuffer;
+import java.nio.NioUtils;
 import java.util.Collections;
 import java.util.List;
 import java.util.Map;
@@ -76,6 +81,17 @@ public final class FontManagerService extends IFontManager.Stub {
         } catch (SystemFontException e) {
             Slog.e(TAG, "Failed to update font family", e);
             return e.getErrorCode();
+        } finally {
+            for (FontUpdateRequest request : requests) {
+                ParcelFileDescriptor fd = request.getFd();
+                if (fd != null) {
+                    try {
+                        fd.close();
+                    } catch (IOException e) {
+                        Slog.w(TAG, "Failed to close fd", e);
+                    }
+                }
+            }
         }
     }
 
@@ -176,13 +192,7 @@ public final class FontManagerService extends IFontManager.Stub {
     private void initialize() {
         synchronized (mUpdatableFontDirLock) {
             if (mUpdatableFontDir == null) {
-                synchronized (mSerializedFontMapLock) {
-                    try {
-                        mSerializedFontMap = Typeface.serializeFontMap(Typeface.getSystemFontMap());
-                    } catch (IOException | ErrnoException e) {
-                        mSerializedFontMap = null;
-                    }
-                }
+                setSerializedFontMap(serializeSystemServerFontMap());
                 return;
             }
             mUpdatableFontDir.loadFontFileMap();
@@ -278,36 +288,57 @@ public final class FontManagerService extends IFontManager.Stub {
     /**
      * Makes new serialized font map data and updates mSerializedFontMap.
      */
-    public void updateSerializedFontMap() {
+    private void updateSerializedFontMap() {
+        SharedMemory serializedFontMap = serializeFontMap(getSystemFontConfig());
+        if (serializedFontMap == null) {
+            // Fallback to the preloaded config.
+            serializedFontMap = serializeSystemServerFontMap();
+        }
+        setSerializedFontMap(serializedFontMap);
+    }
+
+    @Nullable
+    private static SharedMemory serializeFontMap(FontConfig fontConfig) {
+        final ArrayMap<String, ByteBuffer> bufferCache = new ArrayMap<>();
         try {
-            final FontConfig fontConfig = getSystemFontConfig();
-            final Map<String, FontFamily[]> fallback = SystemFonts.buildSystemFallback(fontConfig);
+            final Map<String, FontFamily[]> fallback =
+                    SystemFonts.buildSystemFallback(fontConfig, bufferCache);
             final Map<String, Typeface> typefaceMap =
                     SystemFonts.buildSystemTypefaces(fontConfig, fallback);
-
-            SharedMemory serializeFontMap = Typeface.serializeFontMap(typefaceMap);
-            synchronized (mSerializedFontMapLock) {
-                mSerializedFontMap = serializeFontMap;
-            }
-            return;
+            return Typeface.serializeFontMap(typefaceMap);
         } catch (IOException | ErrnoException e) {
             Slog.w(TAG, "Failed to serialize updatable font map. "
                     + "Retrying with system image fonts.", e);
+            return null;
+        } finally {
+            // Unmap buffers promptly, as we map a lot of files and may hit mmap limit before
+            // GC collects ByteBuffers and unmaps them.
+            for (ByteBuffer buffer : bufferCache.values()) {
+                if (buffer instanceof DirectByteBuffer) {
+                    NioUtils.freeDirectBuffer(buffer);
+                }
+            }
         }
+    }
 
+    @Nullable
+    private static SharedMemory serializeSystemServerFontMap() {
         try {
-            final FontConfig fontConfig = SystemFonts.getSystemPreinstalledFontConfig();
-            final Map<String, FontFamily[]> fallback = SystemFonts.buildSystemFallback(fontConfig);
-            final Map<String, Typeface> typefaceMap =
-                    SystemFonts.buildSystemTypefaces(fontConfig, fallback);
-
-            SharedMemory serializeFontMap = Typeface.serializeFontMap(typefaceMap);
-            synchronized (mSerializedFontMapLock) {
-                mSerializedFontMap = serializeFontMap;
-            }
+            return Typeface.serializeFontMap(Typeface.getSystemFontMap());
         } catch (IOException | ErrnoException e) {
             Slog.e(TAG, "Failed to serialize SystemServer system font map", e);
+            return null;
         }
     }
 
+    private void setSerializedFontMap(SharedMemory serializedFontMap) {
+        SharedMemory oldFontMap = null;
+        synchronized (mSerializedFontMapLock) {
+            oldFontMap = mSerializedFontMap;
+            mSerializedFontMap = serializedFontMap;
+        }
+        if (oldFontMap != null) {
+            oldFontMap.close();
+        }
+    }
 }
diff --git a/services/core/java/com/android/server/graphics/fonts/OtfFontFileParser.java b/services/core/java/com/android/server/graphics/fonts/OtfFontFileParser.java
index bc6ceec7e269..1ed3972ed309 100644
--- a/services/core/java/com/android/server/graphics/fonts/OtfFontFileParser.java
+++ b/services/core/java/com/android/server/graphics/fonts/OtfFontFileParser.java
@@ -31,6 +31,7 @@ import java.io.File;
 import java.io.FileInputStream;
 import java.io.IOException;
 import java.nio.ByteBuffer;
+import java.nio.DirectByteBuffer;
 import java.nio.NioUtils;
 import java.nio.channels.FileChannel;
 
@@ -41,7 +42,7 @@ import java.nio.channels.FileChannel;
         try {
             return FontFileUtil.getPostScriptName(buffer, 0);
         } finally {
-            NioUtils.freeDirectBuffer(buffer);
+            unmap(buffer);
         }
     }
 
@@ -65,7 +66,7 @@ import java.nio.channels.FileChannel;
             }
             return psName + extension;
         } finally {
-            NioUtils.freeDirectBuffer(buffer);
+            unmap(buffer);
         }
 
     }
@@ -76,37 +77,42 @@ import java.nio.channels.FileChannel;
         try {
             return FontFileUtil.getRevision(buffer, 0);
         } finally {
-            NioUtils.freeDirectBuffer(buffer);
+            unmap(buffer);
         }
     }
 
     @Override
     public void tryToCreateTypeface(File file) throws Throwable {
-        Font font = new Font.Builder(file).build();
-        FontFamily family = new FontFamily.Builder(font).build();
-        Typeface typeface = new Typeface.CustomFallbackBuilder(family).build();
+        ByteBuffer buffer = mmap(file);
+        try {
+            Font font = new Font.Builder(buffer).build();
+            FontFamily family = new FontFamily.Builder(font).build();
+            Typeface typeface = new Typeface.CustomFallbackBuilder(family).build();
 
-        TextPaint p = new TextPaint();
-        p.setTextSize(24f);
-        p.setTypeface(typeface);
+            TextPaint p = new TextPaint();
+            p.setTextSize(24f);
+            p.setTypeface(typeface);
 
-        // Test string to try with the passed font.
-        // TODO: Good to extract from font file.
-        String testTextToDraw = "abcXYZ@- "
-                + "\uD83E\uDED6" // Emoji E13.0
-                + "\uD83C\uDDFA\uD83C\uDDF8" // Emoji Flags
-                + "\uD83D\uDC8F\uD83C\uDFFB" // Emoji Skin tone Sequence
-                // ZWJ Sequence
-                + "\uD83D\uDC68\uD83C\uDFFC\u200D\u2764\uFE0F\u200D\uD83D\uDC8B\u200D"
-                + "\uD83D\uDC68\uD83C\uDFFF";
+            // Test string to try with the passed font.
+            // TODO: Good to extract from font file.
+            String testTextToDraw = "abcXYZ@- "
+                    + "\uD83E\uDED6" // Emoji E13.0
+                    + "\uD83C\uDDFA\uD83C\uDDF8" // Emoji Flags
+                    + "\uD83D\uDC8F\uD83C\uDFFB" // Emoji Skin tone Sequence
+                    // ZWJ Sequence
+                    + "\uD83D\uDC68\uD83C\uDFFC\u200D\u2764\uFE0F\u200D\uD83D\uDC8B\u200D"
+                    + "\uD83D\uDC68\uD83C\uDFFF";
 
-        int width = (int) Math.ceil(Layout.getDesiredWidth(testTextToDraw, p));
-        StaticLayout layout = StaticLayout.Builder.obtain(
-                testTextToDraw, 0, testTextToDraw.length(), p, width).build();
-        Bitmap bmp = Bitmap.createBitmap(
-                layout.getWidth(), layout.getHeight(), Bitmap.Config.ALPHA_8);
-        Canvas canvas = new Canvas(bmp);
-        layout.draw(canvas);
+            int width = (int) Math.ceil(Layout.getDesiredWidth(testTextToDraw, p));
+            StaticLayout layout = StaticLayout.Builder.obtain(
+                    testTextToDraw, 0, testTextToDraw.length(), p, width).build();
+            Bitmap bmp = Bitmap.createBitmap(
+                    layout.getWidth(), layout.getHeight(), Bitmap.Config.ALPHA_8);
+            Canvas canvas = new Canvas(bmp);
+            layout.draw(canvas);
+        } finally {
+            unmap(buffer);
+        }
     }
 
     private static ByteBuffer mmap(File file) throws IOException {
@@ -115,4 +121,10 @@ import java.nio.channels.FileChannel;
             return fileChannel.map(FileChannel.MapMode.READ_ONLY, 0, fileChannel.size());
         }
     }
+
+    private static void unmap(ByteBuffer buffer) {
+        if (buffer instanceof DirectByteBuffer) {
+            NioUtils.freeDirectBuffer(buffer);
+        }
+    }
 }
diff --git a/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java b/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java
index 898b8d4cbdc1..44f96c5a987b 100644
--- a/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java
+++ b/tests/UpdatableSystemFontTest/src/com/android/updatablesystemfont/UpdatableSystemFontTest.java
@@ -56,6 +56,11 @@ import java.io.InputStream;
 import java.io.OutputStream;
 import java.nio.file.Files;
 import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.regex.Pattern;
 
 /**
  * Tests if fonts can be updated by {@link FontManager} API.
@@ -95,6 +100,12 @@ public class UpdatableSystemFontTest {
             EMOJI_RENDERING_TEST_APP_ID + "/.EmojiRenderingTestActivity";
     private static final long ACTIVITY_TIMEOUT_MILLIS = SECONDS.toMillis(10);
 
+    private static final Pattern PATTERN_FONT_FILES = Pattern.compile("\\.(ttf|otf|ttc|otc)$");
+    private static final Pattern PATTERN_TMP_FILES = Pattern.compile("^/data/local/tmp/");
+    private static final Pattern PATTERN_DATA_FONT_FILES = Pattern.compile("^/data/fonts/files/");
+    private static final Pattern PATTERN_SYSTEM_FONT_FILES =
+            Pattern.compile("^/(system|product)/fonts/");
+
     private String mKeyId;
     private FontManager mFontManager;
 
@@ -236,6 +247,32 @@ public class UpdatableSystemFontTest {
         assertThat(fontPathAfterReboot).isEqualTo(fontPath);
     }
 
+
+    @Test
+    public void fdLeakTest() throws Exception {
+        long originalOpenFontCount =
+                countMatch(getOpenFiles("system_server"), PATTERN_FONT_FILES);
+        Pattern patternEmojiVPlus1 =
+                Pattern.compile(Pattern.quote(TEST_NOTO_COLOR_EMOJI_VPLUS1_TTF));
+        for (int i = 0; i < 10; i++) {
+            assertThat(updateFontFile(
+                    TEST_NOTO_COLOR_EMOJI_VPLUS1_TTF, TEST_NOTO_COLOR_EMOJI_VPLUS1_TTF_FSV_SIG))
+                    .isEqualTo(FontManager.RESULT_SUCCESS);
+            List<String> openFiles = getOpenFiles("system_server");
+            for (Pattern p : Arrays.asList(PATTERN_FONT_FILES, PATTERN_SYSTEM_FONT_FILES,
+                    PATTERN_DATA_FONT_FILES, PATTERN_TMP_FILES)) {
+                Log.i(TAG, String.format("num of %s: %d", p, countMatch(openFiles, p)));
+            }
+            // system_server should not keep /data/fonts files open.
+            assertThat(countMatch(openFiles, PATTERN_DATA_FONT_FILES)).isEqualTo(0);
+            // system_server should not keep passed FD open.
+            assertThat(countMatch(openFiles, patternEmojiVPlus1)).isEqualTo(0);
+            // The number of open font FD should not increase.
+            assertThat(countMatch(openFiles, PATTERN_FONT_FILES))
+                    .isAtMost(originalOpenFontCount);
+        }
+    }
+
     private static String insertCert(String certPath) throws Exception {
         Pair<String, String> result;
         try (InputStream is = new FileInputStream(certPath)) {
@@ -338,7 +375,37 @@ public class UpdatableSystemFontTest {
         return !expectCommandToSucceed(cmd).trim().isEmpty();
     }
 
+    private static List<String> getOpenFiles(String appId) throws Exception {
+        String pid = pidOf(appId);
+        if (pid.isEmpty()) {
+            return Collections.emptyList();
+        }
+        String cmd = String.format("lsof -p %s", pid);
+        String out = expectCommandToSucceed(cmd);
+        List<String> paths = new ArrayList<>();
+        boolean first = true;
+        for (String line : out.split("\n")) {
+            // Skip the header.
+            if (first) {
+                first = false;
+                continue;
+            }
+            String[] records = line.split(" ");
+            if (records.length > 0) {
+                paths.add(records[records.length - 1]);
+            }
+        }
+        return paths;
+    }
+
     private static String pidOf(String appId) throws Exception {
         return expectCommandToSucceed("pidof " + appId).trim();
     }
+
+    private static long countMatch(List<String> paths, Pattern pattern) {
+        // Note: asPredicate() returns true for partial matching.
+        return paths.stream()
+                .filter(pattern.asPredicate())
+                .count();
+    }
 }