Skip to content

fix: assert module in the renderer process#39540

Merged
codebytere merged 1 commit intomainfrom
fix-assert-renderer
Aug 23, 2023
Merged

fix: assert module in the renderer process#39540
codebytere merged 1 commit intomainfrom
fix-assert-renderer

Conversation

@codebytere
Copy link
Copy Markdown
Member

@codebytere codebytere commented Aug 16, 2023 •
edited
Loading

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 recently label Aug 16, 2023
@ckerr
Copy link
Copy Markdown
Member

ckerr commented Aug 16, 2023 •
edited
Loading

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
Copy Markdown
Member Author

codebytere commented Aug 16, 2023 •
edited
Loading

@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!

@codebytere codebytere force-pushed the fix-assert-renderer branch from 199a387 to d8c97df Compare August 16, 2023 18:55
@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
Copy Markdown
Member Author

codebytere commented Aug 17, 2023

Upstream PR: nodejs/node#49212

@electron-cation electron-cation Bot removed the new-pr 🌱 PR opened recently 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
@codebytere codebytere deleted the fix-assert-renderer branch August 23, 2023 09:38
@release-clerk
Copy link
Copy Markdown

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.

This was referenced Aug 23, 2023
Merged
Merged
@trop
Copy link
Copy Markdown
Contributor

trop Bot commented Aug 23, 2023

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

@trop trop Bot mentioned this pull request Aug 23, 2023
Merged
@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 trop Bot mentioned this pull request Aug 23, 2023
Merged
@trop
Copy link
Copy Markdown
Contributor

trop Bot commented Aug 23, 2023

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

@trop
Copy link
Copy Markdown
Contributor

trop Bot commented Aug 23, 2023

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

@trop
Copy link
Copy Markdown
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 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
@commit-lint commit-lint Bot mentioned this pull request Nov 17, 2023
Open
MrHuangJser pushed a commit to MrHuangJser/electron that referenced this pull request Dec 11, 2023
@codebytere
fix: assert module in the renderer process (electron#39540)
b6b99ff
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

@jkleinsc jkleinsc Awaiting requested review from jkleinsc

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

@zcbenz zcbenz Awaiting requested review from zcbenz

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

2 participants

@codebytere @ckerr