Skip to content

Commit

Permalink
[X-Device Restore] Fix profile native initialization crash
Browse files Browse the repository at this point in the history 
Fix for the profile native initialization crash occurring in the RootUiCoordinator. It seems the root cause for this is that the profile is being cached when the incognito tab switcher is entered first. After exiting and switching back to incognito, the off-the-record profile gets destroyed but this code attempts to reference it.

This change now uses only the most up to date information rather than caching, as well as null/off-the-record checking the profile and not showing the promo in the rare case that happens.

(cherry picked from commit 1157f70)

Bug: 1457516
Change-Id: I7f3d802b7bed0830d0c394c6c8e056348436d9d1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4647214
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Brandon Fong <bjfong@google.com>
Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org>
Reviewed-by: Theresa Sullivan <twellington@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1163689}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4654214
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Owners-Override: Erhu Akpobaro <eakpobaro@google.com>
Reviewed-by: Erhu Akpobaro <eakpobaro@google.com>
Reviewed-by: Brandon Fong <bjfong@google.com>
Commit-Queue: Erhu Akpobaro <eakpobaro@google.com>
Cr-Commit-Position: refs/branch-heads/5845@{#189}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
  • Loading branch information
Brandon Fong authored and Chromium LUCI CQ committed Jun 28, 2023
1 parent e28ed98 commit e5403ee
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 28 deletions.
12 changes: 7 additions & 5 deletions chrome/android/java/src/org/chromium/chrome/browser/ui/RootUiCoordinator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1305,7 +1305,10 @@ public void onStartedShowing(int layoutType) {
if (mFindToolbarManager != null) mFindToolbarManager.hideToolbar();
hideAppMenu();
// Attempt to show the promo sheet for the restore tabs feature.
if (RestoreTabsFeatureHelper.RESTORE_TABS_PROMO.isEnabled()) {
// Do not attempt to show the promo if in incognito mode.
if (RestoreTabsFeatureHelper.RESTORE_TABS_PROMO.isEnabled()
&& !mTabModelSelectorSupplier.get().isIncognitoSelected()) {
// TODO(1458646): Add support for triggering in incognito mode.
attemptToShowRestoreTabsPromo();
}
}
Expand Down Expand Up @@ -1659,11 +1662,10 @@ public void restoreUiState(Bundle savedInstanceState) {

private void attemptToShowRestoreTabsPromo() {
if (mRestoreTabsFeatureHelper == null) {
mRestoreTabsFeatureHelper =
new RestoreTabsFeatureHelper(mActivity, mProfileSupplier.get(),
mTabCreatorManagerSupplier.get(), getBottomSheetController());
mRestoreTabsFeatureHelper = new RestoreTabsFeatureHelper();
}
mRestoreTabsFeatureHelper.maybeShowPromo();
mRestoreTabsFeatureHelper.maybeShowPromo(mActivity, mProfileSupplier.get(),
mTabCreatorManagerSupplier.get(), getBottomSheetController());
}

// Testing methods
Expand Down
34 changes: 16 additions & 18 deletions ...bs/android/java/src/org/chromium/chrome/browser/recent_tabs/RestoreTabsFeatureHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,19 +35,9 @@ public class RestoreTabsFeatureHelper {
private RestoreTabsController mController;
private RestoreTabsControllerDelegate mDelegate;
private RestoreTabsControllerDelegate mDelegateForTesting;
private Activity mActivity;
private Profile mProfile;
private TabCreatorManager mTabCreatorManager;
private BottomSheetController mBottomSheetController;
private ForeignSessionHelper mForeignSessionHelper;

public RestoreTabsFeatureHelper(Activity activity, Profile profile,
TabCreatorManager tabCreatorManager, BottomSheetController bottomSheetController) {
mActivity = activity;
mProfile = profile;
mTabCreatorManager = tabCreatorManager;
mBottomSheetController = bottomSheetController;
}
public RestoreTabsFeatureHelper() {}

public void destroy() {
if (mForeignSessionHelper != null) {
Expand All @@ -68,8 +58,15 @@ public void destroy() {
/**
* Check the criteria for displaying the restore tabs promo.
*/
public void maybeShowPromo() {
Tracker tracker = TrackerFactory.getTrackerForProfile(mProfile);
public void maybeShowPromo(Activity activity, Profile profile,
TabCreatorManager tabCreatorManager, BottomSheetController bottomSheetController) {
if (profile == null || profile.isOffTheRecord()) {
RestoreTabsMetricsHelper.recordPromoShowResultHistogram(
RestoreTabsOnFREPromoShowResult.NULL_PROFILE);
return;
}

Tracker tracker = TrackerFactory.getTrackerForProfile(profile);

if (!RESTORE_TABS_PROMO_SKIP_FEATURE_ENGAGEMENT.getValue()
&& !tracker.wouldTriggerHelpUI(FeatureConstants.RESTORE_TABS_ON_FRE_FEATURE)) {
Expand All @@ -78,7 +75,7 @@ public void maybeShowPromo() {
return;
}

mForeignSessionHelper = new ForeignSessionHelper(mProfile);
mForeignSessionHelper = new ForeignSessionHelper(profile);
List<ForeignSession> sessions = mForeignSessionHelper.getMobileAndTabletForeignSessions();

// Determines whether the promo is to be shown for the first or second time.
Expand All @@ -103,7 +100,7 @@ public void maybeShowPromo() {
&& (RESTORE_TABS_PROMO_SKIP_FEATURE_ENGAGEMENT.getValue()
|| tracker.shouldTriggerHelpUI(
FeatureConstants.RESTORE_TABS_ON_FRE_FEATURE))) {
setDelegate();
setDelegate(activity, profile, tabCreatorManager, bottomSheetController);
mDelegate.showPromo(sessions);
RestoreTabsMetricsHelper.recordPromoShowResultHistogram(
RestoreTabsOnFREPromoShowResult.SHOWN);
Expand All @@ -120,7 +117,8 @@ public void maybeShowPromo() {
}
}

private void setDelegate() {
private void setDelegate(Activity activity, Profile profile,
TabCreatorManager tabCreatorManager, BottomSheetController bottomSheetController) {
mDelegate = (mDelegateForTesting != null)
? mDelegateForTesting
: new RestoreTabsControllerDelegate() {
Expand All @@ -129,7 +127,7 @@ private void setDelegate() {
@Override
public void showPromo(List<ForeignSession> sessions) {
mController = RestoreTabsControllerFactory.createInstance(
mActivity, mProfile, mTabCreatorManager, mBottomSheetController);
activity, profile, tabCreatorManager, bottomSheetController);
mController.showHomeScreen(mForeignSessionHelper, sessions, mDelegate);
}

Expand All @@ -139,7 +137,7 @@ public void onDismissed() {
mWasDismissed = true;

if (!RESTORE_TABS_PROMO_SKIP_FEATURE_ENGAGEMENT.getValue()) {
TrackerFactory.getTrackerForProfile(mProfile).dismissed(
TrackerFactory.getTrackerForProfile(profile).dismissed(
FeatureConstants.RESTORE_TABS_ON_FRE_FEATURE);
}

Expand Down
7 changes: 3 additions & 4 deletions ...id/java/src/org/chromium/chrome/browser/recent_tabs/RestoreTabsFeatureHelperUnitTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,7 @@ public void setUp() {
when(mForeignSessionHelperJniMock.isTabSyncEnabled(1L)).thenReturn(true);

mActivity = Robolectric.buildActivity(Activity.class).setup().get();
mHelper = new RestoreTabsFeatureHelper(
mActivity, mProfile, mTabCreatorManager, mBottomSheetController);
mHelper = new RestoreTabsFeatureHelper();
mHelper.setRestoreTabsControllerDelegateForTesting(mDelegate);
}

Expand All @@ -92,7 +91,7 @@ public void tearDown() {

@Test
public void testRestoreTabsFeatureHelper_noSyncedDevices() {
mHelper.maybeShowPromo();
mHelper.maybeShowPromo(mActivity, mProfile, mTabCreatorManager, mBottomSheetController);
verify(mForeignSessionHelperJniMock, times(1))
.getMobileAndTabletForeignSessions(1L, new ArrayList<ForeignSession>());
verify(mForeignSessionHelperJniMock, times(1)).destroy(1L);
Expand Down Expand Up @@ -120,7 +119,7 @@ public void testRestoreTabsFeatureHelper_hasValidSyncedDevices() {
})
.when(mForeignSessionHelperJniMock)
.getMobileAndTabletForeignSessions(1L, new ArrayList<ForeignSession>());
mHelper.maybeShowPromo();
mHelper.maybeShowPromo(mActivity, mProfile, mTabCreatorManager, mBottomSheetController);
verify(mDelegate, times(1)).showPromo(anyList());
}
}
4 changes: 3 additions & 1 deletion ...al/android/java/src/org/chromium/chrome/browser/recent_tabs/RestoreTabsMetricsHelper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,16 @@ public class RestoreTabsMetricsHelper {
// numeric values should never be reused.
@IntDef({RestoreTabsOnFREPromoShowResult.SHOWN, RestoreTabsOnFREPromoShowResult.NOT_ELIGIBLE,
RestoreTabsOnFREPromoShowResult.NO_SYNCED_TABS,
RestoreTabsOnFREPromoShowResult.NULL_PROFILE,
RestoreTabsOnFREPromoShowResult.NUM_ENTRIES})
public @interface RestoreTabsOnFREPromoShowResult {
int SHOWN = 0;
int NOT_ELIGIBLE = 1;
int NO_SYNCED_TABS = 2;
int NULL_PROFILE = 3;

// Be sure to also update enums.xml when updating these values.
int NUM_ENTRIES = 3;
int NUM_ENTRIES = 4;
}

// These values are persisted to logs. Entries should not be renumbered and
Expand Down
1 change: 1 addition & 0 deletions tools/metrics/histograms/enums.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92661,6 +92661,7 @@ https://www.dmtf.org/sites/default/files/standards/documents/DSP0134_2.7.1.pdf
<int value="2"
label="Either sync was not enabled, no tabs were synced or synced tabs
were not finished syncing"/>
<int value="3" label="Promo was not shown due to a null profile"/>
</enum>

<enum name="RestoreTabsOnFRERestoredTabsResult">
Expand Down

0 comments on commit e5403ee

Please sign in to comment.