Improve PasswordProtectionService and PasswordProtectionRequest

components/safe_browsing/password_protection/password_protection_request.cc

 // Copyright 2017 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.
 #include "components/safe_browsing/password_protection/password_protection_request.h"
 
 #include "base/memory/ptr_util.h"
 #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"
 #include "net/http/http_status_code.h"
 
 using content::BrowserThread;
 
 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);
 }
 
 PasswordProtectionRequest::~PasswordProtectionRequest() {
   weakptr_factory_.InvalidateWeakPtrs();
 }
 
 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();
 }
 
 void PasswordProtectionRequest::CheckCachedVerdicts() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (!password_protection_service_) {
     Finish(RequestOutcome::SERVICE_DESTROYED, nullptr);
     return;
@@ skipping 11 matching lines @@
 
 void PasswordProtectionRequest::FillRequestProto() {
   request_proto_ = base::MakeUnique<LoginReputationClientRequest>();
   request_proto_->set_page_url(main_frame_url_.spec());
   request_proto_->set_trigger_type(request_type_);
   request_proto_->set_stored_verdict_cnt(
       password_protection_service_->GetStoredVerdictCount());
   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() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   FillRequestProto();
 
   std::string serialized_request;
   if (!request_proto_->SerializeToString(&serialized_request)) {
     Finish(RequestOutcome::REQUEST_MALFORMED, nullptr);
     return;
@@ skipping 46 matching lines @@
   }
 
   std::unique_ptr<LoginReputationClientResponse> response =
       base::MakeUnique<LoginReputationClientResponse>();
   std::string response_body;
   bool received_data = source->GetResponseAsString(&response_body);
   DCHECK(received_data);
   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);
 
   if (response) {
     switch (request_type_) {
       case LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE:
         UMA_HISTOGRAM_ENUMERATION(
             "PasswordProtection.UnfamiliarLoginPageVerdict",
             response->verdict_type(),
@@ skipping 15 matching lines @@
 }
 
 void PasswordProtectionRequest::Cancel(bool timed_out) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   fetcher_.reset();
 
   Finish(timed_out ? TIMEDOUT : CANCELED, nullptr);
 }
 
 }  // namespace safe_browsing