[Bug]: View.addChildView can be used to add the same view multiple times, causing crash
Hans-Halverson opened this issue · 0 comments
Preflight Checklist
- I have read the Contributing Guidelines for this project.
- I agree to follow the Code of Conduct that this project adheres to.
- I have searched the issue tracker for a bug report that matches the one I want to file, without success.
Electron Version
30.0.2
What operating system are you using?
macOS
Operating System Version
macOS 14.4.1
What arch are you using?
arm64 (including Apple Silicon)
Last Known Working Electron version
No response
Expected Behavior
When upgrading to Electron 30 with the new WebContentsView API I noticed crashes occurring in View.addChildView. An Electron fiddle is attached, but the minimal repro is:
parent.addChildView(view1);
parent.addChildView(view1); // no crash
parent.addChildView(view2); // crash
I know that it's a little weird to call addChildView on a view that is already attached, but with setTopBrowserView deprecated this is the only way I see to reorder WebContentsViews (since you can pass an index as the second parameter to addChildView to reorder views). Without this you have to first call removeChildView then re-add the view, but that causes the child view to become hidden for a moment which breaks content you may be trying to interact with. Allowing reordering by calling addChildView on a child view that is already attached would prevent this crash and give us a primitive that is at least as powerful as setTopBrowserView (as requested in this issue as well: #42061).
From poking around it looks like Chromium already supports reordering a child view that is already attached using View::AddChildViewAt here. But it looks like Electron is missing a check if the view is already in View::child_views_ here (note that we already check if the child view is attached in View::RemoveChildView here).
I'm pretty sure that without this check we end up adding the child multiple times to View::child_views_, which then causes us to pass an out of bounds index to Chromium's View::AddChildViewAt, which is not supported given the DCHECK here.
I'm hoping that Electron will be able to make this relatively small change to prevent crashes in addChildView while allowing for this convenient view re-ordering behavior (without having to call removeChildView first).
Actual Behavior
Entire app crashes when adding a new view with addChildView after Electron's internal View::child_views_ holds duplicate entries.
Testcase Gist URL
https://gist.github.com/Hans-Halverson/8de9cbacec6253ec3d8de03723b7a914
Additional Information
No response