Skip to content

Fix Image's logical flow which disposes its image too early, causing errors such as "Cannot clone a disposed image" #110131

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
auto-submit merged 11 commits into from
Aug 26, 2022
Merged

Fix Image's logical flow which disposes its image too early, causing errors such as "Cannot clone a disposed image" #110131

auto-submit merged 11 commits into from
Aug 26, 2022

Conversation

@fzyzcjy
Copy link
Contributor

@fzyzcjy fzyzcjy commented Aug 24, 2022 •
edited
Loading

Close #110129

Please see the issue for paragraphs about the bug cause and fix etc

If you had to change anything in the flutter/tests repo, include a link to the migration guide as per the breaking change policy.

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 flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Aug 24, 2022
@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Aug 24, 2022

The test, without the fix, fails. This verifies the PR is needed.

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8804990179272978753/+/u/run_test.dart_for_framework_tests_shard_and_subshard_widgets/test_stdout

══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════
The following StateError was thrown building KeyedSubtree-[#c5088]:
Bad state: Cannot clone a disposed image.
The clone() method of a previously-disposed Image was called. Once an Image object has been
disposed, it can no longer be used to create handles, as the underlying data may have been released.

When the exception was thrown, this was the stack:
#0      Image.clone (dart:ui/painting.dart:1808:7)
#1      RawImage.createRenderObject (package:flutter/src/widgets/basic.dart:5951:21)
#2      RenderObjectElement.mount (package:flutter/src/widgets/framework.dart:5744:52)
...     Normal element mounting (10 frames)
#12     Element.inflateWidget (package:flutter/src/widgets/framework.dart:3883:16)
#13     Element.updateChild (package:flutter/src/widgets/framework.dart:3606:20)
#14     ComponentElement.performRebuild (package:flutter/src/widgets/framework.dart:4920:16)
#15     StatefulElement.performRebuild (package:flutter/src/widgets/framework.dart:5060:11)
#16     Element.rebuild (package:flutter/src/widgets/framework.dart:4617:5)
#17     BuildOwner.buildScope (package:flutter/src/widgets/framework.dart:2687:19)
#18     AutomatedTestWidgetsFlutterBinding.drawFrame (package:flutter_test/src/binding.dart:1244:19)
#19     RendererBinding._handlePersistentFrameCallback (package:flutter/src/rendering/binding.dart:374:5)
#20     SchedulerBinding._invokeFrameCallback (package:flutter/src/scheduler/binding.dart:1175:15)
#21     SchedulerBinding.handleDrawFrame (package:flutter/src/scheduler/binding.dart:1104:9)
#22     AutomatedTestWidgetsFlutterBinding.pump.<anonymous closure> (package:flutter_test/src/binding.dart:1093:9)
#25     TestAsyncUtils.guard (package:flutter_test/src/test_async_utils.dart:71:41)
#26     AutomatedTestWidgetsFlutterBinding.pump (package:flutter_test/src/binding.dart:1079:27)
#27     WidgetTester.pump.<anonymous closure> (package:flutter_test/src/widget_tester.dart:618:53)
#30     TestAsyncUtils.guard (package:flutter_test/src/test_async_utils.dart:71:41)
#31     WidgetTester.pump (package:flutter_test/src/widget_tester.dart:618:27)
#32     main.<anonymous closure> (file:///b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/image_test.dart:83:20)
<asynchronous suspension>
<asynchronous suspension>
(elided 5 frames from dart:async and package:stack_trace)

════════════════════════════════════════════════════════════════════════════════════════════════════

02:21 +3329 ~2 -1: /b/s/w/ir/x/w/flutter/packages/flutter/test/widgets/image_test.dart: Verify Image does not use disposed handles [E]                                                                 
  Test failed. See exception logs above.
  The test description was: Verify Image does not use disposed handles
  
══╡ EXCEPTION CAUGHT BY WIDGET INSPECTOR ╞══════════════════════════════════════════════════════════

@fzyzcjy fzyzcjy marked this pull request as ready for review August 24, 2022 03:04
@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Aug 24, 2022

All tests have passed

image

@fzyzcjy fzyzcjy mentioned this pull request Aug 24, 2022
Closed
@jonahwilliams jonahwilliams requested a review from dnfield August 24, 2022 13:22
dnfield
dnfield suggested changes Aug 24, 2022
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

The change makes sense to me and I think it's the right one. Thanks for doing some hard work here!

There are no expectations in the test that actually tell us whether the images ever actually complete, and its currently depending on some implementation details like RawImage being wrapped in a Semantics or being in the tree for this at all. Someone refactoring that code may easily misunderstand that this test actually is using those details to find the image.

It would be better to have a test that asserts we've done something meaningful as well. For example, someone could introduce a breakage to what you're fixing in the future where the second image never loads or the expected build order does not happen and this test would still pass.

I'd like to see the test not use any while loops, and have some more expectations about what's happening. Happy to chat some more about how to do this if it's not clear.

@fzyzcjy fzyzcjy requested a review from dnfield August 24, 2022 23:03
@dnfield
Copy link
Contributor

dnfield commented Aug 25, 2022

LGTM once tests are passing :)

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Aug 25, 2022

