Layer ... was previously used as oldLayer assertion error in debug mode, and page being blank in release mode, caused by LeaderLayer addToScene bug

[fzyzcjy]

Close #113995
Please see the issue for a bug reproduction. Here I will discuss how it is solved and what caused it.

The real world bug

My app has a part of it not rendered (i.e. page blank) sometimes, which is very weird. After I manage to reproduce it in debug environment, the error is:

======== Exception caught by scheduler library =====================================================
The following assertion was thrown during a scheduler callback:
Layer ClipRectEngineLayer was previously used as oldLayer.
Once a layer is used as oldLayer, it may not be used again. Instead, after calling one of the SceneBuilder.push* methods and passing an oldLayer to it, use the layer returned by the method as oldLayer in subsequent frames.
'dart:ui/compositing.dart':
Failed assertion: line 88 pos 9: '<optimized out>'


Either the assertion indicates an error in the framework itself, or we should provide substantially more information in this error message to help you determine and fix the underlying cause.
In either case, please report this assertion by filing a bug on GitHub:
  https://github.com/flutter/flutter/issues/new?template=2_bug.md

When the exception was thrown, this was the stack: 
#2      _EngineLayerWrapper._debugCheckNotUsedAsOldLayer (dart:ui/compositing.dart:88:9)
#3      SceneBuilder.addRetained.<anonymous closure>.recursivelyCheckChildrenUsedOnce (dart:ui/compositing.dart:656:21)
#4      List.forEach (dart:core-patch/growable_array.dart:416:8)
#5      SceneBuilder.addRetained.<anonymous closure>.recursivelyCheckChildrenUsedOnce (dart:ui/compositing.dart:662:18)
#6      SceneBuilder.addRetained.<anonymous closure> (dart:ui/compositing.dart:665:7)
#7      SceneBuilder.addRetained (dart:ui/compositing.dart:668:6)
#8      Layer._addToSceneWithRetainedRendering (package:flutter/src/rendering/layer.dart:643:15)
#9      ContainerLayer.addChildrenToScene (package:flutter/src/rendering/layer.dart:1241:13)
#10     OffsetLayer.addToScene (package:flutter/src/rendering/layer.dart:1378:5)
#11     Layer._addToSceneWithRetainedRendering (package:flutter/src/rendering/layer.dart:646:5)
#12     ContainerLayer.addChildrenToScene (package:flutter/src/rendering/layer.dart:1241:13)
#13     OffsetLayer.addToScene (package:flutter/src/rendering/layer.dart:1378:5)
#14     Layer._addToSceneWithRetainedRendering (package:flutter/src/rendering/layer.dart:646:5)
#15     ContainerLayer.addChildrenToScene (package:flutter/src/rendering/layer.dart:1241:13)
#16     EnhancedLeaderLayer.addToScene (package:flutter_portal/src/enhanced_composited_transform/flutter_src/rendering_layer.dart:159:5)
#17     Layer._addToSceneWithRetainedRendering (package:flutter/src/rendering/layer.dart:646:5)
#18     ContainerLayer.addChildrenToScene (package:flutter/src/rendering/layer.dart:1241:13)
#19     EnhancedLeaderLayer.addToScene (package:flutter_portal/src/enhanced_composited_transform/flutter_src/rendering_layer.dart:159:5)
#20     Layer._addToSceneWithRetainedRendering (package:flutter/src/rendering/layer.dart:646:5)
#21     ContainerLayer.addChildrenToScene (package:flutter/src/rendering/layer.dart:1241:13)
#22     EnhancedLeaderLayer.addToScene (package:flutter_portal/src/enhanced_composited_transform/flutter_src/rendering_layer.dart:159:5)
#23     Layer._addToSceneWithRetainedRendering (package:flutter/src/rendering/layer.dart:646:5)
#24     ContainerLayer.addChildrenToScene (package:flutter/src/rendering/layer.dart:1241:13)
#25     EnhancedLeaderLayer.addToScene (package:flutter_portal/src/enhanced_composited_transform/flutter_src/rendering_layer.dart:159:5)
#26     Layer._addToSceneWithRetainedRendering (package:flutter/src/rendering/layer.dart:646:5)
#27     ContainerLayer.addChildrenToScene (package:flutter/src/rendering/layer.dart:1241:13)
#28     OffsetLayer.addToScene (package:flutter/src/rendering/layer.dart:1378:5)
#29     Layer._addToSceneWithRetainedRendering (package:flutter/src/rendering/layer.dart:646:5)
#30     ContainerLayer.addChildrenToScene (package:flutter/src/rendering/layer.dart:1241:13)
#31     TransformLayer.addToScene (package:flutter/src/rendering/layer.dart:1834:5)
#32     ContainerLayer.buildScene (package:flutter/src/rendering/layer.dart:1054:5)
#33     RenderView.compositeFrame (package:flutter/src/rendering/view.dart:231:37)
#34     RendererBinding.drawFrame (package:flutter/src/rendering/binding.dart:517:18)
#35     WidgetsBinding.drawFrame (package:flutter/src/widgets/binding.dart:884:13)
#36     RendererBinding._handlePersistentFrameCallback (package:flutter/src/rendering/binding.dart:378:5)
#37     SchedulerBinding._invokeFrameCallback (package:flutter/src/scheduler/binding.dart:1175:15)
#38     SchedulerBinding.handleDrawFrame (package:flutter/src/scheduler/binding.dart:1104:9)
#39     SchedulerBinding._handleDrawFrame (package:flutter/src/scheduler/binding.dart:1015:5)
#40     _invoke (dart:ui/hooks.dart:148:13)
#41     PlatformDispatcher._drawFrame (dart:ui/platform_dispatcher.dart:318:5)
#42     _drawFrame (dart:ui/hooks.dart:115:31)
(elided 2 frames from class _AssertionError)
====================================================================================================

