Repository: electron/electron (language: cpp)

Issue:
segfault when moving WebContentsView between BrowserWindows
### Preflight Checklist

- [x] I have read the [Contributing Guidelines](https://github.com/electron/electron/blob/main/CONTRIBUTING.md) for this project.
- [x] I agree to follow the [Code of Conduct](https://github.com/electron/electron/blob/main/CODE_OF_CONDUCT.md) that this project adheres to.
- [x] I have searched the [issue tracker](https://www.github.com/electron/electron/issues) for a bug report that matches the one I want to file, without success.

### Electron Version

33, 34

### What operating system(s) are you using?

Ubuntu

### Operating System Version

23.04

### What arch are you using?

x64

### Last Known Working Electron version

_No response_

### Expected Behavior

When moving WebContentsViews between different browser windows, the app doesn't crash

### Actual Behavior

When moving WebContentsViews from BrowserWindow A to BrowserWindow B and back to BrowserWindow A, the app crashes. Going just from A BrowserWindow A to BrowserWindow B works, but then back to A segfaults.

### Testcase Gist URL

https://gist.github.com/Kilian/8014d47a193a8f66b05f490def934166

### Additional Information

_No response_


Discussion / hints:
@Kilian is there a last known version?

edit: this seems to have been broken since at least e30
.
e29 fails on this line:

```
const contentsView = new WebContentsView();
```
with
```sh
(node:2038244) UnhandledPromiseRejectionWarning: TypeError: Insufficient number of arguments.
    at createWindow (/tmp/electron-fiddle-2030700-MSoL9sQJYrOk/main.js:21:24)
    at /tmp/electron-fiddle-2030700-MSoL9sQJYrOk/main.js:43:3
```
I can't find the webcontentsview docs in the 29 branch, so I don't know if there were any required arguments at that point.

It first started crashing in 30.0.0-beta.8, though in 30.0.0-beta.7 and earlier there is no webcontentsview visible but there is also no error visible like above.

WebContentsView was [reverted in v29](https://github.com/electron/electron/pull/41361), so that is why you cannot find any docs in that branch. If it started crashing in a v30, then it has basically been crashing from the start from the looks of things. 
I hit the same issue and the solution was to remove the view from its current parent and then add it to the new parent.
It complicated our code a bit, but solved the problem.
@estoykov do you have a code example?

I have an different fiddle here that first removes the webcontentsview but it still crashes: https://gist.github.com/Kilian/8f0365a3c962b4d5927f2c21284fb1e6
Well, may be I miss leaded you as our structure is a bit more complex - we need to have more than one view in our containers, so the flow is something like this:
- create BrowserWindow
- create rootView = new View()
- browserWindow.setContentView(rootView)
- create multiple "hosts" which are simply Views, i.e.
- host1 has rootView = new View()
- host2 has rootView = new View()
- put host1 and host2 rootVews in the BrowerWindow content rootView 
- and then we move some other view from host1 and host2 rootView - it crashed before when added to host1, then host2 and then back to host1 - removing from the current host root view before adding to the new one solved the problem

Changing BrowserWindow content view comes with an additional processing - you need to track this content view (View in my case) bounds change and re-pos/re-size the real content (WebContentsView) if any, ditto for the hosts and their content...


Fix patch:
diff --git a/shell/browser/api/electron_api_view.cc b/shell/browser/api/electron_api_view.cc
index b01a08be88cb7..c65f80b164fec 100644
--- a/shell/browser/api/electron_api_view.cc
+++ b/shell/browser/api/electron_api_view.cc
@@ -124,6 +124,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,
@@ -257,11 +266,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);
   }
 }
 
@@ -388,6 +399,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();
diff --git a/shell/browser/api/electron_api_view.h b/shell/browser/api/electron_api_view.h
index 6eeb16521906f..1b0a3f815a6f9 100644
--- a/shell/browser/api/electron_api_view.h
+++ b/shell/browser/api/electron_api_view.h
@@ -47,6 +47,8 @@ class View : public gin_helper::EventEmitter<View>,
   // 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_; }
   std::optional<int> border_radius() const { return border_radius_; }
diff --git a/spec/fixtures/crash-cases/webview-move-between-windows/index.js b/spec/fixtures/crash-cases/webview-move-between-windows/index.js
new file mode 100644
index 0000000000000..de9053ec65ca9
--- /dev/null
+++ b/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();
+});


Test patch: