Commit ae5f7ade authored by Ioana Pandele's avatar Ioana Pandele Committed by Commit Bot

[PwdCheckAndroid] Add logic to create and destroy PasswordCheckBridge

The PasswordCheckBridge needs to signal its C++ counterpart when it's no
longer needed in order to free up memory. Since the C++ bridge gets
destructed, we need to destroy the entire PasswordCheck component, so
that we don't end up with a partially initialized object.

The object responsible for the tear-down is the last object to need the
component. Possible cases are:

MainSettings > PasswordSettings > PasswordCheck -> MainSettings should call destroy
PasswordSettings > PasswordCheck -> PasswordSettings should call destroy
MainSettings > SafetyCheck > PasswordCheck -> MainSettings should call destroy
PasswordCheck (launched from the leak dialog) -> PasswordCheck should call destroy


Bug:1102025, 1101256

Change-Id: I6706ab489049da2192ccb8c03632cc5a7fe38609
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2297504
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Reviewed-by: default avatarBoris Sazonov <bsazonov@chromium.org>
Reviewed-by: default avatarNatalie Chouinard <chouinard@chromium.org>
Reviewed-by: default avatarFriedrich [CET] <fhorschig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#792276}
parent c2cbb827
...@@ -972,7 +972,7 @@ android_library("chrome_test_java") { ...@@ -972,7 +972,7 @@ android_library("chrome_test_java") {
"//chrome/browser/image_fetcher:java", "//chrome/browser/image_fetcher:java",
"//chrome/browser/offline_pages/android:java", "//chrome/browser/offline_pages/android:java",
"//chrome/browser/paint_preview/android:java", "//chrome/browser/paint_preview/android:java",
"//chrome/browser/password_check/android:public_java", "//chrome/browser/password_check:public_java",
"//chrome/browser/password_manager/android_test_helpers:test_support_java", "//chrome/browser/password_manager/android_test_helpers:test_support_java",
"//chrome/browser/performance_hints/android:java", "//chrome/browser/performance_hints/android:java",
"//chrome/browser/preferences:java", "//chrome/browser/preferences:java",
......
...@@ -11,6 +11,7 @@ import org.chromium.chrome.browser.AppHooks; ...@@ -11,6 +11,7 @@ import org.chromium.chrome.browser.AppHooks;
import org.chromium.chrome.browser.ChromeActivity; import org.chromium.chrome.browser.ChromeActivity;
import org.chromium.chrome.browser.flags.ChromeFeatureList; import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.password_check.PasswordCheckFactory; import org.chromium.chrome.browser.password_check.PasswordCheckFactory;
import org.chromium.chrome.browser.password_check.PasswordCheckReferrer;
import org.chromium.ui.base.WindowAndroid; import org.chromium.ui.base.WindowAndroid;
/** /**
...@@ -31,7 +32,8 @@ public class PasswordCheckupLauncher { ...@@ -31,7 +32,8 @@ public class PasswordCheckupLauncher {
private static void launchLocalCheckup(WindowAndroid windowAndroid) { private static void launchLocalCheckup(WindowAndroid windowAndroid) {
assert ChromeFeatureList.isEnabled(ChromeFeatureList.PASSWORD_CHECK); assert ChromeFeatureList.isEnabled(ChromeFeatureList.PASSWORD_CHECK);
if (windowAndroid.getContext().get() == null) return; // Window not available yet/anymore. if (windowAndroid.getContext().get() == null) return; // Window not available yet/anymore.
PasswordCheckFactory.create().showUi(windowAndroid.getContext().get()); PasswordCheckFactory.getOrCreate().showUi(
windowAndroid.getContext().get(), PasswordCheckReferrer.LEAK_DIALOG);
} }
private static boolean tryLaunchingNativePasswordCheckup(ChromeActivity activity) { private static boolean tryLaunchingNativePasswordCheckup(ChromeActivity activity) {
......
...@@ -5,6 +5,7 @@ ...@@ -5,6 +5,7 @@
package org.chromium.chrome.browser.password_manager; package org.chromium.chrome.browser.password_manager;
import android.app.Activity; import android.app.Activity;
import android.os.Bundle;
import com.google.android.gms.common.ConnectionResult; import com.google.android.gms.common.ConnectionResult;
...@@ -30,6 +31,11 @@ import java.lang.ref.WeakReference; ...@@ -30,6 +31,11 @@ import java.lang.ref.WeakReference;
* Bridge between Java and native PasswordManager code. * Bridge between Java and native PasswordManager code.
*/ */
public class PasswordManagerLauncher { public class PasswordManagerLauncher {
// Key for the argument with which PasswordsSettings will be launched. The value for
// this argument should be part of the ManagePasswordsReferrer enum, which contains
// all points of entry to the passwords settings.
public static final String MANAGE_PASSWORDS_REFERRER = "manage-passwords-referrer";
private static final String GOOGLE_ACCOUNT_PWM_UI = "google-password-manager"; private static final String GOOGLE_ACCOUNT_PWM_UI = "google-password-manager";
// Name of the parameter for the google-password-manager feature, used to override the default // Name of the parameter for the google-password-manager feature, used to override the default
...@@ -63,7 +69,9 @@ public class PasswordManagerLauncher { ...@@ -63,7 +69,9 @@ public class PasswordManagerLauncher {
} }
SettingsLauncher settingsLauncher = new SettingsLauncherImpl(); SettingsLauncher settingsLauncher = new SettingsLauncherImpl();
settingsLauncher.launchSettingsActivity(activity, PasswordSettings.class); Bundle fragmentArgs = new Bundle();
fragmentArgs.putInt(MANAGE_PASSWORDS_REFERRER, referrer);
settingsLauncher.launchSettingsActivity(activity, PasswordSettings.class, fragmentArgs);
} }
@CalledByNative @CalledByNative
......
...@@ -16,6 +16,7 @@ import android.view.MenuInflater; ...@@ -16,6 +16,7 @@ import android.view.MenuInflater;
import android.view.MenuItem; import android.view.MenuItem;
import android.view.View; import android.view.View;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting; import androidx.annotation.VisibleForTesting;
import androidx.appcompat.widget.Toolbar; import androidx.appcompat.widget.Toolbar;
import androidx.fragment.app.FragmentManager; import androidx.fragment.app.FragmentManager;
...@@ -32,6 +33,8 @@ import org.chromium.chrome.browser.help.HelpAndFeedback; ...@@ -32,6 +33,8 @@ import org.chromium.chrome.browser.help.HelpAndFeedback;
import org.chromium.chrome.browser.password_check.PasswordCheck; import org.chromium.chrome.browser.password_check.PasswordCheck;
import org.chromium.chrome.browser.password_check.PasswordCheckFactory; import org.chromium.chrome.browser.password_check.PasswordCheckFactory;
import org.chromium.chrome.browser.password_check.PasswordCheckPreference; import org.chromium.chrome.browser.password_check.PasswordCheckPreference;
import org.chromium.chrome.browser.password_check.PasswordCheckReferrer;
import org.chromium.chrome.browser.password_manager.ManagePasswordsReferrer;
import org.chromium.chrome.browser.password_manager.PasswordManagerLauncher; import org.chromium.chrome.browser.password_manager.PasswordManagerLauncher;
import org.chromium.chrome.browser.preferences.Pref; import org.chromium.chrome.browser.preferences.Pref;
import org.chromium.chrome.browser.profiles.Profile; import org.chromium.chrome.browser.profiles.Profile;
...@@ -108,6 +111,9 @@ public class PasswordSettings ...@@ -108,6 +111,9 @@ public class PasswordSettings
private boolean mSearchRecorded; private boolean mSearchRecorded;
private Menu mMenu; private Menu mMenu;
private @Nullable PasswordCheck mPasswordCheck;
private @ManagePasswordsReferrer int mManagePasswordsReferrer;
/** /**
* For controlling the UX flow of exporting passwords. * For controlling the UX flow of exporting passwords.
*/ */
...@@ -141,6 +147,12 @@ public class PasswordSettings ...@@ -141,6 +147,12 @@ public class PasswordSettings
setHasOptionsMenu(true); // Password Export might be optional but Search is always present. setHasOptionsMenu(true); // Password Export might be optional but Search is always present.
Bundle extras = getArguments();
assert extras.containsKey(PasswordManagerLauncher.MANAGE_PASSWORDS_REFERRER)
: "PasswordSettings must be launched with a manage-passwords-referrer fragment"
+ "argument, but none was provided.";
mManagePasswordsReferrer = extras.getInt(PasswordManagerLauncher.MANAGE_PASSWORDS_REFERRER);
if (savedInstanceState == null) return; if (savedInstanceState == null) return;
if (savedInstanceState.containsKey(SAVED_STATE_SEARCH_QUERY)) { if (savedInstanceState.containsKey(SAVED_STATE_SEARCH_QUERY)) {
...@@ -149,6 +161,12 @@ public class PasswordSettings ...@@ -149,6 +161,12 @@ public class PasswordSettings
} }
} }
@Override
public void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
mPasswordCheck = PasswordCheckFactory.getOrCreate();
}
@Override @Override
public void onViewCreated(View view, Bundle savedInstanceState) { public void onViewCreated(View view, Bundle savedInstanceState) {
super.onViewCreated(view, savedInstanceState); super.onViewCreated(view, savedInstanceState);
...@@ -240,8 +258,7 @@ public class PasswordSettings ...@@ -240,8 +258,7 @@ public class PasswordSettings
if (mSearchQuery == null) { if (mSearchQuery == null) {
createSavePasswordsSwitch(); createSavePasswordsSwitch();
createAutoSignInCheckbox(); createAutoSignInCheckbox();
if (ChromeFeatureList.isEnabled(ChromeFeatureList.PASSWORD_CHECK) if (mPasswordCheck != null) {
&& PasswordUIView.hasAccountForLeakCheckRequest()) {
createCheckPasswords(); createCheckPasswords();
} }
} }
...@@ -405,6 +422,13 @@ public class PasswordSettings ...@@ -405,6 +422,13 @@ public class PasswordSettings
public void onDestroy() { public void onDestroy() {
super.onDestroy(); super.onDestroy();
PasswordManagerHandlerProvider.getInstance().removeObserver(this); PasswordManagerHandlerProvider.getInstance().removeObserver(this);
// The component should only be destroyed when the activity has been closed by the user
// (e.g. by pressing on the back button) and not when the activity is temporarily destroyed
// by the system.
if (getActivity().isFinishing() && mPasswordCheck != null
&& mManagePasswordsReferrer != ManagePasswordsReferrer.CHROME_SETTINGS) {
PasswordCheckFactory.destroy();
}
} }
/** /**
...@@ -486,8 +510,8 @@ public class PasswordSettings ...@@ -486,8 +510,8 @@ public class PasswordSettings
mCheckPasswords.setOnPreferenceClickListener(preference -> { mCheckPasswords.setOnPreferenceClickListener(preference -> {
getPrefService().setInteger( getPrefService().setInteger(
Pref.SETTINGS_LAUNCHED_PASSWORD_CHECKS, numCheckLaunched + 1); Pref.SETTINGS_LAUNCHED_PASSWORD_CHECKS, numCheckLaunched + 1);
PasswordCheck passwordCheck = PasswordCheckFactory.create(); PasswordCheck passwordCheck = PasswordCheckFactory.getOrCreate();
passwordCheck.showUi(getStyledContext()); passwordCheck.showUi(getStyledContext(), PasswordCheckReferrer.PASSWORD_SETTINGS);
// Return true to notify the click was handled // Return true to notify the click was handled
return true; return true;
}); });
......
...@@ -11,6 +11,7 @@ import android.os.Handler; ...@@ -11,6 +11,7 @@ import android.os.Handler;
import android.provider.Settings; import android.provider.Settings;
import android.view.View; import android.view.View;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting; import androidx.annotation.VisibleForTesting;
import androidx.preference.Preference; import androidx.preference.Preference;
import androidx.preference.PreferenceFragmentCompat; import androidx.preference.PreferenceFragmentCompat;
...@@ -23,6 +24,8 @@ import org.chromium.chrome.browser.homepage.HomepageManager; ...@@ -23,6 +24,8 @@ import org.chromium.chrome.browser.homepage.HomepageManager;
import org.chromium.chrome.browser.net.spdyproxy.DataReductionProxySettings; import org.chromium.chrome.browser.net.spdyproxy.DataReductionProxySettings;
import org.chromium.chrome.browser.night_mode.NightModeUtils; import org.chromium.chrome.browser.night_mode.NightModeUtils;
import org.chromium.chrome.browser.offlinepages.prefetch.PrefetchConfiguration; import org.chromium.chrome.browser.offlinepages.prefetch.PrefetchConfiguration;
import org.chromium.chrome.browser.password_check.PasswordCheck;
import org.chromium.chrome.browser.password_check.PasswordCheckFactory;
import org.chromium.chrome.browser.password_manager.ManagePasswordsReferrer; import org.chromium.chrome.browser.password_manager.ManagePasswordsReferrer;
import org.chromium.chrome.browser.password_manager.PasswordManagerLauncher; import org.chromium.chrome.browser.password_manager.PasswordManagerLauncher;
import org.chromium.chrome.browser.profiles.Profile; import org.chromium.chrome.browser.profiles.Profile;
...@@ -75,6 +78,7 @@ public class MainSettings extends PreferenceFragmentCompat ...@@ -75,6 +78,7 @@ public class MainSettings extends PreferenceFragmentCompat
private final Map<String, Preference> mAllPreferences = new HashMap<>(); private final Map<String, Preference> mAllPreferences = new HashMap<>();
private SignInPreference mSignInPreference; private SignInPreference mSignInPreference;
private ChromeBasePreference mManageSync; private ChromeBasePreference mManageSync;
private @Nullable PasswordCheck mPasswordCheck;
public MainSettings() { public MainSettings() {
setHasOptionsMenu(true); setHasOptionsMenu(true);
...@@ -86,6 +90,12 @@ public class MainSettings extends PreferenceFragmentCompat ...@@ -86,6 +90,12 @@ public class MainSettings extends PreferenceFragmentCompat
createPreferences(); createPreferences();
} }
@Override
public void onCreate(Bundle savedInstanceState) {
super.onCreate(savedInstanceState);
mPasswordCheck = PasswordCheckFactory.getOrCreate();
}
@Override @Override
public void onViewCreated(View view, Bundle savedInstanceState) { public void onViewCreated(View view, Bundle savedInstanceState) {
super.onViewCreated(view, savedInstanceState); super.onViewCreated(view, savedInstanceState);
...@@ -98,6 +108,10 @@ public class MainSettings extends PreferenceFragmentCompat ...@@ -98,6 +108,10 @@ public class MainSettings extends PreferenceFragmentCompat
public void onDestroy() { public void onDestroy() {
super.onDestroy(); super.onDestroy();
mSignInPreference.onPreferenceFragmentDestroyed(); mSignInPreference.onPreferenceFragmentDestroyed();
// The component should only be destroyed when the activity has been closed by the user
// (e.g. by pressing on the back button) and not when the activity is temporarily destroyed
// by the system.
if (getActivity().isFinishing() && mPasswordCheck != null) PasswordCheckFactory.destroy();
} }
@Override @Override
......
...@@ -4,6 +4,7 @@ ...@@ -4,6 +4,7 @@
package org.chromium.chrome.browser.settings; package org.chromium.chrome.browser.settings;
import android.app.Activity;
import android.text.TextUtils; import android.text.TextUtils;
import android.view.View; import android.view.View;
...@@ -40,6 +41,8 @@ import org.chromium.chrome.browser.language.settings.LanguageSettings; ...@@ -40,6 +41,8 @@ import org.chromium.chrome.browser.language.settings.LanguageSettings;
import org.chromium.chrome.browser.night_mode.NightModeUtils; import org.chromium.chrome.browser.night_mode.NightModeUtils;
import org.chromium.chrome.browser.night_mode.settings.ThemeSettingsFragment; import org.chromium.chrome.browser.night_mode.settings.ThemeSettingsFragment;
import org.chromium.chrome.browser.notifications.settings.NotificationSettings; import org.chromium.chrome.browser.notifications.settings.NotificationSettings;
import org.chromium.chrome.browser.password_check.PasswordCheck;
import org.chromium.chrome.browser.password_check.PasswordCheckFactory;
import org.chromium.chrome.browser.password_manager.settings.PasswordSettings; import org.chromium.chrome.browser.password_manager.settings.PasswordSettings;
import org.chromium.chrome.browser.privacy.settings.PrivacySettings; import org.chromium.chrome.browser.privacy.settings.PrivacySettings;
import org.chromium.chrome.browser.safety_check.SafetyCheckSettingsFragment; import org.chromium.chrome.browser.safety_check.SafetyCheckSettingsFragment;
...@@ -58,6 +61,7 @@ import org.chromium.chrome.test.util.browser.sync.SyncTestUtil; ...@@ -58,6 +61,7 @@ import org.chromium.chrome.test.util.browser.sync.SyncTestUtil;
import org.chromium.components.browser_ui.site_settings.SiteSettings; import org.chromium.components.browser_ui.site_settings.SiteSettings;
import org.chromium.components.search_engines.TemplateUrl; import org.chromium.components.search_engines.TemplateUrl;
import org.chromium.components.search_engines.TemplateUrlService; import org.chromium.components.search_engines.TemplateUrlService;
import org.chromium.content_public.browser.test.util.CriteriaHelper;
import org.chromium.content_public.browser.test.util.TestThreadUtils; import org.chromium.content_public.browser.test.util.TestThreadUtils;
import java.io.IOException; import java.io.IOException;
...@@ -93,6 +97,9 @@ public class MainSettingsFragmentTest { ...@@ -93,6 +97,9 @@ public class MainSettingsFragmentTest {
@Mock @Mock
public TemplateUrl mMockSearchEngine; public TemplateUrl mMockSearchEngine;
@Mock
private PasswordCheck mPasswordCheck;
private @Nullable TemplateUrlService mActualTemplateUrlService; private @Nullable TemplateUrlService mActualTemplateUrlService;
private MainSettings mMainSettings; private MainSettings mMainSettings;
...@@ -100,6 +107,7 @@ public class MainSettingsFragmentTest { ...@@ -100,6 +107,7 @@ public class MainSettingsFragmentTest {
@Before @Before
public void setup() { public void setup() {
MockitoAnnotations.initMocks(this); MockitoAnnotations.initMocks(this);
PasswordCheckFactory.setPasswordCheckForTesting(mPasswordCheck);
DeveloperSettings.setIsEnabledForTests(true); DeveloperSettings.setIsEnabledForTests(true);
NightModeUtils.setNightModeSupportedForTesting(true); NightModeUtils.setNightModeSupportedForTesting(true);
} }
...@@ -365,6 +373,17 @@ public class MainSettingsFragmentTest { ...@@ -365,6 +373,17 @@ public class MainSettingsFragmentTest {
mMainSettings.findPreference(MainSettings.PREF_DEVELOPER)); mMainSettings.findPreference(MainSettings.PREF_DEVELOPER));
} }
@Test
@SmallTest
@EnableFeatures({ChromeFeatureList.PASSWORD_CHECK})
public void testDestroysPasswordCheck() {
launchSettingsActivity();
Activity activity = mMainSettings.getActivity();
activity.finish();
CriteriaHelper.pollInstrumentationThread(() -> activity.isDestroyed());
Assert.assertNull(PasswordCheckFactory.getPasswordCheckInstance());
}
/** /**
* Assert the target preference exists in the main settings and creates the expected fragment, * Assert the target preference exists in the main settings and creates the expected fragment,
* then return that preference. * then return that preference.
......
...@@ -39,6 +39,7 @@ android_library("public_java") { ...@@ -39,6 +39,7 @@ android_library("public_java") {
"java/src/org/chromium/chrome/browser/password_check/PasswordCheck.java", "java/src/org/chromium/chrome/browser/password_check/PasswordCheck.java",
"java/src/org/chromium/chrome/browser/password_check/PasswordCheckFragmentView.java", "java/src/org/chromium/chrome/browser/password_check/PasswordCheckFragmentView.java",
"java/src/org/chromium/chrome/browser/password_check/PasswordCheckPreference.java", "java/src/org/chromium/chrome/browser/password_check/PasswordCheckPreference.java",
"java/src/org/chromium/chrome/browser/password_check/PasswordCheckReferrer.java",
] ]
resources_package = "org.chromium.chrome.browser.password_check" resources_package = "org.chromium.chrome.browser.password_check"
} }
...@@ -72,7 +73,10 @@ junit_binary("password_check_junit_tests") { ...@@ -72,7 +73,10 @@ junit_binary("password_check_junit_tests") {
android_library("test_java") { android_library("test_java") {
testonly = true testonly = true
sources = [ "javatests/src/org/chromium/chrome/browser/password_check/PasswordCheckViewTest.java" ] sources = [
"javatests/src/org/chromium/chrome/browser/password_check/PasswordCheckIntegrationTest.java",
"javatests/src/org/chromium/chrome/browser/password_check/PasswordCheckViewTest.java",
]
deps = [ deps = [
":public_java", ":public_java",
...@@ -85,6 +89,7 @@ android_library("test_java") { ...@@ -85,6 +89,7 @@ android_library("test_java") {
"//chrome/android:chrome_test_java", "//chrome/android:chrome_test_java",
"//chrome/android:chrome_test_util_java", "//chrome/android:chrome_test_util_java",
"//chrome/browser/flags:java", "//chrome/browser/flags:java",
"//chrome/browser/password_check/android/internal:public_factory_java",
"//chrome/browser/settings:test_support_java", "//chrome/browser/settings:test_support_java",
"//chrome/test/android:chrome_java_test_support", "//chrome/test/android:chrome_java_test_support",
"//components/browser_ui/widget/android:java", "//components/browser_ui/widget/android:java",
...@@ -102,6 +107,7 @@ android_library("test_java") { ...@@ -102,6 +107,7 @@ android_library("test_java") {
"//ui/android:ui_utils_java", "//ui/android:ui_utils_java",
] ]
} }
android_resources("java_resources") { android_resources("java_resources") {
sources = [ sources = [
"java/res/drawable-night/password_check_neutral.xml", "java/res/drawable-night/password_check_neutral.xml",
......
...@@ -20,7 +20,12 @@ android_library_factory("public_factory_java") { ...@@ -20,7 +20,12 @@ android_library_factory("public_factory_java") {
} }
android_library("internal_factory_java") { android_library("internal_factory_java") {
deps = [ ":internal_java" ] deps = [
":internal_java",
"//chrome/browser/flags:java",
"//third_party/android_deps:androidx_annotation_annotation_java",
_public_target,
]
sources = _factory_sources sources = _factory_sources
} }
......
...@@ -11,7 +11,7 @@ import org.chromium.base.annotations.NativeMethods; ...@@ -11,7 +11,7 @@ import org.chromium.base.annotations.NativeMethods;
* messages to and from its C++ counterpart. * messages to and from its C++ counterpart.
*/ */
class PasswordCheckBridge { class PasswordCheckBridge {
private final long mNativePasswordCheckBridge; private long mNativePasswordCheckBridge;
private final PasswordCheckObserver mPasswordCheckObserver; private final PasswordCheckObserver mPasswordCheckObserver;
/** /**
...@@ -88,6 +88,16 @@ class PasswordCheckBridge { ...@@ -88,6 +88,16 @@ class PasswordCheckBridge {
mNativePasswordCheckBridge, credentials); mNativePasswordCheckBridge, credentials);
} }
/**
* Destroys its C++ counterpart.
*/
void destroy() {
if (mNativePasswordCheckBridge != 0) {
mNativePasswordCheckBridge = 0;
PasswordCheckBridgeJni.get().destroy(mNativePasswordCheckBridge);
}
}
/** /**
* C++ method signatures. * C++ method signatures.
*/ */
......
...@@ -49,7 +49,7 @@ class PasswordCheckCoordinator implements PasswordCheckComponentUi, LifecycleObs ...@@ -49,7 +49,7 @@ class PasswordCheckCoordinator implements PasswordCheckComponentUi, LifecycleObs
mModel = PasswordCheckProperties.createDefaultModel(); mModel = PasswordCheckProperties.createDefaultModel();
PasswordCheckMediator mediator = new PasswordCheckMediator(); PasswordCheckMediator mediator = new PasswordCheckMediator();
PasswordCheckCoordinator.setUpModelChangeProcessors(mModel, mFragmentView); PasswordCheckCoordinator.setUpModelChangeProcessors(mModel, mFragmentView);
mediator.initialize(mModel, PasswordCheckFactory.create()); mediator.initialize(mModel, PasswordCheckFactory.getOrCreate());
} }
} }
...@@ -66,6 +66,11 @@ class PasswordCheckCoordinator implements PasswordCheckComponentUi, LifecycleObs ...@@ -66,6 +66,11 @@ class PasswordCheckCoordinator implements PasswordCheckComponentUi, LifecycleObs
return false; return false;
} }
@Override
public void destroy() {
PasswordCheckFactory.destroy();
}
/** /**
* Connects the given model with the given view using Model Change Processors. * Connects the given model with the given view using Model Change Processors.
* @param model A {@link PropertyModel} built with {@link PasswordCheckProperties}. * @param model A {@link PropertyModel} built with {@link PasswordCheckProperties}.
......
...@@ -4,21 +4,52 @@ ...@@ -4,21 +4,52 @@
package org.chromium.chrome.browser.password_check; package org.chromium.chrome.browser.password_check;
import androidx.annotation.Nullable;
import androidx.annotation.VisibleForTesting;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
/** /**
* Use {@link #create()} to instantiate a {@link PasswordCheckImpl}. * Use {@link #getOrCreate()} to instantiate a {@link PasswordCheckImpl}
* and {@link #destroy()} when the instance is no longer needed.
*/ */
public class PasswordCheckFactory { public class PasswordCheckFactory {
private static PasswordCheck sPasswordCheck; private static PasswordCheck sPasswordCheck;
private PasswordCheckFactory() {} private PasswordCheckFactory() {}
/** /**
* Creates a {@link PasswordCheckImpl}. * Creates a {@link PasswordCheckImpl} if none exists. Otherwise it returns the existing
* @return A {@link PasswordCheckImpl}. * instance.
* @return A {@link PasswordCheckImpl} or null if the feature is disabled.
*/ */
public static PasswordCheck create() { public static @Nullable PasswordCheck getOrCreate() {
if (!ChromeFeatureList.isEnabled(ChromeFeatureList.PASSWORD_CHECK)) return null;
if (sPasswordCheck == null) { if (sPasswordCheck == null) {
sPasswordCheck = new PasswordCheckImpl(); sPasswordCheck = new PasswordCheckImpl();
} }
return sPasswordCheck; return sPasswordCheck;
} }
}
\ No newline at end of file /**
* Destroys the C++ layer to free up memory. In order to not leave a partially initialized
* feature component alive, it sets the {@link PasswordCheckImpl} to null.
*
* Should be called by the last object alive who needs the feature. This is, in general,
* the outermost settings screen. Note that this is not always {@link MainSettings}.
*/
public static void destroy() {
if (sPasswordCheck == null) return;
sPasswordCheck.destroy();
sPasswordCheck = null;
}
@VisibleForTesting
public static void setPasswordCheckForTesting(PasswordCheck passwordCheck) {
sPasswordCheck = passwordCheck;
}
@VisibleForTesting
public static PasswordCheck getPasswordCheckInstance() {
return sPasswordCheck;
}
}
...@@ -5,18 +5,44 @@ ...@@ -5,18 +5,44 @@
package org.chromium.chrome.browser.password_check; package org.chromium.chrome.browser.password_check;
import android.content.Context; import android.content.Context;
import android.os.Bundle;
import org.chromium.chrome.browser.password_check.PasswordCheckBridge.PasswordCheckObserver;
import org.chromium.chrome.browser.settings.SettingsLauncher; import org.chromium.chrome.browser.settings.SettingsLauncher;
import org.chromium.chrome.browser.settings.SettingsLauncherImpl; import org.chromium.chrome.browser.settings.SettingsLauncherImpl;
/** /**
* This class is responsible for managing the saved passwords check for signed-in users. * This class is responsible for managing the saved passwords check for signed-in users.
*/ */
class PasswordCheckImpl implements PasswordCheck { class PasswordCheckImpl implements PasswordCheck, PasswordCheckObserver {
private final PasswordCheckBridge mPasswordCheckBridge;
PasswordCheckImpl() {
mPasswordCheckBridge = new PasswordCheckBridge(this);
}
@Override @Override
public void showUi(Context context) { public void showUi(Context context, @PasswordCheckReferrer int passwordCheckReferrer) {
SettingsLauncher launcher = new SettingsLauncherImpl(); SettingsLauncher launcher = new SettingsLauncherImpl();
launcher.launchSettingsActivity(context, PasswordCheckFragmentView.class); Bundle fragmentArgs = new Bundle();
fragmentArgs.putInt(
PasswordCheckFragmentView.PASSWORD_CHECK_REFERRER, passwordCheckReferrer);
launcher.launchSettingsActivity(context, PasswordCheckFragmentView.class, fragmentArgs);
}
@Override
public void destroy() {
mPasswordCheckBridge.destroy();
}
@Override
public void onCompromisedCredentialFound(String originUrl, String username, String password) {
// TODO(crbug.com): Broadcast to registered observers.
}
@Override
public void onCompromisedCredentialsFetched(int count) {
// TODO(crbug.com): Broadcast to registered observers.
} }
@Override @Override
......
...@@ -15,6 +15,12 @@ public interface PasswordCheck extends PasswordCheckComponentUi.Delegate { ...@@ -15,6 +15,12 @@ public interface PasswordCheck extends PasswordCheckComponentUi.Delegate {
/** /**
* Initializes the PasswordCheck UI and launches it. * Initializes the PasswordCheck UI and launches it.
* @param context A {@link Context} to create views and retrieve resources. * @param context A {@link Context} to create views and retrieve resources.
* @param passwordCheckReferrer The place which launched the check UI.
*/ */
void showUi(Context context); void showUi(Context context, @PasswordCheckReferrer int passwordCheckReferrer);
/**
* Cleans up the C++ part, thus removing the compromised credentials from memory.
*/
void destroy();
} }
...@@ -26,4 +26,9 @@ interface PasswordCheckComponentUi { ...@@ -26,4 +26,9 @@ interface PasswordCheckComponentUi {
* @param item A {@link MenuItem}. * @param item A {@link MenuItem}.
*/ */
boolean handleHelp(MenuItem item); boolean handleHelp(MenuItem item);
/**
* Tears down the component when it's no longer needed.
*/
void destroy();
} }
...@@ -14,7 +14,13 @@ import androidx.preference.PreferenceFragmentCompat; ...@@ -14,7 +14,13 @@ import androidx.preference.PreferenceFragmentCompat;
* This class is responsible for rendering the check passwords view in the settings menu. * This class is responsible for rendering the check passwords view in the settings menu.
*/ */
public class PasswordCheckFragmentView extends PreferenceFragmentCompat { public class PasswordCheckFragmentView extends PreferenceFragmentCompat {
// Key for the argument with which the PasswordsCheck fragment will be launched. The value for
// this argument should be part of the PasswordCheckReferrer enum, which contains
// all points of entry to the password check UI.
public static final String PASSWORD_CHECK_REFERRER = "password-check-referrer";
private PasswordCheckComponentUi mComponentDelegate; private PasswordCheckComponentUi mComponentDelegate;
private @PasswordCheckReferrer int mPasswordCheckReferrer;
/** /**
* Set the delegate that handles view events which affect the state of the component. * Set the delegate that handles view events which affect the state of the component.
...@@ -28,6 +34,24 @@ public class PasswordCheckFragmentView extends PreferenceFragmentCompat { ...@@ -28,6 +34,24 @@ public class PasswordCheckFragmentView extends PreferenceFragmentCompat {
public void onCreatePreferences(Bundle savedInstanceState, String rootKey) { public void onCreatePreferences(Bundle savedInstanceState, String rootKey) {
getActivity().setTitle(R.string.passwords_check_title); getActivity().setTitle(R.string.passwords_check_title);
setPreferenceScreen(getPreferenceManager().createPreferenceScreen(getStyledContext())); setPreferenceScreen(getPreferenceManager().createPreferenceScreen(getStyledContext()));
Bundle extras = getArguments();
assert extras.containsKey(PASSWORD_CHECK_REFERRER)
: "PasswordCheckFragmentView"
+ "must be launched with a password-check-referrer fragment argument, but none was"
+ "provided.";
mPasswordCheckReferrer = extras.getInt(PASSWORD_CHECK_REFERRER);
}
@Override
public void onDestroy() {
super.onDestroy();
// The component should only be destroyed when the activity has been closed by the user
// (e.g. by pressing on the back button) and not when the activity is temporarily destroyed
// by the system.
if (getActivity().isFinishing()
&& mPasswordCheckReferrer == PasswordCheckReferrer.LEAK_DIALOG) {
mComponentDelegate.destroy();
}
} }
@Override @Override
......
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
package org.chromium.chrome.browser.password_check;
import androidx.annotation.IntDef;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
@IntDef({PasswordCheckReferrer.PASSWORD_SETTINGS, PasswordCheckReferrer.SAFETY_CHECK,
PasswordCheckReferrer.LEAK_DIALOG})
@Retention(RetentionPolicy.SOURCE)
public @interface PasswordCheckReferrer {
/**
* Corresponds to the Settings > Passwords page.
*/
int PASSWORD_SETTINGS = 0;
/**
* Corresponds to the safety check settings page.
*/
int SAFETY_CHECK = 1;
/**
* Represents the leak dialog prompted to the user when they sign in with a credential
* which was part of a data breach;
*/
int LEAK_DIALOG = 2;
}
// Copyright 2020 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
package org.chromium.chrome.browser.password_check;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertNull;
import android.os.Bundle;
import androidx.test.filters.MediumTest;
import org.junit.Before;
import org.junit.Rule;
import org.junit.Test;
import org.junit.rules.TestRule;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.MockitoAnnotations;
import org.chromium.base.test.util.CommandLineFlags;
import org.chromium.base.test.util.JniMocker;
import org.chromium.chrome.browser.flags.ChromeFeatureList;
import org.chromium.chrome.browser.flags.ChromeSwitches;
import org.chromium.chrome.browser.settings.SettingsActivity;
import org.chromium.chrome.browser.settings.SettingsActivityTestRule;
import org.chromium.chrome.test.ChromeJUnit4ClassRunner;
import org.chromium.chrome.test.util.browser.Features;
import org.chromium.chrome.test.util.browser.Features.EnableFeatures;
import org.chromium.content_public.browser.test.util.CriteriaHelper;
/**
* Integration test for the Password Check component, testing the interaction between sub-components
* of the password check feature as well as the creation and destruction of the component.
**/
@RunWith(ChromeJUnit4ClassRunner.class)
@CommandLineFlags.Add({ChromeSwitches.DISABLE_FIRST_RUN_EXPERIENCE})
@EnableFeatures({ChromeFeatureList.PASSWORD_CHECK})
public class PasswordCheckIntegrationTest {
@Rule
public final SettingsActivityTestRule<PasswordCheckFragmentView> mTestRule =
new SettingsActivityTestRule<>(PasswordCheckFragmentView.class);
@Rule
public TestRule mProcessor = new Features.JUnitProcessor();
@Rule
public final JniMocker mJniMocker = new JniMocker();
@Mock
private PasswordCheckBridge.Natives mPasswordCheckBridge;
@Before
public void setUp() {
MockitoAnnotations.initMocks(this);
mJniMocker.mock(PasswordCheckBridgeJni.TEST_HOOKS, mPasswordCheckBridge);
}
@Test
@MediumTest
public void testDestroysComponentIfFirstInSettingsStack() {
PasswordCheckFactory.getOrCreate();
SettingsActivity activity = setUpUiLaunchedFromDialog();
activity.finish();
CriteriaHelper.pollInstrumentationThread(() -> activity.isDestroyed());
assertNull(PasswordCheckFactory.getPasswordCheckInstance());
}
@Test
@MediumTest
public void testDoesNotDestroyComponentIfNotFirstInSettingsStack() {
PasswordCheckFactory.getOrCreate();
SettingsActivity activity = setUpUiLaunchedFromSettings();
activity.finish();
CriteriaHelper.pollInstrumentationThread(() -> activity.isDestroyed());
assertNotNull(PasswordCheckFactory.getPasswordCheckInstance());
// Clean up the password check component.
PasswordCheckFactory.destroy();
}
private SettingsActivity setUpUiLaunchedFromSettings() {
Bundle fragmentArgs = new Bundle();
fragmentArgs.putInt(PasswordCheckFragmentView.PASSWORD_CHECK_REFERRER,
PasswordCheckReferrer.PASSWORD_SETTINGS);
SettingsActivity activity = mTestRule.startSettingsActivity(fragmentArgs);
return activity;
}
private SettingsActivity setUpUiLaunchedFromDialog() {
Bundle fragmentArgs = new Bundle();
fragmentArgs.putInt(PasswordCheckFragmentView.PASSWORD_CHECK_REFERRER,
PasswordCheckReferrer.LEAK_DIALOG);
SettingsActivity activity = mTestRule.startSettingsActivity(fragmentArgs);
return activity;
}
}
\ No newline at end of file
...@@ -23,6 +23,7 @@ import static org.chromium.chrome.browser.password_check.PasswordCheckProperties ...@@ -23,6 +23,7 @@ import static org.chromium.chrome.browser.password_check.PasswordCheckProperties
import static org.chromium.content_public.browser.test.util.CriteriaHelper.pollUiThread; import static org.chromium.content_public.browser.test.util.CriteriaHelper.pollUiThread;
import static org.chromium.content_public.browser.test.util.TestThreadUtils.runOnUiThreadBlocking; import static org.chromium.content_public.browser.test.util.TestThreadUtils.runOnUiThreadBlocking;
import android.os.Bundle;
import android.view.View; import android.view.View;
import android.widget.ImageButton; import android.widget.ImageButton;
import android.widget.TextView; import android.widget.TextView;
...@@ -67,6 +68,7 @@ public class PasswordCheckViewTest { ...@@ -67,6 +68,7 @@ public class PasswordCheckViewTest {
private PropertyModel mModel = PasswordCheckProperties.createDefaultModel(); private PropertyModel mModel = PasswordCheckProperties.createDefaultModel();
private PasswordCheckFragmentView mPasswordCheckView; private PasswordCheckFragmentView mPasswordCheckView;
@Mock @Mock
private PasswordCheckComponentUi mComponentUi; private PasswordCheckComponentUi mComponentUi;
@Mock @Mock
...@@ -84,7 +86,7 @@ public class PasswordCheckViewTest { ...@@ -84,7 +86,7 @@ public class PasswordCheckViewTest {
mPasswordCheckView.setComponentDelegate(mComponentUi); mPasswordCheckView.setComponentDelegate(mComponentUi);
return mComponentUi; return mComponentUi;
}); });
mTestRule.startSettingsActivity(); setUpUiLaunchedFromSettings();
runOnUiThreadBlocking(() -> { runOnUiThreadBlocking(() -> {
PasswordCheckCoordinator.setUpModelChangeProcessors(mModel, mPasswordCheckView); PasswordCheckCoordinator.setUpModelChangeProcessors(mModel, mPasswordCheckView);
}); });
...@@ -182,6 +184,13 @@ public class PasswordCheckViewTest { ...@@ -182,6 +184,13 @@ public class PasswordCheckViewTest {
.build()); .build());
} }
private void setUpUiLaunchedFromSettings() {
Bundle fragmentArgs = new Bundle();
fragmentArgs.putInt(PasswordCheckFragmentView.PASSWORD_CHECK_REFERRER,
PasswordCheckReferrer.PASSWORD_SETTINGS);
mTestRule.startSettingsActivity(fragmentArgs);
}
private View getStatus() { private View getStatus() {
return mPasswordCheckView.getListView().getChildAt(0); return mPasswordCheckView.getListView().getChildAt(0);
} }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment