[Windows] Add/remove view failures should not hang #52164
Conversation
| FlutterEngineResult result = embedder_api_.RemoveView(engine_, &info); | ||
| if (result != kSuccess) { | ||
| // Starting the remove view operation failed. This is unexpected and | ||
| // indicates a bug in the Windows embedder. |
There was a problem hiding this comment.
If failures indicate bugs of the Windows embedder (impossible with any user input), should we just assert it?
There was a problem hiding this comment.
On the fence on this one. I was going to make the same comment exactly -- at least in debug mode, but I can see the argument for either approach depending on how severe the repercussions of the failure are.
There was a problem hiding this comment.
It feels unease to me that if somehow this branch is triggered due to a bug (for example, we somehow removed the outer check), the bug can exist on for months unreported because it's just a harmless log in the user's app, and the user might think "oh, right, I shouldn't add an implicit view again". It's not the severity of the failure that matters the most.
There was a problem hiding this comment.
I switched to assertions for the result value. The engine does basic spot checks here (is it an implicit view ID? Did you provide invalid arguments?) that the Windows embedder should never fail.
loic-sharma
added
the
autosubmit
…146930) flutter/engine@818191d...bc6382e 2024-04-17 [email protected] [web] move AccessibilityAnnouncements into SemanticsOwner (flutter/engine#52138) 2024-04-17 [email protected] [Windows] Add/remove view failures should not hang (flutter/engine#52164) 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 [email protected],[email protected],[email protected] 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
…lutter#146930) flutter/engine@818191d...bc6382e 2024-04-17 [email protected] [web] move AccessibilityAnnouncements into SemanticsOwner (flutter/engine#52138) 2024-04-17 [email protected] [Windows] Add/remove view failures should not hang (flutter/engine#52164) 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 [email protected],[email protected],[email protected] 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
If the embedder API's
FlutterEngineAddViewandFlutterEngineRemoveViewdon't returnkSuccess, their callbacks won't be invoked. See: flutter.dev/go/multi-view-embedder-apis.Previously, the Windows embedder would hang in this scenario as it blocks until the callbacks are invoked. Now, the Windows embedder only blocks if these embedder APIs return
kSuccess.Kudos to @dkwingsmt for catching this!
Part of flutter/flutter#144810
Part of flutter/flutter#142845
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.