[Android, Crash] Prevent Accessory trying to send data too early

FHorschig · Commit Bot · commit f178cf1047d0 · 2018-07-24T12:47:52.000Z
There is a nullptr exception connected to SendViewItems. This CL guesses a solution to this problem: 1. It's only called if the controller was checked to be null, so it must be in the implementation. 2. The view_ is synchronously constructed at construction time and can outside of tests never be null. 3. It's bound to web_contents_, so this also cannot be null. The only dereference left is the GetFocusedFrame() method which can be null. Therefore, accessing the correct frame moves to the driver-side which is much more appropriate for frame-based actions anyway. Bug: 865447 Change-Id: Ie5ea7aed980cdf53e784e5d918a9d54717ad6ab1 Reviewed-on: https://chromium-review.googlesource.com/1143477 Reviewed-by: Vasilii Sukhanov <vasilii@chromium.org> Commit-Queue: Friedrich Horschig <fhorschig@chromium.org> Cr-Commit-Position: refs/heads/master@{#577496}
diff --git a/chrome/browser/password_manager/chrome_password_manager_client.cc b/chrome/browser/password_manager/chrome_password_manager_client.cc
@@ -1084,7 +1084,10 @@ void ChromePasswordManagerClient::FocusedInputChanged(bool is_fillable,
       PasswordAccessoryController::FromWebContents(web_contents());
   if (!accessory)
     return;  // No accessory needs change here.
-  accessory->RefreshSuggestionsForField(is_fillable, is_password_field);
+  accessory->RefreshSuggestionsForField(
+      password_manager_driver_bindings_.GetCurrentTargetFrame()
+          ->GetLastCommittedOrigin(),
+      is_fillable, is_password_field);
 #endif  // defined(OS_ANDROID)
 }
 
diff --git a/chrome/browser/password_manager/password_accessory_controller.cc b/chrome/browser/password_manager/password_accessory_controller.cc
@@ -8,7 +8,6 @@
 
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/android/preferences/preferences_launcher.h"
-#include "chrome/browser/password_manager/password_accessory_view_interface.h"
 #include "chrome/browser/password_manager/password_generation_dialog_view_interface.h"
 #include "chrome/browser/ui/passwords/manage_passwords_view_utils.h"
 #include "chrome/grit/generated_resources.h"
@@ -22,8 +21,6 @@
 #include "content/public/browser/web_contents.h"
 #include "ui/base/l10n/l10n_util.h"
 
-#include "chrome/browser/android/preferences/preferences_launcher.h"
-
 using autofill::PasswordForm;
 using Item = PasswordAccessoryViewInterface::AccessoryItem;
 
