Fixes crash caused by FileObserver on LG devices (#39)

* Fix for persistent OOM crashing issue (cherry picked from commit 4874d79) * Added unit test and logging for OOM issue (cherry picked from commit 8d4c475) * Fix issue with crash due to bad transaction file (#36) * Fix for persistent OOM crashing issue (cherry picked from commit 4874d79) * Added unit test and logging for OOM issue (cherry picked from commit 8d4c475) * Update all libraries, roll version * Update to AGP 7 and update other gradle plugins * Use LG device for testing Harmony * Use proper model name * Create test to replicate issue with LG * Check for info on what is available for LG devices * Remove testing run * Potential fix for LG device crashes * Clean up code, testing negative case * Revert negative test case * Fix test upload * Update test dependencies * Fix issue where FileObserver thread my not be started yet * Improve on FileObserver thread start * Rename some lock objects, ensure FileObserver is initialized immediately * Revert some changes, cleanup * More cleanup * Test negative case again * Reset back to normal test case
pablobaxter · Oct 10, 2021 · 9b39b6f · 9b39b6f
1 parent df43a08
commit 9b39b6f
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 16 deletions.
diff --git a/.circleci/config.yml b/.circleci/config.yml
@@ -47,7 +47,7 @@ jobs:
               --test ${HOME}/repo/harmony/build/outputs/apk/androidTest/debug/harmony-debug-androidTest.apk \
               --results-bucket cloud-test-${GOOGLE_PROJECT_ID} \
               --results-dir harmony-${CIRCLE_BRANCH}_${CIRCLE_BUILD_NUM} \
-              --device model=Pixel2,version=29 \
+              --device model=joan,version=26 \
               --no-performance-metrics \
               --no-record-video \
               --use-orchestrator
@@ -70,7 +70,7 @@ jobs:
           name: Install gsutil dependency and copy test results data
           command: |
             sudo pip install -U crcmod
-            sudo gsutil -m cp -r -U `sudo gsutil ls gs://cloud-test-${GOOGLE_PROJECT_ID}/harmony-${CIRCLE_BRANCH}_${CIRCLE_BUILD_NUM}/Pixel2-29-en-portrait/test_result_1.xml | tail -1` ${HOME}/temp_results/test_result_1_harmony.xml | true
+            sudo gsutil -m cp -r -U `sudo gsutil ls gs://cloud-test-${GOOGLE_PROJECT_ID}/harmony-${CIRCLE_BRANCH}_${CIRCLE_BUILD_NUM}/joan-26-en-portrait/test_result_1.xml | tail -1` ${HOME}/temp_results/test_result_1_harmony.xml | true
             sudo gsutil -m cp -r -U `sudo gsutil ls gs://cloud-test-${GOOGLE_PROJECT_ID}/crypto-${CIRCLE_BRANCH}_${CIRCLE_BUILD_NUM}/Pixel2-29-en-portrait/test_result_1.xml | tail -1` ${HOME}/temp_results/test_result_1_crypto.xml | true
 
       - store_test_results:

diff --git a/app/build.gradle b/app/build.gradle
@@ -76,9 +76,7 @@ repositories{
 }
 
 dependencies {
-    debugImplementation project (':crypto')
-    releaseImplementation "com.frybits.harmony:harmony-crypto:$crypto_version_name"
-    releaseImplementation "com.frybits.harmony:harmony:$harmony_version_name"
+    implementation project (':crypto')
     implementation 'com.tencent:mmkv-static:1.2.8'
     implementation (name: "tray-0.12.0", ext: 'aar')
 

diff --git a/build.gradle b/build.gradle
@@ -3,8 +3,8 @@
 buildscript { 
 
     ext {
-        kotlin_version = '1.5.21'
-        kotlin_coroutine_version = '1.5.0'
+        kotlin_version = '1.5.31'
+        kotlin_coroutine_version = '1.5.2'
         lifecycle_scope_version = '2.3.1'
 
         // Android libraries
@@ -30,11 +30,11 @@ buildscript {
         targetSdk_version = 30
         buildtools_version = '30.0.3'
         nav_version = "2.3.5"
-        constraint_version = '2.1.0'
+        constraint_version = '2.1.1'
         hilt_version = '2.38.1'
         room_version = "2.3.0"
 
-        harmony_version_name = "1.1.9"
+        harmony_version_name = "1.1.10"
         crypto_version_name = "0.0.2"
 
     }
@@ -45,7 +45,7 @@ buildscript {
     }
 
     dependencies {
-        classpath 'com.android.tools.build:gradle:7.0.1'
+        classpath 'com.android.tools.build:gradle:7.0.2'
         classpath "org.jetbrains.kotlin:kotlin-gradle-plugin:$kotlin_version"
         classpath 'org.jetbrains.dokka:dokka-gradle-plugin:1.5.0'
         classpath "io.codearte.gradle.nexus:gradle-nexus-staging-plugin:0.30.0"

diff --git a/crypto/build.gradle b/crypto/build.gradle
@@ -77,8 +77,7 @@ artifacts {
 
 dependencies {
     // Harmony
-    debugApi project(":harmony")
-    releaseApi "com.frybits.harmony:harmony:$harmony_version_name"
+    api project(":harmony")
 
     // Kotlin
     implementation "org.jetbrains.kotlin:kotlin-stdlib-jdk8:$kotlin_version"

diff --git a/harmony/src/androidTest/java/com/frybits/harmony/HarmonyFileObserverTest.kt b/harmony/src/androidTest/java/com/frybits/harmony/HarmonyFileObserverTest.kt
@@ -0,0 +1,37 @@
+package com.frybits.harmony
+
+import androidx.test.ext.junit.runners.AndroidJUnit4
+import androidx.test.platform.app.InstrumentationRegistry
+import kotlinx.coroutines.Dispatchers
+import kotlinx.coroutines.async
+import kotlinx.coroutines.awaitAll
+import kotlinx.coroutines.runBlocking
+import org.junit.Test
+import org.junit.runner.RunWith
+import java.util.UUID
+
+@RunWith(AndroidJUnit4::class)
+class HarmonyFileObserverTest {
+
+    // This tests for regression on a bug specific to LG devices running Android 9 and below
+    // See https://github.com/pablobaxter/Harmony/issues/38
+    @OptIn(ExperimentalStdlibApi::class)
+    @Test
+    fun testMultipleHarmonyPrefsImplementation() = runBlocking {
+        // Context of the app under test.
+        val appContext = InstrumentationRegistry.getInstrumentation().targetContext
+
+        val prefsJob = buildList {
+            repeat(100) {
+                val prefs = appContext.getHarmonySharedPreferences("prefs-${UUID.randomUUID()}")
+                add(async(Dispatchers.IO) {
+                    prefs.getString("test-$it", null)
+                    return@async
+                })
+            }
+        }
+
+        prefsJob.awaitAll()
+        return@runBlocking
+    }
+}
diff --git a/harmony/src/main/java/com/frybits/harmony/Harmony.kt b/harmony/src/main/java/com/frybits/harmony/Harmony.kt
@@ -118,6 +118,9 @@ private class HarmonyImpl constructor(
 
     private val shouldNotifyClearToListeners = context.applicationContext.applicationInfo.targetSdkVersion >= Build.VERSION_CODES.R
 
+    // Flag to check if fileobserver should be synchronized
+    private val shouldSynchronizeFileObserver = Build.MANUFACTURER.contains("lge", ignoreCase = true) && Build.VERSION.SDK_INT <= Build.VERSION_CODES.P
+
     // Runnable to handle transactions. Used as an object to allow the Handler to remove from the queue. Prevents build up of this job in the looper.
     private var transactionUpdateJob = Runnable {
         handleTransactionUpdate()
@@ -168,8 +171,15 @@ private class HarmonyImpl constructor(
     // Task for loading Harmony
     private val isLoadedTask = FutureTask {
         initialLoad()
-        // Start the file observer on the the prefs folder for this Harmony object
-        harmonyFileObserver.startWatching()
+
+        // Fixes crashing bug that occurs on LG devices running Android 9 and lower
+        if (shouldSynchronizeFileObserver) {
+            synchronized(FILE_OBSERVER_SYNC_OBJECT) {
+                startFileObserver()
+            }
+        } else {
+            startFileObserver()
+        }
     }
 
     init {
@@ -243,7 +253,7 @@ private class HarmonyImpl constructor(
     // This listener will also listen for changes that occur to the Harmony preference with the same name from other processes.
     override fun registerOnSharedPreferenceChangeListener(listener: SharedPreferences.OnSharedPreferenceChangeListener) {
         mapReentrantReadWriteLock.write {
-            listenerMap[listener] = CONTENT
+            listenerMap[listener] = EmptyContent
         }
     }
 
@@ -253,6 +263,11 @@ private class HarmonyImpl constructor(
         }
     }
 
+    private fun startFileObserver() {
+        // Start the file observer on the the prefs folder for this Harmony object
+        harmonyFileObserver.startWatching()
+    }
+
     private fun awaitForLoad() {
         if (!isLoadedTask.isDone) {
             isLoadedTask.get()
@@ -1186,7 +1201,10 @@ private const val TRANSACTION_FILE_VERSION_2 = (TRANSACTION_FILE_VERSION_1 - 1).
 private const val CURR_TRANSACTION_FILE_VERSION = TRANSACTION_FILE_VERSION_2.toInt()
 
 // Empty singleton to support WeakHashmap
-private object CONTENT
+private object EmptyContent
+
+// Hack to force FileObserver to initialize static fields. This starts the ObserverThread.
+private val FILE_OBSERVER_SYNC_OBJECT: Any = Class.forName(FileObserver::class.java.name)
 
 private object SingletonLockObj