From 5341887efb0c68b5eb72c0fa0d7f9b24c2d570ec Mon Sep 17 00:00:00 2001
From: Matt Ginzton <matt@ginzton.net>
Date: Tue, 14 Oct 2014 10:53:02 -0700
Subject: [PATCH] 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.

Fixes #35.
---
 src/weakref.cc   |  9 +++++++--
 test/callback.js | 23 +++++++++++++++++++++++
 2 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/src/weakref.cc b/src/weakref.cc
index 18dfb89..b540b1e 100644
--- a/src/weakref.cc
+++ b/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->Call() which uses
+  // node::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);
diff --git a/test/callback.js b/test/callback.js
index 591dd5a..095046f 100644
--- a/test/callback.js
+++ b/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)
+    })
+
   })
 })