Repository navigation
[CP-beta][skwasm] Implement Wasm worker futex semaphores - #196127
flutteractionsbot wants to merge 1 commit into
Conversation
Replaces Emscripten's no-op `-sWASM_WORKERS=1` POSIX semaphore stubs (`sem_init`, `sem_destroy`, `sem_post`, `sem_wait`, `sem_trywait`) with a WebAssembly atomic futex implementation in multi-threaded Skwasm and Web Impeller (`wimp`) builds. ### Rationale Under Emscripten's `-sWASM_WORKERS=1` runtime without `-pthread` (`skwasm_heavy` / `wimp_heavy`), `libc-ww.a` compiles `system/lib/pthread/library_pthread_stub.c`, which stubs `sem_init`, `sem_wait`, `sem_trywait`, `sem_post`, and `sem_destroy` (as well as `emscripten_futex_wait` / `emscripten_futex_wake`) to `return 0`. In Skia (`third_party/skia/src/core/SkSemaphore.cpp`), `SkSemaphore` uses POSIX `sem_t` on `__EMSCRIPTEN__` as the contended slow-path backing for `SkMutex` and `SkSharedMutex`. Because `sem_wait` is a no-op, any contended acquisition on FreeType's global `f_t_mutex()` (`third_party/skia/src/ports/SkFontHost_FreeType.cpp:221-226`) or `SkTypeface_FreeType::fC2GCacheMutex` (`third_party/skia/src/ports/SkTypeface_FreeType.h:87`) returns immediately when the browser main UI thread performs `Paragraph::layout` / `MakeTextFrameFromTextBlobSkia` concurrently with the background render worker rasterizing glyphs from the shared `FT_Library` / `FT_Face`, causing memory corruption and worker stalls ([flutter#193695](flutter#193695)). ### Summary of Changes - [`engine/src/flutter/skwasm/wasm_worker_semaphore.cc`](https://github.com/kevmoo/flutter/blob/87e66a27b6b/engine/src/flutter/skwasm/wasm_worker_semaphore.cc) & [`engine/src/flutter/skwasm/BUILD.gn`](https://github.com/kevmoo/flutter/blob/87e66a27b6b/engine/src/flutter/skwasm/BUILD.gn): Override `sem_init`, `sem_destroy`, `sem_post`, `sem_wait`, and `sem_trywait` via linker `-Wl,--wrap=sem_*` in multi-threaded `skwasm_variant` (`#if defined(__EMSCRIPTEN_WASM_WORKERS__)`) builds: - Stores the atomic counter inline in `sem_t`'s 16-byte `__val` buffer (`cache/sysroot/include/semaphore.h:17-19`), requiring zero heap allocation or teardown in `sem_destroy`. - Background `DedicatedWorker` threads (`emscripten_wasm_worker_self_id() != 0`, cached in a `thread_local bool` to avoid a Wasm-to-JS FFI call per contended wait) sleep via `emscripten_atomic_wait_u32` and wake via `emscripten_atomic_notify`. - The browser main thread (`emscripten_wasm_worker_self_id() == 0`, where `memory.atomic.wait32` / `Atomics.wait` is disallowed by the WebAssembly Threads spec because `[[CanBlock]] == false`) uses an atomic compare-exchange spin-acquire loop. ### Verification - Built `wasm_release:skwasm/skwasm_heavy.js` and `wasm_release:skwasm/wimp_heavy.js` and verified `20/20` multi-threaded text-grid runs (`grid@100` and `grid@1000`) complete without `FT_Face` memory corruption or worker stalls in Chrome 154. Fixes flutter#193695 ## 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/tests]: https://github.com/flutter/tests [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 [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
|
@harryterkelsen please fill out the PR description above, afterwards the release team will review this request. |
|
This pull request was opened from and to a release candidate branch. This should only be done as part of the official Flutter release process. If you are attempting to make a regular contribution to the Flutter project, please close this PR and follow the instructions at Tree Hygiene for detailed instructions on contributing to Flutter. Reviewers: Use caution before merging pull requests to release branches. Ensure the proper procedure has been followed. |
There was a problem hiding this comment.
Code Review
This pull request implements POSIX semaphores using Emscripten's Wasm Worker semaphore primitives in a new file wasm_worker_semaphore.cc, updates the build configuration in BUILD.gn to link these overrides, and adds regression tests in semaphore_test.dart and text_test.dart. The review feedback identifies critical issues in wasm_worker_semaphore.cc where the return values of emscripten_semaphore_try_acquire are incorrectly checked, which would cause sem_trywait to falsely report success and the busy-wait loop in sem_wait to terminate prematurely.
This pull request is created by automatic cherry pick workflow
Please fill in the form below, and a flutter domain expert will evaluate this cherry pick request.
Issue Link:
What is the link to the issue this cherry-pick is addressing?
#193695 (also reported in #193439)
Impact Description:
What is the impact (ex. visual jank on Samsung phones, app crash, cannot ship an iOS app)?
Does it impact development (ex. flutter doctor crashes when Android Studio is installed),
or the shipping of production apps (the app crashes on launch).
This information is for domain experts and release engineers to understand the consequences of saying yes or no to the cherry pick.
Production Flutter Web apps compiled with
--wasm(skwasm) and served with cross-origin isolation (Cross-Origin-Opener-Policy/Cross-Origin-Embedder-Policy, enabling multi-threaded rendering) freeze or crash with WebAssembly memory corruption whenever main-thread text layout (Paragraph.layout) runs concurrently with background worker glyph rasterization. Under Emscripten's-sWASM_WORKERS=1runtime, POSIXsem_*functions inlibc-ww.aare no-op stubs returning0, leaving Skia'sSkSemaphoreand FreeType's globalf_t_mutex()unprotected across threads.Changelog Description:
Explain this cherry pick:
See best practices for examples.
[flutter/193695] When rendering dynamic text on Web (
--wasm) with cross-origin isolation enabled, the app may freeze or crash.Workaround:
Is there a workaround for this issue?
Configure
forceSingleThreadedSkwasm: trueinflutter_bootstrap.js(or serve without COOP/COEP headers), which disables multi-threaded WebAssembly rendering and moves all rasterization back onto the browser main thread.Risk:
What is the risk level of this cherry-pick?
Test Coverage:
Are you confident that your fix is well-tested by automated tests?
Validation Steps:
What are the steps to validate that this fix works?
flutter build web --wasm --releasethat performs dynamic text layout while rendering frames.build/webwithCross-Origin-Opener-Policy: same-originandCross-Origin-Embedder-Policy: require-corpsowindow.crossOriginIsolated === true.RuntimeError(covered in CI byengine/src/flutter/lib/web_ui/test/skwasm/semaphore_test.dartandengine/src/flutter/lib/web_ui/test/ui/text_test.dart).