fix: improve memory leak#344
Merged
Merged
Conversation
✅ Deploy Preview for hoppdocs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough
ChangesGraphics ownership and window lifecycle refactor
Sequence Diagram(s)sequenceDiagram
participant EventLoop
participant Application
participant WindowManager
participant GraphicsContext
participant RemoteControl
rect rgba(70, 130, 180, 0.5)
Note over EventLoop,RemoteControl: Redraw path (new flow)
EventLoop->>Application: WindowEvent::RedrawRequested
Application->>WindowManager: active_gfx_mut()
WindowManager-->>Application: &mut GraphicsContext
Application->>RemoteControl: render_frame(&mut GraphicsContext)
RemoteControl->>GraphicsContext: update cursors / auto-clear / draw
end
rect rgba(60, 179, 113, 0.5)
Note over EventLoop,GraphicsContext: Resize path
EventLoop->>Application: WindowEvent::Resized(new_size)
Application->>WindowManager: resize_active_window(new_size)
WindowManager->>GraphicsContext: resize(new_size)
GraphicsContext->>GraphicsContext: reconfigure wgpu surface + request_redraw
end
rect rgba(210, 105, 30, 0.5)
Note over Application,GraphicsContext: CallEnd GPU reset
Application->>WindowManager: reset_engines(&mut context_manager)
WindowManager->>GraphicsContext: surface_format()
WindowManager->>GraphicsContext: reset_renderer(context_manager)
GraphicsContext->>GraphicsContext: reset engine + recreate IcedRenderer
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
iparaskev
force-pushed
the
do_not_recreate_windows
branch
from
3b57711 to
dde4f12
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
core/src/window_manager.rs (1)
70-74: ⚡ Quick winRename
gfxto a descriptive identifier.
gfxis an abbreviation in a core type boundary (WindowEntry), which hurts readability across this file’s call sites.Proposed refactor
-struct WindowEntry<'a> { +struct WindowEntry<'a> { window: Arc<Window>, monitor_id: MonitorId, - gfx: GraphicsContext<'a>, + graphics_context: GraphicsContext<'a>, }As per coding guidelines, "Use descriptive variable names; avoid single letters or abbreviations like
ssfor screen_sharing".🤖 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 `@core/src/window_manager.rs` around lines 70 - 74, The field `gfx` in the `WindowEntry` struct uses an abbreviation instead of a descriptive name, which reduces readability across the codebase. Rename the `gfx` field in the `WindowEntry` struct to `graphics_context` to match the field's type `GraphicsContext<'a>` and follow the coding guideline of using descriptive variable names instead of abbreviations. Update all references to this field throughout the file to use the new name.Source: Coding guidelines
core/src/lib.rs (1)
536-545: ⚡ Quick winRename
ssto a descriptive screen-sharing flag.
ssis hard to scan in this lifecycle helper and matches the abbreviation called out by the Rust guideline. As per coding guidelines, use descriptive variable names; avoid single letters or abbreviations likessfor screen_sharing.Proposed fix
- let ss = self.screensharing_active; + let screen_sharing_active = self.screensharing_active; if let Some(cam) = &mut self.camera_window { if !cam.is_visible() { - cam.show(mic, ss); + cam.show(mic, screen_sharing_active); } return; }Ok(mut cam) => { - cam.set_screensharing_active(ss); + cam.set_screensharing_active(screen_sharing_active); self.camera_window = Some(cam); }🤖 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 `@core/src/lib.rs` around lines 536 - 545, Rename the abbreviated variable `ss` to a more descriptive name that reflects its purpose as a screen-sharing status flag. Replace `ss` with a descriptive variable name like `screen_sharing_active` in both the assignment where it is set from `self.screensharing_active` and in the `cam.show(mic, ss)` method call to improve code readability and comply with naming guidelines that discourage abbreviations.Source: Coding guidelines
🤖 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 `@core/src/lib.rs`:
- Around line 542-546: The camera window reuse logic in this block does not
refresh the participants source when reusing a hidden camera window. When the
camera window exists but is not visible, before calling cam.show(mic, ss), you
need to check if the participants Arc passed to this function differs from the
one stored in the camera window. If the Arc differs (indicating a different room
or service), either update the participants in the existing camera window or
recreate it entirely. This ensures that when the camera window becomes visible
again, it renders current participants from the correct room/service rather than
stale participants from a previous session.
- Around line 332-335: The Result returned by the wm.update method call is being
ignored with let _, which masks failures in updating the window manager and can
lead to stale state being used in the screenshare path. Instead of discarding
the Result with let _, properly handle the Result by checking for errors and
either propagating the error up to the caller or explicitly handling it before
proceeding with the rest of the logic. This ensures that any window manager
update failures are surfaced rather than silently ignored.
- Around line 2153-2167: The RedrawRequested event handler in the WindowEvent
match block processes redraws for any window without validating that the event's
window_id matches the active window, causing incorrect renders and path updates
from non-active windows. Add a window_id guard check before calling
active_gfx_mut(), similar to the pattern used for the resize guard, to ensure
redraws are only routed to the active overlay window.
- Around line 1644-1656: The screensharing_window.focus_window() call is
executed unconditionally even when update_window_with_new_sharer fails to
execute due to missing buffer or redraw channels. Move the focus_window() call
inside the condition that checks for valid buffer and redraw channel pair so it
only executes after a successful window update. Additionally, implement fallback
channel creation for the redraw channel pair similar to the new-window path to
handle missing redraw_tx, and add logging or early return when buffer is
unavailable rather than silently proceeding without updating the window content.
In `@core/src/window_manager.rs`:
- Around line 237-240: The add_participant call in show_window for the "local"
participant is failing when show_window is called multiple times without a prior
clear, because AlreadyExists error is being converted to WindowCreationError.
Make this operation idempotent by modifying the error handling on the
add_participant call to specifically check for and ignore the AlreadyExists
error case, while only converting other actual errors to WindowCreationError.
This allows show_window to be called safely multiple times without requiring an
explicit clear first.
In `@core/src/window/camera_window.rs`:
- Around line 401-440: The visibility state is not centralized because
focus_window() can be called directly to reveal the winit window without
updating self.visible or sending RedrawCommand::Resume. This creates an
inconsistent state where the window can be visible but is_visible() returns
false and redraws remain paused. Either route all window reveal operations
(visibility changes) through the show() method to ensure self.visible is set and
Resume is sent, or modify focus_window() to only focus the window without
affecting its visibility state, ensuring all visibility state changes flow
exclusively through show() and hide().
In `@core/src/window/drawing_window.rs`:
- Around line 322-330: The show() method in DrawingWindow only resets the cache
but does not clear transient input and path state, causing old state like
left_mouse_pressed and stale drawing paths to persist into the next window
reuse. Reset all relevant transient state fields (such as left_mouse_pressed,
current drawing paths, and other input-related state) along with the cache reset
in the show() method, and trigger an initial redraw to ensure a completely clean
state when the window becomes visible.
---
Nitpick comments:
In `@core/src/lib.rs`:
- Around line 536-545: Rename the abbreviated variable `ss` to a more
descriptive name that reflects its purpose as a screen-sharing status flag.
Replace `ss` with a descriptive variable name like `screen_sharing_active` in
both the assignment where it is set from `self.screensharing_active` and in the
`cam.show(mic, ss)` method call to improve code readability and comply with
naming guidelines that discourage abbreviations.
In `@core/src/window_manager.rs`:
- Around line 70-74: The field `gfx` in the `WindowEntry` struct uses an
abbreviation instead of a descriptive name, which reduces readability across the
codebase. Rename the `gfx` field in the `WindowEntry` struct to
`graphics_context` to match the field's type `GraphicsContext<'a>` and follow
the coding guideline of using descriptive variable names instead of
abbreviations. Update all references to this field throughout the file to use
the new name.
🪄 Autofix (Beta)
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
Run ID: 1e2ae7b3-a358-4c95-a584-f92bc4f7295a
📒 Files selected for processing (9)
core/src/graphics/graphics_context.rscore/src/graphics/graphics_window_context.rscore/src/graphics/iced_renderer.rscore/src/graphics/participant.rscore/src/lib.rscore/src/window/camera_window.rscore/src/window/drawing_window.rscore/src/window/screensharing_window.rscore/src/window_manager.rs
Comment on lines
1644
to
1656
| if let Some(screensharing_window) = &mut self.screensharing_window { | ||
| let redraw_rx = redraw_rx.and_then(|arc| arc.lock().ok()?.take()); | ||
| if let Some((rx, tx)) = redraw_rx.zip(redraw_tx) { | ||
| screensharing_window.update_window_with_new_sharer(&participants, rx, tx); | ||
| if let (Some((rx, tx)), Some(buffer)) = (redraw_rx.zip(redraw_tx), buffer) { | ||
| screensharing_window.update_window_with_new_sharer( | ||
| buffer, | ||
| &participants, | ||
| draw_persist, | ||
| last_mode, | ||
| rx, | ||
| tx, | ||
| ); | ||
| } | ||
| screensharing_window.focus_window(); |
There was a problem hiding this comment.
Don’t focus stale screen-share content when reuse inputs are missing.
In the reuse branch, failing to get buffer or the redraw channel pair skips update_window_with_new_sharer, but still focuses the existing window and later marks screensharing active. Use the same fallback channel creation as the new-window path, and return/log when the buffer is unavailable.
Proposed fix
if let Some(screensharing_window) = &mut self.screensharing_window {
let redraw_rx = redraw_rx.and_then(|arc| arc.lock().ok()?.take());
- if let (Some((rx, tx)), Some(buffer)) = (redraw_rx.zip(redraw_tx), buffer) {
- screensharing_window.update_window_with_new_sharer(
- buffer,
- &participants,
- draw_persist,
- last_mode,
- rx,
- tx,
- );
- }
+ let Some(buffer) = buffer else {
+ log::warn!("user_event: No screen share buffer available");
+ return;
+ };
+ let (rx, tx) = redraw_rx.zip(redraw_tx).unwrap_or_else(|| {
+ let (tx, rx) = std::sync::mpsc::channel::<
+ window::screensharing_window::RedrawCommand,
+ >();
+ (rx, tx)
+ });
+ screensharing_window.update_window_with_new_sharer(
+ buffer,
+ &participants,
+ draw_persist,
+ last_mode,
+ rx,
+ tx,
+ );
screensharing_window.focus_window();
} else {🤖 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 `@core/src/lib.rs` around lines 1644 - 1656, The
screensharing_window.focus_window() call is executed unconditionally even when
update_window_with_new_sharer fails to execute due to missing buffer or redraw
channels. Move the focus_window() call inside the condition that checks for
valid buffer and redraw channel pair so it only executes after a successful
window update. Additionally, implement fallback channel creation for the redraw
channel pair similar to the new-window path to handle missing redraw_tx, and add
logging or early return when buffer is unavailable rather than silently
proceeding without updating the window content.
Comment on lines
+237
to
+240
| entry | ||
| .gfx | ||
| .add_participant("local".to_string(), "Me ", true) | ||
| .map_err(|_| WindowManagerError::WindowCreationError)?; |
There was a problem hiding this comment.
Make local participant initialization in show_window idempotent.
show_window always tries to add "local". If this entry is shown again without a prior clear(), add_participant returns AlreadyExists and the whole show flow fails with WindowCreationError.
Proposed fix
- entry
- .gfx
- .add_participant("local".to_string(), "Me ", true)
- .map_err(|_| WindowManagerError::WindowCreationError)?;
+ if let Err(err) = entry
+ .gfx
+ .add_participant("local".to_string(), "Me ", true)
+ {
+ if !matches!(
+ err,
+ crate::graphics::participant::ParticipantError::AlreadyExists(_)
+ ) {
+ return Err(WindowManagerError::WindowCreationError);
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| entry | |
| .gfx | |
| .add_participant("local".to_string(), "Me ", true) | |
| .map_err(|_| WindowManagerError::WindowCreationError)?; | |
| if let Err(err) = entry | |
| .gfx | |
| .add_participant("local".to_string(), "Me ", true) | |
| { | |
| if !matches!( | |
| err, | |
| crate::graphics::participant::ParticipantError::AlreadyExists(_) | |
| ) { | |
| return Err(WindowManagerError::WindowCreationError); | |
| } | |
| } |
🤖 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 `@core/src/window_manager.rs` around lines 237 - 240, The add_participant call
in show_window for the "local" participant is failing when show_window is called
multiple times without a prior clear, because AlreadyExists error is being
converted to WindowCreationError. Make this operation idempotent by modifying
the error handling on the add_participant call to specifically check for and
ignore the AlreadyExists error case, while only converting other actual errors
to WindowCreationError. This allows show_window to be called safely multiple
times without requiring an explicit clear first.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/window/screensharing_window.rs (1)
857-861: 💤 Low valueConsider resetting
click_animation_rendererstate when switching sharers.The method resets
last_rendered_frame_idand other window-level state, butclick_animation_renderermay retain an active click animation from the previous sharer session. If animations are still in progress when switching, they could briefly appear on the new sharer's stream.// Window-level state. self.local_participant_in_control = false; self.programmatic_resize_target = None; self.mouse_in_participant_area = false; self.last_rendered_frame_id = 0; +self.click_animation_renderer = ClickAnimationRenderer::new(clock::default_clock());🤖 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 `@core/src/window/screensharing_window.rs` around lines 857 - 861, The click_animation_renderer state is not being reset in the window-level state initialization block, which can cause click animations from a previous sharer to persist into the new sharer's session. Add a reset of click_animation_renderer (clearing any active animation state) in the same section where other window-level state variables like last_rendered_frame_id, local_participant_in_control, programmatic_resize_target, and mouse_in_participant_area are being reset to their initial values.
🤖 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.
Nitpick comments:
In `@core/src/window/screensharing_window.rs`:
- Around line 857-861: The click_animation_renderer state is not being reset in
the window-level state initialization block, which can cause click animations
from a previous sharer to persist into the new sharer's session. Add a reset of
click_animation_renderer (clearing any active animation state) in the same
section where other window-level state variables like last_rendered_frame_id,
local_participant_in_control, programmatic_resize_target, and
mouse_in_participant_area are being reset to their initial values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b34e589-a648-4485-83fa-e2ed821a59dd
📒 Files selected for processing (1)
core/src/window/screensharing_window.rs
iparaskev
force-pushed
the
do_not_recreate_windows
branch
from
ca8b552 to
8434f76
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On every window we close we are leaking 3 IOSurfaces because of gfx-rs/wgpu#9021. This series of patches modifies our logic to reuse every window instead of killing and re creating them. This way we are stopping the leak.
Summary by CodeRabbit
Release Notes
New Features
show/hideand visibility helpers for camera, drawing, and screensharing windows.clear()utility.Refactor
Bug Fixes