Improve PasswordProtectionService and PasswordProtectionRequest

components/safe_browsing/password_protection/password_protection_request.cc

Index: components/safe_browsing/password_protection/password_protection_request.cc
diff --git a/components/safe_browsing/password_protection/password_protection_request.cc b/components/safe_browsing/password_protection/password_protection_request.cc
index e7db6e05543e8d8f801d6e1dd65cec38f4d59a11..d733a6a18ddc46760882b9fdd50229b3c73fa22c 100644
--- a/components/safe_browsing/password_protection/password_protection_request.cc
+++ b/components/safe_browsing/password_protection/password_protection_request.cc
@@ -7,7 +7,7 @@
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/histogram_macros.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
-#include "content/public/browser/browser_thread.h"
+#include "components/safe_browsing_db/database_manager.h"
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
 #include "net/base/url_util.h"
@@ -20,15 +20,15 @@ namespace safe_browsing {
 PasswordProtectionRequest::PasswordProtectionRequest(
     const GURL& main_frame_url,
     LoginReputationClientRequest::TriggerType type,
-    bool is_extended_reporting,
-    bool is_incognito,
+    std::unique_ptr<PasswordProtectionFrames> pending_password_frames,
     base::WeakPtr<PasswordProtectionService> pps,
+    scoped_refptr<SafeBrowsingDatabaseManager> database_manager,
     int request_timeout_in_ms)
     : main_frame_url_(main_frame_url),
       request_type_(type),
-      is_extended_reporting_(is_extended_reporting),
-      is_incognito_(is_incognito),
+      pending_password_frames_(std::move(pending_password_frames)),
       password_protection_service_(pps),

[Nathan Parker, 2017/04/17 20:28:02]

Do we need both the PPS and the DBMGR? We can get the DBMGR from the PPS.

[Jialiu Lin, 2017/04/18 00:58:26]

Ah, you're totally right. 

+      database_manager_(database_manager),
       request_timeout_in_ms_(request_timeout_in_ms),
       weakptr_factory_(this) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
@@ -42,33 +42,34 @@ void PasswordProtectionRequest::Start() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   // Initially we only send ping for Safe Browsing Extended Reporting users when
   // they are not in incognito mode. We may loose these conditions later.
-  if (is_incognito_) {
+  if (password_protection_service_->IsIncognito()) {
     Finish(RequestOutcome::INCOGNITO, nullptr);
     return;
   }
-  if (!is_extended_reporting_) {
+  if (!password_protection_service_->IsExtendedReporting()) {
     Finish(RequestOutcome::NO_EXTENDED_REPORTING, nullptr);
     return;
   }
 
-  CheckWhitelistsOnUIThread();
+  CheckWhitelistOnUIThread();
 }
 
-void PasswordProtectionRequest::CheckWhitelistsOnUIThread() {
+void PasswordProtectionRequest::CheckWhitelistOnUIThread() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  DCHECK(password_protection_service_);
-
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
+  bool* match_whitelist = new bool(false);

[Nathan Parker, 2017/04/17 20:28:02]

This seems like it'd get leaked.  Why a ptr, and then why not a unique_ptr?

[Jialiu Lin, 2017/04/18 00:58:26]

I was following this example.
http://dev.chromium.org/developers/design-documents/threading#CancelableTaskT...

Here I relies on this bool object to live long enough till the end of
OnWhitelistCheckDone(). unique_ptr may not be a good idea, since the first
callback (PPS::CheckCsdWhitelistOnIOThread) does not take ownership of this
bool, and unique_ptr is not thread safe. It is much easier to handle the
ownership by using base::Owned() here. 

+  tracker_.PostTaskAndReply(
+      BrowserThread::GetTaskRunnerForThread(BrowserThread::IO).get(), FROM_HERE,
       base::Bind(&PasswordProtectionService::CheckCsdWhitelistOnIOThread,
-                 password_protection_service_, main_frame_url_,
-                 base::Bind(&PasswordProtectionRequest::OnWhitelistCheckDone,
-                            GetWeakPtr())));
+                 base::Unretained(password_protection_service_.get()),
+                 main_frame_url_, match_whitelist),
+      base::Bind(&PasswordProtectionRequest::OnWhitelistCheckDone, this,

[Nathan Parker, 2017/04/17 20:28:02]

What's wrong with the weak-ptr approach here? 

[Jialiu Lin, 2017/04/18 00:58:26]

WeakPtr is not thread safe when handling cancelable task across different
threads.
http://dev.chromium.org/developers/design-documents/threading#base_WeakPtr_an...

The recommended way is to use CancelableTaskTracker. 

+                 base::Owned(match_whitelist)));
 }
 
-void PasswordProtectionRequest::OnWhitelistCheckDone(bool match_whitelist) {
+void PasswordProtectionRequest::OnWhitelistCheckDone(
+    const bool* match_whitelist) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
-  if (match_whitelist)
+  if (*match_whitelist)
     Finish(RequestOutcome::MATCHED_WHITELIST, nullptr);
   else
     CheckCachedVerdicts();
@@ -100,9 +101,14 @@ void PasswordProtectionRequest::FillRequestProto() {
   LoginReputationClientRequest::Frame* main_frame =
       request_proto_->add_frames();
   main_frame->set_url(main_frame_url_.spec());
+  main_frame->set_frame_index(0 /* main frame */);
+  main_frame->set_has_password_field(true);
   password_protection_service_->FillReferrerChain(
       main_frame_url_, -1 /* tab id not available */, main_frame);
-  // TODO(jialiul): Add sub-frame information and password form information.
+
+  // TODO(jialiul): Fill more password form related info based on
+  // |password_frame_map_| when Safe Browsing backend is ready to handle these
+  // pieces of information.
 }
 
 void PasswordProtectionRequest::SendRequest() {
@@ -169,17 +175,17 @@ void PasswordProtectionRequest::OnURLFetchComplete(
   fetcher_.reset();  // We don't need it anymore.
   UMA_HISTOGRAM_TIMES("PasswordProtection.RequestNetworkDuration",
                       base::TimeTicks::Now() - request_start_time_);
-  if (response->ParseFromString(response_body)) {
+  if (response->ParseFromString(response_body))
     Finish(RequestOutcome::SUCCEEDED, std::move(response));
-  } else {
+  else
     Finish(RequestOutcome::RESPONSE_MALFORMED, nullptr);
-  }
 }
 
 void PasswordProtectionRequest::Finish(
     RequestOutcome outcome,
     std::unique_ptr<LoginReputationClientResponse> response) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  tracker_.TryCancelAll();
 
   UMA_HISTOGRAM_ENUMERATION("PasswordProtection.RequestOutcome", outcome,
                             RequestOutcome::MAX_OUTCOME);