Delete all URL data when the given timerange is valid

Better alignment with the requirements (delete everything when a user
deletes history in a time range) could eliminate the crashes seen in the
linked bug.

Bug: 1133268
Change-Id: I35690a4c70a9447949467731e67f163fa6acbae2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2439636
Commit-Queue: Brandon Wylie <wylieb@chromium.org>
Reviewed-by: Michael Bai <michaelbai@chromium.org>
Reviewed-by: Ted Choc <tedchoc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#813862}

chrome/android/BUILD.gn

@@ -845,6 +845,7 @@ junit_binary("chrome_junit_tests") {
     "//components/browser_ui/site_settings/android:java",
     "//components/browser_ui/util/android:java",
     "//components/browser_ui/widget/android:java",
+    "//components/content_capture/android:java",
     "//components/content_settings/android:content_settings_enums_java",
     "//components/dom_distiller/core/android:dom_distiller_core_java",
     "//components/embedder_support/android:browser_context_java",

chrome/android/chrome_junit_test_java_sources.gni

@@ -48,6 +48,7 @@ chrome_junit_test_java_sources = [
   "junit/src/org/chromium/chrome/browser/compositor/layouts/StaticLayoutUnitTest.java",
   "junit/src/org/chromium/chrome/browser/compositor/overlays/strip/StripLayoutHelperTest.java",
   "junit/src/org/chromium/chrome/browser/compositor/overlays/toolbar/TopToolbarOverlayMediatorTest.java",
+  "junit/src/org/chromium/chrome/browser/content_capture/ContentCaptureHistoryDeletionObserverTest.java",
   "junit/src/org/chromium/chrome/browser/contextmenu/RevampedContextMenuCoordinatorTest.java",
   "junit/src/org/chromium/chrome/browser/contextmenu/RevampedContextMenuHeaderMediatorTest.java",
   "junit/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchContextTest.java",

chrome/android/java/src/org/chromium/chrome/browser/content_capture/ContentCaptureHistoryDeletionObserver.java

@@ -4,27 +4,37 @@
 
 package org.chromium.chrome.browser.content_capture;
 
+import org.chromium.base.supplier.Supplier;
 import org.chromium.chrome.browser.history.HistoryDeletionBridge;
 import org.chromium.chrome.browser.history.HistoryDeletionInfo;
 import org.chromium.components.content_capture.ContentCaptureController;
 
 /** History deletion observer that calls ContentCapture methods. */
 public class ContentCaptureHistoryDeletionObserver implements HistoryDeletionBridge.Observer {
+    Supplier<ContentCaptureController> mContentCaptureControllerSupplier;
+
+    public ContentCaptureHistoryDeletionObserver(
+            Supplier<ContentCaptureController> contentCaptureControllerSupplier) {
+        mContentCaptureControllerSupplier = contentCaptureControllerSupplier;
+    }
+
     /** Observer method when a bit of history is deleted. */
     @Override
     public void onURLsDeleted(HistoryDeletionInfo historyDeletionInfo) {
-        ContentCaptureController contentCaptureController = ContentCaptureController.getInstance();
+        ContentCaptureController contentCaptureController = mContentCaptureControllerSupplier.get();
         if (contentCaptureController == null) return;
 
-        if (historyDeletionInfo.isTimeRangeForAllTime()
-                || (historyDeletionInfo.isTimeRangeValid()
-                        && historyDeletionInfo.getTimeRangeBegin()
-                                != historyDeletionInfo.getTimeRangeEnd())) {
+        // A timerange deletion is equivalent to deleting "all" history.
+        if (historyDeletionInfo.isTimeRangeForAllTime() || historyDeletionInfo.isTimeRangeValid()) {
             contentCaptureController.clearAllContentCaptureData();
         } else {
             String[] deletedURLs = historyDeletionInfo.getDeletedURLs();
             if (deletedURLs.length > 0) {
-                contentCaptureController.clearContentCaptureDataForURLs(deletedURLs);
+                try {
+                    contentCaptureController.clearContentCaptureDataForURLs(deletedURLs);
+                } catch (RuntimeException e) {
+                    throw new RuntimeException("Deleted URLs length: " + deletedURLs.length, e);
+                }
             }
         }
     }

chrome/android/java/src/org/chromium/chrome/browser/history/HistoryDeletionInfo.java

@@ -45,26 +45,10 @@ public class HistoryDeletionInfo {
         return HistoryDeletionInfoJni.get().isTimeRangeForAllTime(mHistoryDeletionInfoPtr);
     }
 
-    /**
-     * @return The beginning of the time range if the time range is valid.
-     */
-    public long getTimeRangeBegin() {
-        return HistoryDeletionInfoJni.get().getTimeRangeBegin(mHistoryDeletionInfoPtr);
-    }
-
-    /**
-     * @return The end of the time range if the time range is valid.
-     */
-    public long getTimeRangeEnd() {
-        return HistoryDeletionInfoJni.get().getTimeRangeBegin(mHistoryDeletionInfoPtr);
-    }
-
     @NativeMethods
     interface Natives {
         String[] getDeletedURLs(long historyDeletionInfoPtr);
         boolean isTimeRangeValid(long historyDeletionInfoPtr);
         boolean isTimeRangeForAllTime(long historyDeletionInfoPtr);
-        long getTimeRangeBegin(long historyDeletionInfoPtr);
-        long getTimeRangeEnd(long historyDeletionInfoPtr);
     }
 }

chrome/android/java/src/org/chromium/chrome/browser/init/ProcessInitializationHandler.java

@@ -83,6 +83,7 @@ import org.chromium.components.browser_ui.photo_picker.DecoderServiceHost;
 import org.chromium.components.browser_ui.photo_picker.PhotoPickerDialog;
 import org.chromium.components.browser_ui.share.ShareImageFileUtils;
 import org.chromium.components.browser_ui.util.ConversionUtils;
+import org.chromium.components.content_capture.ContentCaptureController;
 import org.chromium.components.minidump_uploader.CrashFileManager;
 import org.chromium.components.signin.AccountManagerFacadeImpl;
 import org.chromium.components.signin.AccountManagerFacadeProvider;
@@ -259,8 +260,8 @@ public class ProcessInitializationHandler {
                 });
 
         SearchWidgetProvider.initialize();
-        HistoryDeletionBridge.getInstance().addObserver(
-                new ContentCaptureHistoryDeletionObserver());
+        HistoryDeletionBridge.getInstance().addObserver(new ContentCaptureHistoryDeletionObserver(
+                () -> ContentCaptureController.getInstance()));
     }
 
     /**

chrome/android/junit/src/org/chromium/chrome/browser/content_capture/ContentCaptureHistoryDeletionObserverTest.java

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.chrome.browser.content_capture;
+
+import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
+import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.verify;
+
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.robolectric.annotation.Config;
+
+import org.chromium.base.test.BaseRobolectricTestRunner;
+import org.chromium.chrome.browser.history.HistoryDeletionInfo;
+import org.chromium.components.content_capture.ContentCaptureController;
+
+/**
+ * Unit tests for the ContentCaptureHistoryDeletionObserver.
+ */
+@RunWith(BaseRobolectricTestRunner.class)
+@Config(manifest = Config.NONE)
+public class ContentCaptureHistoryDeletionObserverTest {
+    @Mock
+    ContentCaptureController mContentCaptureController;
+    @Mock
+    HistoryDeletionInfo mHistoryDeletionInfo;
+
+    ContentCaptureHistoryDeletionObserver mContentCaptureHistoryDeletionObserver;
+
+    @Before
+    public void setUp() {
+        MockitoAnnotations.initMocks(this);
+
+        mContentCaptureHistoryDeletionObserver =
+                new ContentCaptureHistoryDeletionObserver(() -> mContentCaptureController);
+    }
+
+    @Test
+    public void clearAllData_FromSpecificTimeRange() {
+        doReturn(false).when(mHistoryDeletionInfo).isTimeRangeForAllTime();
+        doReturn(true).when(mHistoryDeletionInfo).isTimeRangeValid();
+
+        mContentCaptureHistoryDeletionObserver.onURLsDeleted(mHistoryDeletionInfo);
+        verify(mContentCaptureController).clearAllContentCaptureData();
+    }
+
+    @Test
+    public void clearAllData_ForAllTime() {
+        doReturn(true).when(mHistoryDeletionInfo).isTimeRangeForAllTime();
+        doReturn(false).when(mHistoryDeletionInfo).isTimeRangeValid();
+
+        mContentCaptureHistoryDeletionObserver.onURLsDeleted(mHistoryDeletionInfo);
+        verify(mContentCaptureController).clearAllContentCaptureData();
+    }
+
+    @Test
+    public void clearAllData_ForSpecficURLs() {
+        doReturn(false).when(mHistoryDeletionInfo).isTimeRangeForAllTime();
+        doReturn(false).when(mHistoryDeletionInfo).isTimeRangeValid();
+        String[] urls = new String[] {"one", "two", "three"};
+        doReturn(urls).when(mHistoryDeletionInfo).getDeletedURLs();
+
+        mContentCaptureHistoryDeletionObserver.onURLsDeleted(mHistoryDeletionInfo);
+        verify(mContentCaptureController).clearContentCaptureDataForURLs(urls);
+    }
+
+    @Test
+    public void clearAllData_ThrowsRuntimeException() {
+        doThrow(RuntimeException.class)
+                .when(mContentCaptureController)
+                .clearContentCaptureDataForURLs(any());
+        doReturn(false).when(mHistoryDeletionInfo).isTimeRangeForAllTime();
+        doReturn(false).when(mHistoryDeletionInfo).isTimeRangeValid();
+        String[] urls = new String[] {"one", "two", "three"};
+        doReturn(urls).when(mHistoryDeletionInfo).getDeletedURLs();
+
+        try {
+            mContentCaptureHistoryDeletionObserver.onURLsDeleted(mHistoryDeletionInfo);
+            fail("Expected exception to be thrown.");
+        } catch (RuntimeException e) {
+            assertTrue(e.toString().contains("Deleted URLs length: " + urls.length));
+        }
+    }
+}
\ No newline at end of file

chrome/browser/android/history/history_deletion_info.cc

@@ -47,21 +47,6 @@ jboolean JNI_HistoryDeletionInfo_IsTimeRangeForAllTime(
   return deletion_info->time_range().IsAllTime();
 }
 
-jlong JNI_HistoryDeletionInfo_GetTimeRangeBegin(
-    JNIEnv* env,
-    jlong history_deletion_info_ptr) {
-  history::DeletionInfo* deletion_info =
-      ToDeletionInfo(history_deletion_info_ptr);
-  return deletion_info->time_range().begin().ToJavaTime();
-}
-
-jlong JNI_HistoryDeletionInfo_GetTimeRangeEnd(JNIEnv* env,
-                                              jlong history_deletion_info_ptr) {
-  history::DeletionInfo* deletion_info =
-      ToDeletionInfo(history_deletion_info_ptr);
-  return deletion_info->time_range().end().ToJavaTime();
-}
-
 ScopedJavaLocalRef<jobject> CreateHistoryDeletionInfo(
     JNIEnv* env,
     const history::DeletionInfo* deletion_info) {