Create PasswordProtectionRequest to handle password pings

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
new file mode 100644
index 0000000000000000000000000000000000000000..c422c9e8a42cf175073d5cba2deebe44cbef8118
--- /dev/null
+++ b/components/safe_browsing/password_protection/password_protection_request.cc
@@ -0,0 +1,215 @@
+// 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 "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,
+    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),
+      password_protection_service_(pps),
+      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_) {
+    Finish(RequestOutcome::INCOGNITO, nullptr);
+    return;
+  }
+  if (!is_extended_reporting_) {
+    Finish(RequestOutcome::NO_EXTENDED_REPORTING, nullptr);
+    return;
+  }
+
+  // In case the request take too long, we set a timer to cancel this request.
+  StartTimeout();
+
+  CheckWhitelistsOnUIThread();
+}
+
+void PasswordProtectionRequest::Cancel(bool timed_out) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (fetcher_.get()) {

[Nathan Parker, 2017/03/23 20:45:50]

nit: You can .reset() unconditionally

[Jialiu Lin, 2017/03/23 22:42:59]

Done.

+    // If there is already a URLFetcher started, we should cancel it.
+    fetcher_.reset();
+  }
+
+  Finish(timed_out ? TIMEDOUT : CANCELED, nullptr);

[Nathan Parker, 2017/03/23 20:45:50]

Is the expectation that Cancel() be followed by destroying this object? 
Otherwise, a pending CheckCsdWhitelistOnIOThread could return and call Finish a
second time.

[Jialiu Lin, 2017/03/23 22:42:59]

Yes, and Finish(..) will destroy this object by asking PasswordProtectionService
to reset its corresponding unique_ptr, therefore the weakptr bind to
OnWhitelistCheckDone will be invalided and call dropped.

[Nathan Parker, 2017/03/24 00:41:50]

Can you add a comment on Cancel() in the .h to that effect?

[Jialiu Lin, 2017/03/24 01:42:42]

Already did. :-)

+}
+
+void PasswordProtectionRequest::StartTimeout() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  timeout_start_time_ = base::TimeTicks::Now();
+
+  // If request is not done withing 10 milliseconds, we cancel this request.

[Nathan Parker, 2017/03/23 20:45:49]

nit: 10 seconds

[Jialiu Lin, 2017/03/23 22:42:59]

Done.

