Delete navigation entries from frozen state

When Chrome is restarted, tabs are initially in a frozen state and have
no WebContents. History deletions need to clean up this frozen state
to correctly remove navigation entries.

Add integration tests for deletion of navigation entries from regular
and frozen tabs.

Bug: 407074
Change-Id: Id69ffcf05622f8e02eace0036e6787cea0b4dfab
Reviewed-on: https://chromium-review.googlesource.com/949323
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Reviewed-by: Bernhard Bauer <bauerb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541768}

chrome/android/BUILD.gn

@@ -596,6 +596,7 @@ android_library("chrome_test_java") {
   data = [
     "//chrome/test/data/android/",
     "//chrome/test/data/banners/",
+    "//chrome/test/data/browsing_data/",
     "//chrome/test/data/encoding_tests/auto_detect/Big5_with_no_encoding_specified.html",
     "//chrome/test/data/geolocation/",
     "//chrome/test/data/google/",

chrome/android/java/src/org/chromium/chrome/browser/ChromeFeatureList.java

@@ -242,6 +242,7 @@ public abstract class ChromeFeatureList {
     public static final String PROGRESS_BAR_THROTTLE = "ProgressBarThrottle";
     public static final String PWA_PERSISTENT_NOTIFICATION = "PwaPersistentNotification";
     public static final String READER_MODE_IN_CCT = "ReaderModeInCCT";
+    public static final String REMOVE_NAVIGATION_HISTORY = "RemoveNavigationHistory";
     public static final String SERVICE_WORKER_PAYMENT_APPS = "ServiceWorkerPaymentApps";
     public static final String SITE_NOTIFICATION_CHANNELS = "SiteNotificationChannels";
     public static final String SOLE_INTEGRATION = "SoleIntegration";

chrome/android/java/src/org/chromium/chrome/browser/TabState.java

@@ -7,6 +7,7 @@ package org.chromium.chrome.browser;
 import android.graphics.Color;
 import android.os.Build;
 import android.os.Handler;
+import android.support.annotation.Nullable;
 import android.util.Log;
 import android.util.Pair;
 
@@ -92,6 +93,21 @@ public class TabState {
             return nativeRestoreContentsFromByteBuffer(mBuffer, mVersion, isHidden);
         }
 
+        /**
+         * Deletes navigation entries from this buffer matching predicate.
+         * @param predicate Handle for a deletion predicate interpreted by native code.
+                            Only valid during this call frame.
+         * @return WebContentsState A new state or null if nothing changed.
+         */
+        @Nullable
+        public WebContentsState deleteNavigationEntries(long predicate) {
+            ByteBuffer newBuffer = nativeDeleteNavigationEntries(mBuffer, mVersion, predicate);
+            if (newBuffer == null) return null;
+            WebContentsState newState = new TabState.WebContentsStateNative(newBuffer);
+            newState.setVersion(TabState.CONTENTS_STATE_CURRENT_VERSION);
+            return newState;
+        }
+
         /**
          * Creates a WebContents for the ContentsState and adds it as an historical tab, then
          * deletes the WebContents.
@@ -470,6 +486,9 @@ public class TabState {
 
     private static native ByteBuffer nativeGetContentsStateAsByteBuffer(Tab tab);
 
+    private static native ByteBuffer nativeDeleteNavigationEntries(
+            ByteBuffer state, int saveStateVersion, long predicate);
+
     private static native ByteBuffer nativeCreateSingleNavigationStateAsByteBuffer(
             String url, String referrerUrl, int referrerPolicy, boolean isIncognito);
 

chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java

@@ -2737,6 +2737,21 @@ public class Tab
         }
     }
 
+    /**
+     * Delete navigation entries from frozen state matching the predicate.
+     * @param predicate Handle for a deletion predicate interpreted by native code.
+     *                  Only valid during this call frame.
+     */
+    @CalledByNative
+    private void deleteNavigationEntriesFromFrozenState(long predicate) {
+        if (mFrozenContentsState == null) return;
+        WebContentsState newState = mFrozenContentsState.deleteNavigationEntries(predicate);
+        if (newState != null) {
+            mFrozenContentsState = newState;
+            notifyNavigationEntriesDeleted();
+        }
+    }
+
     /**
      * @return The reason the Tab was launched.
      */

chrome/android/javatests/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferencesTest.java

@@ -4,6 +4,12 @@
 
 package org.chromium.chrome.browser.preferences.privacy;
 
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;
+
 import android.content.Intent;
 import android.preference.CheckBoxPreference;
 import android.preference.Preference;
@@ -15,11 +21,13 @@ import android.support.v7.app.AlertDialog;
 import android.widget.Button;
 import android.widget.ListView;
 
+import org.hamcrest.Matchers;
 import org.junit.After;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.TestRule;
 import org.junit.runner.RunWith;
 
 import org.chromium.base.ThreadUtils;
@@ -31,23 +39,31 @@ import org.chromium.chrome.browser.ChromeActivity;
 import org.chromium.chrome.browser.ChromeFeatureList;
 import org.chromium.chrome.browser.ChromeSwitches;
 import org.chromium.chrome.browser.ShortcutHelper;
+import org.chromium.chrome.browser.TabState;
 import org.chromium.chrome.browser.browsing_data.ClearBrowsingDataTab;
 import org.chromium.chrome.browser.preferences.PrefServiceBridge;
 import org.chromium.chrome.browser.preferences.Preferences;
 import org.chromium.chrome.browser.preferences.privacy.ClearBrowsingDataPreferences.DialogOption;
 import org.chromium.chrome.browser.preferences.website.ContentSetting;
 import org.chromium.chrome.browser.preferences.website.NotificationInfo;
+import org.chromium.chrome.browser.tab.Tab;
 import org.chromium.chrome.browser.webapps.TestFetchStorageCallback;
 import org.chromium.chrome.browser.webapps.WebappDataStorage;
 import org.chromium.chrome.browser.webapps.WebappRegistry;
 import org.chromium.chrome.test.ChromeActivityTestRule;
 import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
+import org.chromium.chrome.test.util.browser.Features;
+import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
 import org.chromium.chrome.test.util.browser.signin.SigninTestUtil;
 import org.chromium.content.browser.test.util.Criteria;
 import org.chromium.content.browser.test.util.CriteriaHelper;
+import org.chromium.content_public.browser.NavigationController;
+import org.chromium.content_public.browser.NavigationEntry;
+import org.chromium.content_public.browser.WebContents;
 import org.chromium.net.test.EmbeddedTestServer;
 
 import java.net.URL;
+import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
@@ -62,6 +78,8 @@ public class ClearBrowsingDataPreferencesTest {
     @Rule
     public ChromeActivityTestRule<ChromeActivity> mActivityTestRule =
             new ChromeActivityTestRule<>(ChromeActivity.class);
+    @Rule
+    public TestRule mProcessor = new Features.InstrumentationProcessor();
 
     private EmbeddedTestServer mTestServer;
 
@@ -527,6 +545,93 @@ public class ClearBrowsingDataPreferencesTest {
                 "true", mActivityTestRule.runJavaScriptCodeInCurrentTab("hasAllStorage()"));
     }
 
+    /**
+     * Tests navigation entries are removed by history deletions.
+     */
+    @Test
+    @EnableFeatures(ChromeFeatureList.REMOVE_NAVIGATION_HISTORY)
+    @MediumTest
+    public void testNavigationDeletion() throws Exception {
+        final String url1 = mTestServer.getURL("/chrome/test/data/browsing_data/a.html");
+        final String url2 = mTestServer.getURL("/chrome/test/data/browsing_data/b.html");
+
+        // Navigate to url1 and url2.
+        Tab tab = mActivityTestRule.loadUrlInNewTab(url1);
+        mActivityTestRule.loadUrl(url2);
+        NavigationController controller = tab.getWebContents().getNavigationController();
+        assertTrue(tab.canGoBack());
+        assertEquals(1, controller.getLastCommittedEntryIndex());
+        assertThat(getUrls(controller), Matchers.contains(url1, url2));
+
+        // Clear history.
+        setDataTypesToClear(Arrays.asList(DialogOption.CLEAR_HISTORY));
+        ClearBrowsingDataPreferences preferences =
+                (ClearBrowsingDataPreferences) startPreferences().getFragmentForTest();
+        ThreadUtils.runOnUiThreadBlocking(() -> clickClearButton(preferences));
+        waitForProgressToComplete(preferences);
+
+        // Check navigation entries.
+        assertFalse(tab.canGoBack());
+        assertEquals(0, controller.getLastCommittedEntryIndex());
+        assertThat(getUrls(controller), Matchers.contains(url2));
+    }
+
+    /**
+     * Tests navigation entries from frozen state are removed by history deletions.
+     */
+    @Test
+    @MediumTest
+    @EnableFeatures(ChromeFeatureList.REMOVE_NAVIGATION_HISTORY)
+    public void testFrozenNavigationDeletion() throws Exception {
+        final String url1 = mTestServer.getURL("/chrome/test/data/browsing_data/a.html");
+        final String url2 = mTestServer.getURL("/chrome/test/data/browsing_data/b.html");
+
+        // Navigate to url1 and url2, close and recreate as frozen tab.
+        Tab tab = mActivityTestRule.loadUrlInNewTab(url1);
+        mActivityTestRule.loadUrl(url2);
+        Tab[] frozen = new Tab[1];
+        WebContents[] restored = new WebContents[1];
+        ThreadUtils.runOnUiThreadBlocking(() -> {
+            TabState state = tab.getState();
+            mActivityTestRule.getActivity().getCurrentTabModel().closeTab(tab);
+            frozen[0] = mActivityTestRule.getActivity().getCurrentTabCreator().createFrozenTab(
+                    state, tab.getId(), 1);
+            restored[0] = frozen[0].getState().contentsState.restoreContentsFromByteBuffer(false);
+        });
+
+        // Check content of frozen state.
+        NavigationController controller = restored[0].getNavigationController();
+        assertEquals(1, controller.getLastCommittedEntryIndex());
+        assertThat(getUrls(controller), Matchers.contains(url1, url2));
+        assertNull(frozen[0].getWebContents());
+
+        // Delete history.
+        setDataTypesToClear(Arrays.asList(DialogOption.CLEAR_HISTORY));
+        ClearBrowsingDataPreferences preferences =
+                (ClearBrowsingDataPreferences) startPreferences().getFragmentForTest();
+        ThreadUtils.runOnUiThreadBlocking(() -> clickClearButton(preferences));
+        waitForProgressToComplete(preferences);
+
+        // Check that frozen state was cleaned up.
+        ThreadUtils.runOnUiThreadBlocking(() -> {
+            restored[0] = frozen[0].getState().contentsState.restoreContentsFromByteBuffer(false);
+        });
+        controller = restored[0].getNavigationController();
+        assertEquals(0, controller.getLastCommittedEntryIndex());
+        assertThat(getUrls(controller), Matchers.contains(url2));
+        assertNull(frozen[0].getWebContents());
+    }
+
+    private List<String> getUrls(NavigationController controller) {
+        List<String> urls = new ArrayList<>();
+        int i = 0;
+        while (true) {
+            NavigationEntry entry = controller.getEntryAtIndex(i++);
+            if (entry == null) return urls;
+            urls.add(entry.getUrl());
+        }
+    }
+
     private void setDataTypesToClear(final List<DialogOption> typesToClear) {
         ThreadUtils.runOnUiThreadBlocking(() -> {
             for (DialogOption option : DialogOption.values()) {

chrome/browser/android/tab_android.cc

@@ -238,6 +238,13 @@ sync_sessions::SyncedTabDelegate* TabAndroid::GetSyncedTabDelegate() const {
   return synced_tab_delegate_.get();
 }
 
+void TabAndroid::DeleteFrozenNavigationEntries(
+    const WebContentsState::DeletionPredicate& predicate) {
+  JNIEnv* env = base::android::AttachCurrentThread();
+  Java_Tab_deleteNavigationEntriesFromFrozenState(
+      env, weak_java_tab_.get(env), reinterpret_cast<intptr_t>(&predicate));
+}
+
 void TabAndroid::SetWindowSessionID(SessionID::id_type window_id) {
   session_window_id_.set_id(window_id);
 

chrome/browser/android/tab_android.h

@@ -16,6 +16,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
+#include "chrome/browser/android/tab_state.h"
 #include "chrome/browser/sync/glue/synced_tab_delegate_android.h"
 #include "chrome/browser/ui/tab_contents/core_tab_helper_delegate.h"
 #include "components/favicon/core/favicon_driver_observer.h"
@@ -114,6 +115,10 @@ class TabAndroid : public CoreTabHelperDelegate,
   Profile* GetProfile() const;
   sync_sessions::SyncedTabDelegate* GetSyncedTabDelegate() const;
 
+  // Delete navigation entries matching predicate from frozen state.
+  void DeleteFrozenNavigationEntries(
+      const WebContentsState::DeletionPredicate& predicate);
+
   void SetWindowSessionID(SessionID::id_type window_id);
   void SetSyncId(int sync_id);
 

chrome/browser/android/tab_state.h

@@ -9,9 +9,8 @@
 
 #include "base/android/scoped_java_ref.h"
 
-namespace content {
-class NavigationEntry;
-class WebContents;
+namespace sessions {
+class SerializedNavigationEntry;
 }
 
 class TabAndroid;
@@ -19,18 +18,20 @@ class TabAndroid;
 // Stores state for a WebContents, including its navigation history.
 class WebContentsState {
  public:
+  using DeletionPredicate = base::RepeatingCallback<bool(
+      const sessions::SerializedNavigationEntry& entry)>;
+
   static base::android::ScopedJavaLocalRef<jobject>
       GetContentsStateAsByteBuffer(JNIEnv* env, TabAndroid* tab);
 
-  // Common implementation for GetContentsStateAsByteBuffer() and
-  // CreateContentsStateAsByteBuffer(). Does not assume ownership of the
-  // navigations.
+  // Returns a new buffer without the navigations matching |predicate|.
+  // Returns null if no deletions happened.
   static base::android::ScopedJavaLocalRef<jobject>
-      WriteNavigationsAsByteBuffer(
-          JNIEnv* env,
-          bool is_off_the_record,
-          const std::vector<content::NavigationEntry*>& navigations,
-          int current_entry);
+  DeleteNavigationEntriesFromByteBuffer(JNIEnv* env,
+                                        void* data,
+                                        int size,
+                                        int saved_state_version,
+                                        const DeletionPredicate& predicate);
 
   // Extracts display title from serialized tab data on restore
   static base::android::ScopedJavaLocalRef<jstring>
@@ -42,13 +43,6 @@ class WebContentsState {
       GetVirtualUrlFromByteBuffer(JNIEnv* env, void* data,
                                   int size, int saved_state_version);
 
-  // Restores a WebContents from the passed in state.
-  static content::WebContents* RestoreContentsFromByteBuffer(
-      void* data,
-      int size,
-      int saved_state_version,
-      bool initially_hidden);
-
   // Restores a WebContents from the passed in state.
   static base::android::ScopedJavaLocalRef<jobject>
   RestoreContentsFromByteBuffer(JNIEnv* env,

chrome/browser/browsing_data/navigation_entry_remover.cc

@@ -60,33 +60,20 @@ bool UrlMatcherForSession(const base::flat_set<GURL>& urls,
 
 base::flat_set<GURL> CreateUrlSet(const history::URLRows& deleted_rows) {
   std::vector<GURL> urls;
-  for (const history::URLRow& row : deleted_rows) {
+  for (const history::URLRow& row : deleted_rows)
     urls.push_back(row.url());
-  }
   return base::flat_set<GURL>(std::move(urls));
 }
 
-// Desktop is using |TabStripModel|, Android |TabModel|. They don't have a
-// common base class but both have a |GetWebContentsAt()| method.
-// TODO(dullweber): Add a common base class?
-template <typename TabList>
 void DeleteNavigationEntries(
-    TabList* tab_list,
-    int tab_count,
+    content::WebContents* web_contents,
     const content::NavigationController::DeletionPredicate& predicate) {
-  for (int i = 0; i < tab_count; i++) {
-    content::WebContents* web_contents = tab_list->GetWebContentsAt(i);
-    // TODO(dullweber): Non-loaded tabs on Android don't have a WebContents.
-    // We need to cleanup their TabState instead.
-    if (!web_contents)
-      continue;
-    content::NavigationController* controller = &web_contents->GetController();
-    controller->DiscardNonCommittedEntries();
-    // We discarded pending and transient entries but there could still be
-    // no last_committed_entry, which would prevent deletion.
-    if (controller->CanPruneAllButLastCommitted())
-      controller->DeleteNavigationEntries(predicate);
-  }
+  content::NavigationController* controller = &web_contents->GetController();
+  controller->DiscardNonCommittedEntries();
+  // We discarded pending and transient entries but there could still be
+  // no last_committed_entry, which would prevent deletion.
+  if (controller->CanPruneAllButLastCommitted())
+    controller->DeleteNavigationEntries(predicate);
 }
 
 void DeleteTabNavigationEntries(Profile* profile,
@@ -99,17 +86,30 @@ void DeleteTabNavigationEntries(Profile* profile,
           : base::BindRepeating(&UrlMatcher, base::ConstRef(url_set));
 
 #if defined(OS_ANDROID)
+  auto session_predicate =
+      time_range.IsValid()
+          ? base::BindRepeating(&TimeRangeMatcherForSession, time_range.begin(),
+                                time_range.end())
+          : base::BindRepeating(&UrlMatcherForSession, base::ConstRef(url_set));
+
   for (auto it = TabModelList::begin(); it != TabModelList::end(); ++it) {
     TabModel* tab_model = *it;
     if (tab_model->GetProfile() == profile) {
-      DeleteNavigationEntries(tab_model, tab_model->GetTabCount(), predicate);
+      for (int i = 0; i < tab_model->GetTabCount(); i++) {
+        TabAndroid* tab = tab_model->GetTabAt(i);
+        tab->DeleteFrozenNavigationEntries(session_predicate);
+        content::WebContents* web_contents = tab->web_contents();
+        if (web_contents)
+          DeleteNavigationEntries(web_contents, predicate);
+      }
     }
   }
 #else
   for (Browser* browser : *BrowserList::GetInstance()) {
     TabStripModel* tab_strip = browser->tab_strip_model();
     if (browser->profile() == profile) {
-      DeleteNavigationEntries(tab_strip, tab_strip->count(), predicate);
+      for (int i = 0; i < tab_strip->count(); i++)
+        DeleteNavigationEntries(tab_strip->GetWebContentsAt(i), predicate);
     }
   }
 #endif
@@ -160,18 +160,19 @@ void DeleteTabRestoreEntries(Profile* profile,
                              const base::flat_set<GURL>& url_set) {
   sessions::TabRestoreService* tab_service =
       TabRestoreServiceFactory::GetForProfile(profile);
-  if (tab_service) {
-    auto predicate =
-        time_range.IsValid()
-            ? base::BindRepeating(&TimeRangeMatcherForSession,
-                                  time_range.begin(), time_range.end())
-            : base::BindRepeating(&UrlMatcherForSession, url_set);
-    if (tab_service->IsLoaded()) {
-      PerformTabRestoreDeletion(tab_service, predicate);
-    } else {
-      // The helper deletes itself when the tab entry deletion is finished.
-      new TabRestoreDeletionHelper(tab_service, predicate);
-    }
+  if (!tab_service)
+    return;
+
+  auto predicate =
+      time_range.IsValid()
+          ? base::BindRepeating(&TimeRangeMatcherForSession, time_range.begin(),
+                                time_range.end())
+          : base::BindRepeating(&UrlMatcherForSession, url_set);
+  if (tab_service->IsLoaded()) {
+    PerformTabRestoreDeletion(tab_service, predicate);
+  } else {
+    // The helper deletes itself when the tab entry deletion is finished.
+    new TabRestoreDeletionHelper(tab_service, predicate);
   }
 }