Skip to content

Conversation

@LongCatIsLooong
Copy link
Contributor

Fixes #154060

The error message doesn't make sense to me since one can call setState during the idle phase, and I'm not sure what this is guarding against without the right error message.

In the case of #154060 the layout builder was never laid out:

├─child 1: _RenderLayoutBuilder#7c319 NEEDS-LAYOUT NEEDS-PAINT
│   creator: LayoutBuilder ← _BodyBuilder ← MediaQuery ←
│     LayoutId-[<_ScaffoldSlot.body>] ← CustomMultiChildLayout ←
│     _ActionsScope ← Actions ← AnimatedBuilder ← DefaultTextStyle ←
│     AnimatedDefaultTextStyle ← _InkFeatures-[GlobalKey#1f6eb ink
│     renderer] ← NotificationListener<LayoutChangedNotification> ← ⋯
│   parentData: offset=Offset(0.0, 0.0); id=_ScaffoldSlot.body
│   constraints: MISSING
│   size: MISSING

So #154681 doesn't really fix #154060 since the layout callback cannot be run without a set of valid constraints.

Before the BuildScope change all _inDirtyList flags were unset after the BuildOwner finishes rebuilding the widget tree, so LayoutBuilder._inDirtyLst is always set to false after a frame even for layout builders that were never laid out.
With the BuildScope change, LayoutBuilder has its own BuildScope which is only flushed after LayoutBuilder gets its constraints.

Pre-launch Checklist

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@github-actions github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Sep 5, 2024
@LongCatIsLooong LongCatIsLooong marked this pull request as ready for review September 6, 2024 00:11
@goderbauer
Copy link
Member

After some digging around, I think this assert is supposed to guard against calling scheduleBuildFor multiple times for the same element. Element.markNeedsBuild (which is called by setState) guards its call to scheduleBuildFor on whether the element is dirty. If it isn't dirty, it returns right there and we skip all the build scheduling steps:

if (dirty) {
return;
}
_dirty = true;
owner!.scheduleBuildFor(this);

Can you help me understand why it should conceptually be acceptable to schedule multiple builds for the same element?

(I do agree that the ErrorHint is bogus.)

@LongCatIsLooong
Copy link
Contributor Author

LongCatIsLooong commented Sep 6, 2024

After some digging around, I think this assert is supposed to guard against calling scheduleBuildFor multiple times for the same element. Element.markNeedsBuild (which is called by setState) guards its call to scheduleBuildFor on whether the element is dirty. If it isn't dirty, it returns right there and we skip all the build scheduling steps:

if (dirty) {
return;
}
_dirty = true;
owner!.scheduleBuildFor(this);

Can you help me understand why it should conceptually be acceptable to schedule multiple builds for the same element?

(I do agree that the ErrorHint is bogus.)

Oh that makes sense thanks for looking into it.

I think it's the LayoutBuilderElement that's triggering the error but it doesn't actually schedule multiple builds. If the render object couldn't do layout then the element stays dirty and _inDirtyList is always true. I'll move the check to the BuildScope class then.

Unfortunately that didn't work. I think LayoutBuilderElement can be inflated via treewalk, but it has its own BuildScope is the underlying problem. I have 2 alternative solutions but both feel sketchy:

@LongCatIsLooong
Copy link
Contributor Author

LongCatIsLooong commented Sep 7, 2024

Alternative 1: Changes the signature of BuildOwner.buildScope so it

