Skip to content

GitLab

T
tangled
Commit e4795a72 authored by aberent's avatar aberent Committed by Commit bot
Browse files
Options

Remove the Android policy cache 

The Android policy cache was originally created to avoid Strict Mode
violations, but doesn't do so in Webview. Actually, although reading
the app restrictions on the UI thread is a strict mode violation it
is unlikely to cause UI visible delays, As such the solution is to
simply to allow file reads in this component.

BUG=628627

Review-Url: https://codereview.chromium.org/2322963002
Cr-Commit-Position: refs/heads/master@{#418818}
parent b75661d3
Hide whitespace changes
Inline Side-by-side
Showing with 11 additions and 108 deletions
components/policy/android/java/src/org/chromium/policy/AbstractAppRestrictionsProvider.java
View file @ e4795a72
......@@ -8,30 +8,20 @@ import android.content.BroadcastReceiver;
import android.content.Context;
import android.content.Intent;
import android.content.IntentFilter;
import android.os.AsyncTask;
import android.os.Bundle;
import android.os.Handler;
import android.os.Parcel;
import android.util.Base64;
import android.os.StrictMode;
import org.chromium.base.ContextUtils;
import org.chromium.base.Log;
import org.chromium.base.ThreadUtils;
import org.chromium.base.VisibleForTesting;
import org.chromium.base.metrics.RecordHistogram;
import java.util.concurrent.Executor;
/**
* Retrieves app restrictions and provides them to the parent class as Bundles. Ensures that
* restrictions can be retrieved early in the application's life cycle by caching previously
* obtained bundles.
* Retrieves app restrictions and provides them to the parent class as Bundles.
*
* Needs to be subclassed to specify how to retrieve the restrictions.
*/
public abstract class AbstractAppRestrictionsProvider extends PolicyProvider {
private static final String PREFERENCE_KEY = "App Restrictions";
private static final String TAG = "policy";
/** {@link Bundle} holding the restrictions to be used during tests. */
......@@ -45,8 +35,6 @@ public abstract class AbstractAppRestrictionsProvider extends PolicyProvider {
}
};
private Executor mExecutor = AsyncTask.THREAD_POOL_EXECUTOR;
/**
* @param context The application context.
*/
......@@ -91,27 +79,16 @@ public abstract class AbstractAppRestrictionsProvider extends PolicyProvider {
return;
}
final Bundle cachedResult = getCachedPolicies();
if (cachedResult != null) {
notifySettingsAvailable(cachedResult);
}
// Because some policies are needed during startup this has to be synchronous. There is
// no way of reading policies (or cached policies from a previous run) without doing
// a disk read, so we have to disable strict mode here.
StrictMode.ThreadPolicy policy = StrictMode.allowThreadDiskReads();
long startTime = System.currentTimeMillis();
final Bundle bundle = getApplicationRestrictions(mContext.getPackageName());
recordStartTimeHistogram(startTime);
StrictMode.setThreadPolicy(policy);
mExecutor.execute(new Runnable() {
@Override
public void run() {
long startTime = System.currentTimeMillis();
final Bundle bundle = getApplicationRestrictions(mContext.getPackageName());
recordStartTimeHistogram(startTime);
ThreadUtils.runOnUiThread(new Runnable() {
@Override
public void run() {
cachePolicies(bundle);
notifySettingsAvailable(bundle);
}
});
}
});
notifySettingsAvailable(bundle);
}
@Override
......@@ -130,56 +107,12 @@ public abstract class AbstractAppRestrictionsProvider extends PolicyProvider {
}
}
private void cachePolicies(Bundle policies) {
Parcel p = Parcel.obtain();
p.writeBundle(policies);
byte bytes[] = p.marshall();
String s = Base64.encodeToString(bytes, 0);
ContextUtils.getAppSharedPreferences().edit().putString(PREFERENCE_KEY, s).apply();
}
private Bundle getCachedPolicies() {
String s = ContextUtils.getAppSharedPreferences().getString(PREFERENCE_KEY, null);
if (s == null) {
return null;
}
byte bytes[] = Base64.decode(s, 0);
Parcel p = Parcel.obtain();
// Unmarshalling the parcel is, in theory, unsafe if the Android version or API version has
// changed, but the worst that is likely to happen is that the bundle comes back empty, and
// this will be corrected once the Android returns the real App Restrictions.
p.unmarshall(bytes, 0, bytes.length);
p.setDataPosition(0);
Bundle result;
try {
result = p.readBundle();
} catch (IllegalStateException e) {
result = null;
}
recordCacheLoadResultHistogram(result != null);
return result;
}
// Extracted to allow stubbing, since it calls a static that can't easily be stubbed
@VisibleForTesting
protected void recordCacheLoadResultHistogram(final boolean success) {
RecordHistogram.recordBooleanHistogram(
"Enterprise.AppRestrictionsCacheLoad", success);
}
// Extracted to allow stubbing, since it calls a static that can't easily be stubbed
@VisibleForTesting
protected void recordStartTimeHistogram(long startTime) {
// TODO(aberent): Re-implement once we understand why the previous implementation was giving
// random crashes (https://crbug.com/535043)
}
/**
* @param testExecutor - The executor to use for this class's AsyncTasks.
*/
@VisibleForTesting
void setTaskExecutor(Executor testExecutor) {
mExecutor = testExecutor;
}
/**
* Restrictions to be used during tests. Subsequent attempts to retrieve the restrictions will
......
components/policy/android/junit/src/org/chromium/policy/AbstractAppRestrictionsProviderTest.java
View file @ e4795a72
......@@ -4,16 +4,11 @@
package org.chromium.policy;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.ArgumentMatchers.anyLong;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
......@@ -30,7 +25,6 @@ import org.robolectric.Robolectric;
import org.robolectric.RuntimeEnvironment;
import org.robolectric.annotation.Config;
import org.robolectric.shadows.ShadowApplication;
import org.robolectric.shadows.ShadowLooper;
import java.util.concurrent.Executor;
......@@ -95,8 +89,6 @@ public class AbstractAppRestrictionsProviderTest {
// Mock out the histogram functions, since they call statics.
AbstractAppRestrictionsProvider provider = spy(new DummyAppRestrictionsProvider(context));
provider.setTaskExecutor(new TestExecutor());
doNothing().when(provider).recordCacheLoadResultHistogram(anyBoolean());
doNothing().when(provider).recordStartTimeHistogram(anyLong());
// Set up the buffer to be returned by getApplicationRestrictions.
......@@ -106,32 +98,10 @@ public class AbstractAppRestrictionsProviderTest {
CombinedPolicyProvider combinedProvider = mock(CombinedPolicyProvider.class);
provider.setManagerAndSource(combinedProvider, 0);
// Refresh with no cache should do nothing immediately.
provider.refresh();
verify(combinedProvider, never()).onSettingsAvailable(anyInt(), any(Bundle.class));
// Let the Async task run and return its result.
Robolectric.getBackgroundThreadScheduler().advanceBy(0);
// The AsyncTask should now have got the restrictions.
verify(provider).getApplicationRestrictions(anyString());
verify(provider).recordStartTimeHistogram(anyLong());
ShadowLooper.runUiThreadTasks();
// The policies should now have been set.
verify(combinedProvider).onSettingsAvailable(0, b1);
// On next refresh onSettingsAvailable should be called twice, once with the current buffer
// and once with the new data.
Bundle b2 = new Bundle();
b2.putString("Key1", "value1");
b2.putInt("Key2", 84);
when(provider.getApplicationRestrictions(anyString())).thenReturn(b2);
provider.refresh();
verify(combinedProvider, times(2)).onSettingsAvailable(0, b1);
Robolectric.getBackgroundThreadScheduler().advanceBy(0);
ShadowLooper.runUiThreadTasks();
verify(combinedProvider).onSettingsAvailable(0, b2);
}
/**
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment