fs: do not crash when using a closed fs event watcher

Before this commit, when the user calls methods on a closed or
errored fs event watcher, they could hit a crash since the
FSEventWrap in C++ land may have already been destroyed with
the internal pointer set to nullptr. This commit makes sure
that the user cannot hit crashes like that, instead the
methods calling on a closed watcher will be noops.

Also explicitly documents that the watchers should not be used
in `close` and `error` event handlers.

PR-URL: https://github.com/nodejs/node/pull/20985
Fixes: https://github.com/nodejs/node/issues/20738
Fixes: https://github.com/nodejs/node/issues/20297
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Ron Korving <ron@ronkorving.nl>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>

1 files changed, 15 insertions, 4 deletions

diff --git a/test/parallel/test-fs-watch.js b/test/parallel/test-fs-watch.js
index ffa82e52c7..e596f32a76 100644
--- a/test/parallel/test-fs-watch.js
+++ b/test/parallel/test-fs-watch.js
@@ -46,7 +46,8 @@ for (const testCase of cases) {
   fs.writeFileSync(testCase.filePath, content1);
 
   let interval;
-  const watcher = fs.watch(testCase[testCase.field]);
+  const pathToWatch = testCase[testCase.field];
+  const watcher = fs.watch(pathToWatch);
   watcher.on('error', (err) => {
     if (interval) {
       clearInterval(interval);
@@ -54,7 +55,11 @@ for (const testCase of cases) {
     }
     assert.fail(err);
   });
-  watcher.on('close', common.mustCall());
+  watcher.on('close', common.mustCall(() => {
+    watcher.close(); // Closing a closed watcher should be a noop
+    // Starting a closed watcher should be a noop
+    watcher.start();
+  }));
   watcher.on('change', common.mustCall(function(eventType, argFilename) {
     if (interval) {
       clearInterval(interval);
@@ -66,10 +71,16 @@ for (const testCase of cases) {
       assert.strictEqual(eventType, 'change');
     assert.strictEqual(argFilename, testCase.fileName);
 
-    watcher.start(); // Starting a started watcher should be a noop
-    // End of test case
+    // Starting a started watcher should be a noop
+    watcher.start();
+    watcher.start(pathToWatch);
+
     watcher.close();
+
+    // We document that watchers cannot be used anymore when it's closed,
+    // here we turn the methods into noops instead of throwing
     watcher.close(); // Closing a closed watcher should be a noop
+    watcher.start();  // Starting a closed watcher should be a noop
   }));
 
   // Long content so it's actually flushed. toUpperCase so there's real change.