+  // 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(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&PasswordProtectionRequest::Cancel, GetWeakPtr(), true),
+      base::TimeDelta::FromMilliseconds(request_timeout_in_ms_));
+}
+
+void PasswordProtectionRequest::Finish(
+    RequestOutcome outcome,
+    std::unique_ptr<LoginReputationClientResponse> response) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  UMA_HISTOGRAM_ENUMERATION("PasswordProtection.RequestOutcome", outcome,
+                            RequestOutcome::MAX_OUTCOME);
+  if (!timeout_start_time_.is_null()) {
+    UMA_HISTOGRAM_TIMES("PasswordProtection.RequestDuration",
+                        base::TimeTicks::Now() - timeout_start_time_);

[Nathan Parker, 2017/03/23 20:45:49]

Is this recorded time any different from
PasswordProtection.RequestNetworkDuration?

[Jialiu Lin, 2017/03/23 22:42:59]

NetworkDuration measures the time from SendRequest() to OnURLFetchComplete(). So
if request is canceled due to whitlist match, cached verdict, etc, we don't log
this NetworkDuration. 

RequestDuration includes everything from Start() to Finish(), the time it
measures include whitelist checking, cache checking and other things. And it is
logged for every single request no matter it is successful or not. 

[Nathan Parker, 2017/03/24 00:41:50]

I suspect the only delay > 1 ms is the network call (whitelist check is
synchronous, other than the thread scheduling).  If you want to measure
performance of the bits you mentioned, you might need better than 1 ms
resolution.

[Jialiu Lin, 2017/03/24 01:42:42]

you're right. The differences will be tiny.  Let me remove this metrics. 

+  }
+
+  if (response) {
+    if (request_type_ == LoginReputationClientRequest::UNFAMILIAR_LOGIN_PAGE) {

[Nathan Parker, 2017/03/23 20:45:50]

How about a switch statement with no default, to ensure we have handled all
possible verdicts?

As I understand it, if the server sends a verdict with an enum value that Chrome
doesn't know about, the proto will fail to unpack, so then we shouldn't expect
any weird values here.

[Nathan Parker, 2017/03/23 20:45:50]

I thought this was going to be LOW_REPUTATION -- maybe we changed plans after
the proto was writte.  Should we rename it? (but also ensure it matches the
histogram names)

[Jialiu Lin, 2017/03/23 22:42:59]

UNFAMILIAR_LOGIN_PAGE is the request trigger type (a.k.a whether it is triggered
by unfamiliar login page or password reuse event). LOW_REPUTATION is a verdict
(together with SAFE, and PHISHING). Here I log verdicts from different trigger
type separately.
 PasswordProtection.UnfamiliarLoginPageVerdict
    -- SAFE
    -- PHISHING
    -- LOW_REPUTATION
PasswordProtection.PasswordReuseEventVerdict
    -- SAFE
    -- PHISHING
    -- LOW_REPUTATION

[Jialiu Lin, 2017/03/23 22:43:00]

Done.

[Nathan Parker, 2017/03/24 00:41:50]

AH yes, nm.

+      UMA_HISTOGRAM_ENUMERATION(
+          "PasswordProtection.UnfamiliarLoginPageVerdict",
+          response->verdict_type(),
+          LoginReputationClientResponse_VerdictType_VerdictType_MAX + 1);
+    } else if (request_type_ ==
+               LoginReputationClientRequest::PASSWORD_REUSE_EVENT) {
+      UMA_HISTOGRAM_ENUMERATION(
+          "PasswordProtection.PasswordReuseEventVerdict",
+          response->verdict_type(),
+          LoginReputationClientResponse_VerdictType_VerdictType_MAX + 1);
+    }
+  }
+
+  DCHECK(password_protection_service_);
+  password_protection_service_->RequestFinished(this, std::move(response));
+}
+
+// TODO(jialiul): there are some duplicated code
+void PasswordProtectionRequest::CheckWhitelistsOnUIThread() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!password_protection_service_) {

[Nathan Parker, 2017/03/23 20:45:50]

Is this possible?  The PPS is shut down on the UI thread, so it can't be
destroyed between Start() and here.  I think this is a safe habit, but it's good
to be clear on the flow -- this could be a DCHECK if it shouldn't be possible.

[Jialiu Lin, 2017/03/23 22:42:59]

Just a safe measure, since PPS is a weakptr. But in reality it shouldn't happen.
Let me change it to DCHECK.

+    Finish(RequestOutcome::SERVICE_DESTROYED, nullptr);
+    return;
+  }
+
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      base::Bind(&PasswordProtectionService::CheckCsdWhitelistOnIOThread,
+                 password_protection_service_, main_frame_url_,
+                 base::Bind(&PasswordProtectionRequest::OnWhitelistCheckDone,
+                            GetWeakPtr())));
+}
+
+void PasswordProtectionRequest::OnWhitelistCheckDone(bool match_whitelist) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  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;
+  }
+
+  std::unique_ptr<LoginReputationClientResponse> cached_response =
+      base::MakeUnique<LoginReputationClientResponse>();
+  auto verdict = password_protection_service_->GetCachedVerdict(
+      password_protection_service_->GetSettingMapForActiveProfile(),
+      main_frame_url_, cached_response.get());
+  if (verdict != LoginReputationClientResponse::VERDICT_TYPE_UNSPECIFIED)
+    Finish(RequestOutcome::RESPONSE_ALREADY_CACHED, std::move(cached_response));
+  else
+    SendRequest();
+}
+
+void PasswordProtectionRequest::OnURLFetchComplete(
+    const net::URLFetcher* source) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  if (!source->GetStatus().is_success() ||
+      net::HTTP_OK != source->GetResponseCode()) {
+    Finish(RequestOutcome::FETCH_FAILED, nullptr);

[Nathan Parker, 2017/03/23 20:45:50]

Add todo: log to UMA the httpResponseOrErrorCode like we do elsewhere

[Jialiu Lin, 2017/03/23 22:42:59]

Done. UMA metrics added.

+    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)) {
+    Finish(RequestOutcome::SUCCEEDED, std::move(response));
+  } else {
+    Finish(RequestOutcome::RESPONSE_MALFORMED, nullptr);
+  }
+}
+
+void PasswordProtectionRequest::SendRequest() {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  // This is the last chance to check whether password_protection_service_ is
+  // still available. If not, drop the request.
+  if (!password_protection_service_) {

[Nathan Parker, 2017/03/23 20:45:50]

This was already checked in CheckCachedVerdicts, w/o intervening thread hops, so
it should still be valid.

[Jialiu Lin, 2017/03/23 22:42:59]

Right. removed this check.

+    Finish(RequestOutcome::SERVICE_DESTROYED, nullptr);
+    return;
+  }
+
+  LoginReputationClientRequest request;
+  request.set_page_url(main_frame_url_.spec());
+  request.set_trigger_type(request_type_);
+  request.set_stored_verdict_cnt(
+      password_protection_service_->GetStoredVerdictCount());
+
+  std::string serialized_request;
+  if (!request.SerializeToString(&serialized_request)) {
+    Finish(RequestOutcome::REQUEST_MALFORMED, nullptr);
+    return;
+  }
+
+  fetcher_ = net::URLFetcher::Create(
+      0, PasswordProtectionService::GetPasswordProtectionRequestUrl(),
+      net::URLFetcher::POST, this);
+  data_use_measurement::DataUseUserData::AttachToFetcher(
+      fetcher_.get(), data_use_measurement::DataUseUserData::SAFE_BROWSING);
+  fetcher_->SetLoadFlags(net::LOAD_DISABLE_CACHE);
+  fetcher_->SetAutomaticallyRetryOn5xx(false);
+  fetcher_->SetRequestContext(
+      password_protection_service_->request_context_getter().get());
+  fetcher_->SetUploadData("application/octet-stream", serialized_request);
+  request_start_time_ = base::TimeTicks::Now();
+  fetcher_->Start();
+}
+
+}  // namespace safe_browsing