Commit 671346e
src: do proper error checking in
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: #21189
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>AsyncWrap::MakeCallback
1 parent 0da910f commit 671346eCopy full SHA for 671346e
File tree
Expand file treeCollapse file tree
6 files changed
+75
-16
lines changedOpen diff view settings
Filter options
- src
- test/parallel
Expand file treeCollapse file tree
6 files changed
+75
-16
lines changedOpen diff view settings
Collapse file
+8-12Lines changed: 8 additions & 12 deletions
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| ||
81 | 81 | |
82 | 82 | |
83 | 83 | |
84 | | - |
85 | | - |
86 | | - |
87 | | - |
88 | | - |
89 | | - |
90 | | - |
91 | | - |
92 | | - |
93 | | - |
94 | | - |
95 | | - |
| 84 | + |
| 85 | + |
| 86 | + |
| 87 | + |
| 88 | + |
| 89 | + |
| 90 | + |
| 91 | + |
96 | 92 | |
97 | 93 | |
98 | 94 | |
|
Collapse file
-3Lines changed: 0 additions & 3 deletions
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| ||
173 | 173 | |
174 | 174 | |
175 | 175 | |
176 | | - |
177 | | - |
178 | | - |
179 | 176 | |
180 | 177 | |
181 | 178 | |
|
Collapse file
+4Lines changed: 4 additions & 0 deletions
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| ||
83 | 83 | |
84 | 84 | |
85 | 85 | |
| 86 | + |
| 87 | + |
| 88 | + |
| 89 | + |
86 | 90 | |
87 | 91 | |
88 | 92 | |
|
Collapse file
+12-1Lines changed: 12 additions & 1 deletion
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| ||
391 | 391 | |
392 | 392 | |
393 | 393 | |
| 394 | + |
394 | 395 | |
395 | 396 | |
396 | 397 | |
| 398 | + |
| 399 | + |
| 400 | + |
| 401 | + |
| 402 | + |
| 403 | + |
| 404 | + |
| 405 | + |
| 406 | + |
| 407 | + |
| 408 | + |
397 | 409 | |
398 | 410 | |
399 | 411 | |
| ||
476 | 488 | |
477 | 489 | |
478 | 490 | |
479 | | - |
480 | 491 | |
481 | 492 | |
482 | 493 | |
|
Collapse file
+2Lines changed: 2 additions & 0 deletions
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| ||
154 | 154 | |
155 | 155 | |
156 | 156 | |
| 157 | + |
| 158 | + |
157 | 159 | |
158 | 160 | |
159 | 161 | |
|
Collapse file
test/parallel/test-async-wrap-missing-method.js
Copy file name to clipboard+49Lines changed: 49 additions & 0 deletions
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| ||
| 1 | + |
| 2 | + |
| 3 | + |
| 4 | + |
| 5 | + |
| 6 | + |
| 7 | + |
| 8 | + |
| 9 | + |
| 10 | + |
| 11 | + |
| 12 | + |
| 13 | + |
| 14 | + |
| 15 | + |
| 16 | + |
| 17 | + |
| 18 | + |
| 19 | + |
| 20 | + |
| 21 | + |
| 22 | + |
| 23 | + |
| 24 | + |
| 25 | + |
| 26 | + |
| 27 | + |
| 28 | + |
| 29 | + |
| 30 | + |
| 31 | + |
| 32 | + |
| 33 | + |
| 34 | + |
| 35 | + |
| 36 | + |
| 37 | + |
| 38 | + |
| 39 | + |
| 40 | + |
| 41 | + |
| 42 | + |
| 43 | + |
| 44 | + |
| 45 | + |
| 46 | + |
| 47 | + |
| 48 | + |
| 49 | + |
0 commit comments