From 6c7715876d4322df4afede6a146c8412eb15cd57 Mon Sep 17 00:00:00 2001 From: Ryan May Date: Wed, 1 Nov 2017 18:11:21 -0600 Subject: [PATCH 1/4] BUG: Fix crash when restarting OSX single shot timer (Fixes #9655) Previously, if we had a pointer to an old timer when calling start(), we would manually get the context for the old timer and from there get a pointer to the attribute. The problem is that for a single-timer, the timer is automatically invalidated (and context cleared) at the end. This resulted in trying to Py_DECREF(0x0). To fix this, use the built-in support for a callback to run when the context is released. By doing so, we can also clean up a lot of code to release the reference in stop() as well. --- src/_macosx.m | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/_macosx.m b/src/_macosx.m index a1254412c0ad..ab1a8ad69c35 100644 --- a/src/_macosx.m +++ b/src/_macosx.m @@ -2887,6 +2887,11 @@ static void timer_callback(CFRunLoopTimerRef timer, void* info) PyGILState_Release(gstate); } +static void context_cleanup(const void* info) +{ + Py_DECREF((PyObject*)info); +} + static PyObject* Timer__timer_start(Timer* self, PyObject* args) { @@ -2904,7 +2909,7 @@ static void timer_callback(CFRunLoopTimerRef timer, void* info) } context.version = 0; context.retain = 0; - context.release = 0; + context.release = context_cleanup; context.copyDescription = 0; attribute = PyObject_GetAttrString((PyObject*)self, "_interval"); if (attribute==NULL) @@ -2958,10 +2963,7 @@ static void timer_callback(CFRunLoopTimerRef timer, void* info) } Py_INCREF(attribute); if (self->timer) { - CFRunLoopTimerGetContext(self->timer, &context); - attribute = context.info; - Py_DECREF(attribute); - CFRunLoopRemoveTimer(runloop, self->timer, kCFRunLoopCommonModes); + CFRunLoopTimerInvalidate(self->timer); CFRelease(self->timer); } CFRunLoopAddTimer(runloop, timer, kCFRunLoopCommonModes); @@ -2977,15 +2979,7 @@ static void timer_callback(CFRunLoopTimerRef timer, void* info) Timer__timer_stop(Timer* self) { if (self->timer) { - PyObject* attribute; - CFRunLoopTimerContext context; - CFRunLoopTimerGetContext(self->timer, &context); - attribute = context.info; - Py_DECREF(attribute); - CFRunLoopRef runloop = CFRunLoopGetCurrent(); - if (runloop) { - CFRunLoopRemoveTimer(runloop, self->timer, kCFRunLoopCommonModes); - } + CFRunLoopTimerInvalidate(self->timer); CFRelease(self->timer); self->timer = NULL; } From f3b4511700b9b170ffac33c5b9ea38efce4be98b Mon Sep 17 00:00:00 2001 From: Ryan May Date: Wed, 1 Nov 2017 21:21:40 -0600 Subject: [PATCH 2/4] MNT: Use Py_RETURN_NONE as appropriate Simplifies things just a bit. --- src/_macosx.m | 54 +++++++++++++++++---------------------------------- 1 file changed, 18 insertions(+), 36 deletions(-) diff --git a/src/_macosx.m b/src/_macosx.m index ab1a8ad69c35..6e4e089d5f7d 100644 --- a/src/_macosx.m +++ b/src/_macosx.m @@ -348,8 +348,7 @@ static CGFloat _get_device_scale(CGContextRef cr) [pool release]; } - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject* @@ -362,8 +361,7 @@ static CGFloat _get_device_scale(CGContextRef cr) return NULL; } [view setNeedsDisplay: YES]; - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject* @@ -376,8 +374,7 @@ static CGFloat _get_device_scale(CGContextRef cr) return NULL; } [view displayIfNeeded]; - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject* @@ -420,8 +417,7 @@ static CGFloat _get_device_scale(CGContextRef cr) } [view setRubberband: rubberband]; - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject* @@ -434,8 +430,7 @@ static CGFloat _get_device_scale(CGContextRef cr) return NULL; } [view removeRubberband]; - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static NSImage* _read_ppm_image(PyObject* obj) @@ -546,8 +541,7 @@ static CGFloat _get_device_scale(CGContextRef cr) if (error==0) close(channel[1]); if (interrupted) raise(SIGINT); - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject* @@ -563,8 +557,7 @@ static CGFloat _get_device_scale(CGContextRef cr) data1: 0 data2: 0]; [NSApp postEvent: event atStart: true]; - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyMethodDef FigureCanvas_methods[] = { @@ -770,8 +763,7 @@ static CGFloat _get_device_scale(CGContextRef cr) [window orderFrontRegardless]; [pool release]; } - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject* @@ -785,8 +777,7 @@ static CGFloat _get_device_scale(CGContextRef cr) [pool release]; self->window = NULL; } - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject* @@ -808,8 +799,7 @@ static CGFloat _get_device_scale(CGContextRef cr) [pool release]; } PyMem_Free(title); - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject* @@ -830,8 +820,7 @@ static CGFloat _get_device_scale(CGContextRef cr) if (result) { return result; } else { - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } } @@ -1280,8 +1269,7 @@ -(void)save_figure:(id)sender [button setEnabled: YES]; } [pool release]; - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject* @@ -1783,8 +1771,7 @@ -(void)save_figure:(id)sender [pool release]; } - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyMethodDef NavigationToolbar2_methods[] = { @@ -1888,8 +1875,7 @@ -(void)save_figure:(id)sender free(buffer); return string; } - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject* @@ -1906,8 +1892,7 @@ -(void)save_figure:(id)sender case 4: break; default: return NULL; } - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } @implementation WindowServerConnectionManager @@ -2845,8 +2830,7 @@ - (int)index Py_BEGIN_ALLOW_THREADS [NSApp run]; Py_END_ALLOW_THREADS - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } typedef struct { @@ -2971,8 +2955,7 @@ static void context_cleanup(const void* info) * the timer lost before we have a chance to decrease the reference count * of the attribute */ self->timer = timer; - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static PyObject* @@ -2983,8 +2966,7 @@ static void context_cleanup(const void* info) CFRelease(self->timer); self->timer = NULL; } - Py_INCREF(Py_None); - return Py_None; + Py_RETURN_NONE; } static void From 3a5c909800d761d957aeefd95bf45343196d7da5 Mon Sep 17 00:00:00 2001 From: Ryan May Date: Wed, 1 Nov 2017 21:25:19 -0600 Subject: [PATCH 3/4] BUG: Fix some incorrect reference handling --- src/_macosx.m | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/_macosx.m b/src/_macosx.m index 6e4e089d5f7d..e30986181786 100644 --- a/src/_macosx.m +++ b/src/_macosx.m @@ -2867,7 +2867,11 @@ static void timer_callback(CFRunLoopTimerRef timer, void* info) PyObject* method = info; PyGILState_STATE gstate = PyGILState_Ensure(); PyObject* result = PyObject_CallFunction(method, NULL); - if (result==NULL) PyErr_Print(); + if (result) { + Py_DECREF(result); + } else { + PyErr_Print(); + } PyGILState_Release(gstate); } @@ -2923,6 +2927,7 @@ static void context_cleanup(const void* info) PyErr_SetString(PyExc_ValueError, "Cannot interpret _single attribute as True of False"); return NULL; } + Py_DECREF(attribute); attribute = PyObject_GetAttrString((PyObject*)self, "_on_timer"); if (attribute==NULL) { @@ -2942,10 +2947,10 @@ static void context_cleanup(const void* info) timer_callback, &context); if (!timer) { + Py_DECREF(attribute); PyErr_SetString(PyExc_RuntimeError, "Failed to create timer"); return NULL; } - Py_INCREF(attribute); if (self->timer) { CFRunLoopTimerInvalidate(self->timer); CFRelease(self->timer); From 259a89b1ec70eb052dad1b88b4c6a95c09879a44 Mon Sep 17 00:00:00 2001 From: Ryan May Date: Wed, 1 Nov 2017 21:46:12 -0600 Subject: [PATCH 4/4] MNT: Clean up initializing osx timer context Group setting all attributes together. Use NULL (rather than 0) where appropriate for clarity. --- src/_macosx.m | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/_macosx.m b/src/_macosx.m index e30986181786..df42a5c099a9 100644 --- a/src/_macosx.m +++ b/src/_macosx.m @@ -2895,10 +2895,6 @@ static void context_cleanup(const void* info) PyErr_SetString(PyExc_RuntimeError, "Failed to obtain run loop"); return NULL; } - context.version = 0; - context.retain = 0; - context.release = context_cleanup; - context.copyDescription = 0; attribute = PyObject_GetAttrString((PyObject*)self, "_interval"); if (attribute==NULL) { @@ -2938,6 +2934,10 @@ static void context_cleanup(const void* info) PyErr_SetString(PyExc_RuntimeError, "_on_timer should be a Python method"); return NULL; } + context.version = 0; + context.retain = NULL; + context.release = context_cleanup; + context.copyDescription = NULL; context.info = attribute; timer = CFRunLoopTimerCreate(kCFAllocatorDefault, 0,