electron: [Bug]: crash on browserView.webContents.destroy() call

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 feature request that matches the one I want to file, without success.

Electron Version

13.1.2

What operating system are you using?

Other Linux

Operating System Version

kernel 5.12.8

What arch are you using?

x64

Last Known Working Electron version

No response

Expected Behavior

The app doesn’t crash.
The BrowserView process gets properly destroyed.

Actual Behavior

The app crashing if browserView.webContents.destroy() called to destroy the browserView-related process (memory releasing). It doesn’t crash on first call but on 5-10th browserView.create() => browserView.destroy() sequential scenario execution (in my case create/destroy calls get triggered by hotkey, so I just hit the respective hotkeys in sequence).

Testcase Gist URL

No response

Additional Information

See the attached crash stack.

As pointed in https://github.com/electron/electron/issues/26929#issuecomment-763837953 wrapping the destroy() call into setImmediate prevents the app from crash. So this is a workaround.

About this issue

Original URL
State: closed
Created 3 years ago
Comments: 19 (8 by maintainers)

Most upvoted comments

So will this allow us to call browserView.webContents.destroy() without calling win.setBrowserView(null)?

pushkin- on Jun 24, 2021

Ok, here’s what I think is happening to cause this crash:

The view.webContents.destroy() call runs. This causes BrowserView::WebContentsDestroyed() to run, which sets its api_web_contents_ to nullptr. (ref)
win.setBrowserView(null) is called, which ends up in BaseWindow::ResetBrowserViews(). (ref).
ResetBrowserView checks if (browser_view->web_contents()), and uses browser_view->web_contents()->owner_window() to check that the BrowserView is still in the same window that it was added to, and hasn’t been reparented. (ref)
HOWEVER, by this point, browser_view->web_contents() is nullptr! So it can’t check the owner_window of the WebContents, so it does not call BaseWindow::RemoveBrowserView on the browser view. It does, however, remove the v8::Global that keeps the BrowserView object alive, freeing it to be garbage-collected by V8. (ref)
This results in the BrowserView pointer sticking around in the NativeWindow::browser_views() list.
Later, when a mouse event is delivered, NativeWindowViews::ShouldDescendIntoChildForEventHandling iterates the browser_views() list to check draggable regions. This is a Use-After-Free, because by this point the BrowserView could have been deleted by V8’s GC. (ref)

I think the solution here has to involve BrowserView knowing which window it’s owned by, rather than using WebContents to indirect that knowledge.

nornagon on Jun 23, 2021