fix(electron-ozone): NativeWindowViews leak on rohtmlwidget cycle OS-21158#140
fix(electron-ozone): NativeWindowViews leak on rohtmlwidget cycle OS-21158#140gostoshbs wants to merge 1 commit into
Conversation
…21158 Continuously creating and destroying Electron BrowserWindows in a loop causes the Electron main process's memory consumption to grow monotonically. Part of this is caused by NativeWindowViews::SetContentView, which when replacing a content view detaches the child from the view hierarchy but does not delete it. Under the views ownership model, child views are implicitly owned by their parent, so the detached subtree, including the WebContentsView and associated widget plumbing, becomes unreachable and leaks. Switch to RemoveChildViewT, which transfers ownership back to the caller as a unique_ptr that is immediately dropped to destroy the subtree. The existing RemoveChildView path is retained for views with owned_by_client() set, since the framework must not delete those (see crbug/40115694). Future upstream merges may address this leak differently. This change should be re-evaluated when Electron/Chromium is upgraded.
gostoshbs
assigned
caneraltinbasak and
t-bashir-bs and unassigned
caneraltinbasak and
t-bashir-bs
| root_view_.RemoveChildView(content_view()); | ||
| auto* cv = content_view(); | ||
| if (cv) { | ||
| set_content_view(nullptr); |
There was a problem hiding this comment.
Do we need this as set_content_view will always be called below and all it does is set content_view_?
There was a problem hiding this comment.
This defends against use after free when RemoveChildViewT path triggers
| auto* cv = content_view(); | ||
| if (cv) { | ||
| set_content_view(nullptr); | ||
| focused_view_ = nullptr; |
There was a problem hiding this comment.
Do we need this as focused_view_ will always be set below?
There was a problem hiding this comment.
Pull request overview
Fixes a memory leak in the Views/Ozone windowing path where repeatedly replacing a NativeWindowViews content view could leave the old View subtree detached but not destroyed, causing main-process memory growth during BrowserWindow create/destroy cycles.
Changes:
- Updates
NativeWindowViews::SetContentViewto remove the previous content view usingRemoveChildViewT()when the view is parent-owned so the detached subtree is destroyed. - Preserves the legacy
RemoveChildView()path forowned_by_client()views to avoid deleting client-owned views (per the referenced Chromium behavior).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!cv->owned_by_client()) { | ||
| root_view_.RemoveChildViewT(cv); | ||
| } else { | ||
| root_view_.RemoveChildView(cv); | ||
| } |
There was a problem hiding this comment.
@gostoshbs
We are almost at the tip of the tree on Electron and haven't been addressed upstream.
Can you please add a test case and send this upstream? That would avoid same issue coming back again or having merge issues.
caneraltinbasak
left a comment
caneraltinbasak
left a comment
There was a problem hiding this comment.
Can you please format the patch with upstream requirements and send there as well as our fork? It would also be good to have a test case.
Continuously creating and destroying Electron BrowserWindows in a loop causes the Electron main process's memory consumption to grow monotonically. Part of this is caused by
NativeWindowViews::SetContentView, which when replacing a content view detaches the child from the view hierarchy but does not delete it. Under the views ownership model, child views are implicitly owned by their parent, so the detached subtree, including the WebContentsView and associated widget plumbing, becomes unreachable and leaks. Switch to RemoveChildViewT, which transfers ownership back to the caller as a unique_ptr that is immediately dropped to destroy the subtree. The existing RemoveChildView path is retained for views with owned_by_client() set, since the framework must not delete those (see crbug/40115694).
Future upstream merges may address this leak differently. This change should be re-evaluated when Electron/Chromium is upgraded.