Switch components/password_manager code from IPC messages to Mojo.

components/password_manager/content/browser/credential_manager_impl.cc

Index: components/password_manager/content/browser/credential_manager_impl.cc
diff --git a/components/password_manager/content/browser/credential_manager_dispatcher.cc b/components/password_manager/content/browser/credential_manager_impl.cc
similarity index 58%
rename from components/password_manager/content/browser/credential_manager_dispatcher.cc
rename to components/password_manager/content/browser/credential_manager_impl.cc
index ecf9ddff8d221e0b29b7951f62671e7d5f0568c4..a42e7bf5b641f481399b5d69a1a5b093c336def1 100644
--- a/components/password_manager/content/browser/credential_manager_dispatcher.cc
+++ b/components/password_manager/content/browser/credential_manager_impl.cc
@@ -2,7 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#include "components/password_manager/content/browser/credential_manager_dispatcher.h"
+#include "components/password_manager/content/browser/credential_manager_impl.h"
 
 #include <utility>
 
@@ -12,67 +12,64 @@
 #include "components/autofill/core/common/password_form.h"
 #include "components/password_manager/content/browser/content_password_manager_driver.h"
 #include "components/password_manager/content/browser/content_password_manager_driver_factory.h"
-#include "components/password_manager/content/common/credential_manager_messages.h"
+#include "components/password_manager/content/public/type_converters.h"
 #include "components/password_manager/core/browser/affiliated_match_helper.h"
 #include "components/password_manager/core/browser/password_manager_client.h"
 #include "components/password_manager/core/browser/password_store.h"
 #include "components/password_manager/core/common/credential_manager_types.h"
 #include "components/password_manager/core/common/password_manager_pref_names.h"
-#include "content/public/browser/render_view_host.h"
 #include "content/public/browser/web_contents.h"
-#include "ipc/ipc_message_macros.h"
+#include "mojo/common/url_type_converters.h"
 
 namespace password_manager {
 
-// CredentialManagerDispatcher -------------------------------------------------
+namespace {
 
-CredentialManagerDispatcher::CredentialManagerDispatcher(
-    content::WebContents* web_contents,
-    PasswordManagerClient* client)
+void RunMojoGetCallback(const mojom::CredentialManager::GetCallback& callback,
+                        const CredentialInfo& info) {
+  mojom::CredentialInfoPtr credential = mojom::CredentialInfo::From(info);
+  callback.Run(mojom::CredentialManagerError::SUCCESS, std::move(credential));
+}
+
+}  // namespace
+
+// CredentialManagerImpl -------------------------------------------------
+
+CredentialManagerImpl::CredentialManagerImpl(content::WebContents* web_contents,
+                                             PasswordManagerClient* client)
     : WebContentsObserver(web_contents), client_(client), weak_factory_(this) {
   DCHECK(web_contents);
   auto_signin_enabled_.Init(prefs::kCredentialsEnableAutosignin,
                             client_->GetPrefs());
 }
 
-CredentialManagerDispatcher::~CredentialManagerDispatcher() {
-}
+CredentialManagerImpl::~CredentialManagerImpl() {}
 
-bool CredentialManagerDispatcher::OnMessageReceived(
-    const IPC::Message& message) {
-  bool handled = true;
-  IPC_BEGIN_MESSAGE_MAP(CredentialManagerDispatcher, message)
-    IPC_MESSAGE_HANDLER(CredentialManagerHostMsg_Store, OnStore);
-    IPC_MESSAGE_HANDLER(CredentialManagerHostMsg_RequireUserMediation,
-                        OnRequireUserMediation);
-    IPC_MESSAGE_HANDLER(CredentialManagerHostMsg_RequestCredential,
-                        OnRequestCredential);
-    IPC_MESSAGE_UNHANDLED(handled = false)
-  IPC_END_MESSAGE_MAP()
-  return handled;
+void CredentialManagerImpl::BindRequest(
+    mojom::CredentialManagerRequest request) {
+  bindings_.AddBinding(this, std::move(request));
 }
 
-void CredentialManagerDispatcher::OnStore(
-    int request_id,
-    const password_manager::CredentialInfo& credential) {
-  DCHECK(credential.type != CredentialType::CREDENTIAL_TYPE_EMPTY);
-  DCHECK(request_id);
-  web_contents()->GetRenderViewHost()->Send(
-      new CredentialManagerMsg_AcknowledgeStore(
-          web_contents()->GetRenderViewHost()->GetRoutingID(), request_id));
+void CredentialManagerImpl::Store(mojom::CredentialInfoPtr credential,
+                                  const StoreCallback& callback) {
+  CredentialInfo info = credential.To<CredentialInfo>();
+  DCHECK_NE(CredentialType::CREDENTIAL_TYPE_EMPTY, info.type);
+
+  // Send acknowledge response back.
+  callback.Run();
 
   if (!client_->IsSavingAndFillingEnabledForCurrentPage())
     return;
 
   scoped_ptr<autofill::PasswordForm> form(CreatePasswordFormFromCredentialInfo(
-      credential, web_contents()->GetLastCommittedURL().GetOrigin()));
+      info, web_contents()->GetLastCommittedURL().GetOrigin()));
   form->skip_zero_click = !IsZeroClickAllowed();
 
   form_manager_.reset(new CredentialManagerPasswordFormManager(
       client_, GetDriver(), *form, this));
 }
 
-void CredentialManagerDispatcher::OnProvisionalSaveComplete() {
+void CredentialManagerImpl::OnProvisionalSaveComplete() {
   DCHECK(form_manager_);
   DCHECK(client_->IsSavingAndFillingEnabledForCurrentPage());
 
@@ -95,30 +92,28 @@ void CredentialManagerDispatcher::OnProvisionalSaveComplete() {
   }
 }
 
-void CredentialManagerDispatcher::OnRequireUserMediation(int request_id) {
-  DCHECK(request_id);
-
+void CredentialManagerImpl::RequireUserMediation(
+    const RequireUserMediationCallback& callback) {
   PasswordStore* store = GetPasswordStore();
   if (!store || !IsUpdatingCredentialAllowed()) {
-    web_contents()->GetRenderViewHost()->Send(
-        new CredentialManagerMsg_AcknowledgeRequireUserMediation(
-            web_contents()->GetRenderViewHost()->GetRoutingID(), request_id));
+    // Send acknowledge response back.

[Ken Rockot(use gerrit already), 2016/03/17 17:28:21]

nit: unnecessary comment

[Anand Mistry (off Chromium), 2016/03/18 01:24:12]

I should point out there are a few of these. You only need to comment
exceptional cases that aren't clear.

[leonhsl(Using Gerrit), 2016/03/18 10:02:16]

Looked through all added comments and removed unnecessary ones.

[leonhsl(Using Gerrit), 2016/03/18 10:02:16]

Done.

+    callback.Run();
     return;
   }
 
   if (store->affiliated_match_helper()) {
     store->affiliated_match_helper()->GetAffiliatedAndroidRealms(
         GetSynthesizedFormForOrigin(),
-        base::Bind(&CredentialManagerDispatcher::ScheduleRequireMediationTask,
-                   weak_factory_.GetWeakPtr(), request_id));
+        base::Bind(&CredentialManagerImpl::ScheduleRequireMediationTask,

[Ken Rockot(use gerrit already), 2016/03/17 17:28:21]

It looks like this callback will be passed to a background thread while work is
being done, and this callback in turn owns a copy of the Mojo response callback.
It is in general not safe to pass Mojo response callbacks to other threads
because they may be destroyed there where they're not allowed to be destroyed.

This is a subtle issue that we need to address at the bindings layer, but we
don't have a solution yet.

I think it may be acceptable to leave this as-is, if and only if
CredentialManagerImpl always lives on the main thread. Otherwise we'd need to do
some unfortunate callback tracking in this class.

I will let Sam or Anand weigh in on this opinion. :)

[Anand Mistry (off Chromium), 2016/03/18 01:24:13]

It looks like this is an issue. Unfortunately, the problem is so rare in
practice that it almost never gets hit (but I have seen it and know it exists).

But, we're actively working on this problem and have solutions in mind. So
you'll need to wait a few more days.

[leonhsl(Using Gerrit), 2016/03/18 10:34:45]

Hi, Ken, Anand, I understand that Mojo is thread hostile and the Mojo response
callback should never run or be destroyed on any other thread except the binding
thread. But, I checked around all CredentialManagerImpl related codes and I
think the current CL is safe.
It is true that here the Mojo response callback is bound as a parameter to the
CredentialManagerImpl::ScheduleRequireMediationTask callback, which is passed to
background thread, but, after several passing around, eventually the
CredentialManagerImpl::ScheduleRequireMediationTask callback will be
*definitely* postback to current thread to execute, even FacetManager got
destroyed accidentally. # FacetManager::~FacetManager() will do the postback.
And, even CrendentialManagerImpl instance destroyed firstly, the callback will
still be postback to current thread, it will notice the null
CredentialManagerImpl weak pointer then the callback get destroyed, still on the
current thread, and the same to the bound parameter(our Mojo response callback).
WDYT? Thanks~

[Anand Mistry (off Chromium), 2016/03/18 14:10:17]

Well... that's not entirely true. The subtle problem with callbacks is that when
you post them to a task runner, you have no guarantee of which thread holds the
last remaining reference to the callback's internal state. In 99.99999% of cases
(I made that number up), what you described is what happens. In the remaining
0.00001% of cases, the callback is posted and the thread immediately switches to
the task runner thread, the task runner runs and completes the callback and
releases its reference, then the original thread continues with its reference
and releases it, thereby destroying the callback's state and hence its bound
parameters.

+                   weak_factory_.GetWeakPtr(), callback));
   } else {
     std::vector<std::string> no_affiliated_realms;
-    ScheduleRequireMediationTask(request_id, no_affiliated_realms);
+    ScheduleRequireMediationTask(callback, no_affiliated_realms);
   }
 }
 
-void CredentialManagerDispatcher::ScheduleRequireMediationTask(
-    int request_id,
+void CredentialManagerImpl::ScheduleRequireMediationTask(
+    const RequireUserMediationCallback& callback,
     const std::vector<std::string>& android_realms) {
   DCHECK(GetPasswordStore());
   if (!pending_require_user_mediation_) {
@@ -136,25 +131,21 @@ void CredentialManagerDispatcher::ScheduleRequireMediationTask(
         web_contents()->GetLastCommittedURL().GetOrigin());
   }
 
-  web_contents()->GetRenderViewHost()->Send(
-      new CredentialManagerMsg_AcknowledgeRequireUserMediation(
-          web_contents()->GetRenderViewHost()->GetRoutingID(), request_id));
+  // Send acknowledge response back.
+  callback.Run();
 }
 
-void CredentialManagerDispatcher::OnRequestCredential(
-    int request_id,
-    bool zero_click_only,
-    bool include_passwords,
-    const std::vector<GURL>& federations) {
-  DCHECK(request_id);
+void CredentialManagerImpl::Get(bool zero_click_only,
+                                bool include_passwords,
+                                mojo::Array<mojo::String> federations,
+                                const GetCallback& callback) {
   PasswordStore* store = GetPasswordStore();
   if (pending_request_ || !store) {
-    web_contents()->GetRenderViewHost()->Send(
-        new CredentialManagerMsg_RejectCredentialRequest(
-            web_contents()->GetRenderViewHost()->GetRoutingID(), request_id,
-            pending_request_
-                ? blink::WebCredentialManagerPendingRequestError
-                : blink::WebCredentialManagerPasswordStoreUnavailableError));
+    // Callback error.
+    callback.Run(pending_request_
+                     ? mojom::CredentialManagerError::PENDINGREQUEST
+                     : mojom::CredentialManagerError::PASSWORDSTOREUNAVAILABLE,
+                 nullptr);
     return;
   }
 
@@ -162,35 +153,35 @@ void CredentialManagerDispatcher::OnRequestCredential(
   // the current page has TLS errors.
   if ((zero_click_only && !IsZeroClickAllowed()) ||
       client_->DidLastPageLoadEncounterSSLErrors()) {
-    web_contents()->GetRenderViewHost()->Send(
-        new CredentialManagerMsg_SendCredential(
-            web_contents()->GetRenderViewHost()->GetRoutingID(), request_id,
-            CredentialInfo()));
+    // Callback with empty credential info.
+    callback.Run(mojom::CredentialManagerError::SUCCESS,
+                 mojom::CredentialInfo::New());
     return;
   }
 
   if (store->affiliated_match_helper()) {
     store->affiliated_match_helper()->GetAffiliatedAndroidRealms(
         GetSynthesizedFormForOrigin(),
-        base::Bind(&CredentialManagerDispatcher::ScheduleRequestTask,
-                   weak_factory_.GetWeakPtr(), request_id, zero_click_only,
-                   include_passwords, federations));
+        base::Bind(&CredentialManagerImpl::ScheduleRequestTask,
+                   weak_factory_.GetWeakPtr(), callback, zero_click_only,
+                   include_passwords, federations.To<std::vector<GURL>>()));
   } else {
     std::vector<std::string> no_affiliated_realms;
-    ScheduleRequestTask(request_id, zero_click_only, include_passwords,
-                        federations, no_affiliated_realms);
+    ScheduleRequestTask(callback, zero_click_only, include_passwords,
+                        federations.To<std::vector<GURL>>(),
+                        no_affiliated_realms);
   }
 }
 
-void CredentialManagerDispatcher::ScheduleRequestTask(
-    int request_id,
+void CredentialManagerImpl::ScheduleRequestTask(
+    const GetCallback& callback,
     bool zero_click_only,
     bool include_passwords,
     const std::vector<GURL>& federations,
     const std::vector<std::string>& android_realms) {
   DCHECK(GetPasswordStore());
   pending_request_.reset(new CredentialManagerPendingRequestTask(
-      this, request_id, zero_click_only,
+      this, base::Bind(&RunMojoGetCallback, callback), zero_click_only,
       web_contents()->GetLastCommittedURL().GetOrigin(), include_passwords,
       federations, android_realms));
 
@@ -199,19 +190,19 @@ void CredentialManagerDispatcher::ScheduleRequestTask(
   GetPasswordStore()->GetAutofillableLogins(pending_request_.get());
 }
 
-PasswordStore* CredentialManagerDispatcher::GetPasswordStore() {
+PasswordStore* CredentialManagerImpl::GetPasswordStore() {
   return client_ ? client_->GetPasswordStore() : nullptr;
 }
 
-bool CredentialManagerDispatcher::IsZeroClickAllowed() const {
+bool CredentialManagerImpl::IsZeroClickAllowed() const {
   return *auto_signin_enabled_ && !client_->IsOffTheRecord();
 }
 
-GURL CredentialManagerDispatcher::GetOrigin() const {
+GURL CredentialManagerImpl::GetOrigin() const {
   return web_contents()->GetLastCommittedURL().GetOrigin();
 }
 
-base::WeakPtr<PasswordManagerDriver> CredentialManagerDispatcher::GetDriver() {
+base::WeakPtr<PasswordManagerDriver> CredentialManagerImpl::GetDriver() {
   ContentPasswordManagerDriverFactory* driver_factory =
       ContentPasswordManagerDriverFactory::FromWebContents(web_contents());
   DCHECK(driver_factory);
@@ -220,10 +211,11 @@ base::WeakPtr<PasswordManagerDriver> CredentialManagerDispatcher::GetDriver() {
   return driver->AsWeakPtr();
 }
 
-void CredentialManagerDispatcher::SendCredential(int request_id,
-                                                 const CredentialInfo& info) {
+void CredentialManagerImpl::SendCredential(
+    const SendCredentialCallback& send_callback,
+    const CredentialInfo& info) {
   DCHECK(pending_request_);
-  DCHECK_EQ(pending_request_->id(), request_id);
+  DCHECK(send_callback.Equals(pending_request_->send_callback()));
 
   if (PasswordStore* store = GetPasswordStore()) {
     if (info.type != CredentialType::CREDENTIAL_TYPE_EMPTY &&
@@ -237,19 +229,17 @@ void CredentialManagerDispatcher::SendCredential(int request_id,
     }
   }
 
-  web_contents()->GetRenderViewHost()->Send(
-      new CredentialManagerMsg_SendCredential(
-          web_contents()->GetRenderViewHost()->GetRoutingID(),
-          pending_request_->id(), info));
+  // Run callback to send credential info back.
+  send_callback.Run(info);
   pending_request_.reset();
 }
 
-PasswordManagerClient* CredentialManagerDispatcher::client() const {
+PasswordManagerClient* CredentialManagerImpl::client() const {
   return client_;
 }
 
-autofill::PasswordForm
-CredentialManagerDispatcher::GetSynthesizedFormForOrigin() const {
+autofill::PasswordForm CredentialManagerImpl::GetSynthesizedFormForOrigin()
+    const {
   autofill::PasswordForm synthetic_form;
   synthetic_form.origin = web_contents()->GetLastCommittedURL().GetOrigin();
   synthetic_form.signon_realm = synthetic_form.origin.spec();
@@ -259,12 +249,12 @@ CredentialManagerDispatcher::GetSynthesizedFormForOrigin() const {
   return synthetic_form;
 }
 
-void CredentialManagerDispatcher::DoneRequiringUserMediation() {
+void CredentialManagerImpl::DoneRequiringUserMediation() {
   DCHECK(pending_require_user_mediation_);
   pending_require_user_mediation_.reset();
 }
 
-bool CredentialManagerDispatcher::IsUpdatingCredentialAllowed() const {
+bool CredentialManagerImpl::IsUpdatingCredentialAllowed() const {
   return !client_->DidLastPageLoadEncounterSSLErrors() &&
          !client_->IsOffTheRecord();
 }