Saves a DeviceHolderVK with the CommandPoolVK by gaaclarke · Pull Request #186749 · flutter/flutter

gaaclarke · 2026-05-19T15:59:32Z

Pre-launch Checklist

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

If this change needs to override an active code freeze, provide a comment explaining why. The code freeze workflow can be overridden by code reviewers. See pinned issues for any active code freezes with guidance.

Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the gemini-code-assist bot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.

gemini-code-assist

Code Review

This pull request updates CommandPoolVK to use DeviceHolderVK for tracking the Vulkan device lifecycle, ensuring resources are released without calling destruction functions if the device is already destroyed. Changes include updates to the destructor, the Destroy method, and the addition of a unit test. Feedback identifies that the Destroy method should also release command buffers when the device is invalid and suggests enhancing the unit test to verify that command buffers are not freed in such scenarios.

gemini-code-assist · 2026-05-19T16:02:00Z

+TEST(CommandPoolVKTest, DestroysCleanlyIfDeviceIsDestroyed) {
+  auto const context = MockVulkanContextBuilder().Build();
+
+  // Create a vk::UniqueCommandPool
+  vk::CommandPoolCreateInfo info;
+  info.setQueueFamilyIndex(context->GetGraphicsQueue()->GetIndex().family);
+  info.setFlags(vk::CommandPoolCreateFlagBits::eTransient);
+
+  auto device = context->GetDevice();
+  auto [result, pool] = device.createCommandPoolUnique(info);
+  ASSERT_EQ(result, vk::Result::eSuccess);
+
+  auto device_holder = std::make_shared<MockDeviceHolder>(device);
+  std::weak_ptr<DeviceHolderVK> weak_device_holder = device_holder;
+
+  auto command_pool = std::make_unique<CommandPoolVK>(
+      std::move(pool), std::vector<vk::UniqueCommandBuffer>{}, context,
+      weak_device_holder);
+
+  // Now, destroy the device holder.
+  device_holder.reset();
+
+  // Now, destroy the command pool. Since the device holder is dead, this should
+  // NOT call vkDestroyCommandPool.
+  command_pool.reset();
+
+  auto const called = ReclaimAndGetMockVulkanFunctions(context);
+  EXPECT_EQ(std::count(called->begin(), called->end(), "vkDestroyCommandPool"),
+            0u);
+
+  context->Shutdown();
+}


The test currently only verifies that vkDestroyCommandPool is not called. It would be more robust to also verify that vkFreeCommandBuffers is not called, especially since the CommandPoolVK holds vectors of command buffers that must also be released when the device is gone. Consider adding some mock command buffers to the pool in this test case to ensure they are handled correctly.

jason-simmons

Have you been able to reproduce something like what was described in #186458?

The stack trace there mentioned BackgroundCommandPoolVK. There may be some similar potential races involving interactions between BackgroundCommandPoolVK and destruction of the ContextVK. But I haven't reproduced it locally and have not identified the sequence of events that would cause the trace in #186458.

In any case, this is a good improvement. The CommandPoolVK ownership model is complicated, and there could be situations where a CommandPoolVK is destructed after the ContextVK and the Vulkan device have been deleted.

gaaclarke · 2026-05-21T16:23:28Z

Have you been able to reproduce something like what was described in #186458?

No, this change is based on the patch the OP posted that they claim removes their crash. In their patch they maintained a shared_ptr to the holder, which is wrong. This follows the same pattern we use for other vulkan object cleanup.

jason-simmons

LGTM

Can you change the PR comment from "fixes #186458" to "see #186458"?

I'd like to keep #186458 open in case anyone has more information about issues involving BackgroundCommandPoolVK.

gaaclarke · 2026-05-21T17:17:26Z

Can you change the PR comment from "fixes #186458" to "see #186458"?

sure, done

auto-submit · 2026-05-21T17:31:27Z

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

auto-submit · 2026-05-21T18:02:14Z

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

