timers: fix setInterval() assert

Test case:

  var t = setInterval(function() {}, 1);
  process.nextTick(t.unref);

Output:

  Assertion failed: (args.Holder()->InternalFieldCount() > 0),
  function Unref, file ../src/handle_wrap.cc, line 78.

setInterval() returns a binding layer object. Make it stop doing that,
wrap the raw process.binding('timer_wrap').Timer object in a Timeout
object.

Fixes #4261.

2 files changed, 32 insertions, 12 deletions

diff --git a/lib/timers.js b/lib/timers.js
index 8431b2ddd..708f0af16 100644
--- a/lib/timers.js
+++ b/lib/timers.js
@@ -222,7 +222,7 @@ exports.setTimeout = function(callback, after) {
 exports.clearTimeout = function(timer) {
   if (timer && (timer.ontimeout || timer._onTimeout)) {
     timer.ontimeout = timer._onTimeout = null;
-    if (timer instanceof Timer || timer instanceof Timeout) {
+    if (timer instanceof Timeout) {
       timer.close(); // for after === 0
     } else {
       exports.unenroll(timer);
@@ -232,39 +232,52 @@ exports.clearTimeout = function(timer) {
 
 
 exports.setInterval = function(callback, repeat) {
-  var timer = new Timer();
-
-  if (process.domain) timer.domain = process.domain;
-
   repeat *= 1; // coalesce to number or NaN
 
   if (!(repeat >= 1 && repeat <= TIMEOUT_MAX)) {
     repeat = 1; // schedule on next tick, follows browser behaviour
   }
 
+  var timer = new Timeout(repeat);
   var args = Array.prototype.slice.call(arguments, 2);
-  timer.ontimeout = function() {
-    callback.apply(timer, args);
-  }
+  timer._onTimeout = wrapper;
+  timer._repeat = true;
+
+  if (process.domain) timer.domain = process.domain;
+  exports.active(timer);
 
-  timer.start(repeat, repeat);
   return timer;
+
+  function wrapper() {
+    callback.apply(this, args);
+    // If callback called clearInterval().
+    if (timer._repeat === false) return;
+    // If timer is unref'd (or was - it's permanently removed from the list.)
+    if (this._handle) {
+      this._handle.start(repeat, 0);
+    } else {
+      timer._idleTimeout = repeat;
+      exports.active(timer);
+    }
+  }
 };
 
 
 exports.clearInterval = function(timer) {
-  if (timer instanceof Timer) {
-    timer.ontimeout = null;
-    timer.close();
+  if (timer && timer._repeat) {
+    timer._repeat = false;
+    clearTimeout(timer);
   }
 };
 
+
 var Timeout = function(after) {
   this._idleTimeout = after;
   this._idlePrev = this;
   this._idleNext = this;
   this._idleStart = null;
   this._onTimeout = null;
+  this._repeat = false;
 };
 
 Timeout.prototype.unref = function() {
diff --git a/test/simple/test-timers-unref.js b/test/simple/test-timers-unref.js
index 4be557e50..1c362cb83 100644
--- a/test/simple/test-timers-unref.js
+++ b/test/simple/test-timers-unref.js
@@ -54,6 +54,13 @@ check_unref = setInterval(function() {
   checks += 1;
 }, 100);
 
+// Should not assert on args.Holder()->InternalFieldCount() > 0. See #4261.
+(function() {
+  var t = setInterval(function() {}, 1);
+  process.nextTick(t.unref.bind({}));
+  process.nextTick(t.unref.bind(t));
+})();
+
 process.on('exit', function() {
   assert.strictEqual(interval_fired, false, 'Interval should not fire');
   assert.strictEqual(timeout_fired, false, 'Timeout should not fire');