fix: segfault when moving WebContentsView between BrowserWindows (#44614

)

* fix: segfault when moving WebContentsView between BrowserWindows

Co-authored-by: John Kleinschmidt <[email protected]>

* chore: actually enable fix

Co-authored-by: John Kleinschmidt <[email protected]>

* fixup segfault when moving WebContentsView between BrowserWindows

Co-authored-by: John Kleinschmidt <[email protected]>

---------

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: John Kleinschmidt <[email protected]>

shell/browser/api/electron_api_view.cc

@@ -122,6 +122,15 @@ struct Converter<views::FlexAllocationOrder> {
   }
 };
 
+template <>
+struct Converter<electron::api::View> {
+  static bool FromV8(v8::Isolate* isolate,
+                     v8::Local<v8::Value> val,
+                     electron::api::View* out) {
+    return gin::ConvertFromV8(isolate, val, &out);
+  }
+};
+
 template <>
 struct Converter<views::SizeBound> {
   static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
@@ -254,11 +263,13 @@ void View::RemoveChildView(gin::Handle<View> child) {
 #if BUILDFLAG(IS_MAC)
     ScopedCAActionDisabler disable_animations;
 #endif
+    // Remove from child_views first so that OnChildViewRemoved doesn't try to
+    // remove it again
+    child_views_.erase(it);
     // It's possible for the child's view to be invalid here
     // if the child's webContents was closed or destroyed.
     if (child->view())
       view_->RemoveChildView(child->view());
-    child_views_.erase(it);
   }
 }
 
@@ -352,6 +363,19 @@ void View::OnViewIsDeleting(views::View* observed_view) {
   view_ = nullptr;
 }
 
+void View::OnChildViewRemoved(views::View* observed_view, views::View* child) {
+  v8::Isolate* isolate = JavascriptEnvironment::GetIsolate();
+  auto it = std::ranges::find_if(
+      child_views_, [&](const v8::Global<v8::Object>& child_view) {
+        View current_view;
+        gin::ConvertFromV8(isolate, child_view.Get(isolate), &current_view);
+        return current_view.view()->GetID() == child->GetID();
+      });
+  if (it != child_views_.end()) {
+    child_views_.erase(it);
+  }
+}
+
 // static
 gin_helper::WrappableBase* View::New(gin::Arguments* args) {
   View* view = new View();

shell/browser/api/electron_api_view.h

@@ -41,6 +41,8 @@ class View : public gin_helper::EventEmitter<View>, public views::ViewObserver {
   // views::ViewObserver
   void OnViewBoundsChanged(views::View* observed_view) override;
   void OnViewIsDeleting(views::View* observed_view) override;
+  void OnChildViewRemoved(views::View* observed_view,
+                          views::View* child) override;
 
   views::View* view() const { return view_; }
 

spec/fixtures/crash-cases/webview-move-between-windows/index.js

@@ -0,0 +1,31 @@
+const { app, BrowserWindow, WebContentsView } = require('electron');
+
+function createWindow () {
+  // Create the browser window.
+  const mainWindow = new BrowserWindow();
+  const secondaryWindow = new BrowserWindow();
+
+  const contentsView = new WebContentsView();
+  mainWindow.contentView.addChildView(contentsView);
+  mainWindow.webContents.setDevToolsWebContents(contentsView.webContents);
+  mainWindow.openDevTools();
+
+  contentsView.setBounds({
+    x: 400,
+    y: 0,
+    width: 400,
+    height: 600
+  });
+
+  setTimeout(() => {
+    secondaryWindow.contentView.addChildView(contentsView);
+    setTimeout(() => {
+      mainWindow.contentView.addChildView(contentsView);
+      app.quit();
+    }, 1000);
+  }, 1000);
+}
+
+app.whenReady().then(() => {
+  createWindow();
+});