The minimal reproduction

(I will omit how I find out the root cause. If you are interested I can write down.)

Please see #113995 with minimal reproduction code

Why that causes bug

The first pumpWidget creates initial tree, and I deliberately set padding to non-zero, such that LeaderLayer.paint will see non-zero Offset, and thus will pushTransform.

In the second pumpWidget, I deliberately set padding to zero. Then, LeaderLayer.paint will not call pushTransform. Correct implementation should set engineLayer to null, but the old code just leave that variable unchanged.

Then comes the third pumpWidget. I change the color inside a RepaintBoundary which is the sibling of LeaderLayer (CompositedTransformTarget). This is carefully constructed (notice the sibling and the RepaintBoundary) to reproduce the following behavior in real-world complicated app: We should (1) ensure LeaderLayer. _addToSceneWithRetainedRendering is called, and (2) ensure LeaderLayer. _needsAddToScene = false. For example, if we do not add that RepaintBoundary, it will not construct the case, because CompositedTransformTarget's RO will repaint and thus _needsAddToScene becomes true.

Then, interesting thing happens. Look at the code:

  void _addToSceneWithRetainedRendering(ui.SceneBuilder builder) {
    if (!_needsAddToScene && _engineLayer != null) {
      builder.addRetained(_engineLayer!);
      return;
    }
    ...

We have constructed a case, such that the if is true. Notice that, if we fix the bug (just as what we do in the PR), the engineLayer will be null so if condition will not be true. Thus, buggy code will call LeaderLayer.addRetained, while correct code will not.

Then the error happens. Notice that, this LeaderLayer._engineLayer indeed is the layer constructed in the second pumpWidget instead of the third (i.e. it is stale from last frame). Therefore, this EngineLayer's children are all from the stale previous frame. For example, the ClipRectLayer from previous frame (second pumpWidget). Since it has already been used as oldLayer in RenderClipRect, that old stale ClipRectLayer should never be used. However we are now using it in addRetained. Therefore, no wonder when addRetained is checking all subtree ensuring _debugCheckNotUsedAsOldLayer, it fails the assertions.

Why is the solution valid

With analysis above, we can clearly see this solves the bug. In addition, looking at ClipRectLayer etc, they also have such "engineLayer = null" logic, so even if only mimicking those we should do this.

Pre-launch Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
• I read the Tree Hygiene wiki page, which explains my responsibilities.
• I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
• I signed the CLA.
• I listed at least one issue that this PR fixes in the description above.
• I updated/added relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or this PR is test-exempt.
• All existing and new tests are passing.

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

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[goderbauer]

LGTM

Thanks for the catch!

[goderbauer]

either move this to the previous line or use proper block syntax.

[fzyzcjy]

You are welcome! Format done

[auto-submit]

auto label is removed for flutter/flutter, pr: 113998, due to - Please get at least one approved review if you are already a member or two member reviews if you are not a member before re-applying this label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[auto-submit]

auto label is removed for flutter/flutter, pr: 113998, due to Validations Fail.

[justinmc]

LGTM 👍

[justinmc]

Sometimes I've seen our tests use expect(tester.takeException(), isNull); for tests like this, but I guess this test will fail if there is an error even without that line.

[fzyzcjy]

The test does fail without the fix IIRC. If I understand correctly, flutter will verify there is no exception at the end of test. So even if I do not take exception, seems it will be verified at end of test so no worries?