auto-submit · 2026-05-21T18:29:16Z

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

fluttergithubbot · 2026-05-21T20:40:21Z

An existing Git SHA, b41b7302f0e2a6960a942007c4b696080b675d50, was detected, and no actions were taken.

To re-trigger presubmits after closing or re-opeing a PR, or pushing a HEAD commit (i.e. with --force) that already was pushed before, push a blank commit (git commit --allow-empty -m "Trigger Build") or rebase to continue.

auto-submit · 2026-05-21T22:30:13Z

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

gaaclarke · 2026-05-21T22:48:09Z

This keeps getting stuck on fuchsia failures. They don't sound apropos I'll try rerunning one last time.

[stderr - main]
embedder.cc (2041): 'FlutterEngineInitialize' returned 'kInvalidArguments'. The assets path in the Flutter project arguments was missing.
embedder.cc (3030): 'FlutterEngineSendPlatformMessage' returned 'kInvalidArguments'. Message size was non-zero but the message data was nullptr.
embedder.cc (2500): 'FlutterEngineRunInitialized' returned 'kInvalidArguments'. Engine handle was invalid.
embedder.cc (2500): 'FlutterEngineRunInitialized' returned 'kInvalidArguments'. Engine handle was invalid.
embedder.cc (2709): 'FlutterEngineSendWindowMetricsEvent' returned 'kInvalidArguments'. Viewport metrics were invalid.
embedder.cc (2603): 'FlutterEngineRemoveView' returned 'kInvalidArguments'. Remove view info was invalid. The implicit view cannot be removed.
embedder.cc (3426): 'FlutterEngineUpdateLocales' returned 'kInvalidArguments'. Language code is required but not present in FlutterLocale.
embedder.cc (2500): 'FlutterEngineRunInitialized' returned 'kInvalidArguments'. Engine handle was invalid.
embedder.cc (2700): 'FlutterEngineSendWindowMetricsEvent' returned 'kInvalidArguments'. Device pixel ratio was invalid. It must be greater than zero.
embedder.cc (2700): 'FlutterEngineSendWindowMetricsEvent' returned 'kInvalidArguments'. Physical view insets are invalid. They must be non-negative.
embedder.cc (2700): 'FlutterEngineSendWindowMetricsEvent' returned 'kInvalidArguments'. Physical view insets are invalid. They cannot be greater than physical height or width.
embedder.cc (2700): 'FlutterEngineSendWindowMetricsEvent' returned 'kInvalidArguments'. Window metrics are invalid. Width and height must be within the specified constraints.
embedder.cc (2700): 'FlutterEngineSendWindowMetricsEvent' returned 'kInvalidArguments'. Window metrics are invalid. Width and height must be within the specified constraints.
embedder.cc (2700): 'FlutterEngineSendWindowMetricsEvent' returned 'kInvalidArguments'. Window metrics are invalid. Width and height must be within the specified constraints.
[FAILED]	main

Failed tests: main
0 out of 1 tests passed...
fuchsia-pkg://fuchsia.com/embedder_tests#meta/embedder_tests.cm completed with result: FAILED
Tests failed.

