Skip to content

Commit

Permalink
Migrate stale tabs to CriticalPersistedTabData
Browse files Browse the repository at this point in the history 
It has been observed that stale Tabs (those which sit uninteracted
with) are not being migrated to CriticalPersistedTabData. This is
because we only migrate a Tab to CriticalPersistedTabData if a Tab
changes (e.g. URL changes). This CL introduces a mechanism to migrate
5 Tabs to CriticalPersistedTabData for every 1 save of TabState for a
Tab. This approach was taken with the following considerations:
- Tabs should not be migrated on startup as this will risk a startup
  regression. After a TabState file is saved is a time of relatively
  low resource utilization.
- Migrating too many Tabs at a time should be avoided such that the
  system doesn't get flooded with saves (some users have 100+ Tabs).
  5 seems reasonable for a Batch size.

(cherry picked from commit b244428)

Bug: 1303027
Change-Id: If1d0159e9f4f0e799e08a771e7b9b7064ce3f7cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3498958
Reviewed-by: David Trainor <dtrainor@chromium.org>
Commit-Queue: David Maunder <davidjm@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#978453}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3508900
Cr-Commit-Position: refs/branch-heads/4896@{#383}
Cr-Branched-From: 1f63ff4-refs/heads/main@{#972766}
  • Loading branch information
David Maunder authored and Chromium LUCI CQ committed Mar 8, 2022
1 parent 382ed12 commit 66ba9a4
Show file tree
Hide file tree
Showing 3 changed files with 195 additions and 4 deletions.
58 changes: 54 additions & 4 deletions chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java
View file
Expand Up @@ -18,6 +18,7 @@

import org.chromium.base.Callback;
import org.chromium.base.CallbackController;
import org.chromium.base.FeatureList;
import org.chromium.base.Log;
import org.chromium.base.ObserverList;
import org.chromium.base.StreamUtil;
Expand Down Expand Up @@ -101,6 +102,9 @@ public class TabPersistentStore {

/** Prevents two TabPersistentStores from saving the same file simultaneously. */
private static final Object SAVE_LIST_LOCK = new Object();
private static final int MIGRATE_TO_CRITICAL_PERSISTED_TAB_DATA_DEFAULT_BATCH_SIZE = 5;
private static final String MIGRATE_TO_CRITICAL_PERSISTED_TAB_DATA_BATCH_SIZE_PARAM =
"migrate_to_critical_persisted_tab_data_batch_size";
private TabModelObserver mTabModelObserver;

@IntDef({ActiveTabState.OTHER, ActiveTabState.NTP, ActiveTabState.EMPTY})
Expand Down Expand Up @@ -280,6 +284,20 @@ public TabModelMetadata(int selectedIndex) {
private final ObserverList<TabPersistentStoreObserver> mObservers;

private final Deque<Tab> mTabsToSave;
// Tabs to migrate to CriticalPersistedTabData (i.e. save a CriticalPersistedTabData
// file for the Tab). When the CriticalPersistedTabData flag is on,
// CriticalPersistedTabData files are saved upon the first successful
// save of a TabState for a Tab. However, this only happens if a Tab attribute
// changes e.g. change of URL or move the Tab to a different group (changing the root
// id). Stale Tabs (i.e. Tabs which sit uninteracted with) stay unmigrated. This queue is used
// to migrate an additional MIGRATE_TO_CRITICAL_PERSISTED_TAB_DATA_BATCH_SIZE Tabs for every 1
// save of a TabState. Considerations (especially in light of users who have a lot of Tabs):
// - Tabs should not be migrated directly on startup, as this may introduce a startup
// metric regression. After a TabState save should be a time of relatively low
// resource utilization.
// - MIGRATE_TO_CRITICAL_PERSISTED_TAB_DATA_BATCH_SIZE should not be too large or
// the system will be flooded with saves.
private final Deque<Tab> mTabsToMigrate;
private final Deque<TabRestoreDetails> mTabsToRestore;
private final Set<Integer> mTabIdsToRestore;

Expand Down Expand Up @@ -320,6 +338,7 @@ public TabPersistentStore(TabPersistencePolicy policy, TabModelSelector modelSel
mTabModelSelector = modelSelector;
mTabCreatorManager = tabCreatorManager;
mTabsToSave = new ArrayDeque<>();
mTabsToMigrate = new ArrayDeque<>();
mTabsToRestore = new ArrayDeque<>();
mTabIdsToRestore = new HashSet<>();
mObservers = new ObserverList<>();
Expand Down Expand Up @@ -763,9 +782,12 @@ protected void restoreTab(TabRestoreDetails tabToRestore, TabState tabState,
: TabRestoreMethod.CRITICAL_PERSISTED_TAB_DATA;
RecordHistogram.recordEnumeratedHistogram(
"Tabs.TabRestoreMethod", tabRestoreMethod, TabRestoreMethod.NUM_ENTRIES);
mTabCreatorManager.getTabCreator(isIncognito)
.createFrozenTab(tabState, serializedCriticalPersistedTabData, tabToRestore.id,
isIncognito, restoredIndex);
Tab tab = mTabCreatorManager.getTabCreator(isIncognito)
.createFrozenTab(tabState, serializedCriticalPersistedTabData,
tabToRestore.id, isIncognito, restoredIndex);
if (useTabState && isCriticalPersistedTabDataEnabled()) {
mTabsToMigrate.add(tab);
}
} else {
if (UrlUtilities.isNTPUrl(tabToRestore.url) && !setAsActive
&& !tabToRestore.fromMerge) {
Expand All @@ -790,6 +812,9 @@ protected void restoreTab(TabRestoreDetails tabToRestore, TabState tabState,
TabRestoreMethod.FAILED_TO_RESTORE, TabRestoreMethod.NUM_ENTRIES);
return;
}
if (isCriticalPersistedTabDataEnabled()) {
mTabsToMigrate.add(fallbackTab);
}

RecordHistogram.recordEnumeratedHistogram("Tabs.TabRestoreMethod",
TabRestoreMethod.CREATE_NEW_TAB, TabRestoreMethod.NUM_ENTRIES);
Expand Down Expand Up @@ -912,6 +937,8 @@ public void removeTabFromQueues(Tab tab) {
mTabsToSave.remove(tab);
mTabsToRestore.remove(getTabToRestoreById(tab.getId()));

if (isCriticalPersistedTabDataEnabled()) mTabsToMigrate.remove(tab);

if (mTabLoader != null && mTabLoader.mTabToRestore.id == tab.getId()) {
mTabLoader.cancel(false);
mTabLoader = null;
Expand Down Expand Up @@ -1340,6 +1367,7 @@ protected void onPostExecute(Void v) {
if (mStateSaved) {
if (!mTab.isDestroyed()) TabStateAttributes.from(mTab).setIsTabStateDirty(false);
mTab.setIsTabSaveEnabled(isCriticalPersistedTabDataEnabled());
migrateSomeRemainingTabsToCriticalPersistedTabData();
}
mSaveTabTask = null;
saveNextTab();
Expand Down Expand Up @@ -1429,7 +1457,6 @@ private boolean saveTabState(int tabId, boolean encrypted, TabState state) {
TAG, "Out of memory error while attempting to save tab state. Erasing.");
deleteTabState(tabId, encrypted);
}

return false;
}

Expand Down Expand Up @@ -1877,6 +1904,16 @@ public static boolean isStateFile(String fileName) {
ChromePreferenceKeys.APP_LAUNCH_LAST_KNOWN_ACTIVE_TAB_STATE, ActiveTabState.EMPTY);
}

private static int getMigrateToCriticalPersistedTabDataBatchSize() {
if (FeatureList.isInitialized()) {
return ChromeFeatureList.getFieldTrialParamByFeatureAsInt(
ChromeFeatureList.CRITICAL_PERSISTED_TAB_DATA,
MIGRATE_TO_CRITICAL_PERSISTED_TAB_DATA_BATCH_SIZE_PARAM,
MIGRATE_TO_CRITICAL_PERSISTED_TAB_DATA_DEFAULT_BATCH_SIZE);
}
return MIGRATE_TO_CRITICAL_PERSISTED_TAB_DATA_DEFAULT_BATCH_SIZE;
}

@VisibleForTesting
SequencedTaskRunner getTaskRunnerForTests() {
return mSequencedTaskRunner;
Expand Down Expand Up @@ -1907,4 +1944,17 @@ public AsyncTask<TabState> getPrefetchTabStateActiveTabTaskForTesting() {
getPrefetchCriticalPersistedTabDataActiveTabTaskForTesting() {
return mPrefetchCriticalPersistedTabDataActiveTabTask;
}

private void migrateSomeRemainingTabsToCriticalPersistedTabData() {
if (!isCriticalPersistedTabDataEnabled()) return;
int numMigrated = 0;
while (numMigrated < getMigrateToCriticalPersistedTabDataBatchSize()
&& !mTabsToMigrate.isEmpty()) {
Tab tabToMigrate = mTabsToMigrate.pollFirst();
if (tabToMigrate != null && !tabToMigrate.isDestroyed()) {
tabToMigrate.setIsTabSaveEnabled(true);
}
numMigrated++;
}
}
}
125 changes: 125 additions & 0 deletions ...me/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java
View file
Expand Up @@ -33,6 +33,7 @@
import org.chromium.base.test.util.AdvancedMockContext;
import org.chromium.base.test.util.CallbackHelper;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.CriteriaHelper;
import org.chromium.base.test.util.Feature;
import org.chromium.base.test.util.MinAndroidSdkLevel;
import org.chromium.chrome.browser.ActivityUtils;
Expand All @@ -56,7 +57,10 @@
import org.chromium.chrome.browser.tab.TabImpl;
import org.chromium.chrome.browser.tab.TabSelectionType;
import org.chromium.chrome.browser.tab.TabState;
import org.chromium.chrome.browser.tab.TabStateAttributes;
import org.chromium.chrome.browser.tab.TabStateExtractor;
import org.chromium.chrome.browser.tab.state.CriticalPersistedTabData;
import org.chromium.chrome.browser.tab.state.FilePersistedTabDataStorage;
import org.chromium.chrome.browser.tab.state.PersistedTabDataConfiguration;
import org.chromium.chrome.browser.tab.state.SerializedCriticalPersistedTabData;
import org.chromium.chrome.browser.tabmodel.NextTabPolicy.NextTabPolicySupplier;
Expand Down Expand Up @@ -949,6 +953,127 @@ public void testTabRestoreMethodEnumValues() {
Assert.assertEquals(5, TabRestoreMethod.SKIPPED_EMPTY_URL);
}

@Test
@SmallTest
@Feature("TabPersistentStore")
public void testMigrateStaleTabsToCriticalPersistedTabData() throws Exception {
CachedFeatureFlags.setForTesting(ChromeFeatureList.CRITICAL_PERSISTED_TAB_DATA, true);
Pair<TabPersistentStore, Tab[]> res = restoreTabsFromTabStateAndPrepareForSaving();
TabPersistentStore store = res.first;
Tab[] restoredTabs = res.second;

// Save the TabState for 2 Tabs. There are info.numRegularTabs (7) Tabs
// to migrate and we migrate MIGRATE_TO_CRITICAL_PERSISTED_TAB_DATA_DEFAULT_BATCH_SIZE (5)
// Tabs for every TabState save, so we need to save 2 Tabs to migrate all
// Tabs.
addTabsToSaveQueue(store, new Tab[] {restoredTabs[0], restoredTabs[1]});
// Verify all Tabs except the first are migrated. The first Tab has an empty
// URL so it won't be saved (this is by design).
for (int i = 1; i < restoredTabs.length; i++) {
Tab tab = restoredTabs[i];
CriteriaHelper.pollUiThread(
() -> FilePersistedTabDataStorage.exists(tab.getId(), false));
}
}

@Test
@SmallTest
@Feature("TabPersistentStore")
public void testDontMigrateDestroyedTab() throws Exception {
// Variation on testMigrateStaleTabsToCriticalPersistedTabData. See comments in
// testMigrateStaleTabsToCriticalPersistedTabData.
CachedFeatureFlags.setForTesting(ChromeFeatureList.CRITICAL_PERSISTED_TAB_DATA, true);
Pair<TabPersistentStore, Tab[]> res = restoreTabsFromTabStateAndPrepareForSaving();
TabPersistentStore store = res.first;
Tab[] restoredTabs = res.second;

TestThreadUtils.runOnUiThreadBlocking(restoredTabs[2] ::destroy);
addTabsToSaveQueue(store, new Tab[] {restoredTabs[0], restoredTabs[1]});

for (int i = 1; i < restoredTabs.length; i++) {
// restoredTabs[2] shouldn't have been migrated because it was destroyed.
if (i == 2) continue;
Tab tab = restoredTabs[i];
CriteriaHelper.pollUiThread(
() -> FilePersistedTabDataStorage.exists(tab.getId(), false));
}
// Check we didn't migrate restoredTabs[2].
Assert.assertFalse(FilePersistedTabDataStorage.exists(restoredTabs[2].getId(), false));
}

@Test
@SmallTest
@Feature("TabPersistentStore")
public void testDontMigrateClosedTab() throws Exception {
// Variation on testMigrateStaleTabsToCriticalPersistedTabData. See comments in
// testMigrateStaleTabsToCriticalPersistedTabData
CachedFeatureFlags.setForTesting(ChromeFeatureList.CRITICAL_PERSISTED_TAB_DATA, true);
Pair<TabPersistentStore, Tab[]> res = restoreTabsFromTabStateAndPrepareForSaving();
TabPersistentStore store = res.first;
Tab[] restoredTabs = res.second;

addTabsToSaveQueue(store, new Tab[] {restoredTabs[0], restoredTabs[1]});
store.removeTabFromQueues(restoredTabs[2]);

for (int i = 1; i < restoredTabs.length; i++) {
// restoredTabs[2] should not have been migrated because it was removed from the
// save queue.
if (i == 2) continue;
Tab tab = restoredTabs[i];
CriteriaHelper.pollUiThread(
() -> FilePersistedTabDataStorage.exists(tab.getId(), false));
}
// Check we didn't migrate Tab 2.
Assert.assertFalse(FilePersistedTabDataStorage.exists(restoredTabs[2].getId(), false));
}

private Pair<TabPersistentStore, Tab[]> restoreTabsFromTabStateAndPrepareForSaving()
throws Exception {
TabModelMetaDataInfo info = TestTabModelDirectory.TAB_MODEL_METADATA_V4;
int numExpectedTabs = info.contents.length;
mMockDirectory.writeTabModelFiles(info, true);
MockTabModelSelector mockSelector =
TestThreadUtils.runOnUiThreadBlocking(() -> new MockTabModelSelector(0, 0, null));
MockTabCreatorManager mockManager = new MockTabCreatorManager(mockSelector);
MockTabCreator regularCreator = mockManager.getTabCreator(false);
MockTabPersistentStoreObserver mockObserver = new MockTabPersistentStoreObserver();
TabPersistencePolicy persistencePolicy = createTabPersistencePolicy(0, false, true);
final TabPersistentStore store =
buildTabPersistentStore(persistencePolicy, mockSelector, mockManager);
TestThreadUtils.runOnUiThreadBlocking(() -> {
store.addObserver(mockObserver);
store.loadState(false /* ignoreIncognitoFiles */);
});
mockObserver.initializedCallback.waitForCallback(0, 1);
mockObserver.detailsReadCallback.waitForCallback(0, numExpectedTabs);
TestThreadUtils.runOnUiThreadBlocking(() -> { store.restoreTabs(true); });
regularCreator.callback.waitForCallback(0, 1);
mockObserver.stateLoadedCallback.waitForCallback(0, 1);
Tab[] restoredTabs = new Tab[info.numRegularTabs];
for (int i = 0; i < info.numRegularTabs; i++) {
restoredTabs[i] = mockSelector.getModel(false).getTabAt(i);
}
TestThreadUtils.runOnUiThreadBlocking(() -> {
for (Tab tab : restoredTabs) {
((TabImpl) tab).registerTabSaving();
}
});
return Pair.create(store, restoredTabs);
}

private void addTabsToSaveQueue(TabPersistentStore store, Tab[] tabsToSave) {
TestThreadUtils.runOnUiThreadBlocking(() -> {
for (int i = 0; i < tabsToSave.length; i++) {
// Tabs are uninitialized so TabState won't save unless we override here.
// It doesn't matter what TabState is saved for the tests which use this function
// only that it is saved. So an arbitrary TabState is used.
TabStateExtractor.setTabStateForTesting(tabsToSave[i].getId(), new TabState());
TabStateAttributes.from(tabsToSave[i]).setIsTabStateDirty(true);
store.addTabToSaveQueue(tabsToSave[i]);
}
});
}

private TestTabModelSelector createAndRestoreRealTabModelImpls(TabModelMetaDataInfo info)
throws Exception {
return createAndRestoreRealTabModelImpls(info, true, true);
Expand Down
16 changes: 16 additions & 0 deletions ...owser/tab/java/src/org/chromium/chrome/browser/tab/state/FilePersistedTabDataStorage.java
View file
Expand Up @@ -588,4 +588,20 @@ public static Boolean isIncognito(int tabId) {
return null;
}
}

/**
* @param tabId {@link Tab} identifier
* @param isIncognito if the {@link Tab} is incognito
* @return true if a file exists for this {@link Tab}
*/
@VisibleForTesting
public static boolean exists(int tabId, boolean isIncognito) {
try (StrictModeContext ignored = StrictModeContext.allowDiskReads()) {
String dataId =
PersistedTabDataConfiguration.get(CriticalPersistedTabData.class, isIncognito)
.getId();
File file = FilePersistedTabDataStorage.getFile(tabId, dataId);
return file != null && file.exists();
}
}
}

0 comments on commit 66ba9a4

Please sign in to comment.