-  void buildScope(Element context, [ VoidCallback? callback ]) {
-    final BuildScope buildScope = context.buildScope;
+  void buildScope(Element context, [ VoidCallback? callback, BuildScope? buildScopeOverride ]) {
+    final BuildScope buildScope = buildScopeOverride ?? context.buildScope;
     if (callback == null && buildScope._dirtyElements.isEmpty) {
       return;
     }
modified   packages/flutter/lib/src/widgets/layout_builder.dart
@@ -78,6 +78,31 @@ abstract class ConstrainedLayoutBuilder<ConstraintType extends Constraints> exte
   // updateRenderObject is redundant with the logic in the LayoutBuilderElement below.
 }
 
+class _BuildScopeOwner extends ProxyWidget {
+  const _BuildScopeOwner(this.buildScope, Widget child) : super(child: child);
+
+  final BuildScope buildScope;
+
+  @override
+  Element createElement() => _ElementWithBuildScope(this);
+}
+
+class _ElementWithBuildScope extends ComponentElement {
+  _ElementWithBuildScope(_BuildScopeOwner super.widget);
+
+  @override
+  BuildScope get buildScope => (widget as _BuildScopeOwner).buildScope;
+
+  @override
+  void update(Widget newWidget) {
+    super.update(newWidget);
+    rebuild(force: true);
+  }
+
+  @override
+  Widget build() => (widget as _BuildScopeOwner).child;
+}
+
 class _LayoutBuilderElement<ConstraintType extends Constraints> extends RenderObjectElement {
   _LayoutBuilderElement(ConstrainedLayoutBuilder<ConstraintType> super.widget);
 
@@ -86,9 +111,6 @@ class _LayoutBuilderElement<ConstraintType extends Constraints> extends RenderOb
 
   Element? _child;
 
-  @override
-  BuildScope get buildScope => _buildScope;
-
   late final BuildScope _buildScope = BuildScope(scheduleRebuild: _scheduleRebuild);
 
   // To schedule a rebuild, markNeedsLayout needs to be called on this Element's
@@ -100,7 +122,6 @@ class _LayoutBuilderElement<ConstraintType extends Constraints> extends RenderOb
     if (_deferredCallbackScheduled) {
       return;
     }
-
     final bool deferMarkNeedsLayout = switch (SchedulerBinding.instance.schedulerPhase) {
       SchedulerPhase.idle || SchedulerPhase.postFrameCallbacks => true,
       SchedulerPhase.transientCallbacks || SchedulerPhase.midFrameMicrotasks || SchedulerPhase.persistentCallbacks => false,
@@ -208,6 +229,7 @@ class _LayoutBuilderElement<ConstraintType extends Constraints> extends RenderOb
           ),
         );
       }
+      built = _BuildScopeOwner(_buildScope, built);
       try {
         _child = updateChild(_child, built, null);
         assert(_child != null);
@@ -233,7 +255,7 @@ class _LayoutBuilderElement<ConstraintType extends Constraints> extends RenderOb
     final VoidCallback? callback = _needsBuild || (constraints != _previousConstraints)
       ? updateChildCallback
       : null;
-    owner!.buildScope(this, callback);
+    owner!.buildScope(this, callback, _buildScope);
   }

Alternative 2, LayoutBuilderElement can never be dirty or be added to the dirty list (because updateChild happens in performLayout):

  @override
  void markNeedsBuild() {
-    super.markNeedsBuild();
+    // super.markNeedsBuild();
    renderObject.markNeedsLayout();
    _needsBuild = true;
  }

  @override
  void performRebuild() {
    // This gets called if markNeedsBuild() is called on us.
    // That might happen if, e.g., our builder uses Inherited widgets.

    // Force the callback to be called, even if the layout constraints are the
    // same. This is because that callback may depend on the updated widget
    // configuration, or an inherited widget.
    renderObject.markNeedsLayout();
    _needsBuild = true;
    super.performRebuild(); // Calls widget.updateRenderObject (a no-op in this case).
  }

@LongCatIsLooong LongCatIsLooong changed the title Remove _inDirtyList assert Mark _LayoutBuilderElement as always clean Sep 10, 2024
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still trying to wrap my head around this. Can you explain a bit the order of things that happens that cause the assertion to be thrown?

@override
void markNeedsBuild() {
super.markNeedsBuild();
// Calling super.markNeedsBuild is not needed. This Element does not need
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is the case, will this class's performRebuild be called at all? if the assumption is this element is never dirty, we should probably assert the performRebuild not getting called at all

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, when the parent Element calls updateChild (i.e. in treewalk).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I see, I just found out the RenderObjectElement overrides the update

}
// When reactivating an inactivate Element, _scheduleBuildFor should only be
// called within _flushDirtyElements.
if (!_debugBuilding && element._inDirtyList) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check assume repeated call happens within the same frame, which is no longer the case in the layoutbuilder.

