Fix for timers in modal dialogs on OSX

[Mike Lischke]

Hi Neil,

just got around fixing a nasty crash in my application that uses Scintilla in modal dialogs on OSX (10.11). What turned out is that Scintilla uses timers in certain places and they are not working well in modal dialogs. So, here is a patch that makes it working nicely:

diff --git a/ext/scintilla/cocoa/ScintillaCocoa.mm b/ext/scintilla/cocoa/ScintillaCocoa.mm
index 621f43f..1d67c35 100644
--- a/ext/scintilla/cocoa/ScintillaCocoa.mm
+++ b/ext/scintilla/cocoa/ScintillaCocoa.mm
@@ -358,7 +358,7 @@ const CGFloat paddingHighlightY = 2;
[notificationQueue enqueueNotification: notification
postingStyle: NSPostWhenIdle
coalesceMask: (NSNotificationCoalescingOnName | NSNotificationCoalescingOnSender)
- forModes: nil];
+ forModes: @[NSModalPanelRunLoopMode, NSDefaultRunLoopMode]];
}

//--------------------------------------------------------------------------------------------------
@@ -890,6 +890,7 @@ void ScintillaCocoa::SetTicking(bool on)
selector: @selector(timerFired:)
userInfo: nil
repeats: YES];
+ [NSRunLoop.currentRunLoop addTimer: tickTimer forMode: NSModalPanelRunLoopMode];
timer.tickerID = reinterpret_cast<TickerID>(tickTimer);
}
else
@@ -917,6 +918,7 @@ bool ScintillaCocoa::SetIdle(bool on)
selector: @selector(idleTimerFired:)
userInfo: nil
repeats: YES];
+ [NSRunLoop.currentRunLoop addTimer: idleTimer forMode: NSModalPanelRunLoopMode];
idler.idlerID = reinterpret_cast<IdlerID>(idleTimer);
}
else

What happens without that is that an idleTimerFired call is triggered when the dialog closes (because it got stuck during the runtime of the dialog) and that triggers idleTriggered:, which then calls IdleTimerFired on an invalid scintilla instance (because in the meantime it was destroyed).

By registering for modal loops too we don’t see that problem because the timer fires as intended during the runtime of the dialog and the timer gets disabled.

A similar problem exists for the tick timer. The caret doesn’t blink in modal dialogs. Now with that patch it does.

Mike
--
www.soft-gems.net

Mike
--
www.soft-gems.net

[Mike Lischke]

I’m sorry, I diff’ed that against an older version of Scintilla. In the newer one the tick timer is gone and instead the FineTimer is used. But the principle applies to it as well. Just check all occurrences of scheduledTimerWithTimeInterval:target:selector:userInfo:repeats:.

Mike
--
www.soft-gems.net

[Neil Hodgson]

Mike Lischke:

> just got around fixing a nasty crash in my application that uses Scintilla in modal dialogs on OSX (10.11). What turned out is that Scintilla uses timers in certain places and they are not working well in modal dialogs. So, here is a patch that makes it working nicely:

It appears to me there may be 2 bugs here: (1) no ticking in modal dialogs (not fatal) and (2) a crash due to object lifetimes. Adding the modal panel run mode fixes (1) and decreases the scope for (2) but does not eliminate it.

Everything discussed here is on the main thread.
The sequence goes like this:

1) The ScintillaCocoa object is created. It is a C++ object, not an NSObject so does not have a reference count and can not directly receive timer events and notifications.
2) In the ScintillaCocoa constructor a TimerTarget object is created to receive events with a pointer back to its ScintillaCocoa ‘owner’. It now has a ref-count of 1.
3) The TimerTarget registers as an observer with the default notification center. This does not change its ref-count.
4) ScintillaCocoa needs some idle events so creates a repeating timer whose target is the TimerTarget. The timer holds a strong reference to the TimerTarget which now has a ref-count of 2.
5) The idle timer times out and calls idleTimerFired: on the TimerTarget. An NSNotification is queued back to the TimerTarget with NSPostWhenIdle so it should only be received when the application is truly idle. The notification stores a reference to the TimerTarget which bumps the ref-count up to 3.
6) The application decides to destroy the ScintillaView and thus the ScintillaCocoa destructor is called. SetIdle(false) is called which calls invalidate on the repeating timer from step 4 which releases the TimerTarget so its ref-count is down to 2. The TimerTarget is released explicitly at the end of the ScintillaCocoa destructor so its ref-count is down to 1.
7) The ScintillaCocoa object is now dead but the TimerTarget is still alive and has a pointer to the ScintillaCocoa object.
8) The NSNotification arrives at the idleTriggered: method of the TimerTarget which calls the IdleTimerFired method on the dead ScintillaCocoa object causing unpleasantness.
9) The NSNotification is finished so is deallocated and releases the TimerTarget which finally has a ref-count of 0 so is deallocated.

This may occur statistically whenever an idling Scintilla is destroyed. Mike’s example with the modal dialog just ensures that any notification will be delayed until after the destruction.

The TimerTarget is shared between idle timers and period timers so the ref-count will not be quite as shown but the period timers are simpler since they never involve the notification queue. All the period timers are cancelled by calling FineTickerCancel in ScintillaCocoa::Finalise in the ScintillaCocoa destructor. Its only the extra references to the TimerTarget in the notification queue which lead to problems.

To fix this correctly, the outstanding notifications should be discarded inside the destructor for the ScintillaCocoa object. This is actually a little tricky as the call for deleting queue elements takes a pointer to the notification object. The notification object is not remembered by the TimerTarget and is ‘slippy’ due to the coalescing nature of the queue: is the new notification dropped or the one already in the queue? While the previous notification could be remembered and dequeued explicitly before enqueuing the new notification, it should be sufficient to release the queue object at shutdown. Further, the pointer back to ScintillaCocoa should be NULLed in the ScintillaCocoa destructor so that there is no possibility of calling back into ScintillaCocoa.

I think the following change should be sufficient for fixing the crashes with modal dialogs without changing the run loop modes. It would help eliminate the possibility of crashes to confirm this in Mike’s code before changing the run loop modes.

diff -r b5b60d746cda cocoa/ScintillaCocoa.mm
--- a/cocoa/ScintillaCocoa.mm Wed Oct 14 14:55:30 2015 +1100
+++ b/cocoa/ScintillaCocoa.mm Fri Oct 16 16:17:20 2015 +1100
@@ -334,6 +334,18 @@
//--------------------------------------------------------------------------------------------------

/**
+ * Method called by owning ScintillaCocoa object when it is destroyed.
+ */
+- (void) ownerDestroyed
+{
+ mTarget = NULL;
+ [notificationQueue release];
+ notificationQueue = nil;
+}
+
+//--------------------------------------------------------------------------------------------------
+
+/**
* Method called by a timer installed by ScintillaCocoa. This two step approach is needed because
* a native Obj-C class is required as target for the timer.
*/
@@ -408,6 +420,7 @@
ScintillaCocoa::~ScintillaCocoa()
{
Finalise();
+ [timerTarget ownerDestroyed];
[timerTarget release];
}

As a further issue, the TimerTarget registers for “Idle” notifications with object nil. This implies that all Scintilla objects receive idle events meant not just for themselves but for any other Scintilla instances in the application. This should be tightened up if that is the case.

Neil

[Neil Hodgson]

   Committed change that makes timers active in modal dialogs as well as severing the connection from the TimerTarget back to ScintillaCocoa when ScintillaCocoa is freed. Also removed a couple of timer variables that aren’t needed.

http://sourceforge.net/p/scintilla/code/ci/3376090d7d15acaecaa6e4c5bfc0cd02a0c9feb9/

http://sourceforge.net/p/scintilla/code/ci/a64c41ee61bf18e5fdf4d63fb81c01ac5970ffd0/

http://sourceforge.net/p/scintilla/code/ci/a98eaae31a3fb4ce608c2bc79cead556b3f08678/

   Neil

