From a69311f29586cd5f374fa98a5a217a45b97e306d Mon Sep 17 00:00:00 2001 From: zachh Date: Fri, 28 Jul 2017 14:49:00 -0700 Subject: Don't enable strict mode in non-bugfood builds. There were some calls to StrictMode#enableDefaults in our codebase that were not guarded by bugfood checks. In testing, #enableDefaults causes warnings to be printed that otherwise would not. This CL makes it so that we never call enableDefaults on non-bugfood builds, and cleans up and consolidates our usage of strict mode logic by introducing a new class called DialerStrictMode. Test: none PiperOrigin-RevId: 163521622 Change-Id: I841b4198a5dd6084ee104dc6907165e3379d0451 --- .../dialer/binary/common/DialerApplication.java | 15 +--- .../configprovider/SharedPrefConfigProvider.java | 25 ++---- .../dialer/strictmode/DialerStrictMode.java | 95 ++++++++++++++++++++++ java/com/android/dialer/strictmode/proguard.flags | 1 + .../impl/transcribe/TranscriptionService.java | 11 +-- 5 files changed, 109 insertions(+), 38 deletions(-) create mode 100644 java/com/android/dialer/strictmode/DialerStrictMode.java create mode 100644 java/com/android/dialer/strictmode/proguard.flags (limited to 'java') diff --git a/java/com/android/dialer/binary/common/DialerApplication.java b/java/com/android/dialer/binary/common/DialerApplication.java index 08666a21c..0d38541e5 100644 --- a/java/com/android/dialer/binary/common/DialerApplication.java +++ b/java/com/android/dialer/binary/common/DialerApplication.java @@ -17,18 +17,17 @@ package com.android.dialer.binary.common; import android.app.Application; -import android.os.StrictMode; import android.os.Trace; import android.support.annotation.NonNull; import android.support.v4.os.BuildCompat; import com.android.dialer.blocking.BlockedNumbersAutoMigrator; import com.android.dialer.blocking.FilteredNumberAsyncQueryHandler; -import com.android.dialer.buildtype.BuildType; import com.android.dialer.calllog.CallLogComponent; import com.android.dialer.common.concurrent.DefaultDialerExecutorFactory; import com.android.dialer.inject.HasRootComponent; import com.android.dialer.notification.NotificationChannelManager; import com.android.dialer.persistentlog.PersistentLogger; +import com.android.dialer.strictmode.DialerStrictMode; /** A common application subclass for all Dialer build variants. */ public abstract class DialerApplication extends Application implements HasRootComponent { @@ -38,9 +37,8 @@ public abstract class DialerApplication extends Application implements HasRootCo @Override public void onCreate() { Trace.beginSection("DialerApplication.onCreate"); - if (BuildType.get() == BuildType.BUGFOOD) { - enableStrictMode(); - } + DialerStrictMode.onApplicationCreate(); + super.onCreate(); new BlockedNumbersAutoMigrator( this.getApplicationContext(), @@ -56,13 +54,6 @@ public abstract class DialerApplication extends Application implements HasRootCo Trace.endSection(); } - private void enableStrictMode() { - StrictMode.setThreadPolicy( - new StrictMode.ThreadPolicy.Builder().detectAll().penaltyLog().penaltyDeath().build()); - StrictMode.setVmPolicy( - new StrictMode.VmPolicy.Builder().detectAll().penaltyLog().penaltyDeath().build()); - } - /** * Returns a new instance of the root component for the application. Sub classes should define a * root component that extends all the sub components "HasComponent" intefaces. The component diff --git a/java/com/android/dialer/configprovider/SharedPrefConfigProvider.java b/java/com/android/dialer/configprovider/SharedPrefConfigProvider.java index fad25a409..6ee469572 100644 --- a/java/com/android/dialer/configprovider/SharedPrefConfigProvider.java +++ b/java/com/android/dialer/configprovider/SharedPrefConfigProvider.java @@ -20,11 +20,11 @@ import android.content.Context; import android.content.Intent; import android.content.SharedPreferences; import android.content.SharedPreferences.Editor; -import android.os.StrictMode; import android.support.annotation.Nullable; import com.android.dialer.common.Assert; import com.android.dialer.common.LogUtil; import com.android.dialer.inject.ApplicationContext; +import com.android.dialer.strictmode.DialerStrictMode; import com.android.dialer.util.DialerUtils; import javax.inject.Inject; @@ -95,37 +95,26 @@ class SharedPrefConfigProvider implements ConfigProvider { @Override public String getString(String key, String defaultValue) { - return bypassStrictMode( + // Reading shared prefs on the main thread is generally safe since a single instance is cached. + return DialerStrictMode.bypass( () -> getSharedPrefs(appContext).getString(PREF_PREFIX + key, defaultValue)); } @Override public long getLong(String key, long defaultValue) { - return bypassStrictMode( + // Reading shared prefs on the main thread is generally safe since a single instance is cached. + return DialerStrictMode.bypass( () -> getSharedPrefs(appContext).getLong(PREF_PREFIX + key, defaultValue)); } @Override public boolean getBoolean(String key, boolean defaultValue) { - return bypassStrictMode( + // Reading shared prefs on the main thread is generally safe since a single instance is cached. + return DialerStrictMode.bypass( () -> getSharedPrefs(appContext).getBoolean(PREF_PREFIX + key, defaultValue)); } private static SharedPreferences getSharedPrefs(Context appContext) { return DialerUtils.getDefaultSharedPreferenceForDeviceProtectedStorageContext(appContext); } - - private interface Provider { - T get(); - } - - // Reading shared prefs on the main thread is generally safe since a single instance is cached. - private static T bypassStrictMode(Provider provider) { - StrictMode.ThreadPolicy oldPolicy = StrictMode.allowThreadDiskReads(); - try { - return provider.get(); - } finally { - StrictMode.setThreadPolicy(oldPolicy); - } - } } diff --git a/java/com/android/dialer/strictmode/DialerStrictMode.java b/java/com/android/dialer/strictmode/DialerStrictMode.java new file mode 100644 index 000000000..c9bbeafbf --- /dev/null +++ b/java/com/android/dialer/strictmode/DialerStrictMode.java @@ -0,0 +1,95 @@ +/* + * 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.dialer.strictmode; + +import android.os.StrictMode; +import android.os.StrictMode.ThreadPolicy; +import android.os.StrictMode.VmPolicy; +import com.android.dialer.buildtype.BuildType; + +/** + * Enables strict mode for the application, and provides means of temporarily disabling it. + * + *