@@ -210,10 +207,15 @@ void PasswordAccessoryController::OnFilledIntoFocusedField(
 }
 
 void PasswordAccessoryController::RefreshSuggestionsForField(
+    const url::Origin& origin,
     bool is_fillable,
     bool is_password_field) {
   // TODO(crbug/853766): Record CTR metric.
-  SendViewItems(is_password_field);
+  view_->OnItemsAvailable(
+      CreateViewItems(origin,
+                      is_fillable ? origin_suggestions_[origin]
+                                  : std::vector<SuggestionElementData>(),
+                      is_password_field));
 }
 
 gfx::NativeView PasswordAccessoryController::container_view() const {
@@ -224,13 +226,11 @@ gfx::NativeWindow PasswordAccessoryController::native_window() const {
   return web_contents_->GetTopLevelNativeWindow();
 }
 
-void PasswordAccessoryController::SendViewItems(bool is_password_field) {
-  DCHECK(view_);
-  last_focused_field_was_for_passwords_ = is_password_field;
-  const url::Origin& origin =
-      web_contents_->GetFocusedFrame()->GetLastCommittedOrigin();
-  const std::vector<SuggestionElementData>& suggestions =
-      origin_suggestions_[origin];
+// static
+std::vector<Item> PasswordAccessoryController::CreateViewItems(
+    const url::Origin& origin,
+    const std::vector<SuggestionElementData>& suggestions,
+    bool is_password_field) {
   std::vector<Item> items;
   base::string16 passwords_title_str;
 
@@ -266,7 +266,5 @@ void PasswordAccessoryController::SendViewItems(bool is_password_field) {
       IDS_PASSWORD_MANAGER_ACCESSORY_ALL_PASSWORDS_LINK);
   items.emplace_back(manage_passwords_title, manage_passwords_title, false,
                      Item::Type::OPTION);
-
-  // Notify the view about all just created elements.
-  view_->OnItemsAvailable(items);
+  return items;
 }
diff --git a/chrome/browser/password_manager/password_accessory_controller.h b/chrome/browser/password_manager/password_accessory_controller.h
@@ -14,6 +14,7 @@
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
 #include "base/strings/string16.h"
+#include "chrome/browser/password_manager/password_accessory_view_interface.h"
 #include "components/autofill/core/common/filling_status.h"
 #include "components/autofill/core/common/password_generation_util.h"
 #include "content/public/browser/web_contents_user_data.h"
@@ -28,7 +29,6 @@ namespace password_manager {
 class PasswordManagerDriver;
 }  // namespace password_manager
 
-class PasswordAccessoryViewInterface;
 class PasswordGenerationDialogViewInterface;
 
 // The controller for the view located below the keyboard accessory.
@@ -91,8 +91,11 @@ class PasswordAccessoryController
   void OnFilledIntoFocusedField(autofill::FillingStatus status);
 
   // Makes sure, that all shown suggestions are appropriate for the currently
-  // focused field.
-  void RefreshSuggestionsForField(bool is_fillable, bool is_password_field);
+  // focused field and for fields that lost the focus. If a field lost focus,
+  // |is_fillable| will be false.
+  void RefreshSuggestionsForField(const url::Origin& origin,
+                                  bool is_fillable,
+                                  bool is_password_field);
 
   // The web page view containing the focused field.
   gfx::NativeView container_view() const;
@@ -129,10 +132,12 @@ class PasswordAccessoryController
       std::unique_ptr<PasswordAccessoryViewInterface> view,
       CreateDialogFactory create_dialog_callback);
 
-  // Creates the view items based on the |origin_suggestions_| and sends them to
-  // the view. If |is_password_field| is false, password suggestions won't be
-  // interactive.
-  void SendViewItems(bool is_password_field);
+  // Creates the view items based on the given |suggestions|.
+  // If |is_password_field| is false, password suggestions won't be interactive.
+  static std::vector<PasswordAccessoryViewInterface::AccessoryItem>
+  CreateViewItems(const url::Origin& origin,
+                  const std::vector<SuggestionElementData>& suggestions,
+                  bool is_password_field);
 
   // Contains the last set of credentials by origin.
   std::map<url::Origin, std::vector<SuggestionElementData>> origin_suggestions_;
diff --git a/chrome/browser/password_manager/password_accessory_controller_unittest.cc b/chrome/browser/password_manager/password_accessory_controller_unittest.cc
@@ -245,7 +245,6 @@ class PasswordAccessoryControllerTest : public ChromeRenderViewHostTestHarness {
         std::make_unique<StrictMock<MockPasswordAccessoryView>>(),
         mock_dialog_factory_.Get());
     NavigateAndCommit(GURL("https://example.com"));
-    FocusWebContentsOnMainFrame();
   }
 
   PasswordAccessoryController* controller() {
@@ -287,8 +286,10 @@ TEST_F(PasswordAccessoryControllerTest, TransformsMatchesToSuggestions) {
 
   controller()->SavePasswordsForOrigin({CreateEntry("Ben", "S3cur3").first},
                                        url::Origin::Create(GURL(kExampleSite)));
-  controller()->RefreshSuggestionsForField(/*is_fillable=*/true,
-                                           /*is_password_field=*/false);
+  controller()->RefreshSuggestionsForField(
+      url::Origin::Create(GURL(kExampleSite)),
+      /*is_fillable=*/true,
+      /*is_password_field=*/false);
 }
 
 TEST_F(PasswordAccessoryControllerTest, HintsToEmptyUserNames) {
@@ -304,8 +305,10 @@ TEST_F(PasswordAccessoryControllerTest, HintsToEmptyUserNames) {
 
   controller()->SavePasswordsForOrigin({CreateEntry("", "S3cur3").first},
                                        url::Origin::Create(GURL(kExampleSite)));
-  controller()->RefreshSuggestionsForField(/*is_fillable=*/true,
-                                           /*is_password_field=*/false);
+  controller()->RefreshSuggestionsForField(
+      url::Origin::Create(GURL(kExampleSite)),
+      /*is_fillable=*/true,
+      /*is_password_field=*/false);
 }
 
 TEST_F(PasswordAccessoryControllerTest, SortsAlphabeticalDuringTransform) {
@@ -338,8 +341,10 @@ TEST_F(PasswordAccessoryControllerTest, SortsAlphabeticalDuringTransform) {
       {CreateEntry("Ben", "S3cur3").first, CreateEntry("Zebra", "M3h").first,
        CreateEntry("Alf", "PWD").first, CreateEntry("Cat", "M1@u").first},
       url::Origin::Create(GURL(kExampleSite)));
-  controller()->RefreshSuggestionsForField(/*is_fillable=*/true,
-                                           /*is_password_field=*/false);
+  controller()->RefreshSuggestionsForField(
+      url::Origin::Create(GURL(kExampleSite)),
+      /*is_fillable=*/true,
+      /*is_password_field=*/false);
 }
 
 TEST_F(PasswordAccessoryControllerTest, RepeatsSuggestionsForSameFrame) {
@@ -357,8 +362,10 @@ TEST_F(PasswordAccessoryControllerTest, RepeatsSuggestionsForSameFrame) {
                                        url::Origin::Create(GURL(kExampleSite)));
 
   // Pretend that any input in the same frame was focused.
-  controller()->RefreshSuggestionsForField(/*is_fillable=*/true,
-                                           /*is_fillable=*/false);
+  controller()->RefreshSuggestionsForField(
+      url::Origin::Create(GURL(kExampleSite)),
+      /*is_fillable=*/true,
+      /*is_fillable=*/false);
 }
 
 TEST_F(PasswordAccessoryControllerTest, ProvidesEmptySuggestionsMessage) {
@@ -370,8 +377,10 @@ TEST_F(PasswordAccessoryControllerTest, ProvidesEmptySuggestionsMessage) {
 
   controller()->SavePasswordsForOrigin({},
                                        url::Origin::Create(GURL(kExampleSite)));
-  controller()->RefreshSuggestionsForField(/*is_fillable=*/true,
-                                           /*is_password_field=*/false);
+  controller()->RefreshSuggestionsForField(
+      url::Origin::Create(GURL(kExampleSite)),
+      /*is_fillable=*/true,
+      /*is_password_field=*/false);
 }
 
 TEST_F(PasswordAccessoryControllerTest, RelaysAutomaticGenerationAvailable) {
@@ -439,8 +448,10 @@ TEST_F(PasswordAccessoryControllerTest, PasswordFieldChangesSuggestionType) {
   // This should result in the non-interactive suggestion expected above.
   controller()->SavePasswordsForOrigin({CreateEntry("Ben", "S3cur3").first},
                                        url::Origin::Create(GURL(kExampleSite)));
-  controller()->RefreshSuggestionsForField(/*is_fillable=*/true,
-                                           /*is_password_field=*/false);
+  controller()->RefreshSuggestionsForField(
+      url::Origin::Create(GURL(kExampleSite)),
+      /*is_fillable=*/true,
+      /*is_password_field=*/false);
 
   // Pretend that we focus a password field now: By triggering a refresh with
   // |is_password_field| set to true, all suggestions should become interactive.
@@ -452,8 +463,10 @@ TEST_F(PasswordAccessoryControllerTest, PasswordFieldChangesSuggestionType) {
                   MatchesItem(ASCIIToUTF16("S3cur3"), password_for_str("Ben"),
                               true, ItemType::SUGGESTION),
                   IsDivider(), MatchesOption(manage_passwords_str()))));
-  controller()->RefreshSuggestionsForField(/*is_fillable=*/true,
-                                           /*is_password_field=*/true);
+  controller()->RefreshSuggestionsForField(
+      url::Origin::Create(GURL(kExampleSite)),
+      /*is_fillable=*/true,
+      /*is_password_field=*/true);
 }
 
 TEST_F(PasswordAccessoryControllerTest, CachesIsReplacedByNewPasswords) {
@@ -467,8 +480,10 @@ TEST_F(PasswordAccessoryControllerTest, CachesIsReplacedByNewPasswords) {
                   IsDivider(), MatchesOption(manage_passwords_str()))));
   controller()->SavePasswordsForOrigin({CreateEntry("Ben", "S3cur3").first},
                                        url::Origin::Create(GURL(kExampleSite)));
-  controller()->RefreshSuggestionsForField(/*is_fillable=*/true,
-                                           /*is_password_field=*/false);
+  controller()->RefreshSuggestionsForField(
+      url::Origin::Create(GURL(kExampleSite)),
+      /*is_fillable=*/true,
+      /*is_password_field=*/false);
 
   EXPECT_CALL(*view(),
               OnItemsAvailable(ElementsAre(
@@ -480,6 +495,38 @@ TEST_F(PasswordAccessoryControllerTest, CachesIsReplacedByNewPasswords) {
                   IsDivider(), MatchesOption(manage_passwords_str()))));
   controller()->SavePasswordsForOrigin({CreateEntry("Alf", "M3lm4k").first},
                                        url::Origin::Create(GURL(kExampleSite)));
-  controller()->RefreshSuggestionsForField(/*is_fillable=*/true,
-                                           /*is_password_field=*/false);
+  controller()->RefreshSuggestionsForField(
+      url::Origin::Create(GURL(kExampleSite)),
+      /*is_fillable=*/true,
+      /*is_password_field=*/false);
+}
+
+TEST_F(PasswordAccessoryControllerTest, UnfillableFieldClearsSuggestions) {
+  EXPECT_CALL(*view(),
+              OnItemsAvailable(ElementsAre(
+                  MatchesLabel(passwords_title_str(kExampleDomain)),
+                  MatchesItem(ASCIIToUTF16("Ben"), ASCIIToUTF16("Ben"), false,
+                              ItemType::SUGGESTION),
+                  MatchesItem(ASCIIToUTF16("S3cur3"), password_for_str("Ben"),
+                              true, ItemType::NON_INTERACTIVE_SUGGESTION),
+                  IsDivider(), MatchesOption(manage_passwords_str()))));
+  // Set any, non-empty password list and pretend a username field was focused.
+  // This should result in the non-emtpy suggestions expected above.
+  controller()->SavePasswordsForOrigin({CreateEntry("Ben", "S3cur3").first},
+                                       url::Origin::Create(GURL(kExampleSite)));
+  controller()->RefreshSuggestionsForField(
+      url::Origin::Create(GURL(kExampleSite)),
+      /*is_fillable=*/true,
+      /*is_password_field=*/false);
+
+  // Pretend that the focus was lost or moved to an unfillable field. Now, only
+  // the empty state message should be sent.
+  EXPECT_CALL(*view(),
+              OnItemsAvailable(ElementsAre(
+                  MatchesLabel(passwords_empty_str(kExampleDomain)),
+                  IsDivider(), MatchesOption(manage_passwords_str()))));
+  controller()->RefreshSuggestionsForField(
+      url::Origin::Create(GURL(kExampleSite)),
+      /*is_fillable=*/false,
+      /*is_password_field=*/false);  // Unused.
 }
