[Impeller] Move queue submission into a callback that is invoked by FenceWaiterVK::AddFence only if it can accept the fence by jason-simmons · Pull Request #187761 · flutter/flutter

jason-simmons · 2026-06-09T22:44:57Z

The FenceWaiterVK owns the lifetime of the fences that signal completion of a Vulkan command buffer submitted through vkQueueSubmit.

Prior to this PR, CommandQueueVK::Submit was submitting the command buffer to the queue with a fence before calling
FenceWaiterVK::AddFence. If AddFence failed, then the fence would be destroyed even though the Vulkan driver still holds its handle. That could cause a crash when the command completes and the Vulkan driver tries to signal the fence.

With this PR, the vkQueueSubmit call is moved into a callback that will be invoked by FenceWaiterVK::AddFence only if the FenceWaiterVK can take ownership of the fence. If the FenceWaiterVK has been terminated, then AddFence will skip the submit and destroy the fence.

Fixes #187703

…enceWaiterVK::AddFence only if it can accept the fence The FenceWaiterVK owns the lifetime of the fences that signal completion of a Vulkan command buffer submitted through vkQueueSubmit. Prior to this PR, CommandQueueVK::Submit was submitting the command buffer to the queue with a fence before calling FenceWaiterVK::AddFence. If AddFence failed, then the fence would be destroyed even though the Vulkan driver still holds its handle. That could cause a crash when the command completes and the Vulkan driver tries to signal the fence. With this PR, the vkQueueSubmit call is moved into a callback that will be invoked by FenceWaiterVK::AddFence only if the FenceWaiterVK can take ownership of the fence. If the FenceWaiterVK has been terminated, then AddFence will skip the submit and destroy the fence. Fixes flutter#187703

gemini-code-assist

Code Review

This pull request refactors Vulkan command submission by moving the queue submission step into FenceWaiterVK::AddFence via a callback, preventing submissions if the fence waiter is terminated. Unit tests and mock Vulkan helpers are updated to verify this behavior. Feedback recommends including <functional> in fence_waiter_vk.h to support std::function and declaring default_submit_callback as const auto in the unit tests to avoid potential linker errors.

gaaclarke

Looking good, just a few notes.

Am I crazy or did I already submit a review on this? Did this PR get recreated? Maybe I just looked at it and didn't get a chance to submit my review =)

gaaclarke · 2026-06-10T16:34:17Z

+  EXPECT_EQ(std::find(called->begin(), called->end(), "vkQueueSubmit"),
+            called->end());


Can you add a simple test to show that vkQueueSubmit is properly recorded too, please?

gaaclarke · 2026-06-10T16:37:01Z

+  ///        the submitted command completes and the fence is signaled.
+  fml::Status AddFence(vk::UniqueFence fence,
+                       std::function<fml::Status(vk::Fence)> submit_callback,
+                       const fml::closure& completion_callback);


nit: we are going to store this so we could std::move it and often people put rvalues in callback parameters.

Suggested change

const fml::closure& completion_callback);

fml::closure completion_callback);

gaaclarke · 2026-06-10T16:38:52Z

-  EXPECT_FALSE(waiter->AddFence(std::move(fence), nullptr));
+  const auto add_status =
+      waiter->AddFence(std::move(fence), default_submit_callback, nullptr);
+  EXPECT_FALSE(add_status.ok());


instead of expecting something is not ok, can you turn it to a equality assertion about what the error should be?

