diff --git a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java b/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java index 0975be9a0dc449..dce21745dd74b0 100644 --- a/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java +++ b/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/TabPersistentStore.java @@ -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; @@ -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}) @@ -280,6 +284,20 @@ public TabModelMetadata(int selectedIndex) { private final ObserverList mObservers; private final Deque 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 mTabsToMigrate; private final Deque mTabsToRestore; private final Set mTabIdsToRestore; @@ -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<>(); @@ -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) { @@ -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); @@ -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; @@ -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(); @@ -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; } @@ -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; @@ -1907,4 +1944,17 @@ public AsyncTask 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++; + } + } } diff --git a/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java b/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java index 0b647262f02f10..f969dd28a4d019 100644 --- a/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java +++ b/chrome/android/javatests/src/org/chromium/chrome/browser/tabmodel/TabPersistentStoreTest.java @@ -34,6 +34,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; @@ -58,7 +59,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; @@ -958,6 +962,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 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 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 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 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); diff --git a/chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/state/FilePersistedTabDataStorage.java b/chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/state/FilePersistedTabDataStorage.java index fc050438e53aad..5a0f4fed79eda1 100644 --- a/chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/state/FilePersistedTabDataStorage.java +++ b/chrome/browser/tab/java/src/org/chromium/chrome/browser/tab/state/FilePersistedTabDataStorage.java @@ -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(); + } + } }