From 7155e9d7223ee21d249df1d3698bc00f8ffb5fab Mon Sep 17 00:00:00 2001 From: Marc Durdin Date: Mon, 29 Jun 2026 09:50:51 +0200 Subject: [PATCH 1/2] fix(android): refactor `KMLog` - simplification and added resilience * Clean up `KMLog` -- simpler code paths, DRY out common validation, remove redundant re-entrancy checks. * Use Sentry `setTag` API and scopes instead of `setExtra`. * Wrap all potential failure points in exception handlers for extra resilience -- do our best to make sure errors are reported in as many cases as possible. Fixes: #16122 Test-bot: skip --- .../java/com/keyman/engine/util/KMLog.java | 185 +++++++++--------- 1 file changed, 92 insertions(+), 93 deletions(-) diff --git a/android/KMEA/app/src/main/java/com/keyman/engine/util/KMLog.java b/android/KMEA/app/src/main/java/com/keyman/engine/util/KMLog.java index 1ce55bf0ce..e6eb05a96e 100644 --- a/android/KMEA/app/src/main/java/com/keyman/engine/util/KMLog.java +++ b/android/KMEA/app/src/main/java/com/keyman/engine/util/KMLog.java @@ -24,16 +24,11 @@ import io.sentry.SentryLevel; public final class KMLog { private static final String TAG = "KMLog"; - private static final String KEYBOARD_TAG = "keyboardId"; - private static final String KEYBOARD_COUNT_TAG = "installedKeyboardCount"; - private static final String MODEL_TAG = "modelId"; - private static final String LANGCODE_TAG = "languageCode"; - - // Some of the methods used to generate debug logging information can, themselves, - // trigger errors that can also trigger the same logging. We must not get - // caught in an infinite loop / stack-overflow; this field helps us avoid states - // that would otherwise cause error-looping, etc. - private static boolean isLogging = false; + private static final String KEYBOARD_TAG = "keyman.keyboardId"; + private static final String KEYBOARD_COUNT_TAG = "keyman.installedKeyboardCount"; + private static final String MODEL_TAG = "keyman.modelId"; + private static final String LANGCODE_TAG = "keyman.languageCode"; + private static final String DEBUG_LOGGING_ERROR_TAG = "keyman.debugLoggingError"; private static void tagDebugInfo() { String kbdId = ""; @@ -57,13 +52,16 @@ public final class KMLog { } } } catch (Exception ex) { - String msg = ex.getMessage() == null ? "" : ex.getMessage(); - Sentry.setExtra("debugLoggingError", msg); + Sentry.setTag(DEBUG_LOGGING_ERROR_TAG, ex.getMessage() == null ? "" : ex.getMessage()); } - Sentry.setExtra(KEYBOARD_TAG, kbdId); - Sentry.setExtra(KEYBOARD_COUNT_TAG, "" + kbdCount); - Sentry.setExtra(LANGCODE_TAG, lngCode); - Sentry.setExtra(MODEL_TAG, modelId); + Sentry.setTag(KEYBOARD_TAG, kbdId); + Sentry.setTag(KEYBOARD_COUNT_TAG, "" + kbdCount); + Sentry.setTag(LANGCODE_TAG, lngCode); + Sentry.setTag(MODEL_TAG, modelId); + } + + private static boolean canLogToSentry() { + return DependencyUtil.libraryExists(LibraryType.SENTRY) && Sentry.isEnabled(); } /** @@ -72,19 +70,20 @@ public final class KMLog { * @param msg String of the info message */ public static void LogInfo(String tag, String msg) { - if(isLogging) { + if (msg == null || msg.isEmpty()) { return; } - isLogging = true; - if (msg != null && !msg.isEmpty()) { - Log.i(tag, msg); - if (DependencyUtil.libraryExists(LibraryType.SENTRY) && Sentry.isEnabled()) { - tagDebugInfo(); - Sentry.captureMessage(msg, SentryLevel.INFO); - } + Log.i(tag, msg); + + if (!canLogToSentry()) { + return; } - isLogging = false; + + Sentry.withScope(scope -> { + tagDebugInfo(); + Sentry.captureMessage(msg, SentryLevel.INFO); + }); } /** @@ -98,15 +97,9 @@ public final class KMLog { return; } - if(isLogging) { - return; - } - isLogging = true; - Log.i(tag, msg); - if (!DependencyUtil.libraryExists(LibraryType.SENTRY) || !Sentry.isEnabled()) { - isLogging = false; + if (!canLogToSentry()) { return; } @@ -114,27 +107,30 @@ public final class KMLog { crumb.setMessage(msg); crumb.setLevel(SentryLevel.INFO); - if(addStackTrace) { - StackTraceElement[] rawTrace = Thread.currentThread().getStackTrace(); + try { + if(addStackTrace) { + StackTraceElement[] rawTrace = Thread.currentThread().getStackTrace(); - // The call that gets us the stack-trace above... shows up in the - // stack trace, so we'll skip the first few (redundant) entries. - int skipCount = 3; + // The call that gets us the stack-trace above... shows up in the + // stack trace, so we'll skip the first few (redundant) entries. + int skipCount = 3; - // Sentry does limit the size of messages... so let's just - // keep 10 entries and call it a day. - int limit = Math.min(rawTrace.length, 10 + skipCount); - if(rawTrace.length > skipCount) { - String[] trace = new String[limit - skipCount]; - for (int i = skipCount; i < limit; i++) { - trace[i-skipCount] = rawTrace[i].toString(); + // Sentry does limit the size of messages... so let's just + // keep 10 entries and call it a day. + int limit = Math.min(rawTrace.length, 10 + skipCount); + if(rawTrace.length > skipCount) { + String[] trace = new String[limit - skipCount]; + for (int i = skipCount; i < limit; i++) { + trace[i-skipCount] = rawTrace[i].toString(); + } + crumb.setData("stacktrace", trace); } - crumb.setData("stacktrace", trace); } + } catch (Exception e) { + Sentry.captureException(e); } - tagDebugInfo(); + Sentry.addBreadcrumb(crumb); - isLogging = false; } /** @@ -143,23 +139,32 @@ public final class KMLog { * @param msg String of the error message */ public static void LogError(String tag, String msg) { - if(isLogging) { + if (msg == null || msg.isEmpty()) { return; } - isLogging = true; - if (msg != null && !msg.isEmpty()) { - Log.e(tag, msg); + Log.e(tag, msg); + + try { + // On alpha and beta tiers, we pop an error toast so testers can be aware + // of the error if (KMManager.getTier(BuildConfig.KEYMAN_ENGINE_VERSION_NAME) != KMManager.Tier.STABLE) { BaseActivity.makeToast(null, msg, Toast.LENGTH_LONG); } - - if (DependencyUtil.libraryExists(LibraryType.SENTRY) && Sentry.isEnabled()) { - tagDebugInfo(); - Sentry.captureMessage(msg, SentryLevel.ERROR); + } catch(Exception e) { + if(canLogToSentry()) { + Sentry.captureException(e); } } - isLogging = false; + + if (!canLogToSentry()) { + return; + } + + Sentry.withScope(scope -> { + tagDebugInfo(); + Sentry.captureMessage(msg, SentryLevel.ERROR); + }); } /** @@ -169,28 +174,7 @@ public final class KMLog { * @param e Throwable exception */ public static void LogException(String tag, String msg, Throwable e) { - if(isLogging) { - return; - } - isLogging = true; - String errorMsg = ""; - if (msg != null && !msg.isEmpty()) { - errorMsg = msg + "\n" + e; - } else if (e != null) { - errorMsg = e.getMessage(); - } - Log.e(tag, errorMsg, e); - - if (KMManager.getTier(BuildConfig.KEYMAN_ENGINE_VERSION_NAME) != KMManager.Tier.STABLE) { - BaseActivity.makeToast(null, errorMsg, Toast.LENGTH_LONG); - } - - if (DependencyUtil.libraryExists(LibraryType.SENTRY) && Sentry.isEnabled()) { - tagDebugInfo(); - Sentry.addBreadcrumb(errorMsg); - Sentry.captureException(e); - } - isLogging = false; + KMLog.LogExceptionWithData(tag, msg, null, null, e); } /** @@ -203,25 +187,40 @@ public final class KMLog { */ public static void LogExceptionWithData(String tag, String msg, String objName, Object obj, Throwable e) { - if(isLogging) { + String errorMsg = ""; + try { + if (msg != null && !msg.isEmpty()) { + errorMsg = msg + "\n" + e.toString(); + } else if (e != null) { + errorMsg = e.toString(); + } + + // On alpha and beta tiers, we pop an error toast so testers can be aware + // of the exception + if (KMManager.getTier(BuildConfig.KEYMAN_ENGINE_VERSION_NAME) != KMManager.Tier.STABLE) { + BaseActivity.makeToast(null, errorMsg, Toast.LENGTH_LONG); + } + } catch (Exception innerE) { + if (canLogToSentry()) { + Sentry.captureException(innerE); + } + } + + if (!canLogToSentry()) { return; } - isLogging = true; - if (obj != null && DependencyUtil.libraryExists(LibraryType.SENTRY) && Sentry.isEnabled()) { + + Sentry.addBreadcrumb(errorMsg); + Sentry.withScope(scope -> { tagDebugInfo(); - String objStr = null; try { - objStr = obj.toString(); - Sentry.setExtra(objName, objStr); - } catch (Exception innerE) { - LogException(TAG, "Sentry.setExtra failed for " + objName, innerE); + if(obj != null && objName != null) { + Sentry.setTag(objName, obj.toString()); + } + } catch(Exception innerE) { + Sentry.captureException(innerE); } - // Report the original exception - LogException(tag, msg, e); - // And remove the exception-specific tagged data, lest it also be - // tracked on subsequent errors not associated with the current call. - Sentry.removeExtra(objName); - } - isLogging = false; + Sentry.captureException(e); + }); } } From 809a3cad4a42df5324d002af1c8362cbd2f2bc00 Mon Sep 17 00:00:00 2001 From: Marc Durdin Date: Tue, 30 Jun 2026 15:12:59 +0200 Subject: [PATCH 2/2] chore(android): address review comments Also add logging if unit test fails --- .../main/java/com/keyman/engine/util/KMLog.java | 14 +++++++++++--- .../java/com/keyman/engine/util/FileUtilsTest.java | 2 +- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/android/KMEA/app/src/main/java/com/keyman/engine/util/KMLog.java b/android/KMEA/app/src/main/java/com/keyman/engine/util/KMLog.java index e6eb05a96e..633f5c853b 100644 --- a/android/KMEA/app/src/main/java/com/keyman/engine/util/KMLog.java +++ b/android/KMEA/app/src/main/java/com/keyman/engine/util/KMLog.java @@ -117,8 +117,8 @@ public final class KMLog { // Sentry does limit the size of messages... so let's just // keep 10 entries and call it a day. - int limit = Math.min(rawTrace.length, 10 + skipCount); if(rawTrace.length > skipCount) { + int limit = Math.min(rawTrace.length, 10 + skipCount); String[] trace = new String[limit - skipCount]; for (int i = skipCount; i < limit; i++) { trace[i-skipCount] = rawTrace[i].toString(); @@ -190,7 +190,7 @@ public final class KMLog { String errorMsg = ""; try { if (msg != null && !msg.isEmpty()) { - errorMsg = msg + "\n" + e.toString(); + errorMsg = msg + "\n" + e; } else if (e != null) { errorMsg = e.toString(); } @@ -206,6 +206,8 @@ public final class KMLog { } } + Log.e(tag, errorMsg); + if (!canLogToSentry()) { return; } @@ -215,12 +217,18 @@ public final class KMLog { tagDebugInfo(); try { if(obj != null && objName != null) { - Sentry.setTag(objName, obj.toString()); + Sentry.setExtra(objName, obj.toString()); } } catch(Exception innerE) { Sentry.captureException(innerE); } Sentry.captureException(e); + + if(obj != null && objName != null) { + // And remove the exception-specific tagged data, lest it also be + // tracked on subsequent errors not associated with the current call. + Sentry.removeExtra(objName); + } }); } } diff --git a/android/KMEA/app/src/test/java/com/keyman/engine/util/FileUtilsTest.java b/android/KMEA/app/src/test/java/com/keyman/engine/util/FileUtilsTest.java index 5118159c9b..9541a59afe 100644 --- a/android/KMEA/app/src/test/java/com/keyman/engine/util/FileUtilsTest.java +++ b/android/KMEA/app/src/test/java/com/keyman/engine/util/FileUtilsTest.java @@ -23,7 +23,7 @@ public class FileUtilsTest { List logs = ShadowLog.getLogs(); // The logs contain type 4, but we only care about type 6 for connection messages - Assert.assertEquals(2, logs.size()); + Assert.assertEquals("Size did not match 2: " + logs.toString(), 2, logs.size()); Assert.assertEquals("Connection", logs.get(0).tag); Assert.assertEquals("Initialization failed:\njava.net.MalformedURLException: no protocol: invalidURL", logs.get(0).msg);