[Bug]: View.addChildView can be used to add the same view multiple times, causing crash #42077
Closed
3 tasks done
Labels
30-x-y
31-x-y
bug 🪲
component/WebContentsView
has-repro-gist
Issue can be reproduced with code at https://gist.github.com/
platform/all
status/confirmed
A maintainer reproduced the bug or agreed with the feature
Preflight Checklist
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 inView.addChildView
. An Electron fiddle is attached, but the minimal repro is:I know that it's a little weird to call
addChildView
on a view that is already attached, but withsetTopBrowserView
deprecated this is the only way I see to reorderWebContentsViews
(since you can pass an index as the second parameter toaddChildView
to reorder views). Without this you have to first callremoveChildView
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 callingaddChildView
on a child view that is already attached would prevent this crash and give us a primitive that is at least as powerful assetTopBrowserView
(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 inView::child_views_
here (note that we already check if the child view is attached inView::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'sView::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 callremoveChildView
first).Actual Behavior
Entire app crashes when adding a new view with
addChildView
after Electron's internalView::child_views_
holds duplicate entries.Testcase Gist URL
https://gist.github.com/Hans-Halverson/8de9cbacec6253ec3d8de03723b7a914
Additional Information
No response
The text was updated successfully, but these errors were encountered: