From bcd8c7158f6c36699076dc3bf198394954c5326c Mon Sep 17 00:00:00 2001 From: Yegor Date: Tue, 16 Feb 2021 14:38:03 -0800 Subject: [PATCH] fix infinite loop in findMinimumFontsForCodeunits (#24441) --- .../src/engine/canvaskit/font_fallbacks.dart | 111 +++++++------- .../src/engine/canvaskit/interval_tree.dart | 12 +- lib/web_ui/lib/src/engine/canvaskit/text.dart | 2 +- .../canvaskit/fallback_fonts_golden_test.dart | 138 +++++++++++++++++- 4 files changed, 210 insertions(+), 53 deletions(-) diff --git a/lib/web_ui/lib/src/engine/canvaskit/font_fallbacks.dart b/lib/web_ui/lib/src/engine/canvaskit/font_fallbacks.dart index 44654e05b..7117bca24 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/font_fallbacks.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/font_fallbacks.dart @@ -12,7 +12,7 @@ bool _registeredSymbolsAndEmoji = false; final Set codeUnitsWithNoKnownFont = {}; -Future _findFontsForMissingCodeunits(List codeunits) async { +Future findFontsForMissingCodeunits(List codeunits) async { _ensureNotoFontTreeCreated(); // If all of the code units are known to have no Noto Font which covers them, @@ -20,11 +20,11 @@ Future _findFontsForMissingCodeunits(List codeunits) async { if (codeunits.every((u) => codeUnitsWithNoKnownFont.contains(u))) { return; } - Set<_NotoFont> fonts = <_NotoFont>{}; + Set fonts = {}; Set coveredCodeUnits = {}; Set missingCodeUnits = {}; for (int codeunit in codeunits) { - List<_NotoFont> fontsForUnit = _notoTree!.intersections(codeunit); + List fontsForUnit = _notoTree!.intersections(codeunit); fonts.addAll(fontsForUnit); if (fontsForUnit.isNotEmpty) { coveredCodeUnits.add(codeunit); @@ -33,15 +33,15 @@ Future _findFontsForMissingCodeunits(List codeunits) async { } } - fonts = _findMinimumFontsForCodeunits(coveredCodeUnits, fonts); + fonts = findMinimumFontsForCodeunits(coveredCodeUnits, fonts); - for (_NotoFont font in fonts) { + for (NotoFont font in fonts) { await font.ensureResolved(); } Set<_ResolvedNotoSubset> resolvedFonts = <_ResolvedNotoSubset>{}; for (int codeunit in coveredCodeUnits) { - for (_NotoFont font in fonts) { + for (NotoFont font in fonts) { if (font.resolvedFont == null) { // We failed to resolve the font earlier. continue; @@ -232,18 +232,20 @@ Future _registerSymbolsAndEmoji() async { /// which finds the font which covers the most codeunits. If multiple CJK /// fonts match the same number of codeunits, we choose one based on the user's /// locale. -Set<_NotoFont> _findMinimumFontsForCodeunits( - Iterable codeunits, Set<_NotoFont> fonts) { +Set findMinimumFontsForCodeunits( + Iterable codeunits, Set fonts) { + assert(fonts.isNotEmpty || codeunits.isEmpty); List unmatchedCodeunits = List.from(codeunits); - Set<_NotoFont> minimumFonts = <_NotoFont>{}; - List<_NotoFont> bestFonts = <_NotoFont>[]; - int maxCodeunitsCovered = 0; + Set minimumFonts = {}; + List bestFonts = []; String language = html.window.navigator.language; // This is guaranteed to terminate because [codeunits] is a list of fonts // which we've already determined are covered by [fonts]. while (unmatchedCodeunits.isNotEmpty) { + int maxCodeunitsCovered = 0; + bestFonts.clear(); for (var font in fonts) { int codeunitsCovered = 0; for (int codeunit in unmatchedCodeunits) { @@ -259,10 +261,13 @@ Set<_NotoFont> _findMinimumFontsForCodeunits( bestFonts.add(font); } } - assert(bestFonts.isNotEmpty); + assert( + bestFonts.isNotEmpty, + 'Did not find any fonts that cover code units: ${unmatchedCodeunits.join(', ')}', + ); // If the list of best fonts are all CJK fonts, choose the best one based // on locale. Otherwise just choose the first font. - _NotoFont bestFont = bestFonts.first; + NotoFont bestFont = bestFonts.first; if (bestFonts.length > 1) { if (bestFonts.every((font) => _cjkFonts.contains(font))) { if (language == 'zh-Hans' || @@ -301,19 +306,19 @@ void _ensureNotoFontTreeCreated() { return; } - Map<_NotoFont, List> ranges = - <_NotoFont, List>{}; + Map> ranges = + >{}; - for (_NotoFont font in _notoFonts) { + for (NotoFont font in _notoFonts) { for (CodeunitRange range in font.unicodeRanges) { ranges.putIfAbsent(font, () => []).add(range); } } - _notoTree = IntervalTree<_NotoFont>.createFromRanges(ranges); + _notoTree = IntervalTree.createFromRanges(ranges); } -class _NotoFont { +class NotoFont { final String name; final List unicodeRanges; @@ -321,7 +326,7 @@ class _NotoFont { _ResolvedNotoFont? resolvedFont; - _NotoFont(this.name, this.unicodeRanges); + NotoFont(this.name, this.unicodeRanges); bool matchesCodeunit(int codeunit) { for (CodeunitRange range in unicodeRanges) { @@ -397,7 +402,7 @@ class _ResolvedNotoSubset { String toString() => '_ResolvedNotoSubset($family, $url)'; } -_NotoFont _notoSansSC = _NotoFont('Noto Sans SC', [ +NotoFont _notoSansSC = NotoFont('Noto Sans SC', [ CodeunitRange(12288, 12591), CodeunitRange(12800, 13311), CodeunitRange(19968, 40959), @@ -405,37 +410,37 @@ _NotoFont _notoSansSC = _NotoFont('Noto Sans SC', [ CodeunitRange(65280, 65519), ]); -_NotoFont _notoSansTC = _NotoFont('Noto Sans TC', [ +NotoFont _notoSansTC = NotoFont('Noto Sans TC', [ CodeunitRange(12288, 12351), CodeunitRange(12549, 12585), CodeunitRange(19968, 40959), ]); -_NotoFont _notoSansHK = _NotoFont('Noto Sans HK', [ +NotoFont _notoSansHK = NotoFont('Noto Sans HK', [ CodeunitRange(12288, 12351), CodeunitRange(12549, 12585), CodeunitRange(19968, 40959), ]); -_NotoFont _notoSansJP = _NotoFont('Noto Sans JP', [ +NotoFont _notoSansJP = NotoFont('Noto Sans JP', [ CodeunitRange(12288, 12543), CodeunitRange(19968, 40959), CodeunitRange(65280, 65519), ]); -List<_NotoFont> _cjkFonts = <_NotoFont>[ +List _cjkFonts = [ _notoSansSC, _notoSansTC, _notoSansHK, _notoSansJP, ]; -List<_NotoFont> _notoFonts = <_NotoFont>[ +List _notoFonts = [ _notoSansSC, _notoSansTC, _notoSansHK, _notoSansJP, - _NotoFont('Noto Naskh Arabic UI', [ + NotoFont('Noto Naskh Arabic UI', [ CodeunitRange(1536, 1791), CodeunitRange(8204, 8206), CodeunitRange(8208, 8209), @@ -444,36 +449,36 @@ List<_NotoFont> _notoFonts = <_NotoFont>[ CodeunitRange(64336, 65023), CodeunitRange(65132, 65276), ]), - _NotoFont('Noto Sans Armenian', [ + NotoFont('Noto Sans Armenian', [ CodeunitRange(1328, 1424), CodeunitRange(64275, 64279), ]), - _NotoFont('Noto Sans Bengali UI', [ + NotoFont('Noto Sans Bengali UI', [ CodeunitRange(2404, 2405), CodeunitRange(2433, 2555), CodeunitRange(8204, 8205), CodeunitRange(8377, 8377), CodeunitRange(9676, 9676), ]), - _NotoFont('Noto Sans Myanmar UI', [ + NotoFont('Noto Sans Myanmar UI', [ CodeunitRange(4096, 4255), CodeunitRange(8204, 8205), CodeunitRange(9676, 9676), ]), - _NotoFont('Noto Sans Egyptian Hieroglyphs', [ + NotoFont('Noto Sans Egyptian Hieroglyphs', [ CodeunitRange(77824, 78894), ]), - _NotoFont('Noto Sans Ethiopic', [ + NotoFont('Noto Sans Ethiopic', [ CodeunitRange(4608, 5017), CodeunitRange(11648, 11742), CodeunitRange(43777, 43822), ]), - _NotoFont('Noto Sans Georgian', [ + NotoFont('Noto Sans Georgian', [ CodeunitRange(1417, 1417), CodeunitRange(4256, 4351), CodeunitRange(11520, 11567), ]), - _NotoFont('Noto Sans Gujarati UI', [ + NotoFont('Noto Sans Gujarati UI', [ CodeunitRange(2404, 2405), CodeunitRange(2688, 2815), CodeunitRange(8204, 8205), @@ -481,7 +486,7 @@ List<_NotoFont> _notoFonts = <_NotoFont>[ CodeunitRange(9676, 9676), CodeunitRange(43056, 43065), ]), - _NotoFont('Noto Sans Gurmukhi UI', [ + NotoFont('Noto Sans Gurmukhi UI', [ CodeunitRange(2404, 2405), CodeunitRange(2561, 2677), CodeunitRange(8204, 8205), @@ -490,13 +495,13 @@ List<_NotoFont> _notoFonts = <_NotoFont>[ CodeunitRange(9772, 9772), CodeunitRange(43056, 43065), ]), - _NotoFont('Noto Sans Hebrew', [ + NotoFont('Noto Sans Hebrew', [ CodeunitRange(1424, 1535), CodeunitRange(8362, 8362), CodeunitRange(9676, 9676), CodeunitRange(64285, 64335), ]), - _NotoFont('Noto Sans Devanagari UI', [ + NotoFont('Noto Sans Devanagari UI', [ CodeunitRange(2304, 2431), CodeunitRange(7376, 7414), CodeunitRange(7416, 7417), @@ -507,29 +512,29 @@ List<_NotoFont> _notoFonts = <_NotoFont>[ CodeunitRange(43056, 43065), CodeunitRange(43232, 43259), ]), - _NotoFont('Noto Sans Kannada UI', [ + NotoFont('Noto Sans Kannada UI', [ CodeunitRange(2404, 2405), CodeunitRange(3202, 3314), CodeunitRange(8204, 8205), CodeunitRange(8377, 8377), CodeunitRange(9676, 9676), ]), - _NotoFont('Noto Sans Khmer UI', [ + NotoFont('Noto Sans Khmer UI', [ CodeunitRange(6016, 6143), CodeunitRange(8204, 8204), CodeunitRange(9676, 9676), ]), - _NotoFont('Noto Sans KR', [ + NotoFont('Noto Sans KR', [ CodeunitRange(12593, 12686), CodeunitRange(12800, 12828), CodeunitRange(12896, 12923), CodeunitRange(44032, 55215), ]), - _NotoFont('Noto Sans Lao UI', [ + NotoFont('Noto Sans Lao UI', [ CodeunitRange(3713, 3807), CodeunitRange(9676, 9676), ]), - _NotoFont('Noto Sans Malayalam UI', [ + NotoFont('Noto Sans Malayalam UI', [ CodeunitRange(775, 775), CodeunitRange(803, 803), CodeunitRange(2404, 2405), @@ -538,20 +543,20 @@ List<_NotoFont> _notoFonts = <_NotoFont>[ CodeunitRange(8377, 8377), CodeunitRange(9676, 9676), ]), - _NotoFont('Noto Sans Sinhala', [ + NotoFont('Noto Sans Sinhala', [ CodeunitRange(2404, 2405), CodeunitRange(3458, 3572), CodeunitRange(8204, 8205), CodeunitRange(9676, 9676), ]), - _NotoFont('Noto Sans Tamil UI', [ + NotoFont('Noto Sans Tamil UI', [ CodeunitRange(2404, 2405), CodeunitRange(2946, 3066), CodeunitRange(8204, 8205), CodeunitRange(8377, 8377), CodeunitRange(9676, 9676), ]), - _NotoFont('Noto Sans Telugu UI', [ + NotoFont('Noto Sans Telugu UI', [ CodeunitRange(2385, 2386), CodeunitRange(2404, 2405), CodeunitRange(3072, 3199), @@ -559,12 +564,12 @@ List<_NotoFont> _notoFonts = <_NotoFont>[ CodeunitRange(8204, 8205), CodeunitRange(9676, 9676), ]), - _NotoFont('Noto Sans Thai UI', [ + NotoFont('Noto Sans Thai UI', [ CodeunitRange(3585, 3675), CodeunitRange(8204, 8205), CodeunitRange(9676, 9676), ]), - _NotoFont('Noto Sans', [ + NotoFont('Noto Sans', [ CodeunitRange(0, 255), CodeunitRange(305, 305), CodeunitRange(338, 339), @@ -639,7 +644,7 @@ class FallbackFontDownloadQueue { downloads.add(Future(() async { ByteBuffer buffer; try { - buffer = await downloader.downloadAsBytes(subset.url); + buffer = await downloader.downloadAsBytes(subset.url, debugDescription: subset.family); } catch (e) { html.window.console .warn('Failed to load font ${subset.family} at ${subset.url}'); @@ -695,7 +700,7 @@ class NotoDownloader { /// Downloads the [url] and returns it as a [ByteBuffer]. /// /// Override this for testing. - Future downloadAsBytes(String url) { + Future downloadAsBytes(String url, {String? debugDescription}) { if (assertionsEnabled) { _debugActiveDownloadCount += 1; } @@ -714,7 +719,7 @@ class NotoDownloader { /// Downloads the [url] and returns is as a [String]. /// /// Override this for testing. - Future downloadAsString(String url) { + Future downloadAsString(String url, {String? debugDescription}) { if (assertionsEnabled) { _debugActiveDownloadCount += 1; } @@ -731,6 +736,12 @@ class NotoDownloader { } /// The Noto font interval tree. -IntervalTree<_NotoFont>? _notoTree; +IntervalTree? _notoTree; + +/// Returns the tree of Noto fonts for tests. +IntervalTree get debugNotoTree { + _ensureNotoFontTreeCreated(); + return _notoTree!; +} FallbackFontDownloadQueue notoDownloadQueue = FallbackFontDownloadQueue(); diff --git a/lib/web_ui/lib/src/engine/canvaskit/interval_tree.dart b/lib/web_ui/lib/src/engine/canvaskit/interval_tree.dart index 88a097e65..fdbcc45f7 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/interval_tree.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/interval_tree.dart @@ -18,7 +18,7 @@ class IntervalTree { /// have a range which contains the point. factory IntervalTree.createFromRanges(Map> rangesMap) { // Get a list of all the ranges ordered by start index. - List> intervals = >[]; + final List> intervals = >[]; rangesMap.forEach((T key, List rangeList) { for (CodeunitRange range in rangeList) { intervals.add(IntervalTreeNode(key, range.start, range.end)); @@ -93,6 +93,16 @@ class IntervalTreeNode { IntervalTreeNode(this.value, this.low, this.high) : computedHigh = high; + Iterable enumerateAllElements() sync* { + if (left != null) { + yield* left!.enumerateAllElements(); + } + yield value; + if (right != null) { + yield* right!.enumerateAllElements(); + } + } + bool contains(int x) { return low <= x && x <= high; } diff --git a/lib/web_ui/lib/src/engine/canvaskit/text.dart b/lib/web_ui/lib/src/engine/canvaskit/text.dart index 7f7e12df9..0b5efd2a5 100644 --- a/lib/web_ui/lib/src/engine/canvaskit/text.dart +++ b/lib/web_ui/lib/src/engine/canvaskit/text.dart @@ -714,7 +714,7 @@ class CkParagraphBuilder implements ui.ParagraphBuilder { missingCodeUnits.add(codeUnits[i]); } } - _findFontsForMissingCodeunits(missingCodeUnits); + findFontsForMissingCodeunits(missingCodeUnits); } } diff --git a/lib/web_ui/test/canvaskit/fallback_fonts_golden_test.dart b/lib/web_ui/test/canvaskit/fallback_fonts_golden_test.dart index cc2fd0d1c..d5c353343 100644 --- a/lib/web_ui/test/canvaskit/fallback_fonts_golden_test.dart +++ b/lib/web_ui/test/canvaskit/fallback_fonts_golden_test.dart @@ -4,6 +4,7 @@ // @dart = 2.12 import 'dart:async'; +import 'dart:math' as math; import 'dart:typed_data'; import 'package:ui/ui.dart' as ui; @@ -206,6 +207,116 @@ void testMain() { expect(notoDownloadQueue.isPending, isFalse); expect(skiaFontCollection.globalFontFallbacks, ['Roboto']); }); + + // Regression test for https://github.com/flutter/flutter/issues/75836 + // When we had this bug our font fallback resolution logic would end up in an + // infinite loop and this test would freeze and time out. + test('Can find fonts for two adjacent unmatched code units from different fonts', () async { + final LoggingDownloader loggingDownloader = LoggingDownloader(NotoDownloader()); + notoDownloadQueue.downloader = loggingDownloader; + // Try rendering text that requires fallback fonts, initially before the fonts are loaded. + + CkParagraphBuilder(CkParagraphStyle()).addText('ヽಠ'); + await notoDownloadQueue.downloader.debugWhenIdle(); + expect( + loggingDownloader.log, + [ + 'https://fonts.googleapis.com/css2?family=Noto+Sans+SC', + 'https://fonts.googleapis.com/css2?family=Noto+Sans+Kannada+UI', + 'Noto Sans SC', + 'Noto Sans Kannada UI', + ], + ); + + // Do the same thing but this time with loaded fonts. + loggingDownloader.log.clear(); + CkParagraphBuilder(CkParagraphStyle()).addText('ヽಠ'); + await notoDownloadQueue.downloader.debugWhenIdle(); + expect(loggingDownloader.log, isEmpty); + }); + + test('findMinimumFontsForCodeunits for all supported code units', () async { + final LoggingDownloader loggingDownloader = LoggingDownloader(NotoDownloader()); + notoDownloadQueue.downloader = loggingDownloader; + + // Collect all supported code units from all fallback fonts in the Noto + // font tree. + final Set testedFonts = {}; + final Set supportedUniqueCodeUnits = {}; + for (NotoFont font in debugNotoTree.root.enumerateAllElements()) { + testedFonts.add(font.name); + for (CodeunitRange range in font.unicodeRanges) { + for (int codeUnit = range.start; codeUnit < range.end; codeUnit += 1) { + supportedUniqueCodeUnits.add(codeUnit); + } + } + } + + expect(supportedUniqueCodeUnits.length, greaterThan(10000)); // sanity check + expect(testedFonts, unorderedEquals({ + 'Noto Sans', + 'Noto Sans Malayalam UI', + 'Noto Sans Armenian', + 'Noto Sans Georgian', + 'Noto Sans Hebrew', + 'Noto Naskh Arabic UI', + 'Noto Sans Devanagari UI', + 'Noto Sans Telugu UI', + 'Noto Sans Tamil UI', + 'Noto Sans Kannada UI', + 'Noto Sans Sinhala', + 'Noto Sans Gurmukhi UI', + 'Noto Sans Gujarati UI', + 'Noto Sans Bengali UI', + 'Noto Sans Thai UI', + 'Noto Sans Lao UI', + 'Noto Sans Myanmar UI', + 'Noto Sans Ethiopic', + 'Noto Sans Khmer UI', + 'Noto Sans SC', + 'Noto Sans JP', + 'Noto Sans TC', + 'Noto Sans HK', + 'Noto Sans KR', + 'Noto Sans Egyptian Hieroglyphs', + })); + + // Construct random paragraphs out of supported code units. + final math.Random random = math.Random(0); + final List supportedCodeUnits = supportedUniqueCodeUnits.toList()..shuffle(random); + const int paragraphLength = 3; + + for (int batchStart = 0; batchStart < supportedCodeUnits.length; batchStart += paragraphLength) { + final int batchEnd = math.min(batchStart + paragraphLength, supportedCodeUnits.length); + final List codeUnits = []; + for (int i = batchStart; i < batchEnd; i += 1) { + codeUnits.add(supportedCodeUnits[i]); + } + final Set fonts = {}; + for (int codeunit in codeUnits) { + List fontsForUnit = debugNotoTree.intersections(codeunit); + + // All code units are extracted from the same tree, so there must + // be at least one font supporting each code unit + expect(fontsForUnit, isNotEmpty); + + // Make sure that every returned font indeed covers the code unit. + expect(fontsForUnit.every((font) => font.matchesCodeunit(codeunit)), isTrue); + fonts.addAll(fontsForUnit); + } + + try { + findMinimumFontsForCodeunits(codeUnits, fonts); + } catch (e) { + print( + 'findMinimumFontsForCodeunits failed:\n' + ' Code units: ${codeUnits.join(', ')}\n' + ' Fonts: ${fonts.map((f) => f.name).join(', ')}', + ); + rethrow; + } + } + }); // TODO: https://github.com/flutter/flutter/issues/60040 }, skip: isIosSafari); } @@ -213,7 +324,7 @@ void testMain() { class TestDownloader extends NotoDownloader { static final Map mockDownloads = {}; @override - Future downloadAsString(String url) async { + Future downloadAsString(String url, {String? debugDescription}) async { if (mockDownloads.containsKey(url)) { return mockDownloads[url]!; } else { @@ -221,3 +332,28 @@ class TestDownloader extends NotoDownloader { } } } + +class LoggingDownloader implements NotoDownloader { + final List log = []; + + LoggingDownloader(this.delegate); + + final NotoDownloader delegate; + + @override + Future debugWhenIdle() { + return delegate.debugWhenIdle(); + } + + @override + Future downloadAsBytes(String url, {String? debugDescription}) { + log.add(debugDescription ?? url); + return delegate.downloadAsBytes(url); + } + + @override + Future downloadAsString(String url, {String? debugDescription}) { + log.add(debugDescription ?? url); + return delegate.downloadAsString(url); + } +} -- GitLab