Roll Flutter from e03b91f1fe34 to f3a4b9897834 (63 revisions) flutter/flutter@e03b91f...f3a4b98 2026-05-26 [email protected] Update batch release doc to reflect latest workflow (flutter/flutter#186979) 2026-05-26 [email protected] Roll Skia from 0442274cc696 to 27a819894f7c (5 revisions) (flutter/flutter#187094) 2026-05-26 [email protected] [Tool Robustness] Gracefully handle asynchronous subprocess crashes and connection timeouts (flutter/flutter#186964) 2026-05-26 [email protected] [pubspec] Bump Dart SDK constraint to ^3.13.0 (flutter/flutter#186957) 2026-05-26 [email protected] Roll Dart SDK from 7eb54169841d to 00e625453c43 (1 revision) (flutter/flutter#187086) 2026-05-26 [email protected] [Impeller] Retire Y-coord-scale plumbing by flipping GLES at the vertex stage (flutter/flutter#186556) 2026-05-26 [email protected] Roll Skia from f4f294bdf98d to 0442274cc696 (2 revisions) (flutter/flutter#187079) 2026-05-26 [email protected] [flutter_tools] Fix version cache poisoning from git environment variables (flutter/flutter#186595) 2026-05-26 [email protected] [Tool] Handle DTD connection failures gracefully in widget-preview (flutter/flutter#186952) 2026-05-25 [email protected] Roll Skia from 9d1adb5f2427 to f4f294bdf98d (1 revision) (flutter/flutter#187056) 2026-05-25 [email protected] Roll Skia from 4dd78179e6ec to 9d1adb5f2427 (1 revision) (flutter/flutter#187048) 2026-05-25 [email protected] Roll Skia from 1f26101197bf to 4dd78179e6ec (4 revisions) (flutter/flutter#187044) 2026-05-24 [email protected] Roll Skia from bbe9ccc2bdbf to 1f26101197bf (1 revision) (flutter/flutter#187016) 2026-05-24 [email protected] Roll Fuchsia Linux SDK from nsgcNDlZOuweOvy3Q... to Itd2Jq_ZIABH2rW7B... (flutter/flutter#187032) 2026-05-23 [email protected] Roll Dart SDK from 7e0f28eb5315 to 7eb54169841d (1 revision) (flutter/flutter#187005) 2026-05-23 [email protected] Roll Dart SDK from 90e55fa88456 to 7e0f28eb5315 (1 revision) (flutter/flutter#186990) 2026-05-23 [email protected] Roll Fuchsia Linux SDK from 6T6BY9PTftoG3vP_1... to nsgcNDlZOuweOvy3Q... (flutter/flutter#186984) 2026-05-23 [email protected] iOS] Migrate VSyncClient to a pure Obj-C implementation (#186166) (flutter/flutter#186935) 2026-05-23 [email protected] Disables embedder_tests.cm for fuchsia (flutter/flutter#186969) 2026-05-23 [email protected] Roll Dart SDK from b8414c46f6c7 to 90e55fa88456 (2 revisions) (flutter/flutter#186977) 2026-05-22 [email protected] Roll Skia from 6fdb013d1953 to bbe9ccc2bdbf (1 revision) (flutter/flutter#186980) 2026-05-22 [email protected] [web] Fix cutoff text in WebParagraph (flutter/flutter#186819) 2026-05-22 [email protected] fix(web): Removes the iterative downscaling hack (flutter/flutter#186914) 2026-05-22 [email protected] opts the linux embedder into sdf rendering (flutter/flutter#186909) 2026-05-22 [email protected] Roll Skia from dae8778ca40d to 6fdb013d1953 (5 revisions) (flutter/flutter#186970) 2026-05-22 [email protected] Fix hooks inputs outputs rebuilt (flutter/flutter#186701) 2026-05-22 [email protected] adds linux impeller integration test for external textures (flutter/flutter#186759) 2026-05-22 [email protected] fix(flutter_tools): defensively catch DWDS unregistered service extension errors (flutter/flutter#186896) 2026-05-22 [email protected] [Impeller] Add golden harness support to the renderer test layer (flutter/flutter#186735) 2026-05-22 [email protected] [web] Remove image codecs from canvaskit_chromium (flutter/flutter#178133) 2026-05-22 [email protected] opts all macos into wide gamut (flutter/flutter#186277) 2026-05-22 [email protected] Roll Skia from 356185490a75 to dae8778ca40d (9 revisions) (flutter/flutter#186949) 2026-05-22 [email protected] Filter out SwiftPM schemes when fetching schemes (flutter/flutter#186006) 2026-05-22 [email protected] Roll Packages from 3754d04 to 69cf959 (1 revision) (flutter/flutter#186950) 2026-05-22 [email protected] Roll Dart SDK from eca46bec956d to b8414c46f6c7 (2 revisions) (flutter/flutter#186944) 2026-05-22 [email protected] Saves a DeviceHolderVK with the CommandPoolVK (flutter/flutter#186749) 2026-05-22 [email protected] Roll Dart SDK from e0d509fd676e to eca46bec956d (1 revision) (flutter/flutter#186922) 2026-05-22 [email protected] [ Tool ] Stop generating widget preview scaffold under $TMP (flutter/flutter#186476) 2026-05-21 [email protected] Fix typo in StretchingOverscrollIndicator docs (flutter/flutter#186897) 2026-05-21 [email protected] Roll Dart SDK from 28c7cb5a8e8d to e0d509fd676e (1 revision) (flutter/flutter#186903) 2026-05-21 [email protected] Fix some issues in the integration between EmbedderExternalViewEmbedder and Impeller (flutter/flutter#184905) 2026-05-21 [email protected] Fix a potential buffer overflow in the animated PNG decoder when parsing malformed fdAT chunks (flutter/flutter#186700) 2026-05-21 [email protected] Roll Skia from 2ff20950975d to 356185490a75 (5 revisions) (flutter/flutter#186892) 2026-05-21 [email protected] Add primitive shadows to rendering benchmark (flutter/flutter#186779) 2026-05-21 [email protected] Move prefetchSwiftPackages to be per platform (flutter/flutter#186468) 2026-05-21 [email protected] Upgrade iOS version (flutter/flutter#186889) ...

Started saving a deviceholder with the command pool

a4b1586

flutter-dashboard Bot added the CICD Run CI/CD label May 19, 2026

gaaclarke requested a review from jason-simmons May 19, 2026 15:59

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

gemini-code-assist Bot reviewed May 19, 2026

View reviewed changes

jason-simmons reviewed May 20, 2026

View reviewed changes

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

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

jason feedback

b41b730

github-actions Bot removed the CICD Run CI/CD label May 21, 2026

gaaclarke requested a review from jason-simmons May 21, 2026 16:23

gaaclarke added the CICD Run CI/CD label May 21, 2026

jason-simmons reviewed May 21, 2026

View reviewed changes

gaaclarke requested a review from jason-simmons May 21, 2026 17:17

jason-simmons approved these changes May 21, 2026

View reviewed changes

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

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

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

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

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

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

gaaclarke added autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD and removed CICD Run CI/CD labels May 21, 2026

Merge branch 'master' into weak-ptr-command-pool

b95b51f

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

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

gaaclarke added this pull request to the merge queue May 22, 2026

Merged via the queue into flutter:master with commit 2485641 May 22, 2026
207 checks passed

gaaclarke deleted the weak-ptr-command-pool branch May 22, 2026 04:49

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

gaaclarke mentioned this pull request May 22, 2026

[Impeller][Vulkan] CommandPoolVK can free command buffers on a destroyed VkDevice, crashing in vkFreeCommandBuffers on the resource manager thread #186458

Open

gaaclarke mentioned this pull request Jun 8, 2026

[Impeller] Remove unused DeviceHolderVK reference from CommandBufferVK #187705

Merged

Conversation

gaaclarke commented May 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist Bot May 19, 2026

Choose a reason for hiding this comment

Uh oh!

jason-simmons left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gaaclarke commented May 21, 2026

Uh oh!

jason-simmons left a comment

Choose a reason for hiding this comment

Uh oh!

gaaclarke commented May 21, 2026

Uh oh!

auto-submit Bot commented May 21, 2026

Uh oh!

auto-submit Bot commented May 21, 2026

Uh oh!

auto-submit Bot commented May 21, 2026

Uh oh!

fluttergithubbot commented May 21, 2026

Uh oh!

auto-submit Bot commented May 21, 2026

Uh oh!

gaaclarke commented May 21, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaaclarke commented May 19, 2026 •

edited

Loading