Skip to content

fix: handle closing webContents in BrowserViews#37420

Merged
codebytere merged 2 commits intomainfrom
handle-bv-destroyed
Mar 1, 2023
Merged

fix: handle closing webContents in BrowserViews#37420
codebytere merged 2 commits intomainfrom
handle-bv-destroyed

Conversation

@codebytere
Copy link
Member

@codebytere codebytere commented Feb 27, 2023 •
edited
Loading

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 recently label Feb 27, 2023
@codebytere codebytere requested a review from zcbenz February 27, 2023 11:32
@codebytere codebytere force-pushed the handle-bv-destroyed branch 2 times, most recently from 2348cb8 to 91e2353 Compare February 27, 2023 13:09
nornagon
nornagon reviewed Feb 27, 2023
View reviewed changes
Copy link
Contributor

@nornagon nornagon left a comment •
edited
Loading

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!

@codebytere codebytere requested a review from nornagon February 28, 2023 10:46
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently 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 trop bot mentioned this pull request Mar 1, 2023
Merged
@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 mentioned this pull request Mar 1, 2023
Merged
@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

pushkin- commented Apr 10, 2023

When can this be backported to v22?

@commit-lint commit-lint bot mentioned this pull request May 25, 2023
Open
@codebytere codebytere mentioned this pull request Aug 15, 2023
Closed
3 tasks
@wujinli
Copy link
Contributor

wujinli commented Mar 4, 2024 •
edited
Loading

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

lizhibo0509 added a commit to lizhibo0509/electron that referenced this pull request Jul 18, 2024
@lizhibo0509
fix: handle closing webContents in BrowserViews electron#37420
edb1d00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MarshallOfSound MarshallOfSound MarshallOfSound approved these changes

@deepak1556 deepak1556 Awaiting requested review from deepak1556

+2 more reviewers

@nornagon nornagon nornagon approved these changes

@zcbenz zcbenz zcbenz approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

6 participants

@codebytere @pushkin- @wujinli @nornagon @zcbenz @MarshallOfSound