KEMBAR78
fix: blend node and blink code generation policy when both are loaded by nornagon · Pull Request #36567 · electron/electron · GitHub
Skip to content

fix: blend node and blink code generation policy when both are loaded #36567

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 5 commits into from
Dec 14, 2022
Merged

fix: blend node and blink code generation policy when both are loaded #36567

merged 5 commits into from
Dec 14, 2022

Conversation

@nornagon
Copy link
Contributor

@nornagon nornagon commented Dec 5, 2022

Description of Change

This fixes a crash in some situations when sandbox: false.

Checklist

Release Notes

Notes: Fixed a crash that could occur when running eval in inline scripts in unsandboxed renderer processes.

@nornagon nornagon requested a review from a team as a code owner December 5, 2022 22:34
@nornagon nornagon 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. labels Dec 5, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Dec 5, 2022
MarshallOfSound
MarshallOfSound reviewed Dec 6, 2022
View reviewed changes
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Dec 6, 2022
deepak1556
deepak1556 reviewed Dec 8, 2022
View reviewed changes
shell/common/node_bindings.cc
// If we're running with contextIsolation enabled in the renderer process,
// fall back to Blink's logic.
if (node::Environment::GetCurrent(context) == nullptr) {
if (gin_helper::Locker::IsBrowserProcess()) {
Copy link
Member

Choose a reason for hiding this comment

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

Which scenario we support today might enter this block ? Don't we always have a node environment in the browser process ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None, hence the NOTREACHED below

Copy link
Member

Choose a reason for hiding this comment

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

Do we even need the NOTREACHED in that case ? My understanding is that NOTREACHED is useful to abort when there is no default action for switch statements or when an method is not implemented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess this could just be a CHECK :)

@nornagon nornagon merged commit 9e7fbc7 into main Dec 14, 2022
@nornagon nornagon deleted the fix-node-csp branch December 14, 2022 18:05
@release-clerk
Copy link

release-clerk bot commented Dec 14, 2022

Release Notes Persisted

Fixed a crash that could occur when running eval in inline scripts in unsandboxed renderer processes.

@trop trop bot mentioned this pull request Dec 14, 2022
Merged
trop bot added a commit that referenced this pull request Dec 14, 2022
@trop @nornagon
fix: blend node and blink code generation policy when both are loaded (…
377fd2d 
…#36567)

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
trop bot added a commit that referenced this pull request Dec 14, 2022
@trop @nornagon
fix: blend node and blink code generation policy when both are loaded (…
7a33424 
…#36567)

Co-authored-by: Jeremy Rose <jeremya@chromium.org>
@trop trop bot mentioned this pull request Dec 14, 2022
Merged
@trop
Copy link
Contributor

trop bot commented Dec 14, 2022

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

@trop
Copy link
Contributor

trop bot commented Dec 14, 2022

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

@trop trop bot added in-flight/23-x-y 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. target/22-x-y PR should also be added to the "22-x-y" branch. in-flight/23-x-y labels Dec 14, 2022
codebytere pushed a commit that referenced this pull request Jan 2, 2023
@trop @nornagon @jkleinsc
fix: blend node and blink code generation policy when both are loaded (…
0bd4cea 
…#36668)

* fix: blend node and blink code generation policy when both are loaded (#36567)

Co-authored-by: Jeremy Rose <jeremya@chromium.org>

* Update .patches

* fix patches

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Jeremy Rose <jeremya@chromium.org>
Co-authored-by: John Kleinschmidt <jkleinsc@electronjs.org>
@trop trop bot added merged/22-x-y PR was merged to the "22-x-y" branch. and removed in-flight/22-x-y labels Jan 2, 2023
@codebytere codebytere mentioned this pull request Jan 10, 2023
Closed
3 tasks
@codebytere codebytere mentioned this pull request Jan 26, 2023
Merged
@andersk andersk mentioned this pull request Feb 11, 2023
Closed
@Soltus Soltus mentioned this pull request Feb 16, 2023
Closed
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@nornagon @khalwa
fix: blend node and blink code generation policy when both are loaded (…
cc55d5f 
…electron#36567)
@commit-lint commit-lint bot mentioned this pull request Mar 23, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@deepak1556 deepak1556 deepak1556 approved these changes

@MarshallOfSound MarshallOfSound MarshallOfSound approved these changes

Assignees

No one assigned

Labels

merged/22-x-y PR was merged to the "22-x-y" branch. merged/23-x-y PR was merged to the "23-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.

3 participants

@nornagon @deepak1556 @MarshallOfSound