[Mike Lischke]

Hi Neil,

sorry, I haven’t found time to read everything and reply properly to your discussion about this fix. But from what I see the 3 commits seem to address it well.

However, looking at the line

[reinterpret_cast<NSTimer*>(timer.tickerID) invalidate];

I remembered something else I wanted to talk about before. Why do we need these (quite dangerous) cast here? It’s because TickerID is defined as void*. Similar for WindowID, SurfaceID etc. Wouldn’t it be safer to wrap those typedefs with platform #ifdefs and define real types for them (e.g. TickerID could defined as NSTimer* on OSX and wouldn’t need a cast after that).

Mike
-- 
www.soft-gems.net

[Neil Hodgson]

Hi Mike,

> [reinterpret_cast<NSTimer*>(timer.tickerID) invalidate];
>
> I remembered something else I wanted to talk about before. Why do we need these (quite dangerous) cast here? It’s because TickerID is defined as void*. Similar for WindowID, SurfaceID etc. Wouldn’t it be safer to wrap those typedefs with platform #ifdefs and define real types for them (e.g. TickerID could defined as NSTimer* on OSX and wouldn’t need a cast after that).

These types do not always correspond to a single platform type. On Win32, for example, a SurfaceID may be either a HDC when using the old GDI API and an ID2D1RenderTarget when using DirectDraw. This is a run-time choice, not a build-time choice as DirectDraw may be used when drawing to the screen and GDI when printing. DirectDraw printing is not supported by Scintilla and is only available on newer versions of Windows.

This change would hard-code particular ways of interacting with the platform in formerly platform-independent code, making it more difficult for downstream projects to implement new platforms, or new APIs on current platforms. For example, someone may prefer on Cocoa to use NSObject types or Core Foundation types, perhaps to move to or share code with iOS or to resupport old releases of OS X. The type for timers could either be NSTimer or CFRunLoopTimerRef.

Very old versions of Scintilla and SciTE worked a little more like this and it was quite painful because it exposed platform-independent code to large, complex, and sometimes weird platform headers.

There are other ways to achieve some more type-safety without exposing platform types. The PIMPL idiom could be used to provide a fixed type-name to platform-independent code. This still often results in much type casting although it may be a less dangerous static_cast or dynamic_cast instead of reinterpret_cast. Another approach would be to define an interface type in Platform.h and then define a subclass within the platform layer containing the platform type. That will result in some more memory allocation and also some more casts but its probably what I would do for a redesign. Implementing this now would take effort which may result in losing some of the platforms so it doesn’t appear worthwhile to me.

Neil

[Mike Lischke]

Hi Neil,

>> I remembered something else I wanted to talk about before. Why do we need these (quite dangerous) cast here? It’s because TickerID is defined as void*. Similar for WindowID, SurfaceID etc. Wouldn’t it be safer to wrap those typedefs with platform #ifdefs and define real types for them (e.g. TickerID could defined as NSTimer* on OSX and wouldn’t need a cast after that).
>

> There are other ways to achieve some more type-safety without exposing platform types. The PIMPL idiom could be used to provide a fixed type-name to platform-independent code.

Not sure if that idiom would also help with platform specific typedefs, but you may have additional things in mind I don’t see here.

> This still often results in much type casting although it may be a less dangerous static_cast or dynamic_cast instead of reinterpret_cast. Another approach would be to define an interface type in Platform.h and then define a subclass within the platform layer containing the platform type. That will result in some more memory allocation and also some more casts but its probably what I would do for a redesign. Implementing this now would take effort which may result in losing some of the platforms so it doesn’t appear worthwhile to me.

Well, I didn’t expect that a handful typedefs would make such problems. I have seen this in many many header files how types are defined depending on a number of other compiler symbols for platforms + supported features. I don’t see why this wouldn’t work with Scintilla.

However, defining such types via platform specific headers is certainly a good way too. I’m not locked into one specific solution. Just want to get rid of those casts.

Mike
--
www.soft-gems.net