Skip to content

Commit

Permalink
[UPMLocalPwd] Fix the frozen progress bar in the export flow
Browse files Browse the repository at this point in the history
The problem: The ExportFlow is not supposed to start the serialization
until the passwords are fetched from the password store. For this
reason, the parameter passwordsAvailable was introduced into the
ExportFlow.startExporting. In the case when startExporting is called
before passwords are available, PasswordManagerHandlerProvider will
notify the PasswordMigrationWarningMediator when the passwords are
fetched and the mediator will trigger the serialization on the export
flow. The problem now is that we rely on that the passwordListAvailable
will always be triggered, which is not the case. It's possible that our
subscription to the PMHP happens after the passwords have been fetched.

The solution: Introduce the isWaitingForPasswordStore in the
PasswordManagerHandler and check it when starting the export flow.

(cherry picked from commit af06d6d)

Bug: 1462936
Change-Id: Ic87ccdb40418fdc2ba0a77a108b99f22cf1941f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4672966
Reviewed-by: Friedrich Horschig <fhorschig@chromium.org>
Code-Coverage: Findit <findit-for-me@appspot.gserviceaccount.com>
Reviewed-by: Ivana Žužić <izuzic@google.com>
Commit-Queue: Anna Tsvirchkova <atsvirchkova@google.com>
Cr-Original-Commit-Position: refs/heads/main@{#1168168}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4679801
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/branch-heads/5845@{#436}
Cr-Branched-From: 5a5dff6-refs/heads/main@{#1160321}
  • Loading branch information
Anna Tsvirchkova authored and Chromium LUCI CQ committed Jul 12, 2023
1 parent e73b5ab commit 086f5f0
Show file tree
Hide file tree
Showing 12 changed files with 42 additions and 18 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ public static String getTargetDirectory() {
}

@Override
public void startExporting(boolean passwordsAvailable) {
public void startExporting() {
assert mExportState == ExportState.INACTIVE;
// Disable re-triggering exporting until the current exporting finishes.
mExportState = ExportState.REQUESTED;
Expand All @@ -274,7 +274,9 @@ public void startExporting(boolean passwordsAvailable) {
// fails the reauthentication, the serialized passwords will simply get ignored when
// they arrive.
mEntriesCount = null;
if (passwordsAvailable) {
if (!PasswordManagerHandlerProvider.getInstance()
.getPasswordManagerHandler()
.isWaitingForPasswordStore()) {
serializePasswords();
}
if (!ReauthenticationManager.isScreenLockSetUp(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,12 @@ void showPasswordEntryEditingView(Context context, SettingsLauncher settingsLaun
* @param bottomSheetController the controller displaying the warning bottom sheet.
*/
void showMigrationWarning(Activity activity, BottomSheetController bottomSheetController);

/**
* Calls C++ to check whether the PasswordManagerHandler is still waiting for the passwords to
* be fetched from the password store.
*
* @return Returns true if the request to fetch the passwords is still pending.
*/
boolean isWaitingForPasswordStore();
}
Original file line number Diff line number Diff line change
Expand Up @@ -234,7 +234,7 @@ public boolean onOptionsItemSelected(MenuItem item) {
RecordHistogram.recordEnumeratedHistogram(PASSWORD_EXPORT_EVENT_HISTOGRAM,
ExportFlow.PasswordExportEvent.EXPORT_OPTION_SELECTED,
ExportFlow.PasswordExportEvent.COUNT);
mExportFlow.startExporting(true);
mExportFlow.startExporting();
return true;
}
if (SearchUtils.handleSearchNavigation(item, mSearchItem, mSearchQuery, getActivity())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,12 @@ public static boolean hasAccountForLeakCheckRequest() {
return PasswordUIViewJni.get().hasAccountForLeakCheckRequest();
}

@Override
public boolean isWaitingForPasswordStore() {
return PasswordUIViewJni.get().isWaitingForPasswordStore(
mNativePasswordUIViewAndroid, PasswordUIView.this);
}

/**
* Destroy the native object.
*/
Expand Down Expand Up @@ -170,6 +176,7 @@ void handleRemoveSavedPasswordException(
String getAccountDashboardURL();
String getTrustedVaultLearnMoreURL();
boolean hasAccountForLeakCheckRequest();
boolean isWaitingForPasswordStore(long nativePasswordUIViewAndroid, PasswordUIView caller);
void destroy(long nativePasswordUIViewAndroid, PasswordUIView caller);
void handleSerializePasswords(long nativePasswordUIViewAndroid, PasswordUIView caller,
String targetPath, IntStringCallback successCallback,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@ public void showMigrationWarning(
mShowWarningWasCalled = true;
}

@Override
public boolean isWaitingForPasswordStore() {
return false;
}

public int getSerializationInvocationCount() {
return mSerializationInvocationCount;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -962,9 +962,9 @@ public void testDontRepeatedlySerialisePasswords() {
mTestHelper.startPasswordSettingsFromMainSettings(mSettingsActivityTestRule);

PasswordSettings fragment = mSettingsActivityTestRule.getFragment();
ExportFlow exportFlow = fragment.getExportFlowForTesting();
exportFlow.startExporting(false);
TestThreadUtils.runOnUiThreadBlocking(() -> {
ExportFlow exportFlow = fragment.getExportFlowForTesting();
exportFlow.startExporting();
exportFlow.passwordsAvailable();
exportFlow.passwordsAvailable();
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,12 @@ jboolean JNI_PasswordUIView_HasAccountForLeakCheckRequest(JNIEnv* env) {
identity_manager);
}

jboolean PasswordUIViewAndroid::IsWaitingForPasswordStore(
JNIEnv* env,
const base::android::JavaRef<jobject>&) {
return saved_passwords_presenter_.IsWaitingForPasswordStore();
}

// static
static jlong JNI_PasswordUIView_Init(JNIEnv* env,
const JavaParamRef<jobject>& obj) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,8 @@ class PasswordUIViewAndroid
JNIEnv* env,
const base::android::JavaParamRef<jobject>& activity,
const base::android::JavaParamRef<jobject>& controller);
jboolean IsWaitingForPasswordStore(JNIEnv* env,
const base::android::JavaRef<jobject>&);
// Destroy the native implementation.
void Destroy(JNIEnv*, const base::android::JavaRef<jobject>&);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,11 +43,8 @@ public interface Delegate {

/**
* Starts the password export flow.
*
* @param passwordsAvailable indicates if the passwords have been fetched and are ready to be
* exported.
*/
public void startExporting(boolean passwordsAvailable);
public void startExporting();

/**
* A hook to be used in a {@link Fragment}'s onResume method. I processes the result of the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public void openSyncSettings() {
}

@Override
public void startExportFlow(FragmentManager fragmentManager, boolean passwordsAvailable) {
public void startExportFlow(FragmentManager fragmentManager) {
// TODO(crbug.com/1445065): Hide the sheet when the export is done.
mExportFlow.onCreate(new Bundle(), new ExportFlowInterface.Delegate() {
@Override
Expand All @@ -96,7 +96,7 @@ public int getViewId() {
return R.id.fragment_container_view;
}
});
mExportFlow.startExporting(passwordsAvailable);
mExportFlow.startExporting();
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,6 @@ class PasswordMigrationWarningMediator
private PropertyModel mModel;
private Profile mProfile;
private MigrationWarningOptionsHandler mOptionsHandler;
private boolean mPasswordsAvailable;

public interface MigrationWarningOptionsHandler {
/**
Expand All @@ -92,7 +91,7 @@ public interface MigrationWarningOptionsHandler {
*
* @param fragmentManager for the fragment that owns the export flow.
*/
void startExportFlow(FragmentManager fragmentManager, boolean passwordsAvailable);
void startExportFlow(FragmentManager fragmentManager);

/**
* Resumes the password export flow.
Expand All @@ -109,7 +108,6 @@ public interface MigrationWarningOptionsHandler {
Profile profile, MigrationWarningOptionsHandler optionsHandler) {
mProfile = profile;
mOptionsHandler = optionsHandler;
mPasswordsAvailable = false;
}

void initializeModel(PropertyModel model) {
Expand Down Expand Up @@ -170,7 +168,7 @@ public void onNext(@MigrationOption int selectedOption, FragmentManager fragment
PasswordMigrationWarningUserActions.SYNC,
PasswordMigrationWarningUserActions.COUNT);
} else {
mOptionsHandler.startExportFlow(fragmentManager, mPasswordsAvailable);
mOptionsHandler.startExportFlow(fragmentManager);

RecordHistogram.recordEnumeratedHistogram(PASSWORD_MIGRATION_WARNING_USER_ACTIONS,
PasswordMigrationWarningUserActions.EXPORT,
Expand Down Expand Up @@ -216,7 +214,6 @@ private void startSyncFlow() {

@Override
public void passwordListAvailable(int count) {
mPasswordsAvailable = true;
mOptionsHandler.passwordsAvailable();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -315,15 +315,15 @@ public void testOnNextRecordsExportUserAction() {
public void testOnNextWithoutPasswordsWithExportingSelectedStartsTheExportFlow() {
mMediator.onNext(MigrationOption.EXPORT_AND_DELETE, mFragmentManager);

verify(mOptionsHandler).startExportFlow(mFragmentManager, false);
verify(mOptionsHandler).startExportFlow(mFragmentManager);
}

@Test
public void testOnNextWithPasswordsWithExportingSelectedStartsTheExportFlow() {
mMediator.passwordListAvailable(2);
mMediator.onNext(MigrationOption.EXPORT_AND_DELETE, mFragmentManager);

verify(mOptionsHandler).startExportFlow(mFragmentManager, true);
verify(mOptionsHandler).startExportFlow(mFragmentManager);
}

@Test
Expand Down

0 comments on commit 086f5f0

Please sign in to comment.