module: fix `e.stack` error when throwing undefined or null #19282

zhanzhenzhen · 2018-03-11T11:12:16Z

Fix #19281

I just deleted one e.stack line. There's another e.stack line in DefaultResolve.js, with a comment. Seems that shouldn't be deleted.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

addaleax

Looks good to me!

Could you add a regression test for this? I think test/es-module would be the right place for it.

zhanzhenzhen · 2018-03-11T11:21:39Z

Sorry this is the first time I made a pull request to Node, I'm really not familiar with writing these tests.

addaleax · 2018-03-11T11:22:57Z

If you want, there’s a guide at https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md – but I wouldn’t consider it a blocker for this PR. :)

hiroppy · 2018-03-11T12:21:29Z

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

zhanzhenzhen · 2018-03-11T13:07:44Z

I'm writing tests now. I want one module to throw an undefined exception. How to make this module itself not tested?

// Flags: --experimental-modules
/* eslint-disable node-core/required-modules */

throw undefined;

I just want another module that imports this module tested.

targos · 2018-03-11T13:13:28Z

@zhanzhenzhen If you don't want that module to be executed as a test, the best is to put it in the fixtures directory.

test/fixtures/es-module-loaders is a good place for it IMO.
Then from the test itself, you can import('../fixtures/es-module-loaders/xxx.mjs').

zhanzhenzhen · 2018-03-11T13:19:59Z

@targos thanks!

benjamingr

Thanks for your contribution!

Blocking until the modules team has had time to review this.

benjamingr · 2018-03-11T13:45:21Z

That is, I think the change is correct but I also think that the original behavior was intentional (probably?) so I want to understand why it was done before we change it.

ljharb · 2018-03-11T14:54:29Z

lib/internal/loader/ModuleJob.js