WDYT about updating the check to take into account that it is not guarantee the build will be flush every frame.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One possible way is to somehow only enforce this on root build scope

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually know I think of it the current pr makes more sense. there is just nothing to do for rebuild for layoutBuilder

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally the assert should still hold: #154681

Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LongCatIsLooong
Copy link
Contributor Author

update Element.dirty docs

@LongCatIsLooong LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Sep 13, 2024
@auto-submit auto-submit bot merged commit f513a69 into flutter:master Sep 13, 2024
@LongCatIsLooong LongCatIsLooong deleted the fix-154060 branch September 13, 2024 01:44
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 13, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 13, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Sep 13, 2024
auto-submit bot pushed a commit to flutter/packages that referenced this pull request Sep 13, 2024
flutter/flutter@303f222...2d30fe4

2024-09-13 [email protected] Roll Packages from 91caa7a to 330581f (4 revisions) (flutter/flutter#155171)
2024-09-13 [email protected] Fix: Flicker when reorderable list doesn't change its position (flutter/flutter#151026)
2024-09-13 [email protected] Stop reading .packages from flutter_tools. (flutter/flutter#154912)
2024-09-13 [email protected] Roll Flutter Engine from 70109e3b40c0 to bef48e87f438 (1 revision) (flutter/flutter#155156)
2024-09-13 [email protected] Roll Flutter Engine from d917a15823f3 to 70109e3b40c0 (1 revision) (flutter/flutter#155151)
2024-09-13 [email protected] Roll Flutter Engine from 94696ed75dea to d917a15823f3 (1 revision) (flutter/flutter#155147)
2024-09-13 [email protected] Fix TextField content should be selected on desktop when gaining focus (flutter/flutter#154916)
2024-09-13 [email protected] Roll Flutter Engine from 04802b779045 to 94696ed75dea (1 revision) (flutter/flutter#155144)
2024-09-13 [email protected] Roll Flutter Engine from 3d8163f47919 to 04802b779045 (2 revisions) (flutter/flutter#155138)
2024-09-13 [email protected] Roll Flutter Engine from 4d5fea97e933 to 3d8163f47919 (1 revision) (flutter/flutter#155136)
2024-09-13 [email protected] Mark `_LayoutBuilderElement` as always clean (flutter/flutter#154694)
2024-09-13 [email protected] Roll Flutter Engine from 8609af642725 to 4d5fea97e933 (7 revisions) (flutter/flutter#155134)
2024-09-12 [email protected] Disable fuchsia in flutter_tools (flutter/flutter#155111)
2024-09-12 [email protected] Address frame policy benchmark flakes (flutter/flutter#155130)
2024-09-12 [email protected] Roll Flutter Engine from 48ddaf578fb0 to 8609af642725 (11 revisions) (flutter/flutter#155128)
2024-09-12 49699333+dependabot[bot]@users.noreply.github.com Bump peter-evans/create-pull-request from 7.0.1 to 7.0.2 (flutter/flutter#155126)
2024-09-12 [email protected] Prevent the keyboard from reshowing on iOS (flutter/flutter#154584)
2024-09-12 [email protected] fix(Linux): specify application id (flutter/flutter#154522)
2024-09-12 [email protected] update changelog on master (flutter/flutter#155109)
2024-09-12 [email protected] iOS: update provisioning profile for 2024-2025 cert (flutter/flutter#155101)
2024-09-12 [email protected] Roll Packages from 4c18648 to 91caa7a (2 revisions) (flutter/flutter#155103)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-packages
Please CC [email protected],[email protected] on the revert to ensure that a human
is aware of the problem.

To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 11, 2024
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Dec 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

App crashes with "BuildOwner.scheduleBuildFor() called inappropriately." in callback for "ext.flutter.reassemble" when hot-reloading multiple times

3 participants