Merge pull request #2297 from keymanapp/fix/windows/2147-exception-closing-keyman

fix(windows): Keyman was crashing sporadically

windows/src/engine/keyman/UfrmKeyman7Main.pas

@@ -442,6 +442,10 @@ begin
   begin
     olestrm := TOLEStream.Create(istrm);
     try
+      // In some situations, launching the app multiple times rapidly can
+      // cause the icon to be loaded multiple times. Make sure we reset the
+      // stream position before we try and read.
+      olestrm.Position := 0;
       Application.Icon.LoadFromStream(olestrm);
     finally
       olestrm.Free;

windows/src/engine/keyman/keyman.dpr

@@ -1,7 +1,5 @@
 program keyman;
 
-
-
 uses
   Forms,
   Dialogs,
@@ -148,6 +146,11 @@ uses
 {$R VERSION.RES}
 {$R MANIFEST.RES}
 
+//
+// PEOPTFLAGS $140 turns on Data Execution Prevention
+//
+{$SETPEOPTFLAGS $140}
+
 begin
   //InitTntEnvironment;
   //ShowMessage('Start');

windows/src/engine/keyman32/Keyman32.cpp

@@ -157,11 +157,13 @@ BOOL __stdcall DllMain(HINSTANCE hinstDll, DWORD fdwReason, LPVOID reserved)
 	case DLL_PROCESS_ATTACH:
     //if(!TestDebugProcess()) return FALSE;
     //if(!ShouldAttachToProcess()) return FALSE;
+    OutputThreadDebugString("DLL_PROCESS_ATTACH");
     if(!Globals_InitProcess()) return FALSE;
 		break;
 	case DLL_PROCESS_DETACH:
     //if(!TestDebugProcess()) return FALSE;
     if (reserved == NULL) {
+      OutputThreadDebugString("DLL_PROCESS_DETACH not terminating");
       // If reserved == NULL, that means the library is being unloaded, but
       // the process is not terminating.
       //
@@ -180,15 +182,20 @@ BOOL __stdcall DllMain(HINSTANCE hinstDll, DWORD fdwReason, LPVOID reserved)
       // CloseTSF from here. This needs further investigation...
       UninitialiseProcess(FALSE);
       Globals_UninitProcess();
+    }
+    else {
+      OutputThreadDebugString("DLL_PROCESS_DETACH terminating");
     }
 		break;
 	case DLL_THREAD_ATTACH:
     //if(!TestDebugProcess()) return FALSE;
+    OutputThreadDebugString("DLL_THREAD_ATTACH");
     Globals_InitThread();
 		break;
 	case DLL_THREAD_DETACH:
     //if(!TestDebugProcess()) return FALSE;
-		UninitialiseProcess(FALSE);
+    OutputThreadDebugString("DLL_THREAD_DETACH");
+    UninitialiseProcess(FALSE);
     Globals_UninitThread();
 		break;
 	}
@@ -214,8 +221,13 @@ BOOL UninitialiseProcess(BOOL Lock)
 
 	  if(_td->IndexStack) delete _td->IndexStack;
 	  _td->IndexStack = NULL;
-  }
 
+    if (_td->miniContext) delete _td->miniContext;
+    _td->miniContext = NULL;
+
+    if (_td->msgbuf) delete _td->msgbuf;
+    _td->msgbuf = NULL;
+  }
 	return TRUE;
 }
 
@@ -512,6 +524,10 @@ extern "C" BOOL _declspec(dllexport) WINAPI Keyman_Exit(void)
     return FALSE;
   }
 
+#ifndef _WIN64
+  Hotkeys::Unload();
+#endif
+
   *Globals::InitialisingThread() = 0;
 
   BOOL RetVal = TRUE;
@@ -801,6 +817,7 @@ void LoadBaseLayoutSettings() {   // I4552   // I4583
 
 void RefreshKeyboards(BOOL Initialising)
 {
+  OutputThreadDebugString("RefreshKeyboards");
 	char sz[_MAX_FNAME];
 	char oldname[_MAX_FNAME];
 	RegistryReadOnly *reg2;
@@ -828,7 +845,7 @@ void RefreshKeyboards(BOOL Initialising)
 		_td->ActiveKeymanID = KEYMANID_NONKEYMAN;
   }
 
-	ReleaseKeyboards(TRUE);
+  ReleaseKeyboards(TRUE);
 
 	/* Read the "keyboard off hotkey", simulate Alt+Gr, Hotkeys-Toggle flags */ 
 
@@ -938,13 +955,14 @@ void RefreshKeyboards(BOOL Initialising)
 
   _td->FInRefreshKeyboards = FALSE;
 }
