[Journeys] Fix crash when hiding cluster that held a deleted visit

The list of associated visits/related searches model items is never
updated when deleting, making it stale. This triggers an index out of
bounds. The most natural way to fix this is to maintain this list
directly as part of the visit metadata.

Bug: 1351392
Change-Id: I090d66e26658f509bbabca13bc76c44794fb8bf9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3821182
Reviewed-by: Brandon Wylie <wylieb@chromium.org>
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1033744}

chrome/android/junit/src/org/chromium/chrome/browser/history_clusters/HistoryClustersMediatorTest.java

@@ -706,6 +706,21 @@ public void testOpenInGroup() {
                         eq(TabLaunchType.FROM_CHROME_UI), eq(mTab2));
     }
 
+    @Test
+    public void testHideAfterDelete() {
+        Promise<HistoryClustersResult> promise = new Promise<>();
+        doReturn(promise).when(mBridge).queryClusters("query");
+
+        mMediator.setQueryState(QueryState.forQuery("query", ""));
+        mMediator.startQuery("query");
+        fulfillPromise(promise, mHistoryClustersResultWithQuery);
+
+        mMediator.deleteVisits(Arrays.asList(mVisit1));
+        assertEquals(ItemType.CLUSTER, mModelList.get(0).type);
+        PropertyModel clusterModel = mModelList.get(0).model;
+        clusterModel.get(HistoryClustersItemProperties.CLICK_HANDLER).onClick(null);
+    }
+
     private <T> void fulfillPromise(Promise<T> promise, T result) {
         promise.fulfill(result);
         ShadowLooper.idleMainLooper();

chrome/browser/history_clusters/java/src/org/chromium/chrome/browser/history_clusters/HistoryClustersMediator.java

@@ -55,7 +55,6 @@
 import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
-import java.util.concurrent.atomic.AtomicInteger;
 
 class HistoryClustersMediator extends RecyclerView.OnScrollListener implements SearchDelegate {
     @VisibleForTesting
@@ -70,15 +69,13 @@ interface Clock {
     private static class VisitMetadata {
         public final ListItem visitListItem;
         public final ListItem clusterListItem;
-        public final ListItem relatedSearchesListItem;
-        public AtomicInteger visitCount;
+        public final List<ListItem> visitsAndRelatedSearches;
 
         private VisitMetadata(ListItem visitListItem, ListItem clusterListItem,
-                ListItem relatedSearchesListItem, AtomicInteger visitCount) {
+                List<ListItem> visitsAndRelatedSearches) {
             this.visitListItem = visitListItem;
             this.clusterListItem = clusterListItem;
-            this.relatedSearchesListItem = relatedSearchesListItem;
-            this.visitCount = visitCount;
+            this.visitsAndRelatedSearches = visitsAndRelatedSearches;
         }
     }
 
@@ -102,8 +99,8 @@ private VisitMetadata(ListItem visitListItem, ListItem clusterListItem,
     private QueryState mQueryState;
     private final ListItem mMoreProgressItem;
     private final HistoryClustersMetricsLogger mMetricsLogger;
-    private Map<String, PropertyModel> mLabelToModelMap = new LinkedHashMap<>();
-    private Map<ClusterVisit, VisitMetadata> mVisitMetadataMap = new HashMap<>();
+    private final Map<String, PropertyModel> mLabelToModelMap = new LinkedHashMap<>();
+    private final Map<ClusterVisit, VisitMetadata> mVisitMetadataMap = new HashMap<>();
     private final AccessibilityUtil mAccessibilityUtil;
     private final boolean mIsScrollToLoadDisabled;
 
@@ -318,13 +315,23 @@ void deleteVisits(List<ClusterVisit> visits) {
     private void removeVisit(ClusterVisit visit) {
         VisitMetadata visitMetadata = mVisitMetadataMap.get(visit);
         if (visitMetadata == null) return;
-        mModelList.remove(visitMetadata.visitListItem);
-        if (visitMetadata.visitCount.decrementAndGet() == 0) {
+        ListItem visitListItem = visitMetadata.visitListItem;
+        assert mModelList.indexOf(visitListItem) != -1
+                && visitMetadata.visitsAndRelatedSearches.indexOf(visitListItem) != -1;
+        mModelList.remove(visitListItem);
+        visitMetadata.visitsAndRelatedSearches.remove(visitListItem);
+        if (visitMetadata.visitsAndRelatedSearches.size() == 1
+                && visitMetadata.visitsAndRelatedSearches.get(0).type
+                        == ItemType.RELATED_SEARCHES) {
+            mModelList.remove(visitMetadata.visitsAndRelatedSearches.get(0));
+            visitMetadata.visitsAndRelatedSearches.clear();
+        }
+
+        if (visitMetadata.visitsAndRelatedSearches.isEmpty()) {
             mModelList.remove(visitMetadata.clusterListItem);
-            if (visitMetadata.relatedSearchesListItem != null) {
-                mModelList.remove(visitMetadata.relatedSearchesListItem);
-            }
         }
+
+        mVisitMetadataMap.remove(visit);
     }
 
     private Tab createNewTab(GURL gurl, boolean incognito, Tab parentTab) {
@@ -407,8 +414,7 @@ private void queryComplete(HistoryClustersResult result) {
                 relatedSearchesItem = new ListItem(ItemType.RELATED_SEARCHES, relatedSearchesModel);
             }
 
-            AtomicInteger visitCount = new AtomicInteger(cluster.getVisits().size());
-            for (int i = 0; i < visitCount.get(); i++) {
+            for (int i = 0; i < cluster.getVisits().size(); i++) {
                 ClusterVisit visit = cluster.getVisits().get(i);
                 PropertyModel visitModel =
                         new PropertyModel.Builder(HistoryClustersItemProperties.ALL_KEYS)
@@ -438,8 +444,8 @@ private void queryComplete(HistoryClustersResult result) {
                 }
 
                 ListItem listItem = new ListItem(ItemType.VISIT, visitModel);
-                mVisitMetadataMap.put(visit,
-                        new VisitMetadata(listItem, clusterItem, relatedSearchesItem, visitCount));
+                mVisitMetadataMap.put(
+                        visit, new VisitMetadata(listItem, clusterItem, visitsAndRelatedSearches));
                 visitsAndRelatedSearches.add(listItem);
             }