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 {

[Nathan Parker, 2017/04/18 23:07:26]

Maybe it'd be good to have a state diagram on what's happening on each thread
(imagine you need to touch this code a year from now.) Something like

UI: Add task to IO thread for whitelist
IO: Check whitelist and add task to UI thread
UI: If not whitelisted: start a timeout task on UI, and start fetcher on UI
[wait for network response]
UI: Handle response, or timeout, or cancel events.

[Jialiu Lin, 2017/04/19 00:21:14]

Added a flow diagram in the .h file.

 
 PasswordProtectionRequest::PasswordProtectionRequest(
     const GURL& main_frame_url,
     LoginReputationClientRequest::TriggerType type,
-    bool is_extended_reporting,
-    bool is_incognito,
+    std::unique_ptr<PasswordProtectionFrameList> pending_password_frames,
     base::WeakPtr<PasswordProtectionService> pps,
     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),
+      database_manager_(password_protection_service_->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);
+  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,
+                 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 16 matching lines @@
   fetcher_->Start();
 }
 
 void PasswordProtectionRequest::StartTimeout() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   // If request is not done withing 10 seconds, we cancel this request.
   // The weak pointer used for the timeout will be invalidated (and
   // hence would prevent the timeout) if the check completes on time and
   // execution reaches Finish().
   BrowserThread::PostDelayedTask(

[Nathan Parker, 2017/04/18 23:07:26]

Should this use the tracker_ too, so it'll get canceled?

[Jialiu Lin, 2017/04/19 00:21:14]

Since this is posted to the same thread, WeakPtr will do the trick. In fact,
CancelableTaskTracker doesn't handle PostDelayedTask().

       BrowserThread::UI, FROM_HERE,
       base::Bind(&PasswordProtectionRequest::Cancel, GetWeakPtr(), true),
       base::TimeDelta::FromMilliseconds(request_timeout_in_ms_));
 }
 
 void PasswordProtectionRequest::OnURLFetchComplete(
     const net::URLFetcher* source) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   net::URLRequestStatus status = source->GetStatus();
   const bool is_success = status.is_success();
   const int response_code = source->GetResponseCode();
 
   UMA_HISTOGRAM_SPARSE_SLOWLY(
       "PasswordProtection.PasswordProtectionResponseOrErrorCode",
       is_success ? response_code : status.error());
 
   if (!is_success || net::HTTP_OK != response_code) {
     Finish(RequestOutcome::FETCH_FAILED, nullptr);
     return;
   }
 
   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();

[Nathan Parker, 2017/04/18 23:07:26]

Will this automatically get called in the destructor, in ->RequestFinished
below?

[Jialiu Lin, 2017/04/19 00:21:14]

Yes. 
The destruction of PasswordProtectionRequest can be triggered in 2 cases only:
(1) request finished, or canceled. --> Finish(..) is explicitly called.
(2) password_protection_service_ is deleted. Then the calling sequence is:
   PasswordProtectionService::~PasswordProtectionService()
     PasswordProtectionService::CancelPendingRequests()
       PasswordProtectionRequest::Cancel()
         PasswordProtectionRequset::Finish()
           PasswordProtectionService::RequestFinished()
             PasswordProtectionRequest::~PasswordProtectionRequest()
 

 
   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