Skip to content

GitLab

T
tangled
Commit 2b0e1334 authored by Peter Kotwicz's avatar Peter Kotwicz Committed by Commit Bot
Browse files
Options

[Android Webapp] Fix potential concurrency issue with WebappDataStorage 

ProcessInitializationHandler#handleDeferredStartupTasksInitialization() and
WebApkUma#recordDeferredUma() call WebappRegistry#warmUpSharedPrefs()
from the background thread and the UI thread respectively. Both calls occur
from DeferredStartupHandler

This CL changes WebappRegistry#initStorages() to mutate
WebappRegistry#mStorages on the UI thread.

BUG=1000949

Change-Id: I71ec4d049000d67a4052502c72da62a2ce1aadf6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1801989
Commit-Queue: Peter Kotwicz <pkotwicz@chromium.org>
Reviewed-by: default avatarDominick Ng <dominickn@chromium.org>
Cr-Commit-Position: refs/heads/master@{#696943}
parent 4f1df16e
Hide whitespace changes
Inline Side-by-side
Showing with 70 additions and 36 deletions
chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappRegistry.java
View file @ 2b0e1334
......@@ -8,16 +8,20 @@ import android.content.Context;
import android.content.SharedPreferences;
import android.text.TextUtils;
import android.text.format.DateUtils;
import android.util.Pair;
import org.chromium.base.ContextUtils;
import org.chromium.base.PackageUtils;
import org.chromium.base.StrictModeContext;
import org.chromium.base.ThreadUtils;
import org.chromium.base.VisibleForTesting;
import org.chromium.base.annotations.CalledByNative;
import org.chromium.base.task.AsyncTask;
import org.chromium.base.task.PostTask;
import org.chromium.chrome.browser.browserservices.permissiondelegation.TrustedWebActivityPermissionStore;
import org.chromium.chrome.browser.browsing_data.UrlFilter;
import org.chromium.chrome.browser.browsing_data.UrlFilterBridge;
import org.chromium.content_public.browser.UiThreadTaskTraits;
import org.chromium.webapk.lib.common.WebApkConstants;
import java.util.ArrayList;
......@@ -60,6 +64,8 @@ public class WebappRegistry {
private static WebappRegistry sInstance = new WebappRegistry();
}
private boolean mIsInitialized;
private HashMap<String, WebappDataStorage> mStorages;
private SharedPreferences mPreferences;
private TrustedWebActivityPermissionStore mTrustedWebActivityPermissionStore;
......@@ -93,24 +99,27 @@ public class WebappRegistry {
}
/**
* Warm up the WebappRegistry and a specific WebappDataStorage SharedPreferences.
* Warm up the WebappRegistry and a specific WebappDataStorage SharedPreferences. Can be called
* from any thread.
* @param id The web app id to warm up in addition to the WebappRegistry.
*/
public static void warmUpSharedPrefsForId(String id) {
getInstance().initStorages(id, false);
getInstance().initStorages(id);
}
/**
* Warm up the WebappRegistry and all WebappDataStorage SharedPreferences.
* Warm up the WebappRegistry and all WebappDataStorage SharedPreferences. Can be called from
* any thread.
*/
public static void warmUpSharedPrefs() {
getInstance().initStorages(null, false);
getInstance().initStorages(null);
}
@VisibleForTesting
public static void refreshSharedPrefsForTesting() {
Holder.sInstance = new WebappRegistry();
getInstance().initStorages(null, true);
getInstance().clearStoragesForTesting();
getInstance().initStorages(null);
}
/**
......@@ -343,23 +352,45 @@ public class WebappRegistry {
}
}
private void initStorages(String idToInitialize, boolean replaceExisting) {
private void clearStoragesForTesting() {
ThreadUtils.assertOnUiThread();
mStorages.clear();
}
private void initStorages(String idToInitialize) {
Set<String> webapps = mPreferences.getStringSet(KEY_WEBAPP_SET, Collections.emptySet());
boolean initAll = (idToInitialize == null || idToInitialize.isEmpty());
// Don't overwrite any entry in mStorages unless replaceExisting is set to true.
if (initAll && !mIsInitialized) {
mTrustedWebActivityPermissionStore.initStorage();
mIsInitialized = true;
}
List<Pair<String, WebappDataStorage>> initedStorages =
new ArrayList<Pair<String, WebappDataStorage>>();
if (initAll) {
for (String id : webapps) {
if (replaceExisting || !mStorages.containsKey(id)) {
mStorages.put(id, WebappDataStorage.open(id));
if (!mStorages.containsKey(id)) {
initedStorages.add(Pair.create(id, WebappDataStorage.open(id)));
}
}
mTrustedWebActivityPermissionStore.initStorage();
} else {
if (webapps.contains(idToInitialize)
&& (replaceExisting || !mStorages.containsKey(idToInitialize))) {
mStorages.put(idToInitialize, WebappDataStorage.open(idToInitialize));
if (webapps.contains(idToInitialize) && !mStorages.containsKey(idToInitialize)) {
initedStorages.add(
Pair.create(idToInitialize, WebappDataStorage.open(idToInitialize)));
}
}
PostTask.runOrPostTask(
UiThreadTaskTraits.DEFAULT, () -> { initStoragesOnUiThread(initedStorages); });
}
private void initStoragesOnUiThread(List<Pair<String, WebappDataStorage>> initedStorages) {
ThreadUtils.assertOnUiThread();
for (Pair<String, WebappDataStorage> initedStorage : initedStorages) {
if (!mStorages.containsKey(initedStorage.first)) {
mStorages.put(initedStorage.first, initedStorage.second);
}
}
}
......
chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappModeTest.java
View file @ 2b0e1334
......@@ -38,6 +38,7 @@ import org.chromium.chrome.test.util.ApplicationTestUtils;
import org.chromium.chrome.test.util.browser.webapps.WebappTestHelper;
import org.chromium.content_public.browser.test.util.Criteria;
import org.chromium.content_public.browser.test.util.CriteriaHelper;
import org.chromium.content_public.browser.test.util.TestThreadUtils;
/**
* Tests that WebappActivities are launched correctly.
......@@ -99,27 +100,29 @@ public class WebappModeTest {
@Before
public void setUp() throws Exception {
WebappRegistry.refreshSharedPrefsForTesting();
// Register the webapps so when the data storage is opened, the test doesn't crash. There is
// no race condition with the retrieval as AsyncTasks are run sequentially on the background
// thread.
WebappRegistry.getInstance().register(
WEBAPP_1_ID, new WebappRegistry.FetchWebappDataStorageCallback() {
@Override
public void onWebappDataStorageRetrieved(WebappDataStorage storage) {
storage.updateFromShortcutIntent(createIntent(
WEBAPP_1_ID, WEBAPP_1_URL, WEBAPP_1_TITLE, WEBAPP_ICON, true));
}
});
WebappRegistry.getInstance().register(
WEBAPP_2_ID, new WebappRegistry.FetchWebappDataStorageCallback() {
@Override
public void onWebappDataStorageRetrieved(WebappDataStorage storage) {
storage.updateFromShortcutIntent(createIntent(
WEBAPP_1_ID, WEBAPP_1_URL, WEBAPP_1_TITLE, WEBAPP_ICON, true));
}
});
TestThreadUtils.runOnUiThreadBlocking(() -> {
WebappRegistry.refreshSharedPrefsForTesting();
// Register the webapps so when the data storage is opened, the test doesn't crash.
// There is no race condition with the retrieval as AsyncTasks are run sequentially on
// the background thread.
WebappRegistry.getInstance().register(
WEBAPP_1_ID, new WebappRegistry.FetchWebappDataStorageCallback() {
@Override
public void onWebappDataStorageRetrieved(WebappDataStorage storage) {
storage.updateFromShortcutIntent(createIntent(
WEBAPP_1_ID, WEBAPP_1_URL, WEBAPP_1_TITLE, WEBAPP_ICON, true));
}
});
WebappRegistry.getInstance().register(
WEBAPP_2_ID, new WebappRegistry.FetchWebappDataStorageCallback() {
@Override
public void onWebappDataStorageRetrieved(WebappDataStorage storage) {
storage.updateFromShortcutIntent(createIntent(
WEBAPP_1_ID, WEBAPP_1_URL, WEBAPP_1_TITLE, WEBAPP_ICON, true));
}
});
});
}
/**
......@@ -319,7 +322,7 @@ public class WebappModeTest {
Activity lastActivity = ApplicationStatus.getLastTrackedFocusedActivity();
return isWebappActivityReady(lastActivity);
}
});
}, 10000, CriteriaHelper.DEFAULT_POLLING_INTERVAL);
return (WebappActivity) ApplicationStatus.getLastTrackedFocusedActivity();
}
......
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