fix: output lifetime - validate raw pushes, defer delete, guard share… - #3
fix: output lifetime - validate raw pushes, defer delete, guard share…#3rustdesk wants to merge 2 commits into
Conversation
…d texture - unregisterTexture deleted D3D11Output on the platform thread while the raster thread could still fetch the surface descriptor or run the release callback, and while Rust's decode thread could still push through the raw output pointer (use-after-free; hung raster thread / black remote view). Pushes now resolve the pointer through a live-object registry and deletion is deferred via UnregisterTexture(id, callback). - EnsureTexture waits briefly while the engine is reading the shared texture instead of overwriting it mid-read (tearing); the single fixed shared handle is kept (flutter/flutter#154716). - New FlutterGpuTextureRendererPluginCApiGetConsumed(output) exposes how many frames the engine actually consumed, for black-output detection/fallback. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 4 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesD3D11 output safety and frame tracking
Estimated code review effort: 3 (Moderate) | ~25 minutes Mergeability Score: 🟠 High · up to Stale producer handles can be accepted after address reuse and send frames to the wrong texture output, producing incorrect or black rendering. The PR is not merge-ready until output identity is validated beyond the raw address. Sequence Diagram(s)sequenceDiagram
participant FlutterEngine
participant PluginCApi
participant D3D11Output
participant LiveOutputRegistry
FlutterEngine->>PluginCApi: SetTexture(output, texture)
PluginCApi->>LiveOutputRegistry: validate output
LiveOutputRegistry->>D3D11Output: set texture
D3D11Output-->>PluginCApi: complete texture update
FlutterEngine->>PluginCApi: GetConsumed(output)
PluginCApi->>LiveOutputRegistry: validate output
LiveOutputRegistry->>D3D11Output: read consumed count
D3D11Output-->>PluginCApi: return consumed count
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses Windows texture output lifetime and synchronization issues in the Flutter GPU texture renderer plugin, aiming to prevent use-after-free during unregister, reduce tearing when updating shared textures, and expose engine-consumption metrics for black-output detection/fallback.
Changes:
- Defers
D3D11Outputdeletion until Flutter engine unregister completes (viaUnregisterTexture(id, callback)with captured ownership). - Adds a live-object registry to validate raw pushes coming from a decode thread before dereferencing
D3D11Output*. - Exposes a new C-API
FlutterGpuTextureRendererPluginCApiGetConsumed(output)to report how many frames the engine has consumed.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| windows/include/flutter_gpu_texture_renderer/flutter_gpu_texture_renderer_plugin_c_api.h | Adds stdint.h include and exports a new consumed-frames C API. |
| windows/flutter_gpu_texture_renderer_plugin.cpp | Changes unregister flow to defer deletion until engine-side unregister callback. |
| windows/flutter_gpu_texture_renderer_plugin_c_api.cpp | Routes pushes through validated helpers; adds consumed-frames getter. |
| windows/d3d11_output.h | Adds consumed counter, makes rendering_ atomic, declares validated helper functions. |
| windows/d3d11_output.cpp | Implements live-object registry, consumed counter increment, and validated push/consume helpers; adds brief wait to reduce tearing. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| bool D3D11OutputSetTexture(void *output, void *texture) { | ||
| std::shared_lock<std::shared_mutex> lock(g_live_mutex); | ||
| if (g_live_outputs.find(output) == g_live_outputs.end()) | ||
| return false; | ||
| return static_cast<D3D11Output *>(output)->SetTexture(texture); |
| D3D11Output::~D3D11Output() { | ||
| if (texture_id_) | ||
| texture_registrar_->UnregisterTexture(texture_id_); | ||
| // Unregistration happens in the plugin's unregisterTexture; here only make | ||
| // sure no push is still running on this object and no later push reaches it. | ||
| std::unique_lock<std::shared_mutex> lock(g_live_mutex); | ||
| g_live_outputs.erase(this); | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@windows/d3d11_output.cpp`:
- Around line 31-32: Replace the address-only entries managed by g_live_mutex
and g_live_outputs with non-reusable opaque handle tokens or generation-tagged
handles, and associate each token with its specific output instance. Update the
producer-handle validation and lookup flow around the g_live_outputs.find check
at lines 169–180 to reject handles from destroyed outputs, even when memory
addresses are reused, while preserving valid current-output frame delivery.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e2a2f0fc-2ded-4864-b6f2-0b0ca0dd2553
📒 Files selected for processing (5)
windows/d3d11_output.cppwindows/d3d11_output.hwindows/flutter_gpu_texture_renderer_plugin.cppwindows/flutter_gpu_texture_renderer_plugin_c_api.cppwindows/include/flutter_gpu_texture_renderer/flutter_gpu_texture_renderer_plugin_c_api.h
| std::shared_mutex g_live_mutex; | ||
| std::unordered_set<void *> g_live_outputs; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Prevent stale handle aliasing.
Line 32 stores only an address. If an old output is destroyed and a new output reuses that address, g_live_outputs.find at Line 171 accepts a stale producer handle and sends its frame to the new output.
Use an opaque handle with a generation value, or retain non-reusable handle tokens until the producer releases them. The registry must validate output identity, not only the current address.
Also applies to: 169-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@windows/d3d11_output.cpp` around lines 31 - 32, Replace the address-only
entries managed by g_live_mutex and g_live_outputs with non-reusable opaque
handle tokens or generation-tagged handles, and associate each token with its
specific output instance. Update the producer-handle validation and lookup flow
around the g_live_outputs.find check at lines 169–180 to reject handles from
destroyed outputs, even when memory addresses are reused, while preserving valid
current-output frame delivery.
- set rendering_ only when a real surface descriptor is handed out: the engine bails before the release callback on a null handle, which left the flag stuck true and cost the full wait on every push until the first successful populate - document GetConsumed honestly: it counts descriptor fetches and an EGL bind failure still advances it, so 0 means never composited, not rendered correctly Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…d texture
handleofFlutterDesktopGpuSurfaceDescriptorflutter/flutter#154716).Summary by CodeRabbit
New Features
Bug Fixes