@@ -105,7 +105,6 @@ class ModuleJob {
    try {
      module.evaluate();
    } catch (e) {
-      e.stack;


I believe if this is an error object, accessing the stack property ensures that the trace is generated?

You could change this to if (e) { e.stack; } to ensure that path is still followed.

I believe if this is an error object, accessing the stack property ensures that the trace is generated?

Yes.

You could change this to if (e) { e.stack; } to ensure that path is still followed.

Node has an isError helper. Since e could be any thrown value and .stack could be any property value (even a getter) it's probably better to at least have an error check just to be on the up and up. Similar usage here.

Can someone explain why it's important to generate the stack trace in advance?

Can someone explain why it's important to generate the stack trace in advance?

Could be to generate the stack before being re-thrown and adding to the stack.
In many cases stacks can be snipped OK with Error.captureStackTrace(error, beforeFunc).

Could be to generate the stack before being re-thrown and adding to the stack.

I'm not aware that re-throwing modifies the stack. Do you have an example?

There's an old V8 bug about it: https://bugs.chromium.org/p/chromium/issues/detail?id=60240. But it was fixed in 2012.

I was able to repro this a while back but cannot now. +1 to removal, but now curious about why it was showing up while working on initial loader.

@bmeck what version of node (and thus v8) were you using when working on it initially?

muddy to see the actual commits due to rebases, but guessing from timeline and other stuff in my fork it was probably around V8 5.4 (which was still from 2016)?

@bmeck Did you test the syntax error case? IIRC that's the one with bad/missing stacks (throw and Promise.reject should both ensure stacks already).

@jkrems I did not.

devsnek · 2018-03-11T17:00:21Z

test/es-module/test-esm-throw-undefined.mjs

+import assert from 'assert';
+
+async function doTest() {
+  try {


you can use assert.rejects here instead of a try/catch

@devsnek OK. BTW, I can't find any async and await in existing es-module tests. I don't know if they can be used.

async/await is safe to use. you can test your tests with python tools/test.py path/to/your/test e.g. python tools/test.py test/es-module/test-esm-throw-undefined.mjs

@devsnek I find in this case I can't use assert.rejects. It has the same problem as assert.throws, because when it comes to undefined or null, many results can be considered OK. See:

assert.rejects(async () => {throw Error;}, null) assert.rejects(async () => {throw Error;}, undefined) assert.rejects(async () => {throw undefined;}, null) assert.rejects(async () => {throw null;}, undefined)

All of these 4 are considered as "rejected". So it will make the test too loose.

@devsnek Sorry just aware that I can use function e => e === undefined as the second parameter. Will modify test.

zhanzhenzhen · 2018-03-11T17:08:25Z

Should I change it to if (e) { e.stack; }?

addaleax · 2018-03-11T17:10:04Z

Should I change it to if (e) { e.stack; }?

I would keep the line removed entirely unless somebody comes up with a non-artificial example for how this would make a difference for user code.

benjamingr · 2018-03-11T17:14:22Z

@zhanzhenzhen this is your first contribution (thank you!) so I'll elabtorate a little. Basically, pull requests in Node.js take 48 hours (or 72h in weekends as is the case here) before landing - collaborators are given a chance to respond in the meantime.

We're hoping @bmeck or another developer knows why this code is there (we have an educated guess but no "proof" yet).

If after 72h we don't have any objections we'll land it (and I'll remove my "request changes" for the clarification).

jkrems

I'm in favor of removing the line entirely unless somebody has a regression test showing the need (and then we can add something back together with the test). It's in experimental code so I don't think we have to be too conservative about landing a change like this.

BridgeAR · 2018-03-11T23:43:06Z

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

ljharb · 2018-03-12T03:51:42Z

I think that removing it without understanding why it’s there is probably unwise; the root of this issue is a nullish e - the if would address that, rather than risking expanding the scope of this PR.

benjamingr

After clarification about e.stack I'm +1 and changes LGTM

BridgeAR · 2018-03-27T00:28:29Z

@zhanzhenzhen can you please rebase this? :-)

zhanzhenzhen · 2018-03-27T18:35:49Z

@BridgeAR Sure

zhanzhenzhen · 2018-03-27T20:00:13Z

@BridgeAR Rebased

TimothyGu

Left two very minor comments. Otherwise, LGTM.

TimothyGu · 2018-03-27T20:07:34Z

test/es-module/test-esm-throw-undefined.mjs

+/* eslint-disable node-core/required-modules */
+import common from '../common/index.js';
+import assert from 'assert';
+


nit: add a common.crashOnUnhandledRejection(); line here, and remove the .then(common.mustCall()) later.

@TimothyGu Thank you. As this is the first time I make a pull request I'm not very familiar with the these functions. You can add it later :)

@TimothyGu I added.

TimothyGu · 2018-03-27T20:08:35Z

test/es-module/test-esm-throw-undefined.mjs

+      await import('../fixtures/es-module-loaders/throw-undefined');
+    },
+    (e) => e === undefined
+  );


Nit:

await assert.rejects( import('../fixtures/es-module-loaders/throw-undefined'), (e) => e === undefined);

The extra arrow function doesn't seem needed.

I just followed the example here:
https://github.com/nodejs/node/blob/master/doc/api/assert.md

You mean, the first parameter can be a promise (not a function)? I don't know, but I think mine is more formal :)

Oh oops, never mind that.

TimothyGu · 2018-03-28T16:59:52Z

Note to whoever applies this: add a common.crashOnUnhandledRejection(); line. See #19282 (comment).

BridgeAR · 2018-04-09T17:49:22Z

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

Adds a test for module loading when throwing undefined or null. PR-URL: nodejs#19282 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

BridgeAR · 2018-04-09T22:47:31Z

Landed in b34a1e1 🎉

Adds a test for module loading when throwing undefined or null. PR-URL: nodejs#19282 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

addaleax approved these changes Mar 11, 2018

View reviewed changes

targos approved these changes Mar 11, 2018

View reviewed changes

benjamingr requested changes Mar 11, 2018

View reviewed changes

cjihrig approved these changes Mar 11, 2018

View reviewed changes

ljharb suggested changes Mar 11, 2018

View reviewed changes

addaleax added the esm Issues and PRs related to the ECMAScript Modules implementation. label Mar 11, 2018

devsnek reviewed Mar 11, 2018

View reviewed changes

jkrems approved these changes Mar 11, 2018

View reviewed changes

BridgeAR approved these changes Mar 11, 2018

View reviewed changes

benjamingr approved these changes Mar 12, 2018

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 12, 2018

MylesBorins added the modules-agenda Issues and PRs to discuss during the meetings of the Modules team. label Mar 12, 2018

MylesBorins mentioned this pull request Mar 12, 2018

Node.js Foundation Modules Team Meeting 2018-03-14 nodejs/modules#47

Closed

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 27, 2018

zhanzhenzhen force-pushed the master branch from 6b7a62b to 6a654f8 Compare March 27, 2018 19:24

MylesBorins mentioned this pull request Mar 27, 2018

Node.js Foundation Modules Team Meeting 2018-03-28 nodejs/modules#52

Closed

TimothyGu approved these changes Mar 27, 2018

View reviewed changes

zhanzhenzhen added 3 commits March 28, 2018 19:40

module: fix e.stack error when throwing undefined or null

1f2a241

test: add test for module loading when throwing undefined or null

4b73eb6

test: previous test changed to assert.rejects style

0b3bd33

zhanzhenzhen force-pushed the master branch from 6a654f8 to 0b3bd33 Compare March 28, 2018 11:43

test: add common.crashOnUnhandledRejection() in previous test

8e3cb79

BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 9, 2018

BridgeAR closed this Apr 9, 2018

targos added backport-requested-v9.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 12, 2018

guybedford mentioned this pull request Mar 12, 2020

ES module loading inconsistent with browser while handling undefined exceptions #19281

Closed

module: fix e.stack error when throwing undefined or null #19282

module: fix e.stack error when throwing undefined or null #19282

Conversation

zhanzhenzhen commented Mar 11, 2018 • edited Loading

Checklist

addaleax left a comment • edited Loading

Choose a reason for hiding this comment

zhanzhenzhen commented Mar 11, 2018

addaleax commented Mar 11, 2018

hiroppy commented Mar 11, 2018

zhanzhenzhen commented Mar 11, 2018

targos commented Mar 11, 2018

zhanzhenzhen commented Mar 11, 2018

benjamingr left a comment • edited Loading

Choose a reason for hiding this comment

benjamingr commented Mar 11, 2018

Choose a reason for hiding this comment

jdalton Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdalton Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Mar 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhanzhenzhen commented Mar 11, 2018

addaleax commented Mar 11, 2018

benjamingr commented Mar 11, 2018

jkrems left a comment • edited Loading

Choose a reason for hiding this comment

BridgeAR commented Mar 11, 2018

ljharb commented Mar 12, 2018

benjamingr left a comment

Choose a reason for hiding this comment

BridgeAR commented Mar 27, 2018

zhanzhenzhen commented Mar 27, 2018

zhanzhenzhen commented Mar 27, 2018

TimothyGu left a comment

Choose a reason for hiding this comment

TimothyGu Mar 27, 2018 • edited Loading

Choose a reason for hiding this comment

zhanzhenzhen Mar 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Mar 28, 2018

BridgeAR commented Apr 9, 2018

BridgeAR commented Apr 9, 2018

module: fix `e.stack` error when throwing undefined or null #19282

module: fix `e.stack` error when throwing undefined or null #19282

zhanzhenzhen commented Mar 11, 2018 •

edited

Loading

addaleax left a comment •

edited

Loading

benjamingr left a comment •

edited

Loading

jdalton Mar 11, 2018 •

edited

Loading

jdalton Mar 11, 2018 •

edited

Loading

devsnek Mar 11, 2018 •

edited

Loading

devsnek Mar 11, 2018 •

edited

Loading

jkrems left a comment •

edited

Loading

TimothyGu Mar 27, 2018 •

edited

Loading

zhanzhenzhen Mar 28, 2018 •

edited

Loading