fs: make callbacks run in global context

Callbacks that were passed to the binding layer ran in the context of the
(internal) binding object. Make sure they run in the global context.

Before:

  fs.symlink('a', 'b', function() {
    console.log(this); // prints "{ oncomplete: [Function] }"
  });

After:

  fs.symlink('a', 'b', function() {
    console.log(this); // prints "{ <global object> }"
  });

2 files changed, 53 insertions, 38 deletions

diff --git a/lib/fs.js b/lib/fs.js
index 00f5805b2..cbc10879d 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -104,7 +104,7 @@ fs.existsSync = function(path) {
 fs.readFile = function(path, encoding_) {
   var encoding = typeof(encoding_) === 'string' ? encoding_ : null;
   var callback = arguments[arguments.length - 1];
-  if (typeof(callback) !== 'function') callback = noop;
+  if (typeof(callback) !== 'function') callback = function() {};
 
   // first, stat the file, so we know the size.
   var size;
@@ -237,13 +237,27 @@ Object.defineProperty(exports, '_stringToFlags', {
   value: stringToFlags
 });
 
-function noop() {}
+
+// Ensure that callbacks run in the global context. Only use this function
+// for callbacks that are passed to the binding layer, callbacks that are
+// invoked from JS already run in the proper scope.
+function makeCallback(cb) {
+  if (typeof cb !== 'function') {
+    // faster than returning a ref to a global no-op function
+    return function() {};
+  }
+
+  return function() {
+    return cb.apply(null, arguments);
+  };
+}
+
 
 // Yes, the follow could be easily DRYed up but I provide the explicit
 // list to make the arguments clear.
 
 fs.close = function(fd, callback) {
-  binding.close(fd, callback || noop);
+  binding.close(fd, makeCallback(callback));
 };
 
 fs.closeSync = function(fd) {
@@ -264,11 +278,7 @@ function modeNum(m, def) {
 }
 
 fs.open = function(path, flags, mode, callback) {
-  callback = arguments[arguments.length - 1];
-  if (typeof(callback) !== 'function') {
-    callback = noop;
-  }
-
+  callback = makeCallback(arguments[arguments.length - 1]);
   mode = modeNum(mode, 438 /*=0666*/);
 
   binding.open(pathModule._makeLong(path),
@@ -376,7 +386,7 @@ fs.writeSync = function(fd, buffer, offset, length, position) {
 fs.rename = function(oldPath, newPath, callback) {
   binding.rename(pathModule._makeLong(oldPath),
                  pathModule._makeLong(newPath),
-                 callback || noop);
+                 makeCallback(callback));
 };
 
 fs.renameSync = function(oldPath, newPath) {
@@ -385,7 +395,7 @@ fs.renameSync = function(oldPath, newPath) {
 };
 
 fs.truncate = function(fd, len, callback) {
-  binding.truncate(fd, len, callback || noop);
+  binding.truncate(fd, len, makeCallback(callback));
 };
 
 fs.truncateSync = function(fd, len) {
@@ -393,7 +403,7 @@ fs.truncateSync = function(fd, len) {
 };
 
 fs.rmdir = function(path, callback) {
-  binding.rmdir(pathModule._makeLong(path), callback || noop);
+  binding.rmdir(pathModule._makeLong(path), makeCallback(callback));
 };
 
 fs.rmdirSync = function(path) {
@@ -401,7 +411,7 @@ fs.rmdirSync = function(path) {
 };
 
 fs.fdatasync = function(fd, callback) {
-  binding.fdatasync(fd, callback || noop);
+  binding.fdatasync(fd, makeCallback(callback));
 };
 
 fs.fdatasyncSync = function(fd) {
@@ -409,7 +419,7 @@ fs.fdatasyncSync = function(fd) {
 };
 
 fs.fsync = function(fd, callback) {
-  binding.fsync(fd, callback || noop);
+  binding.fsync(fd, makeCallback(callback));
 };
 
 fs.fsyncSync = function(fd) {
@@ -418,8 +428,9 @@ fs.fsyncSync = function(fd) {
 
 fs.mkdir = function(path, mode, callback) {
   if (typeof mode === 'function') callback = mode;
-  binding.mkdir(pathModule._makeLong(path), modeNum(mode, 511 /*=0777*/),
-                callback || noop);
+  binding.mkdir(pathModule._makeLong(path),
+                modeNum(mode, 511 /*=0777*/),
+                makeCallback(callback));
 };
 
 fs.mkdirSync = function(path, mode) {
@@ -428,7 +439,7 @@ fs.mkdirSync = function(path, mode) {
 };
 
 fs.sendfile = function(outFd, inFd, inOffset, length, callback) {
-  binding.sendfile(outFd, inFd, inOffset, length, callback || noop);
+  binding.sendfile(outFd, inFd, inOffset, length, makeCallback(callback));
 };
 
 fs.sendfileSync = function(outFd, inFd, inOffset, length) {
@@ -436,7 +447,7 @@ fs.sendfileSync = function(outFd, inFd, inOffset, length) {
 };
 
 fs.readdir = function(path, callback) {
-  binding.readdir(pathModule._makeLong(path), callback || noop);
+  binding.readdir(pathModule._makeLong(path), makeCallback(callback));
 };
 
 fs.readdirSync = function(path) {
@@ -444,15 +455,15 @@ fs.readdirSync = function(path) {
 };
 
 fs.fstat = function(fd, callback) {
-  binding.fstat(fd, callback || noop);
+  binding.fstat(fd, makeCallback(callback));
 };
 
 fs.lstat = function(path, callback) {
-  binding.lstat(pathModule._makeLong(path), callback || noop);
+  binding.lstat(pathModule._makeLong(path), makeCallback(callback));
 };
 
 fs.stat = function(path, callback) {
-  binding.stat(pathModule._makeLong(path), callback || noop);
+  binding.stat(pathModule._makeLong(path), makeCallback(callback));
 };
 
 fs.fstatSync = function(fd) {
@@ -468,7 +479,7 @@ fs.statSync = function(path) {
 };
 
 fs.readlink = function(path, callback) {
-  binding.readlink(pathModule._makeLong(path), callback || noop);
+  binding.readlink(pathModule._makeLong(path), makeCallback(callback));
 };
 
 fs.readlinkSync = function(path) {
@@ -477,8 +488,7 @@ fs.readlinkSync = function(path) {
 
 fs.symlink = function(destination, path, type_, callback) {
   var type = (typeof(type_) == 'string' ? type_ : null);
-  var callback_ = arguments[arguments.length - 1];
-  callback = (typeof(callback_) == 'function' ? callback_ : noop);
+  var callback = makeCallback(arguments[arguments.length - 1]);
 
   if (isWindows && type === 'junction') {
     destination = pathModule._makeLong(destination);
@@ -500,7 +510,7 @@ fs.symlinkSync = function(destination, path, type) {
 fs.link = function(srcpath, dstpath, callback) {
   binding.link(pathModule._makeLong(srcpath),
                pathModule._makeLong(dstpath),
-               callback || noop);
+               makeCallback(callback));
 };
 
 fs.linkSync = function(srcpath, dstpath) {
@@ -509,7 +519,7 @@ fs.linkSync = function(srcpath, dstpath) {
 };
 
 fs.unlink = function(path, callback) {
-  binding.unlink(pathModule._makeLong(path), callback || noop);
+  binding.unlink(pathModule._makeLong(path), makeCallback(callback));
 };
 
 fs.unlinkSync = function(path) {
@@ -517,7 +527,7 @@ fs.unlinkSync = function(path) {
 };
 
 fs.fchmod = function(fd, mode, callback) {
-  binding.fchmod(fd, modeNum(mode), callback || noop);
+  binding.fchmod(fd, modeNum(mode), makeCallback(callback));
 };
 
 fs.fchmodSync = function(fd, mode) {
@@ -526,7 +536,7 @@ fs.fchmodSync = function(fd, mode) {
 
 if (constants.hasOwnProperty('O_SYMLINK')) {
   fs.lchmod = function(path, mode, callback) {
-    callback = callback || noop;
+    callback = callback || (function() {});
     fs.open(path, constants.O_WRONLY | constants.O_SYMLINK, function(err, fd) {
       if (err) {
         callback(err);
@@ -565,7 +575,9 @@ if (constants.hasOwnProperty('O_SYMLINK')) {
 
 
 fs.chmod = function(path, mode, callback) {
-  binding.chmod(pathModule._makeLong(path), modeNum(mode), callback || noop);
+  binding.chmod(pathModule._makeLong(path),
+                modeNum(mode),
+                makeCallback(callback));
 };
 
 fs.chmodSync = function(path, mode) {
@@ -574,7 +586,7 @@ fs.chmodSync = function(path, mode) {
 
 if (constants.hasOwnProperty('O_SYMLINK')) {
   fs.lchown = function(path, uid, gid, callback) {
-    callback = callback || noop;
+    callback = callback || (function() {});
     fs.open(path, constants.O_WRONLY | constants.O_SYMLINK, function(err, fd) {
       if (err) {
         callback(err);
@@ -591,7 +603,7 @@ if (constants.hasOwnProperty('O_SYMLINK')) {
 }
 
 fs.fchown = function(fd, uid, gid, callback) {
-  binding.fchown(fd, uid, gid, callback || noop);
+  binding.fchown(fd, uid, gid, makeCallback(callback));
 };
 
 fs.fchownSync = function(fd, uid, gid) {
@@ -599,7 +611,7 @@ fs.fchownSync = function(fd, uid, gid) {
 };
 
 fs.chown = function(path, uid, gid, callback) {
-  binding.chown(pathModule._makeLong(path), uid, gid, callback || noop);
+  binding.chown(pathModule._makeLong(path), uid, gid, makeCallback(callback));
 };
 
 fs.chownSync = function(path, uid, gid) {
@@ -622,9 +634,10 @@ function toUnixTimestamp(time) {
 fs._toUnixTimestamp = toUnixTimestamp;
 
 fs.utimes = function(path, atime, mtime, callback) {
-  atime = toUnixTimestamp(atime);
-  mtime = toUnixTimestamp(mtime);
-  binding.utimes(pathModule._makeLong(path), atime, mtime, callback || noop);
+  binding.utimes(pathModule._makeLong(path),
+                 toUnixTimestamp(atime),
+                 toUnixTimestamp(mtime),
+                 makeCallback(callback));
 };
 
 fs.utimesSync = function(path, atime, mtime) {
@@ -636,7 +649,7 @@ fs.utimesSync = function(path, atime, mtime) {
 fs.futimes = function(fd, atime, mtime, callback) {
   atime = toUnixTimestamp(atime);
   mtime = toUnixTimestamp(mtime);
-  binding.futimes(fd, atime, mtime, callback || noop);
+  binding.futimes(fd, atime, mtime, makeCallback(callback));
 };
 
 fs.futimesSync = function(fd, atime, mtime) {
diff --git a/test/simple/test-fs-stat.js b/test/simple/test-fs-stat.js
index 40782029b..90b0ba52d 100644
--- a/test/simple/test-fs-stat.js
+++ b/test/simple/test-fs-stat.js
@@ -19,9 +19,6 @@
 // OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE
 // USE OR OTHER DEALINGS IN THE SOFTWARE.
 
-
-
-
 var common = require('../common');
 var assert = require('assert');
 var fs = require('fs');
@@ -36,6 +33,7 @@ fs.stat('.', function(err, stats) {
     assert.ok(stats.mtime instanceof Date);
     success_count++;
   }
+  assert(this === global);
 });
 
 fs.lstat('.', function(err, stats) {
@@ -46,6 +44,7 @@ fs.lstat('.', function(err, stats) {
     assert.ok(stats.mtime instanceof Date);
     success_count++;
   }
+  assert(this === global);
 });
 
 // fstat
@@ -62,7 +61,10 @@ fs.open('.', 'r', undefined, function(err, fd) {
       success_count++;
       fs.close(fd);
     }
+    assert(this === global);
   });
+
+  assert(this === global);
 });
 
 // fstatSync