From 80ee3c1efb375e24858ff192365764c9dda7fa9a Mon Sep 17 00:00:00 2001 From: LongCatIsLooong <31859944+LongCatIsLooong@users.noreply.github.com> Date: Fri, 29 Jul 2022 22:45:05 -0700 Subject: [PATCH] TextPainter throw with stack trace to help track down read-before-layout (#108571) --- .../lib/src/painting/text_painter.dart | 51 +++++++++++++------ .../flutter/lib/src/rendering/paragraph.dart | 32 +++++++++++- .../test/painting/system_fonts_test.dart | 40 ++++++++++++++- .../test/painting/text_painter_test.dart | 35 +++++++++++++ 4 files changed, 139 insertions(+), 19 deletions(-) diff --git a/packages/flutter/lib/src/painting/text_painter.dart b/packages/flutter/lib/src/painting/text_painter.dart index ebead4e548..18115a8e48 100644 --- a/packages/flutter/lib/src/painting/text_painter.dart +++ b/packages/flutter/lib/src/painting/text_painter.dart @@ -206,7 +206,18 @@ class TextPainter { // rebuilt before painting. bool _rebuildParagraphForPaint = true; - bool get _debugNeedsLayout => _paragraph == null; + bool get _debugAssertTextLayoutIsValid { + if (_paragraph == null) { + throw FlutterError.fromParts([ + ErrorSummary('Text layout not available'), + if (_debugMarkNeedsLayoutCallStack != null) DiagnosticsStackTrace('The calls that first invalidated the text layout were', _debugMarkNeedsLayoutCallStack) + else ErrorDescription('The TextPainter has never been laid out.') + ]); + } + return true; + } + + StackTrace? _debugMarkNeedsLayoutCallStack; /// Marks this text painter's layout information as dirty and removes cached /// information. @@ -215,6 +226,12 @@ class TextPainter { /// layout changes in engine. In most cases, updating text painter properties /// in framework will automatically invoke this method. void markNeedsLayout() { + assert(() { + if (_paragraph != null) { + _debugMarkNeedsLayoutCallStack ??= StackTrace.current; + } + return true; + }()); _paragraph = null; _lineMetricsCache = null; _previousCaretPosition = null; @@ -540,7 +557,7 @@ class TextPainter { /// /// Valid only after [layout] has been called. double get minIntrinsicWidth { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); return _applyFloatingPointHack(_paragraph!.minIntrinsicWidth); } @@ -548,7 +565,7 @@ class TextPainter { /// /// Valid only after [layout] has been called. double get maxIntrinsicWidth { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); return _applyFloatingPointHack(_paragraph!.maxIntrinsicWidth); } @@ -556,7 +573,7 @@ class TextPainter { /// /// Valid only after [layout] has been called. double get width { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); return _applyFloatingPointHack( textWidthBasis == TextWidthBasis.longestLine ? _paragraph!.longestLine : _paragraph!.width, ); @@ -566,7 +583,7 @@ class TextPainter { /// /// Valid only after [layout] has been called. double get height { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); return _applyFloatingPointHack(_paragraph!.height); } @@ -574,7 +591,7 @@ class TextPainter { /// /// Valid only after [layout] has been called. Size get size { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); return Size(width, height); } @@ -583,7 +600,7 @@ class TextPainter { /// /// Valid only after [layout] has been called. double computeDistanceToActualBaseline(TextBaseline baseline) { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); assert(baseline != null); switch (baseline) { case TextBaseline.alphabetic: @@ -605,7 +622,7 @@ class TextPainter { /// /// Valid only after [layout] has been called. bool get didExceedMaxLines { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); return _paragraph!.didExceedMaxLines; } @@ -623,6 +640,10 @@ class TextPainter { final ui.ParagraphBuilder builder = ui.ParagraphBuilder(_createParagraphStyle()); text.build(builder, textScaleFactor: textScaleFactor, dimensions: _placeholderDimensions); _inlinePlaceholderScales = builder.placeholderScales; + assert(() { + _debugMarkNeedsLayoutCallStack = null; + return true; + }()); _paragraph = builder.build(); _rebuildParagraphForPaint = false; } @@ -859,7 +880,7 @@ class TextPainter { } Offset get _emptyOffset { - assert(!_debugNeedsLayout); // implies textDirection is non-null + assert(_debugAssertTextLayoutIsValid); // implies textDirection is non-null assert(textAlign != null); switch (textAlign) { case TextAlign.left: @@ -920,7 +941,7 @@ class TextPainter { // Checks if the [position] and [caretPrototype] have changed from the cached // version and recomputes the metrics required to position the caret. void _computeCaretMetrics(TextPosition position, Rect caretPrototype) { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); if (position == _previousCaretPosition && caretPrototype == _previousCaretPrototype) { return; } @@ -969,7 +990,7 @@ class TextPainter { ui.BoxHeightStyle boxHeightStyle = ui.BoxHeightStyle.tight, ui.BoxWidthStyle boxWidthStyle = ui.BoxWidthStyle.tight, }) { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); assert(boxHeightStyle != null); assert(boxWidthStyle != null); return _paragraph!.getBoxesForRange( @@ -982,7 +1003,7 @@ class TextPainter { /// Returns the position within the text for the given pixel offset. TextPosition getPositionForOffset(Offset offset) { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); return _paragraph!.getPositionForOffset(offset); } @@ -996,7 +1017,7 @@ class TextPainter { /// . /// {@endtemplate} TextRange getWordBoundary(TextPosition position) { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); return _paragraph!.getWordBoundary(position); } @@ -1004,7 +1025,7 @@ class TextPainter { /// /// The newline (if any) is not returned as part of the range. TextRange getLineBoundary(TextPosition position) { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); return _paragraph!.getLineBoundary(position); } @@ -1021,7 +1042,7 @@ class TextPainter { /// /// Valid only after [layout] has been called. List computeLineMetrics() { - assert(!_debugNeedsLayout); + assert(_debugAssertTextLayoutIsValid); return _lineMetricsCache ??= _paragraph!.computeLineMetrics(); } } diff --git a/packages/flutter/lib/src/rendering/paragraph.dart b/packages/flutter/lib/src/rendering/paragraph.dart index 74d4fe14cb..e98e54be58 100644 --- a/packages/flutter/lib/src/rendering/paragraph.dart +++ b/packages/flutter/lib/src/rendering/paragraph.dart @@ -8,6 +8,7 @@ import 'dart:ui' as ui show BoxHeightStyle, BoxWidthStyle, Gradient, Placeholder import 'package:flutter/foundation.dart'; import 'package:flutter/gestures.dart'; +import 'package:flutter/scheduler.dart'; import 'package:flutter/semantics.dart'; import 'box.dart'; @@ -640,10 +641,37 @@ class RenderParagraph extends RenderBox ); } + bool _systemFontsChangeScheduled = false; @override void systemFontsDidChange() { - super.systemFontsDidChange(); - _textPainter.markNeedsLayout(); + final SchedulerPhase phase = SchedulerBinding.instance.schedulerPhase; + switch (phase) { + case SchedulerPhase.idle: + case SchedulerPhase.postFrameCallbacks: + if (_systemFontsChangeScheduled) { + return; + } + _systemFontsChangeScheduled = true; + SchedulerBinding.instance.scheduleFrameCallback((Duration timeStamp) { + assert(_systemFontsChangeScheduled); + _systemFontsChangeScheduled = false; + assert( + attached || (debugDisposed ?? true), + '$this is detached during $phase but not disposed.', + ); + if (attached) { + super.systemFontsDidChange(); + _textPainter.markNeedsLayout(); + } + }); + break; + case SchedulerPhase.transientCallbacks: + case SchedulerPhase.midFrameMicrotasks: + case SchedulerPhase.persistentCallbacks: + super.systemFontsDidChange(); + _textPainter.markNeedsLayout(); + break; + } } // Placeholder dimensions representing the sizes of child inline widgets. diff --git a/packages/flutter/test/painting/system_fonts_test.dart b/packages/flutter/test/painting/system_fonts_test.dart index 34034eb296..567f658dbb 100644 --- a/packages/flutter/test/painting/system_fonts_test.dart +++ b/packages/flutter/test/painting/system_fonts_test.dart @@ -2,14 +2,43 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. +import 'dart:async'; + import 'package:flutter/cupertino.dart'; import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; +import 'package:flutter/rendering.dart'; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; void main() { testWidgets('RenderParagraph relayout upon system fonts changes', (WidgetTester tester) async { + await tester.pumpWidget( + const MaterialApp( + home: Text('text widget'), + ), + ); + final RenderObject renderObject = tester.renderObject(find.text('text widget')); + + const Map data = { + 'type': 'fontsChange', + }; + await ServicesBinding.instance.defaultBinaryMessenger.handlePlatformMessage( + 'flutter/system', + SystemChannels.system.codec.encodeMessage(data), + (ByteData? data) { }, + ); + + final Completer animation = Completer(); + tester.binding.scheduleFrameCallback((Duration timeStamp) { + animation.complete(renderObject.debugNeedsLayout); + }); + expect(renderObject.debugNeedsLayout, isFalse); + await tester.pump(); + expect(await animation.future, isTrue); + }); + + testWidgets('Safe to query RenderParagraph for text layout after system fonts changes', (WidgetTester tester) async { await tester.pumpWidget( const MaterialApp( home: Text('text widget'), @@ -23,8 +52,15 @@ void main() { SystemChannels.system.codec.encodeMessage(data), (ByteData? data) { }, ); - final RenderObject renderObject = tester.renderObject(find.text('text widget')); - expect(renderObject.debugNeedsLayout, isTrue); + final RenderParagraph paragraph = tester.renderObject(find.text('text widget')); + Object? exception; + try { + paragraph.getPositionForOffset(Offset.zero); + paragraph.hitTest(BoxHitTestResult(), position: Offset.zero); + } catch (e) { + exception = e; + } + expect(exception, isNull); }); testWidgets('RenderEditable relayout upon system fonts changes', (WidgetTester tester) async { diff --git a/packages/flutter/test/painting/text_painter_test.dart b/packages/flutter/test/painting/text_painter_test.dart index c27e443946..044b610b11 100644 --- a/packages/flutter/test/painting/text_painter_test.dart +++ b/packages/flutter/test/painting/text_painter_test.dart @@ -1034,6 +1034,41 @@ void main() { lines = painter.computeLineMetrics(); expect(lines.length, 1); }, skip: kIsWeb && !isCanvasKit); // https://github.com/flutter/flutter/issues/62819 + + test('TextPainter throws with stack trace when accessing text layout', () { + final TextPainter painter = TextPainter() + ..text = const TextSpan(text: 'TEXT') + ..textDirection = TextDirection.ltr; + + FlutterError? exception; + try { + painter.getPositionForOffset(Offset.zero); + } on FlutterError catch (e) { + exception = e; + } + expect(exception?.message, contains('The TextPainter has never been laid out.')); + exception = null; + + try { + painter.layout(); + painter.getPositionForOffset(Offset.zero); + } on FlutterError catch (e) { + exception = e; + } + + expect(exception, isNull); + exception = null; + + try { + painter.markNeedsLayout(); + painter.getPositionForOffset(Offset.zero); + } on FlutterError catch (e) { + exception = e; + } + + expect(exception?.message, contains('The calls that first invalidated the text layout were:')); + exception = null; + }); } class MockCanvas extends Fake implements Canvas {