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

aduh95 · 2021-04-12T15:53:25Z

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.

nodejs-github-bot · 2021-04-12T17:40:08Z

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

lib/assert.js

bmeck

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

aduh95 · 2021-04-13T08:24:59Z

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%

aduh95 · 2021-04-13T10:59:53Z

lib/internal/errors.js

+    return ObjectIsExtensible(Error);
+  }
+
+  return desc.writable ?? typeof desc.set === 'function';


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

lib/internal/errors.js

nodejs-github-bot · 2021-04-13T16:23:25Z

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

nodejs-github-bot · 2021-04-13T17:20:20Z

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

aduh95 · 2021-04-13T18:08:29Z

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%

lib/internal/errors.js

nodejs-github-bot · 2021-04-14T09:27:43Z

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

aduh95 · 2021-04-14T13:33:50Z

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 :)

bmeck

LGTM

nodejs-github-bot · 2021-04-14T14:30:56Z

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

nodejs-github-bot · 2021-04-14T21:41:30Z

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

PR-URL: nodejs#38215 Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

aduh95 · 2021-04-14T22:18:19Z

Landed in 09c9e5d

PR-URL: #38215 Reviewed-By: Bradley Farias <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

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 pending requests for changes, and a CI started. label Apr 12, 2021

mscdex reviewed Apr 12, 2021

View reviewed changes

lib/assert.js Outdated Show resolved Hide resolved

bmeck suggested changes Apr 12, 2021

View reviewed changes

aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, 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

lib/internal/errors.js Outdated Show resolved Hide resolved

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

lib/internal/errors.js Outdated Show resolved Hide resolved

bmeck approved these changes Apr 14, 2021

View reviewed changes

aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, 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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

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 <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

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

-  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;

Uh oh!

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

aduh95 commented Apr 13, 2021 •

edited

Loading

aduh95 commented Apr 13, 2021 •

edited

Loading