patch 7.4.2332v7.4.2332

Problem:    Crash when stop_timer() is called in a callback of a callback.
            Vim hangs when the timer callback uses too much time.
Solution:   Set tr_id to -1 when a timer is to be deleted. Don't keep calling
            callbacks forever. (Ozaki Kiichi)

6 files changed, 130 insertions, 81 deletions

diff --git a/src/evalfunc.c b/src/evalfunc.c
index 1c1dcf7120..906fa39afc 100644
--- a/src/evalfunc.c
+++ b/src/evalfunc.c
@@ -12398,12 +12398,14 @@ f_timer_pause(typval_T *argvars, typval_T *rettv UNUSED)
     static void
 f_timer_start(typval_T *argvars, typval_T *rettv)
 {
-    long    msec = (long)get_tv_number(&argvars[0]);
-    timer_T *timer;
-    int	    repeat = 0;
-    char_u  *callback;
-    dict_T  *dict;
+    long	msec = (long)get_tv_number(&argvars[0]);
+    timer_T	*timer;
+    int		repeat = 0;
+    char_u	*callback;
+    dict_T	*dict;
+    partial_T	*partial;
 
+    rettv->vval.v_number = -1;
     if (check_secure())
 	return;
     if (argvars[2].v_type != VAR_UNKNOWN)
@@ -12418,13 +12420,13 @@ f_timer_start(typval_T *argvars, typval_T *rettv)
 	    repeat = get_dict_number(dict, (char_u *)"repeat");
     }
 
-    timer = create_timer(msec, repeat);
-    callback = get_callback(&argvars[1], &timer->tr_partial);
+    callback = get_callback(&argvars[1], &partial);
     if (callback == NULL)
-    {
-	stop_timer(timer);
-	rettv->vval.v_number = -1;
-    }
+	return;
+
+    timer = create_timer(msec, repeat);
+    if (timer == NULL)
+	free_callback(callback, partial);
     else
     {
 	if (timer->tr_partial == NULL)
@@ -12432,7 +12434,8 @@ f_timer_start(typval_T *argvars, typval_T *rettv)
 	else
 	    /* pointer into the partial */
 	    timer->tr_callback = callback;
-	rettv->vval.v_number = timer->tr_id;
+	timer->tr_partial = partial;
+	rettv->vval.v_number = (varnumber_T)timer->tr_id;
     }
 }
 
diff --git a/src/ex_cmds2.c b/src/ex_cmds2.c
index acaef9fcef..8df67536c5 100644
--- a/src/ex_cmds2.c
+++ b/src/ex_cmds2.c
@@ -1088,10 +1088,17 @@ profile_zero(proftime_T *tm)
 
 # if defined(FEAT_TIMERS) || defined(PROTO)
 static timer_T	*first_timer = NULL;
-static int	last_timer_id = 0;
+static long	last_timer_id = 0;
 
-static timer_T	*current_timer = NULL;
-static int	free_current_timer = FALSE;
+# ifdef WIN3264
+#  define GET_TIMEDIFF(timer, now) \
+	(long)(((double)(timer->tr_due.QuadPart - now.QuadPart) \
+					   / (double)fr.QuadPart) * 1000);
+# else
+#  define GET_TIMEDIFF(timer, now) \
+	(timer->tr_due.tv_sec - now.tv_sec) * 1000 \
+			   + (timer->tr_due.tv_usec - now.tv_usec) / 1000;
+# endif
 
 /*
  * Insert a timer in the list of timers.
@@ -1124,13 +1131,8 @@ remove_timer(timer_T *timer)
     static void
 free_timer(timer_T *timer)
 {
-    if (timer == current_timer)
-	free_current_timer = TRUE;
-    else
-    {
-	free_callback(timer->tr_callback, timer->tr_partial);
-	vim_free(timer);
-    }
+    free_callback(timer->tr_callback, timer->tr_partial);
+    vim_free(timer);
 }
 
 /*
@@ -1144,7 +1146,10 @@ create_timer(long msec, int repeat)
 
     if (timer == NULL)
 	return NULL;
-    timer->tr_id = ++last_timer_id;
+    if (++last_timer_id < 0)
+	/* Overflow!  Might cause duplicates... */
+	last_timer_id = 0;
+    timer->tr_id = last_timer_id;
     insert_timer(timer);
     if (repeat != 0)
 	timer->tr_repeat = repeat - 1;
@@ -1165,7 +1170,7 @@ timer_callback(timer_T *timer)
     typval_T	argv[2];
 
     argv[0].v_type = VAR_NUMBER;
