Remove netlogging for safe browsing thread details.

NetLog will only be for net/ with out-of-process networking. This follow earlier changes to remove safe browsing requests' use of NetLog.

Per Luke, chrome://safe-browsing shows similar data and we can add extra data if we need it.

Change-Id: Ibc018d34e735ed79c6b4acb01724d2e743078595
Reviewed-on: https://chromium-review.googlesource.com/998072Reviewed-by: Eric Roman <eroman@chromium.org>
Reviewed-by: Luke Z <lpz@chromium.org>
Commit-Queue: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548483}

components/safe_browsing/base_ping_manager.cc

@@ -6,18 +6,15 @@
 
 #include <utility>
 
-#include "base/base64.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
-#include "base/values.h"
 #include "components/data_use_measurement/core/data_use_user_data.h"
 #include "content/public/browser/browser_thread.h"
 #include "google_apis/google_api_keys.h"
 #include "net/base/escape.h"
 #include "net/base/load_flags.h"
-#include "net/log/net_log_source_type.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
 #include "net/url_request/url_fetcher.h"
 #include "net/url_request/url_request_context.h"
@@ -28,34 +25,6 @@
 using content::BrowserThread;
 
 namespace {
-// Returns a dictionary with "url"=|url-spec| and "data"=|payload| for
-// netlogging the start phase of a ping.
-std::unique_ptr<base::Value> NetLogPingStartCallback(
-    const net::NetLogWithSource& net_log,
-    const GURL& url,
-    const std::string& payload,
-    net::NetLogCaptureMode) {
-  std::unique_ptr<base::DictionaryValue> event_params(
-      new base::DictionaryValue());
-  event_params->SetString("url", url.spec());
-  event_params->SetString("payload", payload);
-  net_log.source().AddToEventParameters(event_params.get());
-  return std::move(event_params);
-}
-
-// Returns a dictionary with "url"=|url-spec|, "status"=|status| and
-// "error"=|error| for netlogging the end phase of a ping.
-std::unique_ptr<base::Value> NetLogPingEndCallback(
-    const net::NetLogWithSource& net_log,
-    const net::URLRequestStatus& status,
-    net::NetLogCaptureMode) {
-  std::unique_ptr<base::DictionaryValue> event_params(
-      new base::DictionaryValue());
-  event_params->SetInteger("status", status.status());
-  event_params->SetInteger("error", status.error());
-  net_log.source().AddToEventParameters(event_params.get());
-  return std::move(event_params);
-}
 
 net::NetworkTrafficAnnotationTag kTrafficAnnotation =
     net::DefineNetworkTrafficAnnotation("safe_browsing_extended_reporting",
@@ -116,12 +85,6 @@ BasePingManager::BasePingManager(
       url_prefix_(config.url_prefix) {
   DCHECK(!url_prefix_.empty());
 
-  if (request_context_getter) {
-    net_log_ = net::NetLogWithSource::Make(
-        request_context_getter->GetURLRequestContext()->net_log(),
-        net::NetLogSourceType::SAFE_BROWSING);
-  }
-
   version_ = ProtocolManagerHelper::Version();
 }
 
@@ -131,9 +94,6 @@ BasePingManager::~BasePingManager() {}
 
 // All SafeBrowsing request responses are handled here.
 void BasePingManager::OnURLFetchComplete(const net::URLFetcher* source) {
-  net_log_.EndEvent(
-      net::NetLogEventType::SAFE_BROWSING_PING,
-      base::Bind(&NetLogPingEndCallback, net_log_, source->GetStatus()));
   auto it =
       std::find_if(safebrowsing_reports_.begin(), safebrowsing_reports_.end(),
                    [source](const std::unique_ptr<net::URLFetcher>& ptr) {
@@ -157,16 +117,8 @@ void BasePingManager::ReportSafeBrowsingHit(
       report, data_use_measurement::DataUseUserData::SAFE_BROWSING);
   report_ptr->SetLoadFlags(net::LOAD_DISABLE_CACHE);
   report_ptr->SetRequestContext(request_context_getter_.get());
-  std::string post_data_base64;
-  if (!hit_report.post_data.empty()) {
+  if (!hit_report.post_data.empty())
     report_ptr->SetUploadData("text/plain", hit_report.post_data);
-    base::Base64Encode(hit_report.post_data, &post_data_base64);
-  }
-
-  net_log_.BeginEvent(
-      net::NetLogEventType::SAFE_BROWSING_PING,
-      base::Bind(&NetLogPingStartCallback, net_log_,
-                 report_ptr->GetOriginalURL(), post_data_base64));
 
   report->Start();
   safebrowsing_reports_.insert(std::move(report_ptr));
@@ -185,12 +137,6 @@ void BasePingManager::ReportThreatDetails(const std::string& report) {
   // Don't try too hard to send reports on failures.
   fetcher->SetAutomaticallyRetryOn5xx(false);
 
-  std::string report_base64;
-  base::Base64Encode(report, &report_base64);
-  net_log_.BeginEvent(net::NetLogEventType::SAFE_BROWSING_PING,
-                      base::Bind(&NetLogPingStartCallback, net_log_,
-                                 fetcher->GetOriginalURL(), report_base64));
-
   fetcher->Start();
   safebrowsing_reports_.insert(std::move(fetcher));
 }

components/safe_browsing/base_ping_manager.h

@@ -18,7 +18,6 @@
 #include "components/safe_browsing/db/hit_report.h"
 #include "components/safe_browsing/db/util.h"
 #include "content/public/browser/permission_type.h"
-#include "net/log/net_log_with_source.h"
 #include "net/url_request/url_fetcher_delegate.h"
 #include "url/gurl.h"
 
@@ -87,8 +86,6 @@ class BasePingManager : public net::URLFetcherDelegate {
   // We add both "hit" and "detail" fetchers in this set.
   Reports safebrowsing_reports_;
 
-  net::NetLogWithSource net_log_;
-
   DISALLOW_COPY_AND_ASSIGN(BasePingManager);
 };
 

components/safe_browsing/base_ping_manager_unittest.cc

@@ -12,10 +12,6 @@
 #include "base/values.h"
 #include "google_apis/google_api_keys.h"
 #include "net/base/escape.h"
-#include "net/log/net_log.h"
-#include "net/log/net_log_source_type.h"
-#include "net/log/test_net_log.h"
-#include "net/log/test_net_log_entry.h"
 #include "net/url_request/report_sender.h"
 #include "net/url_request/test_url_fetcher_factory.h"
 #include "testing/gtest/include/gtest/gtest.h"
@@ -33,10 +29,7 @@ namespace safe_browsing {
 
 class BasePingManagerTest : public testing::Test {
  public:
-  BasePingManagerTest() : net_log_(new net::TestNetLog()) {
-    net_log_with_source_ = net::NetLogWithSource::Make(
-        net_log_.get(), net::NetLogSourceType::SAFE_BROWSING);
-  }
+  BasePingManagerTest() {}
 
  protected:
   void SetUp() override {
@@ -51,14 +44,11 @@ class BasePingManagerTest : public testing::Test {
     config.url_prefix = kUrlPrefix;
     ping_manager_.reset(new BasePingManager(nullptr, config));
     ping_manager_->version_ = kAppVer;
-    ping_manager_->net_log_ = net_log_with_source_;
   }
 
   BasePingManager* ping_manager() { return ping_manager_.get(); }
 
   std::string key_param_;
-  std::unique_ptr<net::TestNetLog> net_log_;
-  net::NetLogWithSource net_log_with_source_;
   net::TestURLFetcherFactory fetcher_factory_;
   std::unique_ptr<BasePingManager> ping_manager_;
 };
@@ -206,126 +196,4 @@ TEST_F(BasePingManagerTest, TestThreatDetailsUrl) {
       ping_manager()->ThreatDetailsUrl().spec());
 }
 
-TEST_F(BasePingManagerTest, TestReportThreatDetails) {
-  const std::string kThreatDetailsReportString = "Threat Details Report String";
-  std::string encoded_threat_report = "";
-  base::Base64Encode(kThreatDetailsReportString, &encoded_threat_report);
-  std::string expected_threat_details_url =
-      ping_manager()->ThreatDetailsUrl().spec();
-  const int kRequestErrorCode = -123;
-
-  // Start the report.
-  ping_manager()->ReportThreatDetails(kThreatDetailsReportString);
-
-  net::TestURLFetcher* fetcher = fetcher_factory_.GetFetcherByID(0);
-  DCHECK(fetcher);
-  // Set some error response data on the fetcher to make things interesting.
-  fetcher->set_status(
-      net::URLRequestStatus(net::URLRequestStatus::FAILED, kRequestErrorCode));
-  // Tell the test fetcher to invoke the fetch callback.
-  fetcher->delegate()->OnURLFetchComplete(fetcher);
-
-  // We expect two net log entries: one when the ping starts, one when it ends.
-  net::TestNetLogEntry::List entries;
-  net_log_->GetEntries(&entries);
-  ASSERT_EQ(2u, entries.size());
-
-  // Check for expected log entries for the begin phase.
-  const net::TestNetLogEntry& start_entry = entries[0];
-  ASSERT_EQ(3u, start_entry.params->size());
-
-  std::string string_value;
-  EXPECT_TRUE(start_entry.GetStringValue("url", &string_value));
-  EXPECT_EQ(expected_threat_details_url, string_value);
-
-  EXPECT_TRUE(start_entry.GetStringValue("payload", &string_value));
-  EXPECT_EQ(encoded_threat_report, string_value);
-
-  // We don't really care what the source_dependency value is, just making sure
-  // it's there.
-  EXPECT_TRUE(start_entry.params->HasKey("source_dependency"));
-
-  // Check for expected log entries for the end phase.
-  const net::TestNetLogEntry& end_entry = entries[1];
-  ASSERT_EQ(3u, end_entry.params->size());
-
-  int int_value;
-  EXPECT_TRUE(end_entry.GetIntegerValue("status", &int_value));
-  EXPECT_EQ(net::URLRequestStatus::FAILED, int_value);
-
-  EXPECT_TRUE(end_entry.GetIntegerValue("error", &int_value));
-  EXPECT_EQ(kRequestErrorCode, int_value);
-
-  // We don't really care what the source_dependency value is, just making sure
-  // it's there.
-  EXPECT_TRUE(end_entry.params->HasKey("source_dependency"));
-}
-
-TEST_F(BasePingManagerTest, TestReportSafeBrowsingHit) {
-  const std::string kHitReportPostData = "Hit Report POST Data";
-  std::string encoded_post_data = "";
-  base::Base64Encode(kHitReportPostData, &encoded_post_data);
-
-  HitReport hp;
-  hp.malicious_url = GURL("http://malicious.url.com");
-  hp.page_url = GURL("http://page.url.com");
-  hp.referrer_url = GURL("http://referrer.url.com");
-  hp.threat_type = SB_THREAT_TYPE_URL_CLIENT_SIDE_MALWARE;
-  hp.threat_source = ThreatSource::LOCAL_PVER4;
-  hp.extended_reporting_level = SBER_LEVEL_OFF;
-  hp.is_metrics_reporting_active = false;
-  hp.is_subresource = true;
-  hp.population_id = "foo bar";
-  hp.post_data = kHitReportPostData;
-  std::string expected_hit_report_url =
-      ping_manager()->SafeBrowsingHitUrl(hp).spec();
-  const int kRequestErrorCode = -321;
-
-  // Start the report.
-  ping_manager()->ReportSafeBrowsingHit(hp);
-
-  net::TestURLFetcher* fetcher = fetcher_factory_.GetFetcherByID(0);
-  DCHECK(fetcher);
-  // Set some error response data on the fetcher to make things interesting.
-  fetcher->set_status(
-      net::URLRequestStatus(net::URLRequestStatus::FAILED, kRequestErrorCode));
-  // Tell the test fetcher to invoke the fetch callback.
-  fetcher->delegate()->OnURLFetchComplete(fetcher);
-
-  // We expect two net log entries: one when the ping starts, one when it ends.
-  net::TestNetLogEntry::List entries;
-  net_log_->GetEntries(&entries);
-  ASSERT_EQ(2u, entries.size());
-
-  // Check for expected log entries for the begin phase.
-  const net::TestNetLogEntry& start_entry = entries[0];
-  ASSERT_EQ(3u, start_entry.params->size());
-
-  std::string string_value;
-  EXPECT_TRUE(start_entry.GetStringValue("url", &string_value));
-  EXPECT_EQ(expected_hit_report_url, string_value);
-
-  EXPECT_TRUE(start_entry.GetStringValue("payload", &string_value));
-  EXPECT_EQ(encoded_post_data, string_value);
-
-  // We don't really care what the source_dependency value is, just making sure
-  // it's there.
-  EXPECT_TRUE(start_entry.params->HasKey("source_dependency"));
-
-  // Check for expected log entries for the end phase.
-  const net::TestNetLogEntry& end_entry = entries[1];
-  ASSERT_EQ(3u, end_entry.params->size());
-
-  int int_value;
-  EXPECT_TRUE(end_entry.GetIntegerValue("status", &int_value));
-  EXPECT_EQ(net::URLRequestStatus::FAILED, int_value);
-
-  EXPECT_TRUE(end_entry.GetIntegerValue("error", &int_value));
-  EXPECT_EQ(kRequestErrorCode, int_value);
-
-  // We don't really care what the source_dependency value is, just making sure
-  // it's there.
-  EXPECT_TRUE(end_entry.params->HasKey("source_dependency"));
-}
-
 }  // namespace safe_browsing

net/log/net_log_event_type_list.h

@@ -3096,50 +3096,6 @@ EVENT_TYPE(DATA_REDUCTION_PROXY_FALLBACK)
 //  }
 EVENT_TYPE(DATA_REDUCTION_PROXY_CONFIG_REQUEST)
 
-// -----------------------------------------------------------------------------
-// Safe Browsing related events
-// -----------------------------------------------------------------------------
-
-// The start/end of an async URL check by Safe Browsing. Will only show up if
-// it can't be classified as "safe" synchronously.
-//
-// The BEGIN phase contains the following parameters:
-//  {
-//    "url": <The URL being checked>,
-//  }
-//
-// The END phase contains the following parameters:
-//  {
-//    "result": <"safe", "unsafe", or "request_canceled">
-//  }
-EVENT_TYPE(SAFE_BROWSING_CHECKING_URL)
-
-// The start/end of some portion of the SAFE_BROWSING_CHECKING_URL during which
-// the request is delayed due to that check.
-//
-// The BEGIN phase contains the following parameters:
-//  {
-//    "url": <The URL being checked>,
-//    "defer_reason" : < "at_start", "at_response", "redirect",
-//                       "resumed_redirect", "unchecked_redirect">
-//  }
-EVENT_TYPE(SAFE_BROWSING_DEFERRED)
-
-// The start/end of a Safe Browsing ping being sent.
-//
-// The BEGIN phase contains the following parameters:
-//  {
-//    "url": <The URL the ping is going to, which identifies the type of ping
-//            that is being sent (eg: ThreatReport, SafeBrowsingHit)>
-//    "data": <The base64 encoding of the payload sent with the ping>
-//
-// The END phase contains the following parameters:
-//  {
-//    "status": <The integer status of the report transmission. Corresponds to
-//               URLRequestStatus::Status>
-//    "error": <The error code returned by the server, 0 indicating success>
-EVENT_TYPE(SAFE_BROWSING_PING)
-
 // Marks start of UploadDataStream that is logged on initialization.
 // The END phase contains the following parameters:
 // {