Fix TooTallNate#35: Call callback directly to avoid unwanted preemption.

Nan's NanCallback::Call goes through Node's MakeCallback, which is
designed to be used only by event callbacks (i.e. functions called
from the libuv event loop), not invoked synchronously from a call
stack where JS code was already running. This is because when
MakeCallback returns it assumes it's the end of the current "tick", so
it calls not only the callback you told it to, but also all tick
callbacks registered for the current tick.

Since node-weak's TargetCallback is invoked from GC, which could be
invoked from anywhere at any time, the code it calls into (the
callback attached to each weakref object) needs to be designed and
written carefully, essentially to the standards of signal or interrupt
handlers, since they preempt any JS code that was already running when
GC kicked in.

This is fine as long as the weakref callbacks are written to this
standard, but it also means TargetCallback needs to avoid accidentally
calling other callbacks that weren't written to this standard.

The fix is to call the callback directly, instead of via MakeCallback.

Author: metamatt

src/weakref.cc

@@ -136,7 +136,8 @@ NAN_PROPERTY_ENUMERATOR(WeakPropertyEnumerator) {
 }
 
 /**
- * Weakref callback function. Invokes the "global" callback function.
+ * Weakref callback function. Invokes the "global" callback function,
+ * which emits the _CB event on the per-object EventEmitter.
  */
 
 NAN_WEAK_CALLBACK(TargetCallback) {
@@ -150,7 +151,11 @@ NAN_WEAK_CALLBACK(TargetCallback) {
     NanNew<Object>(cont->cbinfo->persistent),
     NanNew<Object>(cont->emitter)
   };
-  globalCallback->Call(2, argv);
+  // invoke callback directly, not via NanCallback which uses MakeCallback
+  // which calls into process._tickCallback too. Those other callbacks are
+  // not safe to run from here.
+  v8::Local<v8::Function> globalCallbackDirect = globalCallback->GetFunction();
+  globalCallbackDirect->Call(NanGetCurrentContext()->Global(), 2, argv);
 
   // clean everything up
   Local<Object> proxy = NanNew<Object>(cont->proxy);

test/callback.js

@@ -45,6 +45,29 @@ describe('weak()', function () {
       assert(called2)
     })
 
+    it('should preempt code for GC callback but not nextTick callbacks'
+    , function(done) {
+      var calledGcCallback = false
+      , calledTickCallback = false
+      weak({}, function() {
+        calledGcCallback = true
+      })
+
+      process.nextTick(function() {
+        calledTickCallback = true
+      });
+
+      assert(!calledGcCallback)
+      assert(!calledTickCallback)
+      gc()
+      assert(calledGcCallback)
+      assert(!calledTickCallback)
+      setTimeout(function() {
+        assert(calledTickCallback);
+        done();
+      }, 0)
+    })
+
   })
 })