From 39bf7dd5307cafa81b4e83d75bfe86295e30d666 Mon Sep 17 00:00:00 2001 From: Chaohui Wang Date: Thu, 4 May 2023 19:26:44 +0800 Subject: [PATCH] Fix dialog leak in RequestPermissionActivity Dialog still show when activity destroyed will cause leak. Dismiss dialog when activity onDestroy to fix this issue. Fix: 279522922 Test: Manually with "Don't keep activities" Test: Robolectric Test Change-Id: I445f4b160020823a6f6e2883055218c1224e2c48 --- .../bluetooth/RequestPermissionActivity.java | 89 ++++++++------- .../bluetooth/RequestPermissionHelper.kt | 16 +-- .../RequestPermissionActivityTest.kt | 102 ++++++++++++++++++ .../bluetooth/RequestPermissionHelperTest.kt | 24 +++-- 4 files changed, 175 insertions(+), 56 deletions(-) create mode 100644 tests/robotests/src/com/android/settings/bluetooth/RequestPermissionActivityTest.kt diff --git a/src/com/android/settings/bluetooth/RequestPermissionActivity.java b/src/com/android/settings/bluetooth/RequestPermissionActivity.java index 620cfb0586d..32ca2777392 100644 --- a/src/com/android/settings/bluetooth/RequestPermissionActivity.java +++ b/src/com/android/settings/bluetooth/RequestPermissionActivity.java @@ -72,6 +72,7 @@ public class RequestPermissionActivity extends Activity implements private int mRequest; private AlertDialog mDialog; + private AlertDialog mRequestDialog; private BroadcastReceiver mReceiver; @@ -96,33 +97,35 @@ public class RequestPermissionActivity extends Activity implements if (mRequest == REQUEST_DISABLE) { switch (btState) { case BluetoothAdapter.STATE_OFF: - case BluetoothAdapter.STATE_TURNING_OFF: { + case BluetoothAdapter.STATE_TURNING_OFF: proceedAndFinish(); - } break; - + break; case BluetoothAdapter.STATE_ON: - case BluetoothAdapter.STATE_TURNING_ON: { - RequestPermissionHelper.INSTANCE.requestDisable(this, mAppLabel, - () -> { - onDisableConfirmed(); - return Unit.INSTANCE; - }, - () -> { - cancelAndFinish(); - return Unit.INSTANCE; - }); - } break; - - default: { + case BluetoothAdapter.STATE_TURNING_ON: + mRequestDialog = + RequestPermissionHelper.INSTANCE.requestDisable(this, mAppLabel, + () -> { + onDisableConfirmed(); + return Unit.INSTANCE; + }, + () -> { + cancelAndFinish(); + return Unit.INSTANCE; + }); + if (mRequestDialog != null) { + mRequestDialog.show(); + } + break; + default: Log.e(TAG, "Unknown adapter state: " + btState); cancelAndFinish(); - } break; + break; } } else { switch (btState) { case BluetoothAdapter.STATE_OFF: case BluetoothAdapter.STATE_TURNING_OFF: - case BluetoothAdapter.STATE_TURNING_ON: { + case BluetoothAdapter.STATE_TURNING_ON: /* * Strictly speaking STATE_TURNING_ON belong with STATE_ON; * however, BT may not be ready when the user clicks yes and we @@ -131,20 +134,23 @@ public class RequestPermissionActivity extends Activity implements * case via the broadcast receiver. */ - // Start the helper activity to ask the user about enabling bt AND discovery - RequestPermissionHelper.INSTANCE.requestEnable(this, mAppLabel, - mRequest == REQUEST_ENABLE_DISCOVERABLE ? mTimeout : -1, - () -> { - onEnableConfirmed(); - return Unit.INSTANCE; - }, - () -> { - cancelAndFinish(); - return Unit.INSTANCE; - }); - } break; - - case BluetoothAdapter.STATE_ON: { + // Show the helper dialog to ask the user about enabling bt AND discovery + mRequestDialog = + RequestPermissionHelper.INSTANCE.requestEnable(this, mAppLabel, + mRequest == REQUEST_ENABLE_DISCOVERABLE ? mTimeout : -1, + () -> { + onEnableConfirmed(); + return Unit.INSTANCE; + }, + () -> { + cancelAndFinish(); + return Unit.INSTANCE; + }); + if (mRequestDialog != null) { + mRequestDialog.show(); + } + break; + case BluetoothAdapter.STATE_ON: if (mRequest == REQUEST_ENABLE) { // Nothing to do. Already enabled. proceedAndFinish(); @@ -152,12 +158,11 @@ public class RequestPermissionActivity extends Activity implements // Ask the user about enabling discovery mode createDialog(); } - } break; - - default: { + break; + default: Log.e(TAG, "Unknown adapter state: " + btState); cancelAndFinish(); - } break; + break; } } } @@ -275,10 +280,6 @@ public class RequestPermissionActivity extends Activity implements } } - if (mDialog != null) { - mDialog.dismiss(); - } - setResult(returnCode); finish(); } @@ -365,6 +366,14 @@ public class RequestPermissionActivity extends Activity implements unregisterReceiver(mReceiver); mReceiver = null; } + if (mDialog != null && mDialog.isShowing()) { + mDialog.dismiss(); + mDialog = null; + } + if (mRequestDialog != null && mRequestDialog.isShowing()) { + mRequestDialog.dismiss(); + mRequestDialog = null; + } } @Override diff --git a/src/com/android/settings/bluetooth/RequestPermissionHelper.kt b/src/com/android/settings/bluetooth/RequestPermissionHelper.kt index 000a7d16295..73084e4371d 100644 --- a/src/com/android/settings/bluetooth/RequestPermissionHelper.kt +++ b/src/com/android/settings/bluetooth/RequestPermissionHelper.kt @@ -30,20 +30,20 @@ object RequestPermissionHelper { timeout: Int, onAllow: () -> Unit, onDeny: () -> Unit, - ) { + ): AlertDialog? { if (context.resources.getBoolean(R.bool.auto_confirm_bluetooth_activation_dialog)) { // Don't even show the dialog if configured this way onAllow() - return + return null } - AlertDialog.Builder(context).apply { + return AlertDialog.Builder(context).apply { setMessage(context.getEnableMessage(timeout, appLabel)) setPositiveButton(R.string.allow) { _, _ -> if (context.isDisallowBluetooth()) onDeny() else onAllow() } setNegativeButton(R.string.deny) { _, _ -> onDeny() } setOnCancelListener { onDeny() } - }.show() + }.create() } fun requestDisable( @@ -51,18 +51,18 @@ object RequestPermissionHelper { appLabel: CharSequence?, onAllow: () -> Unit, onDeny: () -> Unit, - ) { + ): AlertDialog? { if (context.resources.getBoolean(R.bool.auto_confirm_bluetooth_activation_dialog)) { // Don't even show the dialog if configured this way onAllow() - return + return null } - AlertDialog.Builder(context).apply { + return AlertDialog.Builder(context).apply { setMessage(context.getDisableMessage(appLabel)) setPositiveButton(R.string.allow) { _, _ -> onAllow() } setNegativeButton(R.string.deny) { _, _ -> onDeny() } setOnCancelListener { onDeny() } - }.show() + }.create() } } diff --git a/tests/robotests/src/com/android/settings/bluetooth/RequestPermissionActivityTest.kt b/tests/robotests/src/com/android/settings/bluetooth/RequestPermissionActivityTest.kt new file mode 100644 index 00000000000..13cbb350feb --- /dev/null +++ b/tests/robotests/src/com/android/settings/bluetooth/RequestPermissionActivityTest.kt @@ -0,0 +1,102 @@ +/* + * Copyright (C) 2023 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package com.android.settings.bluetooth + +import android.bluetooth.BluetoothAdapter +import android.content.Intent +import com.android.settings.testutils.shadow.ShadowAlertDialogCompat +import com.google.common.truth.Truth.assertThat +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import org.robolectric.android.controller.ActivityController +import org.robolectric.annotation.Config +import org.robolectric.shadow.api.Shadow +import org.robolectric.shadows.ShadowBluetoothAdapter + +@RunWith(RobolectricTestRunner::class) +@Config(shadows = [ShadowAlertDialogCompat::class, ShadowBluetoothAdapter::class]) +class RequestPermissionActivityTest { + private lateinit var activityController: ActivityController + private lateinit var bluetoothAdapter: ShadowBluetoothAdapter + + @Before + fun setUp() { + bluetoothAdapter = Shadow.extract(BluetoothAdapter.getDefaultAdapter()) + } + + @After + fun tearDown() { + activityController.pause().stop().destroy() + ShadowAlertDialogCompat.reset() + } + + @Test + fun requestEnable_whenBluetoothIsOff_showConfirmDialog() { + bluetoothAdapter.setState(BluetoothAdapter.STATE_OFF) + + createActivity(action = BluetoothAdapter.ACTION_REQUEST_ENABLE) + + val dialog = ShadowAlertDialogCompat.getLatestAlertDialog() + val shadowDialog = ShadowAlertDialogCompat.shadowOf(dialog) + assertThat(shadowDialog.message.toString()) + .isEqualTo("An app wants to turn on Bluetooth") + } + + @Test + fun requestEnable_whenBluetoothIsOn_doNothing() { + bluetoothAdapter.setState(BluetoothAdapter.STATE_ON) + + createActivity(action = BluetoothAdapter.ACTION_REQUEST_ENABLE) + + val dialog = ShadowAlertDialogCompat.getLatestAlertDialog() + assertThat(dialog).isNull() + } + + @Test + fun requestDisable_whenBluetoothIsOff_doNothing() { + bluetoothAdapter.setState(BluetoothAdapter.STATE_OFF) + + createActivity(action = BluetoothAdapter.ACTION_REQUEST_DISABLE) + + val dialog = ShadowAlertDialogCompat.getLatestAlertDialog() + assertThat(dialog).isNull() + } + + @Test + fun requestDisable_whenBluetoothIsOn_showConfirmDialog() { + bluetoothAdapter.setState(BluetoothAdapter.STATE_ON) + + createActivity(action = BluetoothAdapter.ACTION_REQUEST_DISABLE) + + val dialog = ShadowAlertDialogCompat.getLatestAlertDialog() + val shadowDialog = ShadowAlertDialogCompat.shadowOf(dialog) + assertThat(shadowDialog.message.toString()) + .isEqualTo("An app wants to turn off Bluetooth") + } + + private fun createActivity(action: String) { + activityController = + ActivityController.of(RequestPermissionActivity(), Intent(action)).apply { + create() + start() + postCreate(null) + resume() + } + } +} \ No newline at end of file diff --git a/tests/robotests/src/com/android/settings/bluetooth/RequestPermissionHelperTest.kt b/tests/robotests/src/com/android/settings/bluetooth/RequestPermissionHelperTest.kt index ce0792c468a..e51a2f92919 100644 --- a/tests/robotests/src/com/android/settings/bluetooth/RequestPermissionHelperTest.kt +++ b/tests/robotests/src/com/android/settings/bluetooth/RequestPermissionHelperTest.kt @@ -49,13 +49,14 @@ class RequestPermissionHelperTest { @Test fun requestEnable_withAppLabelAndNoTimeout_hasCorrectMessage() { val activity = activityController.get() + RequestPermissionHelper.requestEnable( context = activity, appLabel = "App Label", timeout = -1, onAllow = {}, onDeny = {}, - ) + )!!.show() assertLatestMessageIs("App Label wants to turn on Bluetooth") } @@ -63,13 +64,14 @@ class RequestPermissionHelperTest { @Test fun requestEnable_withAppLabelAndZeroTimeout_hasCorrectMessage() { val activity = activityController.get() + RequestPermissionHelper.requestEnable( context = activity, appLabel = "App Label", timeout = 0, onAllow = {}, onDeny = {}, - ) + )!!.show() assertLatestMessageIs( "App Label wants to turn on Bluetooth and make your phone visible to other devices. " + @@ -80,13 +82,14 @@ class RequestPermissionHelperTest { @Test fun requestEnable_withAppLabelAndNormalTimeout_hasCorrectMessage() { val activity = activityController.get() + RequestPermissionHelper.requestEnable( context = activity, appLabel = "App Label", timeout = 120, onAllow = {}, onDeny = {}, - ) + )!!.show() assertLatestMessageIs( "App Label wants to turn on Bluetooth and make your phone visible to other devices " + @@ -97,13 +100,14 @@ class RequestPermissionHelperTest { @Test fun requestEnable_withNoAppLabelAndNoTimeout_hasCorrectMessage() { val activity = activityController.get() + RequestPermissionHelper.requestEnable( context = activity, appLabel = null, timeout = -1, onAllow = {}, onDeny = {}, - ) + )!!.show() assertLatestMessageIs("An app wants to turn on Bluetooth") } @@ -111,13 +115,14 @@ class RequestPermissionHelperTest { @Test fun requestEnable_withNoAppLabelAndZeroTimeout_hasCorrectMessage() { val activity = activityController.get() + RequestPermissionHelper.requestEnable( context = activity, appLabel = null, timeout = 0, onAllow = {}, onDeny = {}, - ) + )!!.show() assertLatestMessageIs( "An app wants to turn on Bluetooth and make your phone visible to other devices. " + @@ -128,13 +133,14 @@ class RequestPermissionHelperTest { @Test fun requestEnable_withNoAppLabelAndNormalTimeout_hasCorrectMessage() { val activity = activityController.get() + RequestPermissionHelper.requestEnable( context = activity, appLabel = null, timeout = 120, onAllow = {}, onDeny = {}, - ) + )!!.show() assertLatestMessageIs( "An app wants to turn on Bluetooth and make your phone visible to other devices for " + @@ -177,12 +183,13 @@ class RequestPermissionHelperTest { @Test fun requestDisable_withAppLabel_hasCorrectMessage() { val activity = activityController.get() + RequestPermissionHelper.requestDisable( context = activity, appLabel = "App Label", onAllow = {}, onDeny = {}, - ) + )!!.show() assertLatestMessageIs("App Label wants to turn off Bluetooth") } @@ -190,12 +197,13 @@ class RequestPermissionHelperTest { @Test fun requestDisable_withNoAppLabel_hasCorrectMessage() { val activity = activityController.get() + RequestPermissionHelper.requestDisable( context = activity, appLabel = null, onAllow = {}, onDeny = {}, - ) + )!!.show() assertLatestMessageIs("An app wants to turn off Bluetooth") }