Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle closing webContents in BrowserViews #37420

Merged
merged 2 commits into from Mar 1, 2023
Merged

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Feb 27, 2023

Description of Change

Closes #37356.
Closes #37419.
Closes #37378.
Refs #37205.

Take 2.

The combination of #35509 and #35603 meant that if a user called webContents.destroy() on a BrowserView's webContents, the associated DraggableRegionProvider would not be removed from the NativeWindow, as the only codepath for doing so relied on the user removing the entire BrowserView via BrowserWindow.removeBrowserView. The first approach I tried fixed a symptom, but then prevented the destroyed event from emitting if a BrowserView still existed.

This PR undoes that change in favor of this better change, which ensures the DraggableRegionProvider isn't left around for a ghost BrowserView webContents.

Tested with:

Checklist

Release Notes

Notes: Fixed destroyed event not emitted on close for BrowserView.webContents.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. labels Feb 27, 2023
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Feb 27, 2023
@codebytere codebytere force-pushed the handle-bv-destroyed branch 2 times, most recently from 2348cb8 to 91e2353 Compare February 27, 2023 13:09
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any chance we could get a test? esp, window.close() from the web content not actually closing the BrowserView seems like something we could have a regression test for!

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Feb 28, 2023
@codebytere codebytere merged commit 5e25d23 into main Mar 1, 2023
@codebytere codebytere deleted the handle-bv-destroyed branch March 1, 2023 10:35
@release-clerk
Copy link

release-clerk bot commented Mar 1, 2023

Release Notes Persisted

Fixed destroyed event not emitted on close for BrowserView.webContents.

@trop
Copy link
Contributor

trop bot commented Mar 1, 2023

I was unable to backport this PR to "22-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/22-x-y PR should also be added to the "22-x-y" branch. label Mar 1, 2023
@trop
Copy link
Contributor

trop bot commented Mar 1, 2023

I have automatically backported this PR to "24-x-y", please check out #37450

@trop trop bot added the in-flight/24-x-y label Mar 1, 2023
@trop trop bot removed the target/24-x-y PR should also be added to the "24-x-y" branch. label Mar 1, 2023
@trop
Copy link
Contributor

trop bot commented Mar 1, 2023

I have automatically backported this PR to "23-x-y", please check out #37451

@trop trop bot added in-flight/23-x-y merged/24-x-y PR was merged to the "24-x-y" branch merged/23-x-y PR was merged to the "23-x-y" branch. and removed target/23-x-y PR should also be added to the "23-x-y" branch. in-flight/24-x-y labels Mar 1, 2023
@pushkin-
Copy link

When can this be backported to v22?

@wujinli
Copy link
Contributor

wujinli commented Mar 4, 2024

When can this be backported to v22?

As #37205 says, the crash is caused by the NativeWindow::NonClientHitTest call. the bug in "NativeWindow::NonClientHitTest" is introduced in #36230. However, the branch of 22-x-y does not contain this refactor.
In my tests, it didn't crash even with revert #37205.
Note: The test is the same as #37205 introduced.

So, just revert it(#37205) in 22-x-y

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/23-x-y PR was merged to the "23-x-y" branch. merged/24-x-y PR was merged to the "24-x-y" branch semver/patch backwards-compatible bug fixes
Projects
None yet
6 participants