lib: avoid mutating `Error.stackTraceLimit` when it is not writable by aduh95 · Pull Request #38215 · nodejs/node · GitHub

This PR aims to avoid getting Cannot assign to read only property 'stackTraceLimit' of function 'function Error() { [native code] }' errors thrown from core when Node.js is executed with the --frozen-intrinsics flag.

CI: https://ci.nodejs.org/job/node-test-pull-request/37405/

Realized this should be checking configurable, not frozen status to handle if someone does an Object.seal(Error), etc.

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/998/

Results look OK-ish:

                                                               confidence improvement accuracy (*)    (**)   (***)
misc/hidestackframes.js n=100000 type='direct-call-noerr'                      0.02 %       ±5.94%  ±7.91% ±10.30%
misc/hidestackframes.js n=100000 type='direct-call-throw'               *     -2.86 %       ±2.36%  ±3.14%  ±4.09%
misc/hidestackframes.js n=100000 type='hide-stackframes-noerr'                 0.34 %      ±15.71% ±20.91% ±27.25%
misc/hidestackframes.js n=100000 type='hide-stackframes-throw'                -2.86 %       ±3.23%  ±4.30%  ±5.60%

We could try to cache the value when the property is not configurable:

Suggested change

      
              return desc.writable ?? typeof desc.set === 'function';
          
              const returnValue = desc.writable ?? typeof desc.set === 'function';
          
              if (desc.configurable === false)
          
                module.exports.isErrorStackTraceLimitWritable = () => returnValue;
          
              return returnValue;

I think it makes the harder to read, and I think we should try to optimize for the most common case (which is when the property has configurable and writable set to true).

I'm fine not cacheing for now

CI: https://ci.nodejs.org/job/node-test-pull-request/37440/

CI: https://ci.nodejs.org/job/node-test-pull-request/37441/

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/999/

EDIT: the results are not as good… I'd say the tradeoff is worth it, I'm open to suggestions to improve those figures.

                                                               confidence improvement accuracy (*)    (**)   (***)
misc/hidestackframes.js n=100000 type='direct-call-noerr'                      2.68 %       ±4.40%  ±5.86%  ±7.62%
misc/hidestackframes.js n=100000 type='direct-call-throw'              **     -4.20 %       ±3.04%  ±4.05%  ±5.29%
misc/hidestackframes.js n=100000 type='hide-stackframes-noerr'                13.07 %      ±14.03% ±18.86% ±24.95%
misc/hidestackframes.js n=100000 type='hide-stackframes-throw'        ***     -4.70 %       ±2.42%  ±3.22%  ±4.19%

CI: https://ci.nodejs.org/job/node-test-pull-request/37457/

I've added more tests. I think this is ready to land but I'd love to get a final approval to make sure I haven't forgotten anything :)

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/37465/

CI: https://ci.nodejs.org/job/node-test-pull-request/37472/

PR-URL: nodejs#38215 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Landed in 09c9e5d

PR-URL: #38215 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Apr 12, 2021

aduh95 mentioned this pull request Apr 12, 2021

bootstrap: freeze more intrinsics #38217

Merged

bmeck approved these changes Apr 12, 2021

View reviewed changes

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 12, 2021

mscdex reviewed Apr 12, 2021

View reviewed changes

Comment thread lib/assert.js Outdated

bmeck suggested changes Apr 12, 2021

View reviewed changes

aduh95 removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 12, 2021

aduh95 changed the title lib: avoid mutating Error.stackTraceLimit when Error is frozen lib: avoid mutating Error.stackTraceLimit when it is not writable Apr 13, 2021

aduh95 requested a review from bmeck April 13, 2021 10:44

aduh95 commented Apr 13, 2021

View reviewed changes

bmeck reviewed Apr 13, 2021

View reviewed changes

Comment thread lib/internal/errors.js Outdated

aduh95 requested a review from bmeck April 13, 2021 14:27

benjamingr approved these changes Apr 13, 2021

View reviewed changes

bmeck approved these changes Apr 13, 2021

View reviewed changes

ExE-Boss reviewed Apr 13, 2021

View reviewed changes

Comment thread lib/internal/errors.js Outdated

bmeck approved these changes Apr 14, 2021

View reviewed changes

aduh95 added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Apr 14, 2021

lib: avoid mutating Error.stackTraceLimit when it is not writable

09c9e5d

PR-URL: nodejs#38215 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

aduh95 force-pushed the frozen-error branch from 0788c7c to 09c9e5d Compare April 14, 2021 22:17

aduh95 merged commit 09c9e5d into nodejs:master Apr 14, 2021

aduh95 deleted the frozen-error branch April 14, 2021 22:18

BethGriggs pushed a commit that referenced this pull request Apr 15, 2021

lib: avoid mutating Error.stackTraceLimit when it is not writable

dfdf348

PR-URL: #38215 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

BethGriggs mentioned this pull request Apr 16, 2021

2021-04-20, Version 16.0.0 (Current) #37678

Merged

targos added the backport-blocked-v14.x label May 30, 2021

liuxingbaoyu mentioned this pull request May 13, 2023

[Bug]: Cannot assign to read only property 'stackTraceLimit' babel/babel#15625

Closed

1 task

lib: avoid mutating `Error.stackTraceLimit` when it is not writable by aduh95 · Pull Request #38215 · nodejs/node · GitHub

Uh oh!

Uh oh!

lib: avoid mutating Error.stackTraceLimit when it is not writable - #38215

lib: avoid mutating Error.stackTraceLimit when it is not writable#38215aduh95 merged 1 commit intonodejs:masternodejs/node:masterfrom aduh95:frozen-erroraduh95/node:frozen-errorCopy head branch name to clipboard

Conversation

aduh95 commented Apr 12, 2021

Uh oh!

nodejs-github-bot commented Apr 12, 2021

Uh oh!

Uh oh!

bmeck left a comment

Choose a reason for hiding this comment

Uh oh!

aduh95 commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

bmeck Apr 13, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 13, 2021

Uh oh!

nodejs-github-bot commented Apr 13, 2021

Uh oh!

aduh95 commented Apr 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Apr 14, 2021

Uh oh!

aduh95 commented Apr 14, 2021

Uh oh!

bmeck left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 14, 2021

Uh oh!

nodejs-github-bot commented Apr 14, 2021

Uh oh!

aduh95 commented Apr 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lib: avoid mutating Error.stackTraceLimit when it is not writable#38215
aduh95 merged 1 commit into
nodejs:masterfrom
aduh95:frozen-error

aduh95 commented Apr 13, 2021 •
edited
Loading

aduh95 commented Apr 13, 2021 •
edited
Loading