-
 void ReleaseKeyboards(BOOL Lock)
 {
+  OutputThreadDebugString("ReleaseKeyboards");
   PKEYMAN64THREADDATA _td = ThreadGlobals();
-	if(!_td || _td->lpKeyboards) return;
+	if(!_td || !_td->lpKeyboards) return;
 
-	if(Lock) if(_td->lpActiveKeyboard && !_td->ForceFileName[0]) DeactivateDLLs(_td->lpActiveKeyboard);
+
+  if(Lock) if(_td->lpActiveKeyboard && !_td->ForceFileName[0]) DeactivateDLLs(_td->lpActiveKeyboard);
 
 	for(int i = 0; i < _td->nKeyboards; i++) 
 	{

windows/src/engine/keyman32/hookutils.cpp

@@ -15,45 +15,48 @@ typedef BOOL
     IN CONST PMINIDUMP_CALLBACK_INFORMATION CallbackParam OPTIONAL
     );
 
+DWORD ExceptionMessage(LPSTR Proc, LPEXCEPTION_POINTERS ep) {
+#ifndef _DEBUG
+  UNREFERENCED_PARAMETER(Proc);
+#endif
 
-DWORD ExceptionMessage(LPSTR Proc, LPEXCEPTION_POINTERS ep)
-{
   MINIDUMP_EXCEPTION_INFORMATION mei;
   char filename[MAX_PATH], temppath[MAX_PATH];
-  if(GetTempPath(MAX_PATH, temppath) == 0 ||
-      GetTempFileName(temppath, "kmc", 0, filename) == 0)
-    SendDebugMessageFormat(0, sdmGlobal, 0, "Minidump failed to generate temp file name");
-  else
-  {
+  if (GetTempPath(MAX_PATH, temppath) == 0 ||
+      GetTempFileName(temppath, "kmc", 0, filename) == 0) {
+    OutputThreadDebugString("Minidump failed to generate temp file name\n");
+  } 
+  else {
     HANDLE hFile = CreateFile(filename, GENERIC_WRITE, 0, NULL, CREATE_ALWAYS, 0, NULL);
-    if(!hFile)
-      SendDebugMessageFormat(0, sdmGlobal, 0, "Minidump failed to create file %s", filename);
-    else
-    {
+    if (!hFile) {
+      OutputThreadDebugString("Minidump failed to create file ");
+      OutputThreadDebugString(filename);
+    }
+    else {
       mei.ClientPointers = TRUE;
       mei.ExceptionPointers = ep;
       mei.ThreadId = GetCurrentThreadId();
       HMODULE hDbgHelp = LoadLibrary("dbghelp.dll");
-      if(!hDbgHelp)
-        SendDebugMessage(0, sdmGlobal, 0, "dbghelp.dll not available");
-      else
-      {
+      if (!hDbgHelp) {
+        OutputThreadDebugString("dbghelp.dll not available");
+      }
+      else {
         PMiniDumpWriteDump mdwd = (PMiniDumpWriteDump) GetProcAddress(hDbgHelp, "MiniDumpWriteDump");
-        if(!mdwd)
-          SendDebugMessage(0, sdmGlobal, 0, "MiniDumpWriteDump not available");
-        else
-        {
+        if (!mdwd) {
+          OutputThreadDebugString("MiniDumpWriteDump not available");
+        }
+        else {
           if (!(*mdwd)(GetCurrentProcess(), GetCurrentProcessId(), hFile,
-              (MINIDUMP_TYPE)(MiniDumpWithDataSegs | MiniDumpWithHandleData),
-              &mei, NULL, NULL))
+            (MINIDUMP_TYPE)(MiniDumpWithDataSegs | MiniDumpWithHandleData),
+            &mei, NULL, NULL)) {
             DebugLastError("MiniDumpWriteDump");
-          else
-          {
-            SendDebugMessageFormat(0, sdmGlobal, 0, "Minidump written to %s", filename);
+          } 
+          else {
+            OutputThreadDebugString("Minidump written to ");
+            OutputThreadDebugString(filename);
             HKEY hkey;
             if(RegCreateKeyEx(HKEY_CURRENT_USER, REGSZ_KeymanEngineDiag, 0, NULL, 0, KEY_ALL_ACCESS,
-              NULL, &hkey, NULL) == ERROR_SUCCESS)
-            {
+              NULL, &hkey, NULL) == ERROR_SUCCESS) {
               DWORD v = 0;
               RegSetValueEx(hkey, filename, 0, REG_DWORD, (PBYTE)&v, sizeof(DWORD));
               RegCloseKey(hkey);
@@ -66,23 +69,26 @@ DWORD ExceptionMessage(LPSTR Proc, LPEXCEPTION_POINTERS ep)
     }
   }
 
-  if(!ep || !ep->ExceptionRecord) 
-  {
-    SendDebugMessageFormat(0, sdmGlobal, 0, "CAUGHT UNKNOWN EXCEPTION");
+  if(!ep || !ep->ExceptionRecord) {
+    OutputThreadDebugString("CAUGHT UNKNOWN EXCEPTION");
     return EXCEPTION_CONTINUE_SEARCH;
   }
   LPEXCEPTION_RECORD er = ep->ExceptionRecord;
   
-  while(er != NULL)
-  {
-    if(er->ExceptionCode == EXCEPTION_ACCESS_VIOLATION)
-    	SendDebugMessageFormat(0, sdmGlobal, 0, "CAUGHT EXCEPTION %d (ACCESS VIOLATION) IN %s AT %x; attempted to %s %x",
-    	  er->ExceptionCode, Proc, er->ExceptionAddress,
-    	  er->ExceptionInformation[0] == 0 ? "read from" : "write to",
-    	  er->ExceptionInformation[1]);
-    else
-  	  SendDebugMessageFormat(0, sdmGlobal, 0, "CAUGHT EXCEPTION %d IN %s AT %x",
-    	  er->ExceptionCode, Proc, er->ExceptionAddress);
+  while(er != NULL) {
+    if (er->ExceptionCode == EXCEPTION_ACCESS_VIOLATION) {
+      OutputThreadDebugString("EXCEPTION_ACCESS_VIOLATION in ");
+      OutputThreadDebugString(Proc);
+      /*SendDebugMessageFormat(0, sdmGlobal, 0, "CAUGHT EXCEPTION %d (ACCESS VIOLATION) IN %s AT %x; attempted to %s %x",
+        er->ExceptionCode, Proc, er->ExceptionAddress,
+        er->ExceptionInformation[0] == 0 ? "read from" : "write to",
+        er->ExceptionInformation[1]);*/
+    }
+    else {
+      OutputThreadDebugString("CAUGHT EXCEPTION");
+      /*SendDebugMessageFormat(0, sdmGlobal, 0, "CAUGHT EXCEPTION %d IN %s AT %x",
+        er->ExceptionCode, Proc, er->ExceptionAddress);*/
+    }
     er = er->ExceptionRecord;
   }
 

windows/src/engine/keyman32/hotkeys.cpp

@@ -45,6 +45,18 @@ Hotkeys *Hotkeys::Instance() {   // I4326
   return g_Hotkeys;
 }
 
+void Hotkeys::Unload() {
+  if (GetCurrentThreadId() != Globals::get_InitialisingThread()) {
+    OutputThreadDebugString("Unexpected: no other thread should be attempting to unload hotkeys");
+    return;
+  }
+
+  if (g_Hotkeys != NULL) {
+    delete g_Hotkeys;
+    g_Hotkeys = NULL;
+  }
+}
+
 void Hotkeys::Reload() {   // I4326   // I4390
   Hotkeys *hotkeys = Hotkeys::Instance();   // I4641
   if(hotkeys == NULL) {

windows/src/engine/keyman32/hotkeys.h

@@ -43,4 +43,5 @@ public:
 	Hotkey *GetHotkey(DWORD hotkey);
   static void Reload();   // I4326
   static Hotkeys *Instance();   // I4326
-};
+  static void Unload();
+ };

windows/src/engine/keyman32/k32_dbg.cpp

@@ -304,4 +304,12 @@ char *Debug_UnicodeString(PWSTR s, int x) {
   }
   //WideCharToMultiByte(CP_ACP, 0, buf, -1, bufout, 128, NULL, NULL);
   return bufout[x];
-}
+}
+
+#ifdef _DEBUG
+void _OutputThreadDebugString(char *s) {
+  char buf[256];
+  sprintf_s(buf, "[%d]: %s\n", GetCurrentThreadId(), s);
+  OutputDebugString(buf);
+}
+#endif

windows/src/engine/keyman32/k32_globals.cpp

@@ -138,7 +138,11 @@ PKEYMAN64THREADDATA Globals_InitThread()
 
 void Globals_UninitThread()
 {
-  if(!Globals_ProcessInitialised()) return;
+  OutputThreadDebugString("Globals_UninitThread");
+  if (!Globals_ProcessInitialised()) {
+    OutputThreadDebugString("Globals_UninitThread aborted without cleanup");
+    return;
+  }
 
   CloseTSF();   // I3933
 
@@ -185,6 +189,7 @@ void Globals_UninitProcess()
   TlsFree(dwTlsIndex);
   dwTlsIndex = TLS_OUT_OF_INDEXES;
   LeaveCriticalSection(&csGlobals);
+  DeleteCriticalSection(&csGlobals);
 }
 
 PKEYMAN64THREADDATA ThreadGlobals()

windows/src/engine/keyman32/k32_lowlevelkeyboardhook.cpp

@@ -53,7 +53,7 @@ LRESULT CALLBACK kmnLowLevelKeyboardProc(
   __except(ExceptionMessage("kmnLowLevelKeyboardProc", GetExceptionInformation())) {
 	}
 #endif
-	return res;
+  return res;
 }
 
 BOOL KeyLanguageSwitchPress(WPARAM wParam, BOOL extended, BOOL isUp, DWORD ShiftState);

windows/src/engine/keyman32/keyboardoptions.cpp

@@ -41,9 +41,10 @@ void LoadSharedKeyboardOptions(LPINTKEYBOARDINFO kp)
 
 void FreeKeyboardOptions(LPINTKEYBOARDINFO kp)
 {
-  assert(kp != NULL);
-  assert(kp->Keyboard != NULL);
-  assert(kp->KeyboardOptions != NULL);
+  // This is a cleanup routine; we don't want to precondition all calls to it
+  // so we do not assert
+  if (kp == NULL || kp->Keyboard == NULL || kp->KeyboardOptions == NULL)
+    return;
 
   for(DWORD i = 0; i < kp->Keyboard->cxStoreArray; i++)
     if(kp->KeyboardOptions[i].Value)

windows/src/engine/keyman32/kmhook_callwndproc.cpp

@@ -66,6 +66,7 @@ LRESULT _kmnCallWndProc(int nCode, WPARAM wParam, LPARAM lParam);
 LRESULT CALLBACK kmnCallWndProc(int nCode, WPARAM wParam, LPARAM lParam)
 {
   LRESULT res = 0;
+
 #ifdef _DEBUG_EXCEPTION
 	res = _kmnCallWndProc(nCode,wParam,lParam);
 #else
@@ -77,7 +78,7 @@ LRESULT CALLBACK kmnCallWndProc(int nCode, WPARAM wParam, LPARAM lParam)
 	{
 	}
 #endif
-	return res;
+  return res;
 }
    // I3617
 BOOL IsSysTrayWindow(HWND hwnd);

windows/src/engine/keyman32/serialkeyeventclient.cpp

@@ -149,6 +149,7 @@ public:
 };
 
 void ISerialKeyEventClient::Startup() {
+  OutputThreadDebugString("ISerialKeyEventClient::Startup");
   PKEYMAN64THREADDATA _td = ThreadGlobals();
   if (_td) {
     _td->pSerialKeyEventClient = new SerialKeyEventClient();
@@ -156,6 +157,7 @@ void ISerialKeyEventClient::Startup() {
 }
 
 void ISerialKeyEventClient::Shutdown() {
+  OutputThreadDebugString("ISerialKeyEventClient::Shutdown");
   PKEYMAN64THREADDATA _td = ThreadGlobals();
   if (_td && _td->pSerialKeyEventClient) {
     delete _td->pSerialKeyEventClient;

windows/src/global/delphi/general/klog.pas

@@ -133,7 +133,7 @@ begin
   end;
   writeln(FLogFile, Format('%12.12d ', [GetTickCount()])+FormatDateTime('yyyy-mm-dd hh:nn:ss.zzz', Now) + ': ' + StringOfChar(' ', FMethodStack.Count*2) + msg);
   flush(FLogFile);
-  OutputDebugString(PChar(msg));
+  OutputDebugString(PChar('KLog:' + msg + #13#10));
 {$ENDIF}
 end;
 

windows/src/global/inc/keyman64.h

@@ -347,6 +347,13 @@ BOOL ShouldDebug_1(); // TSDMState state);
 
 #endif
 
+#ifdef _DEBUG
+#define OutputThreadDebugString(s) _OutputThreadDebugString(s)
+void _OutputThreadDebugString(char *s);
+#else
+#define OutputThreadDebugString(s) 
+#endif
+
 /* Keyboard selection functions */ 
 
 void HandleRefresh(int code, LONG tag);