@dnfield Tests are passing :)

image

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Aug 26, 2022

What should I do now 👀

@dnfield dnfield added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2022
@dnfield
Copy link
Contributor

dnfield commented Aug 26, 2022

Sorry, thought I had added the label!

@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 26, 2022

  • 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-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2022
@auto-submit
Copy link
Contributor

auto-submit bot commented Aug 26, 2022

Validations Fail.

@fzyzcjy
Copy link
Contributor Author

fzyzcjy commented Aug 26, 2022 •
edited
Loading

@dnfield That's OK. Oops seems to need one more reviewer

@dnfield dnfield requested a review from goderbauer August 26, 2022 01:05
goderbauer
goderbauer approved these changes Aug 26, 2022
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

@goderbauer goderbauer added the autosubmit Merge PR when tree becomes green via auto submit App label Aug 26, 2022
@auto-submit auto-submit bot merged commit 5be7e49 into flutter:master Aug 26, 2022
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 26, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 26, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
d464220 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 26, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 26, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
1c9f61b 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 26, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 26, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
79c8790 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 26, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 26, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
3b53cda 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 26, 2022
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Aug 26, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
2ae693c 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 26, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 26, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
77bed82 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 26, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 26, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
2deb3d1 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 26, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 26, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
4047821 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 26, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 26, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
536680e 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 26, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 26, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
96fe34c 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 26, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 26, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
d671eb6 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Aug 29, 2022
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Aug 29, 2022
@engine-flutter-autoroll
5be7e49 Fix Image's logical flow which disposes its image too early, …
cca4842 
…causing errors such as "Cannot clone a disposed image" (flutter/flutter#110131)
zmtzawqlp added a commit to fluttercandies/extended_image that referenced this pull request May 11, 2023
@zmtzawqlp
## 8.0.0
6ca24b7 
* Migrate to Flutter 3.10.0 and Dart 3.0.0 (#557,#563,#570,#572,#573)
* Cherry Pick flutter/flutter#110131
* Cherry Pick flutter/flutter#119495
zmtzawqlp added a commit to fluttercandies/extended_image that referenced this pull request May 11, 2023
@zmtzawqlp
* Migrate to Flutter 3.10.0 and Dart 3.0.0 (#557,#563,#570,#572,#573)
fc1cb04 
* Cherry Pick flutter/flutter#110131
* Cherry Pick flutter/flutter#119495
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@goderbauer goderbauer goderbauer approved these changes

+1 more reviewer

@dnfield dnfield dnfield approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Image disposes its image too early, causing errors such as "Cannot clone a disposed image"

3 participants

@fzyzcjy @dnfield @goderbauer