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: don't destroy BrowserView webContents on owner window close #42136

Closed
wants to merge 1 commit into from

Conversation

codebytere
Copy link
Member

Description of Change

Closes #42033.
Refs #35658

The above PR made it such that we explicitly destroyed the webContents in all BrowserViews currently attached to a give BrowserWindow when the BrowserWindow closed, which was a breaking change from previous BrowserView behavior. We should preserve the previous behavior.

cc @nornagon in case this was for a specific reason i'm not aware of!

Checklist

Release Notes

Notes: Fixed an issue where BrowserView webContents were destroyed on owner BrowserWindow closure.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/30-x-y PR should also be added to the "30-x-y" branch. target/31-x-y PR should also be added to the "31-x-y" branch. labels May 13, 2024
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 13, 2024
@codebytere
Copy link
Member Author

codebytere commented May 13, 2024

Looks like this affects:

webContents module will-prevent-unload event emits if beforeunload returns false in a BrowserView

seems like this answers my question for impetus of the original change - will investigate alternatives 🤔

@yangannyx
Copy link
Contributor

@codebytere Felix and I took a look at this on Friday and came up with the approach of moving the webContents forEach loop from the browserWindow 'close' event to the 'closed' event. This works when testing with the fiddle attached in #42033, and was in the process of writing tests when I saw this PR.

What do you think abt this approach?

@felixrieseberg
Copy link
Member

@codebytere Thank you! For what it's worth, a lot of the behavior here is undocumented anyway (see also #28382 😉) so I think we kind of get to solve the problem however we feel like it

@codebytere
Copy link
Member Author

codebytere commented May 14, 2024

@yangannyx i like that approach! let's go with that - I'll close this out. Let me know when you'd like some review on. PR and i'll take a look 😁

@codebytere codebytere closed this May 14, 2024
@codebytere codebytere deleted the dont-destroy-bv-on-close branch May 14, 2024 08:03
@codebytere
Copy link
Member Author

@yangannyx is this still something you're planning to pursue? Given it's a stable blocker, happy to help if necessary!

@yangannyx
Copy link
Contributor

yangannyx commented May 30, 2024

@codebytere I ended up scrapping the approach of using closed instead of close because it seemed to also not support this behavior. I haven't worked on it since. Still happy to open a PR to confirm this from CI

webContents module will-prevent-unload event emits if beforeunload returns false in a BrowserView

@yangannyx
Copy link
Contributor

yangannyx commented Jun 4, 2024

Just put up the PR. It looks like the webContents module will-prevent-unload event emits if beforeunload returns false in a BrowserView test passes woo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-pr 🌱 PR opened in the last 24 hours semver/patch backwards-compatible bug fixes target/30-x-y PR should also be added to the "30-x-y" branch. target/31-x-y PR should also be added to the "31-x-y" branch.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Calling preventDefault on BrowserWindow close event still destroys BrowserViews' webContents
3 participants