gaaclarke · 2026-06-10T16:41:25Z

 namespace impeller {
 namespace testing {

+const auto default_submit_callback = [](vk::Fence) { return fml::Status(); };


I think gemini's advice here wrong. I would have just put it in an anonymous namespace.

gaaclarke · 2026-06-10T16:41:49Z

-  EXPECT_FALSE(waiter->AddFence(vk::UniqueFence(), []() {}));
+  const auto add_status =
+      waiter->AddFence(vk::UniqueFence(), default_submit_callback, []() {});
+  EXPECT_FALSE(add_status.ok());


same here, eq test not false test.

gaaclarke · 2026-06-10T16:47:52Z

-    VALIDATION_LOG << "Failed to submit queue: " << vk::to_string(status);
-    return fml::Status(fml::StatusCode::kCancelled, "Failed to submit queue: ");
-  }
+  auto submit_callback = [&](vk::Fence submit_fence) {


"Use default capture by reference ([&]) only when the lifetime of the lambda is obviously shorter than any potential captures."

https://google.github.io/styleguide/cppguide.html#Lambda_expressions

It doesn't escape the current scope, but since we are passing the lambda along instead of calling it, it's probably better to treat it that way.

gaaclarke · 2026-06-10T16:50:26Z

+  /// @brief Invokes the [submit_callback] and adds the fence to the wait set
+  ///        if it succeeds.  The [completion_callback] will be called when


We should specify that the submit_callback is invoked synchronously so people can reason about lifetimes of objects captured by it.

jason-simmons · 2026-06-10T18:25:09Z

Am I crazy or did I already submit a review on this? Did this PR get recreated? Maybe I just looked at it and didn't get a chance to submit my review =)

This PR is an updated variant of #187698

gaaclarke

lgtm, thanks!

auto-submit · 2026-06-15T21:42:38Z

autosubmit label was removed for flutter/flutter/187761, because - The status or check suite Linux linux_clang_tidy has failed. Please fix the issues identified (or deflake) before re-applying this label.

gaaclarke

still lgtm

gaaclarke · 2026-06-16T00:38:40Z

Oops, I said "still lgtm" I mean to say, "thanks, lgtm!" I forgot I didn't approve this one yet.

flutter/flutter@3a0420c...b10d0f1 2026-06-17 mr-peipei@web.de Skip platform-specific plugin registration if no platforms enabled (flutter/flutter#186304) 2026-06-17 engine-flutter-autoroll@skia.org Roll Packages from 8286d39 to 6ce00a8 (1 revision) (flutter/flutter#188109) 2026-06-17 engine-flutter-autoroll@skia.org Roll Skia from 79f93fd5f36e to 5d19002eb73e (1 revision) (flutter/flutter#188108) 2026-06-17 simon@journeyapps.com Import `dart:_js_interop_wasm` in addition to `dart:_wasm` to convert between `JSAny` and `WasmExternRef?` (flutter/flutter#186974) 2026-06-17 engine-flutter-autoroll@skia.org Roll Dart SDK from f811ecae9ca0 to e39bde5b1bfc (2 revisions) (flutter/flutter#188107) 2026-06-17 engine-flutter-autoroll@skia.org Roll Skia from 026f6a6be2b9 to 79f93fd5f36e (1 revision) (flutter/flutter#188105) 2026-06-17 engine-flutter-autoroll@skia.org Roll Dart SDK from 462bf0a1d489 to f811ecae9ca0 (1 revision) (flutter/flutter#188099) 2026-06-17 engine-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from VeLhhlDcod09NR4Hb... to or21OEdGtairm6nl9... (flutter/flutter#188098) 2026-06-17 engine-flutter-autoroll@skia.org Roll Skia from 2ffd155313f5 to 026f6a6be2b9 (10 revisions) (flutter/flutter#188097) 2026-06-17 engine-flutter-autoroll@skia.org Roll Dart SDK from 824b4b48b6d4 to 462bf0a1d489 (1 revision) (flutter/flutter#188093) 2026-06-17 jason-simmons@users.noreply.github.com Manual Dart roll from f6c31f4c3a63 to 824b4b48b6d4 (flutter/flutter#188023) 2026-06-17 awolff@google.com Add a platform view test to android_hardware_smoke_test (flutter/flutter#188069) 2026-06-17 44747303+theprantadutta@users.noreply.github.com [flutter_tools] Format empty app template with latest dart format (flutter/flutter#187443) 2026-06-16 49699333+dependabot[bot]@users.noreply.github.com Bump the all-github-actions group across 1 directory with 3 updates (flutter/flutter#188086) 2026-06-16 engine-flutter-autoroll@skia.org Roll Skia from d7196b0b4939 to 2ffd155313f5 (9 revisions) (flutter/flutter#188081) 2026-06-16 43089218+chika3742@users.noreply.github.com Prevent downgrading `project.pbxproj` when greater version number (flutter/flutter#186250) 2026-06-16 magder@google.com Only allow dependabot to autoupdate GitHub-owned actions (flutter/flutter#187936) 2026-06-16 matt.boetger@gmail.com Fall back to source AndroidManifest.xml if AAPT fails or returns garbage (flutter/flutter#187197) 2026-06-16 137456488+flutter-pub-roller-bot@users.noreply.github.com Roll pub packages (flutter/flutter#187769) 2026-06-16 jason-simmons@users.noreply.github.com [Impeller] Move queue submission into a callback that is invoked by FenceWaiterVK::AddFence only if it can accept the fence (flutter/flutter#187761) 2026-06-16 jhy03261997@gmail.com Reland [a11y] Map some framework semantics roles to android classes. (flutter/flutter#188037) 2026-06-16 1961493+harryterkelsen@users.noreply.github.com refactor(web): Unify Image on Skwasm and CanvasKit (flutter/flutter#187873) 2026-06-16 30870216+gaaclarke@users.noreply.github.com Adds arm64 variant of impeller devicelab tests for windows. (flutter/flutter#188053) 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 bmparr@google.com,stuartmorgan@google.com 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

…enceWaiterVK::AddFence only if it can accept the fence (flutter#187761) The FenceWaiterVK owns the lifetime of the fences that signal completion of a Vulkan command buffer submitted through vkQueueSubmit. Prior to this PR, CommandQueueVK::Submit was submitting the command buffer to the queue with a fence before calling FenceWaiterVK::AddFence. If AddFence failed, then the fence would be destroyed even though the Vulkan driver still holds its handle. That could cause a crash when the command completes and the Vulkan driver tries to signal the fence. With this PR, the vkQueueSubmit call is moved into a callback that will be invoked by FenceWaiterVK::AddFence only if the FenceWaiterVK can take ownership of the fence. If the FenceWaiterVK has been terminated, then AddFence will skip the submit and destroy the fence. Fixes flutter#187703

jason-simmons requested a review from gaaclarke June 9, 2026 22:44

flutter-dashboard Bot added the CICD Run CI/CD label Jun 9, 2026

github-actions Bot added engine flutter/engine related. See also e: labels. e: impeller Impeller rendering backend issues and features requests labels Jun 9, 2026

gemini-code-assist Bot reviewed Jun 9, 2026

View reviewed changes

Comment thread engine/src/flutter/impeller/renderer/backend/vulkan/fence_waiter_vk.h

Comment thread engine/src/flutter/impeller/renderer/backend/vulkan/fence_waiter_vk_unittests.cc

jason-simmons mentioned this pull request Jun 9, 2026

[Impeller] Submit a Vulkan command buffer only if its fence was successfully added to the FenceWaiterVK #187698

Closed

feedback

792171a

github-actions Bot removed the CICD Run CI/CD label Jun 9, 2026

jason-simmons added the CICD Run CI/CD label Jun 9, 2026

gaaclarke requested changes Jun 10, 2026

View reviewed changes

feedback 2

1039f93

github-actions Bot removed the CICD Run CI/CD label Jun 10, 2026

jason-simmons added the CICD Run CI/CD label Jun 10, 2026

gaaclarke self-requested a review June 15, 2026 18:00

gaaclarke previously approved these changes Jun 15, 2026

View reviewed changes

gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 15, 2026

auto-submit Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 15, 2026

clang-tidy

9dbae06

jason-simmons dismissed gaaclarke’s stale review via 9dbae06 June 15, 2026 23:17

github-actions Bot removed the CICD Run CI/CD label Jun 15, 2026

jason-simmons added the CICD Run CI/CD label Jun 15, 2026

jason-simmons requested a review from gaaclarke June 16, 2026 00:33

gaaclarke approved these changes Jun 16, 2026

View reviewed changes

jason-simmons added the autosubmit Merge PR when tree becomes green via auto submit App label Jun 16, 2026

auto-submit Bot added this pull request to the merge queue Jun 16, 2026

Merged via the queue into flutter:master with commit d135f1e Jun 16, 2026
206 checks passed

flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jun 16, 2026

jason-simmons mentioned this pull request Jun 16, 2026

[Impeller] HandleUsingDestroyedMutex crash on Mali GPUs during disposal #187140

Open

		EXPECT_EQ(std::find(called->begin(), called->end(), "vkQueueSubmit"),
		called->end());

	const fml::closure& completion_callback);
	fml::closure completion_callback);

		/// @brief Invokes the [submit_callback] and adds the fence to the wait set
		/// if it succeeds. The [completion_callback] will be called when

Uh oh!

Conversation

jason-simmons commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jason-simmons commented Jun 10, 2026

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit Bot commented Jun 15, 2026

Uh oh!

gaaclarke left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented Jun 16, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jason-simmons commented Jun 9, 2026 •

edited

Loading