Skip to content

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

Rodrigo Muino Tomonari requested to merge github/fork/addaleax/worker-dns-terminate-async-wrap into master

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: |-
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Merge request reports