commit b86d16c46084e415009e631ae8186de42a740278
author Lorenzo Colitti <lorenzo@google.com> Tue Mar 29 09:45:35 2022 +0000
committer Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com> Tue Mar 29 09:45:35 2022 +0000
tree 5a366e853ef747fdec52ddf97b973ab9560a8c45
parent d8281e4e61a25bec808cc52c7b0b21b60363cb5c
parent 046df6d617941ede4905405d489f1c39a40d9040

Merge "Don't throw in FullScore#policyNameOf." am: 644fe232cd am: 046df6d617

Original change: https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/2041564

Change-Id: Ia237d2a7ba544d1d30c9a923509b7c1aa0fc15c7
Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>

service/src/com/android/server/connectivity/FullScore.java

diff --git a/service/src/com/android/server/connectivity/FullScore.java b/service/src/com/android/server/connectivity/FullScore.java
index 799f46b..b13ba93 100644
--- a/service/src/com/android/server/connectivity/FullScore.java
+++ b/service/src/com/android/server/connectivity/FullScore.java
@@ -29,6 +29,7 @@
 import android.net.NetworkCapabilities;
 import android.net.NetworkScore;
 import android.net.NetworkScore.KeepConnectedReason;
+import android.util.Log;
 import android.util.SparseArray;
 
 import com.android.internal.annotations.VisibleForTesting;
@@ -46,6 +47,8 @@
  * they are handling a score that had the CS-managed bits set.
  */
 public class FullScore {
+    private static final String TAG = FullScore.class.getSimpleName();
+
     // This will be removed soon. Do *NOT* depend on it for any new code that is not part of
     // a migration.
     private final int mLegacyInt;
@@ -126,7 +129,15 @@
     @VisibleForTesting
     static @NonNull String policyNameOf(final int policy) {
         final String name = sMessageNames.get(policy);
-        if (name == null) throw new IllegalArgumentException("Unknown policy: " + policy);
+        if (name == null) {
+            // Don't throw here because name might be null due to proguard stripping out the
+            // POLICY_* constants, potentially causing a crash only on user builds because proguard
+            // does not run on userdebug builds.
+            // TODO: make MessageUtils safer by not returning the array and instead storing it
+            // internally and providing a getter (that does not throw) for individual values.
+            Log.wtf(TAG, "Unknown policy: " + policy);
+            return Integer.toString(policy);
+        }
         return name.substring("POLICY_".length());
     }
 

tests/unit/java/com/android/server/connectivity/FullScoreTest.kt

diff --git a/tests/unit/java/com/android/server/connectivity/FullScoreTest.kt b/tests/unit/java/com/android/server/connectivity/FullScoreTest.kt
index e7f6245..c03a9cd 100644
--- a/tests/unit/java/com/android/server/connectivity/FullScoreTest.kt
+++ b/tests/unit/java/com/android/server/connectivity/FullScoreTest.kt
@@ -22,6 +22,7 @@
 import android.os.Build
 import android.text.TextUtils
 import android.util.ArraySet
+import android.util.Log
 import androidx.test.filters.SmallTest
 import com.android.server.connectivity.FullScore.MAX_CS_MANAGED_POLICY
 import com.android.server.connectivity.FullScore.POLICY_ACCEPT_UNVALIDATED
@@ -32,11 +33,12 @@
 import com.android.server.connectivity.FullScore.POLICY_IS_VPN
 import com.android.testutils.DevSdkIgnoreRule
 import com.android.testutils.DevSdkIgnoreRunner
+import org.junit.After
+import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
 import kotlin.reflect.full.staticProperties
 import kotlin.test.assertEquals
-import kotlin.test.assertFailsWith
 import kotlin.test.assertFalse
 import kotlin.test.assertTrue
 
@@ -63,6 +65,23 @@
         return mixInScore(nc, nac, validated, false /* yieldToBadWifi */, destroyed)
     }
 
+    private val TAG = this::class.simpleName
+
+    private var wtfHandler: Log.TerribleFailureHandler? = null
+
+    @Before
+    fun setUp() {
+        // policyNameOf will call Log.wtf if passed an invalid policy.
+        wtfHandler = Log.setWtfHandler() { tagString, what, system ->
+            Log.d(TAG, "WTF captured, ignoring: $tagString $what")
+        }
+    }
+
+    @After
+    fun tearDown() {
+        Log.setWtfHandler(wtfHandler)
+    }
+
     @Test
     fun testGetLegacyInt() {
         val ns = FullScore(50, 0L /* policy */, KEEP_CONNECTED_NONE)
@@ -101,10 +120,9 @@
             assertFalse(foundNames.contains(name))
             foundNames.add(name)
         }
-        assertFailsWith<IllegalArgumentException> {
-            FullScore.policyNameOf(MAX_CS_MANAGED_POLICY + 1)
-        }
         assertEquals("IS_UNMETERED", FullScore.policyNameOf(POLICY_IS_UNMETERED))
+        val invalidPolicy = MAX_CS_MANAGED_POLICY + 1
+        assertEquals(Integer.toString(invalidPolicy), FullScore.policyNameOf(invalidPolicy))
     }
 
     fun getAllPolicies() = Regex("POLICY_.*").let { nameRegex ->