-    argv[0].vval.v_number = timer->tr_id;
+    argv[0].vval.v_number = (varnumber_T)timer->tr_id;
     argv[1].v_type = VAR_UNKNOWN;
 
     call_func(timer->tr_callback, (int)STRLEN(timer->tr_callback),
@@ -1182,77 +1187,76 @@ timer_callback(timer_T *timer)
 check_due_timer(void)
 {
     timer_T	*timer;
+    timer_T	*timer_next;
     long	this_due;
     long	next_due = -1;
     proftime_T	now;
     int		did_one = FALSE;
+    long	current_id = last_timer_id;
 # ifdef WIN3264
     LARGE_INTEGER   fr;
 
     QueryPerformanceFrequency(&fr);
 # endif
-    while (!got_int)
+    profile_start(&now);
+    for (timer = first_timer; timer != NULL && !got_int; timer = timer_next)
     {
-	profile_start(&now);
-	next_due = -1;
-	for (timer = first_timer; timer != NULL; timer = timer->tr_next)
+	timer_next = timer->tr_next;
+
+	if (timer->tr_id == -1 || timer->tr_firing || timer->tr_paused)
+	    continue;
+	this_due = GET_TIMEDIFF(timer, now);
+	if (this_due <= 1)
 	{
-	    if (timer->tr_paused)
-		continue;
-# ifdef WIN3264
-	    this_due = (long)(((double)(timer->tr_due.QuadPart - now.QuadPart)
-					       / (double)fr.QuadPart) * 1000);
-# else
-	    this_due = (timer->tr_due.tv_sec - now.tv_sec) * 1000
-			       + (timer->tr_due.tv_usec - now.tv_usec) / 1000;
-# endif
-	    if (this_due <= 1)
+	    timer->tr_firing = TRUE;
+	    timer_callback(timer);
+	    timer->tr_firing = FALSE;
+	    timer_next = timer->tr_next;
+	    did_one = TRUE;
+
+	    /* Only fire the timer again if it repeats and stop_timer() wasn't
+	     * called while inside the callback (tr_id == -1). */
+	    if (timer->tr_repeat != 0 && timer->tr_id != -1)
 	    {
-		current_timer = timer;
-		free_current_timer = FALSE;
-		timer_callback(timer);
-		current_timer = NULL;
-
-		did_one = TRUE;
-		if (timer->tr_repeat != 0 && !free_current_timer)
-		{
-		    profile_setlimit(timer->tr_interval, &timer->tr_due);
-		    if (timer->tr_repeat > 0)
-			--timer->tr_repeat;
-		}
-		else
-		{
-		    remove_timer(timer);
-		    free_timer(timer);
-		}
-		/* the callback may do anything, start all over */
-		break;
+		profile_setlimit(timer->tr_interval, &timer->tr_due);
+		this_due = GET_TIMEDIFF(timer, now);
+		if (this_due < 1)
+		    this_due = 1;
+		if (timer->tr_repeat > 0)
+		    --timer->tr_repeat;
+	    }
+	    else
+	    {
+		this_due = -1;
+		remove_timer(timer);
+		free_timer(timer);
 	    }
-	    if (next_due == -1 || next_due > this_due)
-		next_due = this_due;
 	}
-	if (timer == NULL)
-	    break;
+	if (this_due > 0 && (next_due == -1 || next_due > this_due))
+	    next_due = this_due;
     }
 
     if (did_one)
 	redraw_after_callback();
 
-    return next_due;
+    return current_id != last_timer_id ? 1 : next_due;
 }
 
 /*
  * Find a timer by ID.  Returns NULL if not found;
  */
     timer_T *
-find_timer(int id)
+find_timer(long id)
 {
     timer_T *timer;
 
-    for (timer = first_timer; timer != NULL; timer = timer->tr_next)
-	if (timer->tr_id == id)
-	    break;
-    return timer;
+    if (id >= 0)
+    {
+	for (timer = first_timer; timer != NULL; timer = timer->tr_next)
+	    if (timer->tr_id == id)
+		return timer;
+    }
+    return NULL;
 }
 
 
@@ -1262,15 +1266,27 @@ find_timer(int id)
     void
 stop_timer(timer_T *timer)
 {
-    remove_timer(timer);
-    free_timer(timer);
+    if (timer->tr_firing)
+	/* Free the timer after the callback returns. */
+	timer->tr_id = -1;
+    else
+    {
+	remove_timer(timer);
+	free_timer(timer);
+    }
 }
 
     void
 stop_all_timers(void)
 {
-    while (first_timer != NULL)
-	stop_timer(first_timer);
+    timer_T *timer;
+    timer_T *timer_next;
+
+    for (timer = first_timer; timer != NULL; timer = timer_next)
+    {
+	timer_next = timer->tr_next;
+	stop_timer(timer);
+    }
 }
 
     void
@@ -1289,18 +1305,14 @@ add_timer_info(typval_T *rettv, timer_T *timer)
 	return;
     list_append_dict(list, dict);
 
-    dict_add_nr_str(dict, "id", (long)timer->tr_id, NULL);
+    dict_add_nr_str(dict, "id", timer->tr_id, NULL);
     dict_add_nr_str(dict, "time", (long)timer->tr_interval, NULL);
 
     profile_start(&now);
 # ifdef WIN3264
     QueryPerformanceFrequency(&fr);
-    remaining = (long)(((double)(timer->tr_due.QuadPart - now.QuadPart)
-					       / (double)fr.QuadPart) * 1000);
-# else
-    remaining = (timer->tr_due.tv_sec - now.tv_sec) * 1000
-			       + (timer->tr_due.tv_usec - now.tv_usec) / 1000;
 # endif
+    remaining = GET_TIMEDIFF(timer, now);
     dict_add_nr_str(dict, "remaining", (long)remaining, NULL);
 
     dict_add_nr_str(dict, "repeat",
@@ -1333,7 +1345,8 @@ add_timer_info_all(typval_T *rettv)
     timer_T *timer;
 
     for (timer = first_timer; timer != NULL; timer = timer->tr_next)
-	add_timer_info(rettv, timer);
+	if (timer->tr_id != -1)
+	    add_timer_info(rettv, timer);
 }
 
 /*
diff --git a/src/proto/ex_cmds2.pro b/src/proto/ex_cmds2.pro
index f241d377e8..c1325a41e3 100644
--- a/src/proto/ex_cmds2.pro
+++ b/src/proto/ex_cmds2.pro
@@ -20,7 +20,7 @@ int profile_passed_limit(proftime_T *tm);
 void profile_zero(proftime_T *tm);
 timer_T *create_timer(long msec, int repeat);
 long check_due_timer(void);
-timer_T *find_timer(int id);
+timer_T *find_timer(long id);
 void stop_timer(timer_T *timer);
 void stop_all_timers(void);
 void add_timer_info(typval_T *rettv, timer_T *timer);
diff --git a/src/structs.h b/src/structs.h
index a44bcec4cb..8bdd3c9cb3 100644
--- a/src/structs.h
+++ b/src/structs.h
@@ -3166,12 +3166,13 @@ typedef struct js_reader js_read_T;
 typedef struct timer_S timer_T;
 struct timer_S
 {
-    int		tr_id;
+    long	tr_id;
 #ifdef FEAT_TIMERS
     timer_T	*tr_next;
     timer_T	*tr_prev;
     proftime_T	tr_due;		    /* when the callback is to be invoked */
-    int		tr_paused;	    /* when TRUE callback is not invoked */
+    char	tr_firing;	    /* when TRUE callback is being called */
+    char	tr_paused;	    /* when TRUE callback is not invoked */
     int		tr_repeat;	    /* number of times to repeat, -1 forever */
     long	tr_interval;	    /* msec */
     char_u	*tr_callback;	    /* allocated */
diff --git a/src/testdir/test_timers.vim b/src/testdir/test_timers.vim
index e6e54a4cbd..07c6876eee 100644
--- a/src/testdir/test_timers.vim
+++ b/src/testdir/test_timers.vim
@@ -143,4 +143,34 @@ func Test_delete_myself()
   call assert_equal([], timer_info(t))
 endfunc
 
+func StopTimer1(timer)
+  let g:timer2 = timer_start(10, 'StopTimer2')
+  " avoid maxfuncdepth error
+  call timer_pause(g:timer1, 1)
+  sleep 40m
+endfunc
+
+func StopTimer2(timer)
+  call timer_stop(g:timer1)
+endfunc
+
+func Test_stop_in_callback()
+  let g:timer1 = timer_start(10, 'StopTimer1')
+  sleep 40m
+endfunc
+
+func StopTimerAll(timer)
+  call timer_stopall()
+endfunc
+
+func Test_stop_all_in_callback()
+  let g:timer1 = timer_start(10, 'StopTimerAll')
+  let info = timer_info()
+  call assert_equal(1, len(info))
+  sleep 40m
+  let info = timer_info()
+  call assert_equal(0, len(info))
+endfunc
+
+
 " vim: shiftwidth=2 sts=2 expandtab
diff --git a/src/version.c b/src/version.c
index 740d978e90..98b7225628 100644
--- a/src/version.c
+++ b/src/version.c
@@ -764,6 +764,8 @@ static char *(features[]) =
 static int included_patches[] =
 {   /* Add new patch number below this line */
 /**/
+    2332,
+/**/
     2331,
 /**/
     2330,