Skip to content

fix: addChildView() crashes when adding a closed WebContentsView #47099

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 3, 2025
Merged

fix: addChildView() crashes when adding a closed WebContentsView #47099

merged 1 commit into from
Jun 3, 2025

Conversation

StaZhu
Copy link
Contributor

@StaZhu StaZhu commented May 15, 2025 •
edited by ckerr
Loading

Description of Change

Closes #47074

Checklist

Release Notes

Notes: Fixed addChildView() crashes when adding a closed WebContentsView

@welcome Welcome
Copy link

welcome bot commented May 15, 2025

💖 Thanks for opening this pull request! 💖

Semantic PR titles

We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix.

Examples of commit messages with semantic prefixes:

  • fix: don't overwrite prevent_default if default wasn't prevented
  • feat: add app.isPackaged() method
  • docs: app.isDefaultProtocolClient is now available on Linux

Commit signing

This repo enforces commit signatures for all incoming PRs.
To sign your commits, see GitHub's documentation on Telling Git about your signing key.

PR tips

Things that will help get your PR across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label May 15, 2025
codebytere
codebytere requested changes May 17, 2025
View reviewed changes
Copy link
Member

@codebytere codebytere 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.

A similar check should be added in View::RemoveChildView and moved up from https://github.com/electron/electron/pull/47099/files#diff-ffd5b94a3537909d007f9eee80f2d4b97d56129965fed44226f50c4aad6e5773R278

Also, this should have a test added.

@codebytere
Copy link
Member

@StaZhu we're prioritizing this as a blocker so if you're able to address these changes please do so or this PR might be superseded by another.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label May 22, 2025
@StaZhu
Copy link
Contributor Author

StaZhu commented May 24, 2025 •
edited
Loading

Sorry, I was OOO last week, I will address the issue this monday🙏.

@StaZhu StaZhu force-pushed the fix-webcontent-view-crash branch from 3fa706b to 2e21e37 Compare May 26, 2025 06:41
@StaZhu
Copy link
Contributor Author

StaZhu commented May 26, 2025

A similar check should be added in View::RemoveChildView

We don't need to do this, the current code won't crash if passing a destroyed child view to View::RemoveChildView.

What's more, there is another test here that verify the function not throw error if view is destroyed.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/35-x-y PR should also be added to the "35-x-y" branch. target/36-x-y PR should also be added to the "36-x-y" branch. target/37-x-y PR should also be added to the "37-x-y" branch. labels May 28, 2025
@StaZhu StaZhu force-pushed the fix-webcontent-view-crash branch from 2e21e37 to 63a9f7c Compare May 28, 2025 10:47
@StaZhu
Copy link
Contributor Author

StaZhu commented May 28, 2025

Linting issue fixed, @codebytere could you help re-trigger the CI?

@ckerr
Copy link
Member

ckerr commented May 29, 2025

Looks like CI is green but the commits aren't signed

ckerr
ckerr approved these changes May 29, 2025
View reviewed changes
Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

The commit needs to be signed, but otherwise LGTM

@StaZhu
Copy link
Contributor Author

StaZhu commented May 29, 2025

@ckerr Anything wrong with my side? what I can do? Thanks!

@codebytere
Copy link
Member

@StaZhu as @ckerr noted:

Merging is blocked
Commits must have verified signatures.

You need to sign your commit - https://docs.github.com/en/authentication/managing-commit-signature-verification/signing-commits

@StaZhu StaZhu force-pushed the fix-webcontent-view-crash branch from 63a9f7c to 0a7f5f9 Compare May 30, 2025 09:21
@StaZhu
Copy link
Contributor Author

StaZhu commented May 30, 2025

You need to sign your comm

Done.

@codebytere
Copy link
Member

@StaZhu rebasing should fix the error

@StaZhu StaZhu force-pushed the fix-webcontent-view-crash branch from 2489e2f to 84704de Compare June 1, 2025 05:41
@StaZhu
Copy link
Contributor Author

StaZhu commented Jun 1, 2025

@StaZhu rebasing should fix the error

Done.

@codebytere codebytere merged commit 158176f into electron:main Jun 3, 2025
57 checks passed
@welcome Welcome
Copy link

welcome bot commented Jun 3, 2025

Congrats on merging your first pull request! 🎉🎉🎉

@release-clerk
Copy link

release-clerk bot commented Jun 3, 2025

Release Notes Persisted

Fixed addChildView() crashes when adding a closed WebContentsView

This was referenced Jun 3, 2025
Merged
Merged
@trop trop bot mentioned this pull request Jun 3, 2025
Merged
@trop
Copy link
Contributor

trop bot commented Jun 3, 2025

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

@trop trop bot added the in-flight/37-x-y label Jun 3, 2025
@trop
Copy link
Contributor

trop bot commented Jun 3, 2025

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

@trop
Copy link
Contributor

trop bot commented Jun 3, 2025

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

@trop trop bot added in-flight/35-x-y in-flight/36-x-y merged/35-x-y PR was merged to the "35-x-y" branch. merged/36-x-y PR was merged to the "36-x-y" branch. merged/37-x-y PR was merged to the "37-x-y" branch. and removed target/37-x-y PR should also be added to the "37-x-y" branch. target/35-x-y PR should also be added to the "35-x-y" branch. target/36-x-y PR should also be added to the "36-x-y" branch. in-flight/35-x-y in-flight/36-x-y in-flight/37-x-y labels Jun 3, 2025
kigh-ota pushed a commit to kigh-ota/electron that referenced this pull request Sep 30, 2025
@StaZhu @kigh-ota
fix: addChildView() crashes when adding a closed WebContentsView (ele…
8c348cf 
…ctron#47099)

fix: addChildView() crashes when add a closed WebContentsView
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ckerr ckerr ckerr approved these changes

@codebytere codebytere codebytere approved these changes

Assignees

No one assigned

Labels

merged/35-x-y PR was merged to the "35-x-y" branch. merged/36-x-y PR was merged to the "36-x-y" branch. merged/37-x-y PR was merged to the "37-x-y" branch. semver/patch backwards-compatible bug fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Using contentView.addChildView with a closed WebContentsView results in EXCEPTION_ACCESS_VIOLATION crash

3 participants

@StaZhu @codebytere @ckerr