From 4a3eda992b70601e26a76d9492b2f9381cf0fe51 Mon Sep 17 00:00:00 2001 From: Bookatz Date: Mon, 10 Apr 2017 13:10:46 -0700 Subject: [PATCH] Fix double-detach DualTimer bug DualTimer attempted to detach its subTimer twice when reset(true) was called, once explicitly and once via a call to the main timer. This fixes that problem by getting rid of the explicit detach in reset. Bug: 37208694 Test: runtest -x frameworks/base/core/tests/coretests/src/com/android/internal/os/BatteryStatsTests.java and manually looked for "Removed unknown observer" error in logcat. Change-Id: Ic5ff7d799d46236a74ab0825e108bef40bac0360 --- .../android/internal/os/BatteryStatsImpl.java | 5 +- .../os/BatteryStatsDualTimerTest.java | 60 +++++++++++++++++++ .../internal/os/BatteryStatsSensorTest.java | 44 ++++++++++++++ .../internal/os/BatteryStatsTests.java | 1 + 4 files changed, 108 insertions(+), 2 deletions(-) create mode 100644 core/tests/coretests/src/com/android/internal/os/BatteryStatsDualTimerTest.java diff --git a/core/java/com/android/internal/os/BatteryStatsImpl.java b/core/java/com/android/internal/os/BatteryStatsImpl.java index 916241c31eba8..fe3860507c27d 100644 --- a/core/java/com/android/internal/os/BatteryStatsImpl.java +++ b/core/java/com/android/internal/os/BatteryStatsImpl.java @@ -2071,15 +2071,16 @@ public class BatteryStatsImpl extends BatteryStats { @Override public boolean reset(boolean detachIfReset) { boolean active = false; + // Do not detach the subTimer explicitly since that'll be done by DualTimer.detach(). + active |= !mSubTimer.reset(false); active |= !super.reset(detachIfReset); - active |= !mSubTimer.reset(detachIfReset); return !active; } @Override public void detach() { - super.detach(); mSubTimer.detach(); + super.detach(); } @Override diff --git a/core/tests/coretests/src/com/android/internal/os/BatteryStatsDualTimerTest.java b/core/tests/coretests/src/com/android/internal/os/BatteryStatsDualTimerTest.java new file mode 100644 index 0000000000000..3a5a9f5bc67a9 --- /dev/null +++ b/core/tests/coretests/src/com/android/internal/os/BatteryStatsDualTimerTest.java @@ -0,0 +1,60 @@ +/* + * Copyright (C) 2017 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.internal.os; + +import android.os.BatteryStats; +import android.support.test.filters.SmallTest; + +import junit.framework.TestCase; + +/** + * Test BatteryStatsImpl.DualTimer. + */ +public class BatteryStatsDualTimerTest extends TestCase { + + @SmallTest + public void testResetDetach() throws Exception { + final MockClocks clocks = new MockClocks(); + clocks.realtime = clocks.uptime = 100; + + final BatteryStatsImpl.TimeBase timeBase = new BatteryStatsImpl.TimeBase(); + timeBase.init(clocks.uptimeMillis(), clocks.elapsedRealtime()); + final BatteryStatsImpl.TimeBase subTimeBase = new BatteryStatsImpl.TimeBase(); + subTimeBase.init(clocks.uptimeMillis(), clocks.elapsedRealtime()); + + final BatteryStatsImpl.DualTimer timer = new BatteryStatsImpl.DualTimer(clocks, + null, BatteryStats.WAKE_TYPE_PARTIAL, null, timeBase, subTimeBase); + + assertTrue(timeBase.hasObserver(timer)); + assertFalse(subTimeBase.hasObserver(timer)); + assertFalse(timeBase.hasObserver(timer.getSubTimer())); + assertTrue(subTimeBase.hasObserver(timer.getSubTimer())); + + // Timer is running so resetting it should not remove it from timerbases. + clocks.realtime = clocks.uptime = 200; + timer.startRunningLocked(clocks.realtime); + timer.reset(true); + assertTrue(timeBase.hasObserver(timer)); + assertTrue(subTimeBase.hasObserver(timer.getSubTimer())); + + // Stop timer and ensure that resetting removes it from timebases. + clocks.realtime = clocks.uptime = 300; + timer.stopRunningLocked(clocks.realtime); + timer.reset(true); + assertFalse(timeBase.hasObserver(timer)); + assertFalse(timeBase.hasObserver(timer.getSubTimer())); + } +} diff --git a/core/tests/coretests/src/com/android/internal/os/BatteryStatsSensorTest.java b/core/tests/coretests/src/com/android/internal/os/BatteryStatsSensorTest.java index 47bc502d4cf1a..af4a6d92cea66 100644 --- a/core/tests/coretests/src/com/android/internal/os/BatteryStatsSensorTest.java +++ b/core/tests/coretests/src/com/android/internal/os/BatteryStatsSensorTest.java @@ -371,4 +371,48 @@ public class BatteryStatsSensorTest extends TestCase { // Test: UID_2 - background count assertEquals(2, bgTimer2.getCountLocked(BatteryStats.STATS_SINCE_CHARGED)); } + + @SmallTest + public void testSensorReset() throws Exception { + final MockClocks clocks = new MockClocks(); + MockBatteryStatsImpl bi = new MockBatteryStatsImpl(clocks); + bi.mForceOnBattery = true; + clocks.realtime = 100; + clocks.uptime = 100; + bi.getOnBatteryTimeBase().setRunning(true, 100_000, 100_000); + bi.noteUidProcessStateLocked(UID, ActivityManager.PROCESS_STATE_RECEIVER); + + clocks.realtime += 100; + clocks.uptime += 100; + + bi.noteStartSensorLocked(UID, SENSOR_ID); + + clocks.realtime += 100; + clocks.uptime += 100; + + // The sensor is started and the timer has been created. + final BatteryStats.Uid uid = bi.getUidStats().get(UID); + assertNotNull(uid); + + BatteryStats.Uid.Sensor sensor = uid.getSensorStats().get(SENSOR_ID); + assertNotNull(sensor); + assertNotNull(sensor.getSensorTime()); + assertNotNull(sensor.getSensorBackgroundTime()); + + // Reset the stats. Since the sensor is still running, we should still see the timer + bi.getUidStatsLocked(UID).reset(); + + sensor = uid.getSensorStats().get(SENSOR_ID); + assertNotNull(sensor); + assertNotNull(sensor.getSensorTime()); + assertNotNull(sensor.getSensorBackgroundTime()); + + bi.noteStopSensorLocked(UID, SENSOR_ID); + + // Now the sensor timer has stopped so this reset should also take out the sensor. + bi.getUidStatsLocked(UID).reset(); + + sensor = uid.getSensorStats().get(SENSOR_ID); + assertNull(sensor); + } } diff --git a/core/tests/coretests/src/com/android/internal/os/BatteryStatsTests.java b/core/tests/coretests/src/com/android/internal/os/BatteryStatsTests.java index 9607a59f58ddd..57d6934d4bdcb 100644 --- a/core/tests/coretests/src/com/android/internal/os/BatteryStatsTests.java +++ b/core/tests/coretests/src/com/android/internal/os/BatteryStatsTests.java @@ -5,6 +5,7 @@ import org.junit.runners.Suite; @RunWith(Suite.class) @Suite.SuiteClasses({ + BatteryStatsDualTimerTest.class, BatteryStatsDurationTimerTest.class, BatteryStatsSamplingTimerTest.class, BatteryStatsServTest.class,