quic: use Check instead of FromJust in node_quic.cc

[danbev]

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[danbev]

CI: https://ci.nodejs.org/job/node-test-commit/39087/

[addaleax]

LGTM

@danbev Since you’re opening quite a few PRs of that kind … would you be interested in helping us move away from .Check()/.FromJust()/.ToLocalChecked() in places where those calls aren’t valid? So far I’ve mostly been trying to avoid that new code uses those inaccurately, but it would of course be nice to also make this work for code like this, where neither .FromJust() nor .Check() are really correct :)

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/31946/
QUIC CI: https://ci.nodejs.org/job/node-test-commit/39127/

[danbev]

would you be interested in helping us move away from .Check()/.FromJust()/.ToLocalChecked() in places where those calls aren’t valid?

I'd be happy to. Could you give me an example of where these calls are not valid so I understand the reason for them not being valid?

[addaleax]

@danbev I think the description in https://github.com/nodejs/node/blob/master/src/README.md#checked-conversion might be helpful? The tl;dr is that anything that potentially runs JS code, including built-in V8 JS code or getters/setters on built-in object’s prototypes, can fail and thus result in a crash.

For example, in this particular case:

$ node -e 'Object.defineProperty(Object.prototype, "sessionConfig", {set() { throw new Error(); }}); net.createQuicSocket()'FATAL ERROR: v8::FromJust Maybe value is Nothing.[...]

(I know that these .Check()/.FromJust()/.ToLocalChecked() patterns are pervasive in our code base, but eventually, I would like to remove all the ones that can make Node.js crash hard.)

[danbev]

Re-run of failing node-test-commit-arm-fanned ✔️

[danbev]

@addaleax Thanks, I'll try to take a closer look this week.

[danbev]

Landed in 013cd1a.