-
Notifications
You must be signed in to change notification settings - Fork 6k
iOS: add null check on create impeller context #56952
iOS: add null check on create impeller context #56952
Conversation
In the case where `CreateImpellerContext` encounters an error while creating an `impeller::ContextMTL`, we logged an error, returned nullptr, then immediately dereferenced the null pointer. This adds a bail-out check in the `FlutterDarwinContextMetalImpeller` initialiser that returns nil if impeller context creation fails, and logs an error message. Further, it moves the error logging alongside the similar bail-out log message on failure to create a Metal device, and makes both such cases `FML_LOG(ERROR)` rather than `FML_DLOG(ERROR)`, so that Flutter app developers can spot and report the error message. Issue: flutter/flutter#157489 Issue: b/378790930
|
It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption, contact "@test-exemption-reviewer" in the #hackers channel in Discord (don't just cc them here, they won't see it!). 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. The test exemption team is a small volunteer group, so all reviewers should feel empowered to ask for tests, without delegating that responsibility entirely to the test exemption group. |
hellohuanlin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!
|
This is mostly just passing the buck, but consistent with the metal device lookup below. There are tests that assume this initialiser can return
Another alternative would be to just |
jonahwilliams
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
test-exempt: changes logging of a fatal error, which we don't have a mechanism to test |
…159909) flutter/engine@31ba1e0...d42645f 2024-12-06 30870216+gaaclarke@users.noreply.github.com Reland: Replaces bespoke call captures from mock gles with gmock (flutter/engine#57019) 2024-12-06 skia-flutter-autoroll@skia.org Roll Skia from c9647f13cded to 0d94e966268b (36 revisions) (flutter/engine#57023) 2024-12-06 chris@bracken.jp iOS: add null check on create impeller context (flutter/engine#56952) 2024-12-06 skia-flutter-autoroll@skia.org Roll Dart SDK from 9c9a333c496c to 67ce49b905f7 (2 revisions) (flutter/engine#57013) 2024-12-06 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Replaces bespoke call captures from mock gles with gmock (#56995)" (flutter/engine#57016) 2024-12-06 30870216+gaaclarke@users.noreply.github.com Replaces bespoke call captures from mock gles with gmock (flutter/engine#56995) 2024-12-06 jason-simmons@users.noreply.github.com [Impeller] Require the GLES multisampled_render_to_texture2 extension for offscreen MSAA (flutter/engine#56997) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC bdero@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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
In the case where `CreateImpellerContext` encounters an error while creating an `impeller::ContextMTL`, we logged an error, returned nullptr, then immediately dereferenced the null pointer. Now, rather than crash due to a segfault, we now intentionally abort with an appropriate error message. This adds checks in the `FlutterDarwinContextMetalImpeller` initialiser that aborts with an appropriate error message if impeller context creation fails, Metal device creation fails, or texture cache creation fails. Rather than bailing out and returning nil from the initialiser to pass the buck to the caller, we terminate since without a graphics context, the app won't be able to render anything to begin with. Issue: flutter#157489 Issue: [b/378790930](http://b/378790930) No test changes since this just changes an accidental crash to an intentional crash. [C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
In the case where
CreateImpellerContextencounters an error while creating animpeller::ContextMTL, we logged an error, returned nullptr, then immediately dereferenced the null pointer. Now, rather than crash due to a segfault, we now intentionally abort with an appropriate error message.This adds checks in the
FlutterDarwinContextMetalImpellerinitialiser that aborts with an appropriate error message if impeller context creation fails, Metal device creation fails, or texture cache creation fails.Rather than bailing out and returning nil from the initialiser to pass the buck to the caller, we terminate since without a graphics context, the app won't be able to render anything to begin with.
Issue: flutter/flutter#157489
Issue: b/378790930
No test changes since this just changes an accidental crash to an intentional crash.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.