Skip to content

Commit

Permalink
[Android][Mfill] Prevent crash from double-tapping back button
Browse files Browse the repository at this point in the history
Clicking the back arrow on fallback sheets resets the active tab to null
and closes the sheet which rerequests the keyboard. Technically, this
could happen twice (maybe with a slow device if users are moving quickly
or use a11y tools).
The second click/tap would then cause chrome to crash when operating on
a null pointer. This CL ignores the second click as a speculative fix
for the linked, exceedingly rare crash.

Bug: 1406863
Change-Id: I8e015c99e71642b5bdd9b96b5e4e84d26ef5a14e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4161666
Commit-Queue: Friedrich Horschig <fhorschig@chromium.org>
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1091945}
  • Loading branch information
FHorschig authored and Chromium LUCI CQ committed Jan 12, 2023
1 parent fff41dc commit 75dcf3e
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class KeyboardAccessoryMediator

// Add mediator as observer so it can use model changes as signal for accessory visibility.
mModel.set(OBFUSCATED_CHILD_AT_CALLBACK, this::onSuggestionObfuscatedAt);
mModel.set(SHOW_KEYBOARD_CALLBACK, this::closeSheet);
mModel.set(SHOW_KEYBOARD_CALLBACK, this::onKeyboardRequested);
mModel.set(SHEET_OPENER_ITEM, new SheetOpenerBarItem(sheetOpenerCallbacks));
mModel.set(ANIMATION_LISTENER, mVisibilityDelegate::onBarFadeInAnimationEnd);
if (ChromeFeatureList.isEnabled(ChromeFeatureList.AUTOFILL_KEYBOARD_ACCESSORY)) {
Expand Down Expand Up @@ -298,6 +298,12 @@ public void onActiveTabReselected() {
closeSheet();
}

private void onKeyboardRequested() {
// Return early if the button was clicked twice and the active tab was already reset.
if (mTabSwitcher.getActiveTab() == null) return;
closeSheet();
}

private void closeSheet() {
assert mTabSwitcher.getActiveTab() != null;
ManualFillingMetricsRecorder.recordSheetTrigger(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.junit.Assert.assertThat;
import static org.mockito.Mockito.atLeast;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.times;
import static org.mockito.Mockito.verify;
Expand All @@ -23,6 +24,7 @@
import static org.chromium.chrome.browser.keyboard_accessory.bar_component.KeyboardAccessoryProperties.HAS_SUGGESTIONS;
import static org.chromium.chrome.browser.keyboard_accessory.bar_component.KeyboardAccessoryProperties.OBFUSCATED_CHILD_AT_CALLBACK;
import static org.chromium.chrome.browser.keyboard_accessory.bar_component.KeyboardAccessoryProperties.SHEET_TITLE;
import static org.chromium.chrome.browser.keyboard_accessory.bar_component.KeyboardAccessoryProperties.SHOW_KEYBOARD_CALLBACK;
import static org.chromium.chrome.browser.keyboard_accessory.bar_component.KeyboardAccessoryProperties.SHOW_SWIPING_IPH;
import static org.chromium.chrome.browser.keyboard_accessory.bar_component.KeyboardAccessoryProperties.SKIP_CLOSING_ANIMATION;
import static org.chromium.chrome.browser.keyboard_accessory.bar_component.KeyboardAccessoryProperties.VISIBLE;
Expand Down Expand Up @@ -592,6 +594,22 @@ public void testFowardsAnimationEventsToVisibilityDelegate() {
verify(mMockVisibilityDelegate).onBarFadeInAnimationEnd();
}

@Test
public void testDoubleTappingCloseButtonHasNoEffect() {
setTabs(new KeyboardAccessoryData.Tab[] {mTestTab});
setActiveTab(mTestTab);

// First click should dismiss.
mModel.get(SHOW_KEYBOARD_CALLBACK).run();
setActiveTab(null); // Simulate the tab was reset by the click.
verify(mMockVisibilityDelegate, atLeast(1)).onChangeAccessorySheet(0);
verify(mMockVisibilityDelegate).onCloseAccessorySheet();

// Second click should not crash but be noop.
verifyNoMoreInteractions(mMockVisibilityDelegate);
mModel.get(SHOW_KEYBOARD_CALLBACK).run();
}

private int getGenerationImpressionCount() {
return RecordHistogram.getHistogramValueCountForTesting(
ManualFillingMetricsRecorder.UMA_KEYBOARD_ACCESSORY_ACTION_IMPRESSION,
Expand Down

0 comments on commit 75dcf3e

Please sign in to comment.