Skip to content

Commit

Permalink
[Journeys] Fix crash in continueQuery post-destroy
Browse files Browse the repository at this point in the history
Bug: 1440128
Change-Id: I2e2a5822b6bae6a642c62dae8e1de49be9f38609
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4477593
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Brandon Wylie <wylieb@chromium.org>
Commit-Queue: Patrick Noland <pnoland@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1136566}
  • Loading branch information
Patrick Noland authored and Chromium LUCI CQ committed Apr 27, 2023
1 parent 04fd593 commit 451836e
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,21 @@ public void testHistoryDeletedExternally() {
verify(mBridge, times(2)).queryClusters("query");
}

@Test
public void testContinueQueryAfterDestroy() {
Promise promise = new Promise<>();
doReturn(promise).when(mBridge).queryClusters("query");

doReturn(3).when(mLayoutManager).findLastVisibleItemPosition();

mMediator.setQueryState(QueryState.forQuery("query", ""));
fulfillPromise(promise, mHistoryClustersResultWithQuery);
mMediator.onScrolled(mRecyclerView, 1, 1);
mMediator.destroy();

ShadowLooper.idleMainLooper();
}

private <T> void fulfillPromise(Promise<T> promise, T result) {
promise.fulfill(result);
ShadowLooper.idleMainLooper();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import org.chromium.base.CallbackController;
import org.chromium.base.ContextUtils;
import org.chromium.base.Promise;
import org.chromium.base.lifetime.DestroyChecker;
import org.chromium.chrome.browser.history_clusters.HistoryCluster.MatchPosition;
import org.chromium.chrome.browser.history_clusters.HistoryClusterView.ClusterViewAccessibilityState;
import org.chromium.chrome.browser.history_clusters.HistoryClustersItemProperties.ItemType;
Expand Down Expand Up @@ -110,6 +111,7 @@ private VisitMetadata(ListItem visitListItem, ListItem clusterListItem,
private final AccessibilityUtil mAccessibilityUtil;
private final Callback<String> mAnnounceForAccessibilityCallback;
private final Handler mHandler;
private final DestroyChecker mDestroyChecker = new DestroyChecker();
private final boolean mIsScrollToLoadDisabled;

/**
Expand Down Expand Up @@ -181,7 +183,10 @@ private VisitMetadata(ListItem visitListItem, ListItem clusterListItem,
new PropertyModel.Builder(HistoryClustersItemProperties.ALL_KEYS)
.with(HistoryClustersItemProperties.PROGRESS_BUTTON_STATE, buttonState)
.with(HistoryClustersItemProperties.CLICK_HANDLER,
(v) -> mPromise.then(this::continueQuery, this::onPromiseRejected))
(v)
-> mPromise.then(mCallbackController.makeCancelable(
this::continueQuery),
this::onPromiseRejected))
.build();
mMoreProgressItem = new ListItem(ItemType.MORE_PROGRESS, moreProgressModel);
mEmptyTextListItem = new ListItem(ItemType.EMPTY_TEXT, new PropertyModel());
Expand Down Expand Up @@ -209,14 +214,16 @@ public void onScrolled(RecyclerView recyclerView, int dx, int dy) {
LinearLayoutManager layoutManager = (LinearLayoutManager) recyclerView.getLayoutManager();
if (layoutManager.findLastVisibleItemPosition()
> (mModelList.size() - REMAINING_ITEM_BUFFER_SIZE)) {
mPromise.then(this::continueQuery, this::onPromiseRejected);
mPromise.then(mCallbackController.makeCancelable(this::continueQuery),
this::onPromiseRejected);
}
}

void destroy() {
mHandler.removeCallbacksAndMessages(null);
mLargeIconBridge.destroy();
mCallbackController.destroy();
mDestroyChecker.destroy();
}

void setQueryState(QueryState queryState) {
Expand All @@ -232,6 +239,7 @@ void setQueryState(QueryState queryState) {

@VisibleForTesting
void startQuery(String query) {
mDestroyChecker.checkNotDestroyed();
if (mQueryState.isSearching()) {
mMetricsLogger.incrementQueryCount();
}
Expand All @@ -247,6 +255,7 @@ void startQuery(String query) {
}

void continueQuery(HistoryClustersResult previousResult) {
mDestroyChecker.checkNotDestroyed();
if (!previousResult.canLoadMore()) return;
mPromise = mHistoryClustersBridge.loadMoreClusters(previousResult.getQuery());
mPromise.then(
Expand Down

0 comments on commit 451836e

Please sign in to comment.