Unverified Commit 22c826f5 authored by Anna Henningsen's avatar Anna Henningsen
Browse files

src: do proper error checking in `AsyncWrap::MakeCallback`

At least one method on a native object is added as a getter,
namely `MessagePort.prototype.onmessage`. When a MessagePort
attempts to call this method from C++ in response to receiving
data, it will first invoke that getter and then call the function.

Since `worker.terminate()` interrupts execution, this means
that the getter may fail (without being faulty code on its own).
This means that at least one test exercising these methods in
combination has been flaky and could have crashed, because
we did not actually check that the getter returns a value
so far, resulting in dereferencing an empty `Local`.

The proper fix for this is to use the non-deprecated overload
of `Get()` and check the result like we should be doing.
Also, as a (related) fix, don’t crash if the method
is not a function but rather something else, like a getter
could provide.

Example test failure: https://ci.nodejs.org/job/node-test-commit-linux-containered/4976/nodes=ubuntu1604_sharedlibs_zlib_x64/console

    17:56:56 not ok 1955 parallel/test-worker-dns-terminate
    17:56:56   ---
    17:56:56   duration_ms: 1.237
    17:56:56   severity: crashed
    17:56:56   exitcode: -11
    17:56:56   stack: |-

PR-URL: https://github.com/nodejs/node/pull/21189


Reviewed-By: default avatarAnatoli Papirovski <apapirovski@mac.com>
Reviewed-By: default avatarBenjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: default avatarJoyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: default avatarJames M Snell <jasnell@gmail.com>
parent 61e9e3c2
Supports Markdown
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment