Skip to content

[engine] Replace fml::Status and fml::StatusOr with absl - #194161

Merged
auto-submit[bot] merged 3 commits into
flutter:masterfrom
pleiondev:gpu-fix/fml-status-owns-message
Oct 22, 2026
Merged

auto-submit[bot] merged 3 commits into
flutter:masterfrom
pleiondev:gpu-fix/fml-status-owns-message

Conversation

@dzolotov

@dzolotov dzolotov commented Oct 11, 2026 •

Copy link
Copy Markdown
Contributor

This started as a fix for fml::Status holding its message as a std::string_view: CommandQueueVK::Submit built a status from a local string inside the fence callback and returned it after the string was gone. In review the ask was to use absl::Status instead, so this PR removes fml::Status and fml::StatusOr and switches every user to absl::Status and absl::StatusOr. absl::Status owns its message, so the dangling view goes away with the type, and so does the TODO in status_or.h that pointed at #134741.

Most of it is mechanical. fml::Status() becomes absl::OkStatus(), the status codes have the same names in both, includes point at third_party/abseil-cpp/absl/status, and //flutter/flow gets abseil status in public_deps because frame_timings.h returns it.

The part that isn't mechanical comes from absl::Status being [[nodiscard]]. fml::Status wasn't, and about 25 call sites dropped a status without looking at it. I didn't want this PR to change what any of them do, so in production code and in the render and playground tests they now call .IgnoreError(). That includes DescriptorPoolVK::AllocateDescriptorSets, which ignores a failed CreateNewPool today. Whether it should return that error is a separate question. The fence waiter, descriptor pool and command encoder tests check the result instead, since there the expected status is known.

One exception. CopyImageByteData in lib/ui/painting/image_encoding.cc built an error for a failed swizzle surface, never returned it, and went on to use the null surface. The compiler only caught it because of [[nodiscard]], and I added the missing return. If you'd rather have that as its own PR I'll split it out.

Where it was verified. macOS arm64, host_debug_unopt_arm64, rebased on current main, gn check clean. fml_unittests 203/203, ui_unittests 73/73, shell_unittests 197/197, flow_unittests 313/313 and the Vulkan, fence waiter, descriptor pool, command queue, Gaussian blur and text frame tests in impeller_unittests (193/193). impeller_golden_tests and flutter_tester build. Locally PerformanceOverlayLayerDefault.Gold and two EmbedderTest.CanRenderTextWithImpeller*Metal tests fail on missing glyphs and ICU data (U_MISSING_RESOURCE_ERROR). They fail the same way on main without this change, so it's my checkout. CI covers the other platforms.

Fixes #194151

Pre-launch Checklist

@gaaclarke

Copy link
Copy Markdown
Member

Let's replace usages of fml::Status with absl::Status instead.

@gaaclarke gaaclarke closed this Oct 12, 2026
@gaaclarke gaaclarke reopened this Oct 12, 2026
@fluttergithubbot

Copy link
Copy Markdown
Contributor

An existing Git SHA, 6591a9662c592a18aeb134173789d95e7db274b3, 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.

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

oops meant to "request changes", not close the issue. we want absl::Status

fml::Status stored the message as a std::string_view, so a status built
from a runtime string pointed into that string's buffer. CommandQueueVK
returns such a status from a lambda whose local string is gone by the
time the caller reads message(). The message is now a std::string;
message() still returns a std::string_view, so callers do not change.
Review asked to use absl::Status instead of fixing fml::Status. absl::Status
owns its message, so the dangling view in CommandQueueVK::Submit goes away
with the type. fml/status.h, fml/status_or.h and the status test are removed.

absl::Status is [[nodiscard]], so call sites that dropped a status now say
so with IgnoreError(), or check it in tests. CopyImageByteData built
an error for a failed swizzle surface without returning it and then used
the null surface; it now returns the error.
@dzolotov
dzolotov force-pushed the gpu-fix/fml-status-owns-message branch from 6591a96 to c75037a Compare October 17, 2026 10:00
@dzolotov dzolotov changed the title [fml] Status keeps its own copy of the message [engine] Replace fml::Status and fml::StatusOr with absl Oct 17, 2026
@dzolotov

Copy link
Copy Markdown
Contributor Author

@gaaclarke Done: fml::Status and fml::StatusOr are gone and everything uses absl now. Since absl::Status is [[nodiscard]], the compiler turned up a missing return in CopyImageByteData (it built an error and then used the null surface). I kept that fix here, but I can split it into its own PR if you prefer. Everything else that used to drop a status silently now calls IgnoreError(), so behavior should be unchanged. Details are in the description.

@github-actions github-actions Bot added a: text input Entering text in a text field or keyboard related problems e: impeller Impeller rendering backend issues and features requests flutter-gpu team-fluttergpu Owned by Flutter GPU team labels Oct 17, 2026
@github-project-automation github-project-automation Bot moved this to 🤔 Needs Triage in Flutter GPU Oct 17, 2026
@dzolotov
dzolotov requested a review from gaaclarke October 17, 2026 10:03
@gaaclarke gaaclarke added the CICD Run CI/CD label Oct 19, 2026
…s-owns-message

# Conflicts:
#	engine/src/flutter/lib/gpu/command_buffer_unittests.cc
@flutter-dashboard flutter-dashboard Bot removed the CICD Run CI/CD label Oct 21, 2026
@gaaclarke gaaclarke added the CICD Run CI/CD label Oct 21, 2026

@gaaclarke gaaclarke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, i was sort of waiting for this to be lifted out of draft =T