NOTE: All methods in this class are stripped by proguard in release builds. + */ +public final class DialerStrictMode { + + /** Initializes strict mode on application start. */ + public static void onApplicationCreate() { + enableDeathPenalty(); + } + + /** + * Disables the strict mode death penalty. If strict mode is enabled for the build, warnings are + * printed instead of the application crashing. + * + *

You should typically do this only temporarily and restore the death penalty in a finally + * block using {@link #enableDeathPenalty()}. + */ + public static void disableDeathPenalty() { + if (isStrictModeAllowed()) { + StrictMode.setThreadPolicy(threadPolicyTemplate().build()); + StrictMode.setVmPolicy(vmPolicyTemplate().build()); + } + } + + /** + * Restore the death penalty. This should typically be called in a finally block after calling + * {@link #disableDeathPenalty()}. + */ + public static void enableDeathPenalty() { + if (isStrictModeAllowed()) { + StrictMode.setThreadPolicy(threadPolicyTemplate().penaltyDeath().build()); + StrictMode.setVmPolicy(vmPolicyTemplate().penaltyDeath().build()); + } + } + + private static ThreadPolicy.Builder threadPolicyTemplate() { + return new StrictMode.ThreadPolicy.Builder().detectAll().penaltyLog(); + } + + private static VmPolicy.Builder vmPolicyTemplate() { + return new StrictMode.VmPolicy.Builder().detectAll().penaltyLog(); + } + + private static boolean isStrictModeAllowed() { + return BuildType.get() == BuildType.BUGFOOD; + } + + /** Functional interface intended to be used with {@link #bypass(Provider)}. */ + public interface Provider { + T get(); + } + + /** + * Convenience method for disabling and enabling the death penalty using lambdas. + * + *

For example: + * + *

+ * DialerStrictMode.bypass(() -> doDiskAccessOnMainThread()); + * + */ + public static T bypass(Provider provider) { + disableDeathPenalty(); + try { + return provider.get(); + } finally { + enableDeathPenalty(); + } + } +} diff --git a/java/com/android/dialer/strictmode/proguard.flags b/java/com/android/dialer/strictmode/proguard.flags new file mode 100644 index 000000000..785f9ae86 --- /dev/null +++ b/java/com/android/dialer/strictmode/proguard.flags @@ -0,0 +1 @@ +-assumenosideeffects class com.android.dialer.strictmode.DialerStrictMode { public * ; } diff --git a/java/com/android/voicemail/impl/transcribe/TranscriptionService.java b/java/com/android/voicemail/impl/transcribe/TranscriptionService.java index 3e80a7f59..9a781e6c2 100644 --- a/java/com/android/voicemail/impl/transcribe/TranscriptionService.java +++ b/java/com/android/voicemail/impl/transcribe/TranscriptionService.java @@ -24,7 +24,6 @@ import android.content.ComponentName; import android.content.Context; import android.content.Intent; import android.net.Uri; -import android.os.StrictMode; import android.support.annotation.MainThread; import android.support.annotation.VisibleForTesting; import android.support.v4.os.BuildCompat; @@ -32,6 +31,7 @@ import android.text.TextUtils; import com.android.dialer.common.Assert; import com.android.dialer.common.LogUtil; import com.android.dialer.constants.ScheduledJobIds; +import com.android.dialer.strictmode.DialerStrictMode; import com.android.voicemail.impl.transcribe.grpc.TranscriptionClientFactory; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -48,7 +48,6 @@ public class TranscriptionService extends JobService { private JobParameters jobParameters; private TranscriptionClientFactory clientFactory; private TranscriptionConfigProvider configProvider; - private StrictMode.VmPolicy originalPolicy; /** Callback used by a task to indicate it has finished processing its work item */ interface JobCallback { @@ -111,8 +110,7 @@ public class TranscriptionService extends JobService { LogUtil.i( "TranscriptionService.onStartJob", "transcription server address: " + configProvider.getServerAddress()); - originalPolicy = StrictMode.getVmPolicy(); - StrictMode.enableDefaults(); + DialerStrictMode.disableDeathPenalty(); // Re-enabled in cleanup. jobParameters = params; return checkForWork(); } @@ -142,10 +140,7 @@ public class TranscriptionService extends JobService { executorService.shutdownNow(); executorService = null; } - if (originalPolicy != null) { - StrictMode.setVmPolicy(originalPolicy); - originalPolicy = null; - } + DialerStrictMode.enableDeathPenalty(); } @MainThread -- cgit v1.2.3