[Messages] Dismiss previous message when new save password message is triggered

This CL addresses a crash when the user submits a password form while
Save Password message is on the screen. The problem is that the previous
message does not get dismissed.

This CL adds a call to dismiss a message before showing the new one. It
ensures that the dismiss callback is called synchronously on the native
side. It also ensures that dismiss callback doesn't get called again
from java when the message is hidden from the screen.

BUG=1153927
R=twellington@chromium.org

Change-Id: I2dd48e501d1eb3eb859784c429eaa6cbc5f8d610
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2583113Reviewed-by: Theresa  <twellington@chromium.org>
Commit-Queue: Pavel Yatsuk <pavely@chromium.org>
Cr-Commit-Position: refs/heads/master@{#835715}

chrome/browser/password_manager/android/save_password_message_delegate.cc

@@ -26,6 +26,11 @@ void SavePasswordMessageDelegate::DisplaySavePasswordPrompt(
     std::unique_ptr<password_manager::PasswordFormManagerForUI> form_to_save) {
   DCHECK_NE(nullptr, web_contents);
   DCHECK(form_to_save);
+
+  // Dismiss previous message if it is displayed.
+  DismissSavePasswordPrompt();
+  DCHECK(message_ == nullptr);
+
   // is_saving_google_account indicates whether the user is syncing
   // passwords to their Google Account.
   const bool is_saving_google_account =
@@ -39,12 +44,14 @@ void SavePasswordMessageDelegate::DisplaySavePasswordPrompt(
   CreateMessage(web_contents, std::move(form_to_save),
                 is_saving_google_account);
 
-  messages::MessageDispatcherBridge::EnqueueMessage(*message_, web_contents_);
+  messages::MessageDispatcherBridge::EnqueueMessage(message_.get(),
+                                                    web_contents_);
 }
 
 void SavePasswordMessageDelegate::DismissSavePasswordPrompt() {
   if (message_ != nullptr) {
-    messages::MessageDispatcherBridge::DismissMessage(*message_, web_contents_);
+    messages::MessageDispatcherBridge::DismissMessage(message_.get(),
+                                                      web_contents_);
   }
 }
 

chrome/browser/password_manager/android/save_password_message_delegate_unittest.cc

@@ -4,6 +4,7 @@
 
 #include "chrome/browser/password_manager/android/save_password_message_delegate.h"
 
+#include "base/android/jni_android.h"
 #include "base/macros.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/strings/utf_string_conversions.h"
@@ -105,11 +106,12 @@ void SavePasswordMessageDelegateTest::CreateMessage(
 }
 
 void SavePasswordMessageDelegateTest::TriggerActionClick() {
-  GetMessageWrapper()->HandleActionClick(nullptr);
+  GetMessageWrapper()->HandleActionClick(base::android::AttachCurrentThread());
 }
 
 void SavePasswordMessageDelegateTest::TriggerMessageDismissedCallback() {
-  GetMessageWrapper()->HandleDismissCallback(nullptr);
+  GetMessageWrapper()->HandleDismissCallback(
+      base::android::AttachCurrentThread());
   EXPECT_EQ(nullptr, GetMessageWrapper());
   metrics_recorder_.reset();
 }

components/messages/android/java/src/org/chromium/components/messages/MessageWrapper.java

@@ -94,6 +94,9 @@ public final class MessageWrapper {
     }
 
     private void handleMessageDismissed() {
+        // mNativeMessageWrapper can be null if the message was dismissed from native API.
+        // In this case dismiss callback should have already been called.
+        if (mNativeMessageWrapper == 0) return;
         MessageWrapperJni.get().handleDismissCallback(mNativeMessageWrapper);
     }
 

components/messages/android/message_dispatcher_bridge.cc

@@ -7,6 +7,7 @@
 #include <jni.h>
 
 #include "base/android/jni_android.h"
+#include "base/android/scoped_java_ref.h"
 #include "components/messages/android/jni_headers/MessageDispatcherBridge_jni.h"
 #include "content/public/browser/web_contents.h"
 
@@ -14,20 +15,24 @@ namespace messages {
 
 // static
 void MessageDispatcherBridge::EnqueueMessage(
-    const MessageWrapper& message,
+    MessageWrapper* message,
     content::WebContents* web_contents) {
   JNIEnv* env = base::android::AttachCurrentThread();
   Java_MessageDispatcherBridge_enqueueMessage(
-      env, message.GetJavaMessageWrapper(), web_contents->GetJavaWebContents());
+      env, message->GetJavaMessageWrapper(),
+      web_contents->GetJavaWebContents());
 }
 
 // static
 void MessageDispatcherBridge::DismissMessage(
-    const MessageWrapper& message,
+    MessageWrapper* message,
     content::WebContents* web_contents) {
+  base::android::ScopedJavaLocalRef<jobject> jmessage(
+      message->GetJavaMessageWrapper());
   JNIEnv* env = base::android::AttachCurrentThread();
+  message->HandleDismissCallback(env);
   Java_MessageDispatcherBridge_dismissMessage(
-      env, message.GetJavaMessageWrapper(), web_contents->GetJavaWebContents());
+      env, jmessage, web_contents->GetJavaWebContents());
 }
 
 }  // namespace messages
\ No newline at end of file

components/messages/android/message_dispatcher_bridge.h

@@ -17,9 +17,9 @@ namespace messages {
 // enqueue/dismiss messages with MessageDispatcher.java.
 class MessageDispatcherBridge {
  public:
-  static void EnqueueMessage(const MessageWrapper& message,
+  static void EnqueueMessage(MessageWrapper* message,
                              content::WebContents* web_contents);
-  static void DismissMessage(const MessageWrapper& message,
+  static void DismissMessage(MessageWrapper* message,
                              content::WebContents* web_contents);
 };
 

components/messages/android/message_wrapper.cc

@@ -22,8 +22,6 @@ MessageWrapper::MessageWrapper(base::OnceClosure action_callback,
 }
 
 MessageWrapper::~MessageWrapper() {
-  JNIEnv* env = base::android::AttachCurrentThread();
-  Java_MessageWrapper_clearNativePtr(env, java_message_wrapper_);
   CHECK(message_dismissed_);
 }
 
@@ -93,7 +91,10 @@ void MessageWrapper::HandleActionClick(JNIEnv* env) {
 }
 
 void MessageWrapper::HandleDismissCallback(JNIEnv* env) {
+  // Make sure message dismissed callback is called exactly once.
+  CHECK(!message_dismissed_);
   message_dismissed_ = true;
+  Java_MessageWrapper_clearNativePtr(env, java_message_wrapper_);
   if (!dismiss_callback_.is_null())
     std::move(dismiss_callback_).Run();
   // Dismiss callback typically deletes the instance of MessageWrapper,