From 7074e6fd4c6289e016666bb21e7eee2f78219141 Mon Sep 17 00:00:00 2001
From: Andreas Gampe <agampe@google.com>
Date: Fri, 16 Mar 2018 16:14:29 -0700
Subject: Frameworks: Handle exceptions in SystemProperties callbacks

When notifying callbacks, catch thrown exceptions.

Bug: 73058952
Test: m
Test: atest SystemPropertiesTest
Change-Id: I1c61e11b833e8ddd30d80b9859b20a9fb3185a33
---
 core/java/android/os/SystemProperties.java         |  7 +++-
 core/jni/android_os_SystemProperties.cpp           |  6 +++
 .../src/android/os/SystemPropertiesTest.java       | 47 ++++++++++++++++++++++
 3 files changed, 59 insertions(+), 1 deletion(-)

diff --git a/core/java/android/os/SystemProperties.java b/core/java/android/os/SystemProperties.java
index 4f6d322ba871..89168ae03a44 100644
--- a/core/java/android/os/SystemProperties.java
+++ b/core/java/android/os/SystemProperties.java
@@ -189,7 +189,12 @@ public class SystemProperties {
             }
             ArrayList<Runnable> callbacks = new ArrayList<Runnable>(sChangeCallbacks);
             for (int i=0; i<callbacks.size(); i++) {
-                callbacks.get(i).run();
+                try {
+                    callbacks.get(i).run();
+                } catch (Throwable t) {
+                    Log.wtf(TAG, "Exception in SystemProperties change callback", t);
+                    // Ignore and try to go on.
+                }
             }
         }
     }
diff --git a/core/jni/android_os_SystemProperties.cpp b/core/jni/android_os_SystemProperties.cpp
index a94cac0f18f5..9ec7517754d9 100644
--- a/core/jni/android_os_SystemProperties.cpp
+++ b/core/jni/android_os_SystemProperties.cpp
@@ -124,6 +124,12 @@ void do_report_sysprop_change() {
         if (sVM->GetEnv((void **)&env, JNI_VERSION_1_4) >= 0) {
             //ALOGI("Java SystemProperties: calling %p", sCallChangeCallbacks);
             env->CallStaticVoidMethod(sClazz, sCallChangeCallbacks);
+            // There should not be any exceptions. But we must guarantee
+            // there are none on return.
+            if (env->ExceptionCheck()) {
+                env->ExceptionClear();
+                LOG(ERROR) << "Exception pending after sysprop_change!";
+            }
         }
     }
 }
diff --git a/core/tests/systemproperties/src/android/os/SystemPropertiesTest.java b/core/tests/systemproperties/src/android/os/SystemPropertiesTest.java
index 282b0011eede..933e54e840c5 100644
--- a/core/tests/systemproperties/src/android/os/SystemPropertiesTest.java
+++ b/core/tests/systemproperties/src/android/os/SystemPropertiesTest.java
@@ -16,6 +16,9 @@
 
 package android.os;
 
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.TimeUnit;
+
 import junit.framework.TestCase;
 
 import android.os.SystemProperties;
@@ -141,4 +144,48 @@ public class SystemPropertiesTest extends TestCase {
         } catch (NullPointerException npe) {
         }
     }
+
+    @SmallTest
+    public void testCallbacks() {
+        // Latches are not really necessary, but are easy to use.
+        final CountDownLatch wait1 = new CountDownLatch(1);
+        final CountDownLatch wait2 = new CountDownLatch(1);
+
+        Runnable r1 = new Runnable() {
+            boolean done = false;
+            @Override
+            public void run() {
+                if (done) {
+                    return;
+                }
+                done = true;
+
+                wait1.countDown();
+                throw new RuntimeException("test");
+            }
+        };
+
+        Runnable r2 = new Runnable() {
+            @Override
+            public void run() {
+                wait2.countDown();
+            }
+        };
+
+        SystemProperties.addChangeCallback(r1);
+        SystemProperties.addChangeCallback(r2);
+
+        SystemProperties.reportSyspropChanged();
+
+        try {
+            assertTrue(wait1.await(5, TimeUnit.SECONDS));
+        } catch (InterruptedException e) {
+            fail("InterruptedException");
+        }
+        try {
+            assertTrue(wait2.await(5, TimeUnit.SECONDS));
+        } catch (InterruptedException e) {
+            fail("InterruptedException");
+        }
+    }
 }
-- 
cgit v1.2.3-59-g8ed1b