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: assert module in the renderer process #39540

Merged
merged 1 commit into from Aug 23, 2023
Merged

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Aug 16, 2023

Description of Change

Closes #39079.

When creating a Node.js Environment, embedders have the option to disable Node.js' default overriding of Error.prepareStackTrace. However, the assert module depends on a WeakMap that is populated with the error stacktraces in the overridden function.

This adds handling to fall back to the default implementation if Error.prepareStackTrace if the override has been disabled.

This will be upstreamed.

Checklist

Release Notes

Notes: Fixes an issue where the Node.js assert module did not work in the renderer process.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/24-x-y PR should also be added to the "24-x-y" branch. target/25-x-y PR should also be added to the "25-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. labels Aug 16, 2023
@codebytere codebytere requested a review from a team as a code owner August 16, 2023 17:19
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 16, 2023
@ckerr
Copy link
Member

ckerr commented Aug 16, 2023

This looks good but I want to ask about this line:

However, the assert module depends on a WeakMap that is initialized in the overridden function.

I'm wondering if it's possible to ensure the WeakMap somehow is always initialized whether or not the function is overridden; and if so, whether that would be simpler than the approach in this PR?

I haven't read the assert module -- if the above idea isn't practical or is more complex then this fix LGTM.

@codebytere
Copy link
Member Author

codebytere commented Aug 16, 2023

@ckerr sorry, i should have phrased that better. It is initialized - it's never populated which means that call will never have its stacktrace. Updated the patch description!

@github-actions github-actions bot added the target/27-x-y PR should also be added to the "27-x-y" branch. label Aug 16, 2023
@codebytere
Copy link
Member Author

Upstream PR: nodejs/node#49212

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 17, 2023
@codebytere codebytere requested a review from zcbenz August 22, 2023 11:12
@codebytere codebytere merged commit ddc7e3e into main Aug 23, 2023
18 checks passed
@codebytere codebytere deleted the fix-assert-renderer branch August 23, 2023 09:38
@release-clerk
Copy link

release-clerk bot commented Aug 23, 2023

Release Notes Persisted

Fixes an issue where the Node.js assert module did not work in the renderer process.

@trop
Copy link
Contributor

trop bot commented Aug 23, 2023

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

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

trop bot commented Aug 23, 2023

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

@trop
Copy link
Contributor

trop bot commented Aug 23, 2023

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

@trop
Copy link
Contributor

trop bot commented Aug 23, 2023

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

@trop trop bot added in-flight/27-x-y in-flight/26-x-y merged/27-x-y PR was merged to the "27-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. merged/24-x-y PR was merged to the "24-x-y" branch and removed target/25-x-y PR should also be added to the "25-x-y" branch. target/27-x-y PR should also be added to the "27-x-y" branch. target/26-x-y PR should also be added to the "26-x-y" branch. in-flight/27-x-y in-flight/26-x-y in-flight/24-x-y labels Aug 23, 2023
@trop trop bot added merged/25-x-y PR was merged to the "25-x-y" branch. and removed in-flight/25-x-y labels Aug 30, 2023
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/24-x-y PR was merged to the "24-x-y" branch merged/25-x-y PR was merged to the "25-x-y" branch. merged/26-x-y PR was merged to the "26-x-y" branch. merged/27-x-y PR was merged to the "27-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: render process: assert.ok(false) throws TypeError instead of AssertionError
2 participants