@gaaclarke
gaaclarke requested a review from andywolff October 21, 2026 16:07
@dzolotov
dzolotov marked this pull request as ready for review October 21, 2026 21:58

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request replaces the custom fml::Status and fml::StatusOr classes with absl::Status and absl::StatusOr from the Abseil library across the repository, resulting in the deletion of the FML status headers. The review feedback highlights two instances in descriptor_pool_vk.cc where the status returned by CreateNewPool is ignored, which could lead to undefined behavior or crashes if pool allocation fails.

@gaaclarke gaaclarke added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 21, 2026
@auto-submit
auto-submit Bot added this pull request to the merge queue Oct 21, 2026
Merged via the queue into flutter:master with commit 40f7050 Oct 22, 2026
31 checks passed
@flutter-dashboard flutter-dashboard Bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Oct 22, 2026
@github-project-automation github-project-automation Bot moved this from 🤔 Needs Triage to ✅ Done in Flutter GPU Oct 22, 2026
@dzolotov
dzolotov deleted the gpu-fix/fml-status-owns-message branch October 22, 2026 00:31
bdero added a commit to bdero/flutter that referenced this pull request Oct 25, 2026
Master replaced fml::Status with absl (flutter#194161).
suryaKommana2662 pushed a commit to suryaKommana2662/flutter that referenced this pull request Oct 29, 2026
)

This started as a fix for `fml::Status` holding its message as a
`std::string_view`: `CommandQueueVK::Submit` built a status from a local
string inside the fence callback and returned it after the string was
gone. In review the ask was to use `absl::Status` instead, so this PR
removes `fml::Status` and `fml::StatusOr` and switches every user to
`absl::Status` and `absl::StatusOr`. `absl::Status` owns its message, so
the dangling view goes away with the type, and so does the TODO in
`status_or.h` that pointed at flutter#134741.

Most of it is mechanical. `fml::Status()` becomes `absl::OkStatus()`,
the status codes have the same names in both, includes point at
`third_party/abseil-cpp/absl/status`, and `//flutter/flow` gets abseil
status in `public_deps` because `frame_timings.h` returns it.

The part that isn't mechanical comes from `absl::Status` being
`[[nodiscard]]`. `fml::Status` wasn't, and about 25 call sites dropped a
status without looking at it. I didn't want this PR to change what any
of them do, so in production code and in the render and playground tests
they now call `.IgnoreError()`. That includes
`DescriptorPoolVK::AllocateDescriptorSets`, which ignores a failed
`CreateNewPool` today. Whether it should return that error is a separate
question. The fence waiter, descriptor pool and command encoder tests
check the result instead, since there the expected status is known.

One exception. `CopyImageByteData` in
`lib/ui/painting/image_encoding.cc` built an error for a failed swizzle
surface, never returned it, and went on to use the null surface. The
compiler only caught it because of `[[nodiscard]]`, and I added the
missing `return`. If you'd rather have that as its own PR I'll split it
out.

**Where it was verified.** macOS arm64, `host_debug_unopt_arm64`,
rebased on current main, `gn check` clean. `fml_unittests` 203/203,
`ui_unittests` 73/73, `shell_unittests` 197/197, `flow_unittests`
313/313 and the Vulkan, fence waiter, descriptor pool, command queue,
Gaussian blur and text frame tests in `impeller_unittests` (193/193).
`impeller_golden_tests` and `flutter_tester` build. Locally
`PerformanceOverlayLayerDefault.Gold` and two
`EmbedderTest.CanRenderTextWithImpeller*Metal` tests fail on missing
glyphs and ICU data (`U_MISSING_RESOURCE_ERROR`). They fail the same way
on main without this change, so it's my checkout. CI covers the other
platforms.

Fixes flutter#194151

## Pre-launch Checklist

- [x] I read the [Contributor Guide] and followed the process outlined
there for submitting PRs.
- [x] I read the [AI contribution guidelines] and understand my
responsibilities, or I am not using AI tools.
- [x] I read the [Tree Hygiene] wiki page, which explains my
responsibilities.
- [x] I read and followed the [Flutter Style Guide], including [Features
we expect every widget to implement].
- [x] I signed the [CLA].
- [x] I listed at least one issue that this PR fixes in the description
above.
- [x] I updated/added relevant in-code documentation (doc comments with
`///`).
- [x] If this PR introduces a new feature or capability, I created and
linked a website documentation issue or PR in [flutter/website] (or
verified none is needed).
- [x] I added new tests to check the change I am making, or this PR is
[test-exempt].
- [x] I followed the [breaking change policy] and added [Data Driven
Fixes] where supported.
- [x] All existing and new tests are passing.

<!-- Links -->
[Contributor Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview
[AI contribution guidelines]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#ai-contribution-guidelines
[Tree Hygiene]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md
[test-exempt]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests
[Flutter Style Guide]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md
[Features we expect every widget to implement]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement
[CLA]: https://cla.developers.google.com/
[flutter/website]: https://github.com/flutter/website
[breaking change policy]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes
[Data Driven Fixes]:
https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

a: text input Entering text in a text field or keyboard related problems CICD Run CI/CD e: impeller Impeller rendering backend issues and features requests engine flutter/engine related. See also e: labels. flutter-gpu team-fluttergpu Owned by Flutter GPU team

Projects

Status: ✅ Done

Development

Successfully merging this pull request may close these issues.

[fml] fml::Status keeps a std::string_view, so a status built from a runtime string dangles

4 participants