Remove LOG and DVlog statements.

Many untested lines of client side detection
related code are log statements. Removing them
as they make the code messier. Also
removed unneeded methods that only does DCHECKs
and made them inline instead. Removed unneeded
methods in tests. There will be follow up
CLs to address untested methods.

Bug: 1069793
Change-Id: I910e01dd89db27351474a9d5bc589f3f67437d5a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2233712
Commit-Queue: Bettina Dea <bdea@chromium.org>
Reviewed-by: Daniel Rubery <drubery@chromium.org>
Cr-Commit-Position: refs/heads/master@{#776176}

chrome/browser/safe_browsing/client_side_detection_host.cc

@@ -390,17 +390,10 @@ void ClientSideDetectionHost::SendModelToRenderFrame(
     if (IsSafeBrowsingEnabled(*profile->GetPrefs())) {
       if (IsExtendedReportingEnabled(*profile->GetPrefs()) ||
           IsEnhancedProtectionEnabled(*profile->GetPrefs())) {
-        DVLOG(2) << "Sending phishing model " << model_loader_extended->name()
-                 << " to RenderFrameHost @" << frame;
         model = model_loader_extended->model_str();
       } else {
-        DVLOG(2) << "Sending phishing model " << model_loader_standard->name()
-                 << " to RenderFrameHost @" << frame;
         model = model_loader_standard->model_str();
       }
-    } else {
-      DVLOG(2) << "Disabling client-side phishing detection for "
-               << "RenderFrameHost @" << frame;
     }
 
     if (phishing_detector_)

chrome/browser/safe_browsing/client_side_detection_service.cc

@@ -10,7 +10,6 @@
 #include "base/bind.h"
 #include "base/containers/queue.h"
 #include "base/location.h"
-#include "base/logging.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/scoped_refptr.h"
 #include "base/metrics/histogram_functions.h"
@@ -260,8 +259,6 @@ void ClientSideDetectionService::StartClientReportPhishingRequest(
     request->mutable_population()->set_user_population(
         ChromeUserPopulation::SAFE_BROWSING);
   }
-  DVLOG(2) << "Starting report for hit on model " << request->model_filename();
-
   request->mutable_population()->set_profile_management_status(
       GetProfileManagementStatus(
           g_browser_process->browser_policy_connector()));
@@ -269,7 +266,6 @@ void ClientSideDetectionService::StartClientReportPhishingRequest(
   std::string request_data;
   if (!request->SerializeToString(&request_data)) {
     UMA_HISTOGRAM_COUNTS_1M("SBClientPhishing.RequestNotSerialized", 1);
-    DVLOG(1) << "Unable to serialize the CSD request. Proto file changed?";
     if (!callback.is_null())
       callback.Run(GURL(request->url()), false);
     return;
@@ -352,10 +348,6 @@ void ClientSideDetectionService::HandlePhishingVerdict(
     cache_[info->phishing_url] =
         base::WrapUnique(new CacheState(response.phishy(), base::Time::Now()));
     is_phishing = response.phishy();
-  } else {
-    DLOG(ERROR) << "Unable to get the server verdict for URL: "
-                << info->phishing_url << " net_error: " << net_error << " "
-                << "response_code:" << response_code;
   }
   if (!info->callback.is_null())
     info->callback.Run(info->phishing_url, is_phishing);

chrome/browser/safe_browsing/client_side_detection_service_unittest.cc

@@ -135,12 +135,6 @@ class ClientSideDetectionServiceTest : public testing::Test {
     return csd_service_->phishing_report_times_;
   }
 
-  void SetCache(const GURL& gurl, bool is_phishing, base::Time time) {
-    csd_service_->cache_[gurl] =
-        std::make_unique<ClientSideDetectionService::CacheState>(is_phishing,
-                                                                 time);
-  }
-
   void TestCache() {
     auto& cache = csd_service_->cache_;
     base::Time now = base::Time::Now();
@@ -193,21 +187,6 @@ class ClientSideDetectionServiceTest : public testing::Test {
     EXPECT_TRUE(is_phishing);
   }
 
-  void AddFeature(const std::string& name, double value,
-                  ClientPhishingRequest* request) {
-    ClientPhishingRequest_Feature* feature = request->add_feature_map();
-    feature->set_name(name);
-    feature->set_value(value);
-  }
-
-  void AddNonModelFeature(const std::string& name, double value,
-                          ClientPhishingRequest* request) {
-    ClientPhishingRequest_Feature* feature =
-        request->add_non_model_feature_map();
-    feature->set_name(name);
-    feature->set_value(value);
-  }
-
  protected:
   content::BrowserTaskEnvironment task_environment_;
   std::unique_ptr<ClientSideDetectionService> csd_service_;

chrome/renderer/safe_browsing/phishing_classifier.cc

@@ -11,7 +11,6 @@
 #include "base/callback.h"
 #include "base/compiler_specific.h"
 #include "base/location.h"
-#include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
@@ -49,11 +48,13 @@ PhishingClassifier::PhishingClassifier(content::RenderFrame* render_frame,
 PhishingClassifier::~PhishingClassifier() {
   // The RenderView should have called CancelPendingClassification() before
   // we are destroyed.
-  CheckNoPendingClassification();
+  DCHECK(done_callback_.is_null());
+  DCHECK(!page_text_);
 }
 
 void PhishingClassifier::set_phishing_scorer(const Scorer* scorer) {
-  CheckNoPendingClassification();
+  DCHECK(done_callback_.is_null());
+  DCHECK(!page_text_);
   scorer_ = scorer;
   if (scorer_) {
     url_extractor_.reset(new PhishingUrlFeatureExtractor);
@@ -85,7 +86,8 @@ void PhishingClassifier::BeginClassification(const base::string16* page_text,
 
   // The RenderView should have called CancelPendingClassification() before
   // starting a new classification, so DCHECK this.
-  CheckNoPendingClassification();
+  DCHECK(done_callback_.is_null());
+  DCHECK(!page_text_);
   // However, in an opt build, we will go ahead and clean up the pending
   // classification so that we can start in a known state.
   CancelPendingClassification();
@@ -220,15 +222,6 @@ void PhishingClassifier::VisualExtractionFinished(bool success) {
   RunCallback(verdict);
 }
 
-void PhishingClassifier::CheckNoPendingClassification() {
-  DCHECK(done_callback_.is_null());
-  DCHECK(!page_text_);
-  if (!done_callback_.is_null() || page_text_) {
-    LOG(ERROR) << "Classification in progress, missing call to "
-               << "CancelPendingClassification";
-  }
-}
-
 void PhishingClassifier::RunCallback(const ClientPhishingRequest& verdict) {
   std::move(done_callback_).Run(verdict);
   Clear();

chrome/renderer/safe_browsing/phishing_classifier.h

@@ -124,11 +124,6 @@ class PhishingClassifier {
   // non-phishy verdict.
   void VisualExtractionFinished(bool success);
 
-  // Helper to verify that there is no pending phishing classification.  Dies
-  // in debug builds if the state is not as expected.  This is a no-op in
-  // release builds.
-  void CheckNoPendingClassification();
-
   // Helper method to run the DoneCallback and clear the state.
   void RunCallback(const ClientPhishingRequest& verdict);
 

chrome/renderer/safe_browsing/phishing_classifier_delegate.cc

@@ -12,7 +12,6 @@
 #include "base/callback.h"
 #include "base/debug/stack_trace.h"
 #include "base/lazy_instance.h"
-#include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/no_destructor.h"
 #include "chrome/renderer/safe_browsing/feature_extractor_clock.h"
@@ -185,8 +184,8 @@ void PhishingClassifierDelegate::PageCaptured(base::string16* page_text,
   have_page_text_ = true;
 
   GURL stripped_last_load_url(StripRef(last_finished_load_url_));
+  // Check if toplevel URL has changed.
   if (stripped_last_load_url == StripRef(last_url_sent_to_classifier_)) {
-    DVLOG(2) << "Toplevel URL is unchanged, not starting classification.";
     return;
   }
 
@@ -214,8 +213,6 @@ void PhishingClassifierDelegate::CancelPendingClassification(
 
 void PhishingClassifierDelegate::ClassificationDone(
     const ClientPhishingRequest& verdict) {
-  DVLOG(2) << "Phishy verdict = " << verdict.is_phishing()
-           << " score = " << verdict.client_score();
   is_phishing_detection_running_ = false;
   if (callback_.is_null())
     return;
@@ -244,7 +241,6 @@ void PhishingClassifierDelegate::MaybeStartClassification() {
   // classified at all (as opposed to deferring it until we get an IPC or
   // the load completes), we discard the page text since it won't be needed.
   if (!classifier_->is_ready()) {
-    DVLOG(2) << "Not starting classification, no Scorer created.";
     is_phishing_detection_running_ = false;
     // Keep classifier_page_text_, in case a Scorer is set later.
     if (!callback_.is_null())
@@ -257,7 +253,6 @@ void PhishingClassifierDelegate::MaybeStartClassification() {
     // Skip loads from session history navigation.  However, update the
     // last URL sent to the classifier, so that we'll properly detect
     // same-document navigations.
-    DVLOG(2) << "Not starting classification for back/forward navigation";
     last_url_sent_to_classifier_ = last_finished_load_url_;
     classifier_page_text_.clear();  // we won't need this.
     have_page_text_ = false;
@@ -270,7 +265,6 @@ void PhishingClassifierDelegate::MaybeStartClassification() {
 
   GURL stripped_last_load_url(StripRef(last_finished_load_url_));
   if (!have_page_text_) {
-    DVLOG(2) << "Not starting classification, there is no page text ready.";
     RecordEvent(SBPhishingClassifierEvent::kPageTextNotLoaded);
     return;
   }
@@ -281,15 +275,11 @@ void PhishingClassifierDelegate::MaybeStartClassification() {
     // so defer classification for now.  Note: the ref does not affect
     // any of the browser's preclassification checks, so we don't require it
     // to match.
-    DVLOG(2) << "Not starting classification, last url from browser is "
-             << last_url_received_from_browser_ << ", last finished load is "
-             << last_finished_load_url_;
     // Keep classifier_page_text_, in case the browser notifies us later that
     // we should classify the URL.
     return;
   }
 
-  DVLOG(2) << "Starting classification for " << last_finished_load_url_;
   last_url_sent_to_classifier_ = last_finished_load_url_;
   is_classifying_ = true;
   classifier_->BeginClassification(

chrome/renderer/safe_browsing/phishing_dom_feature_extractor_browsertest.cc

@@ -106,9 +106,6 @@ class TestPhishingDOMFeatureExtractor : public PhishingDOMFeatureExtractor {
         const std::string frame_domain = it->second;
         full_url = GURL("http://" + it->second).Resolve(partial_url.Utf8());
         url_to_frame_domain_map_[full_url.spec()] = it->second;
-      } else {
-        NOTREACHED() << "Testing input setup is incorrect. "
-                        "Please check url_to_frame_domain_map_ setup.";
       }
     }
     return blink::WebURL(full_url);

chrome/renderer/safe_browsing/phishing_term_feature_extractor.cc

@@ -15,7 +15,6 @@
 #include "base/i18n/break_iterator.h"
 #include "base/i18n/case_conversion.h"
 #include "base/location.h"
-#include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/utf_string_conversions.h"
@@ -74,11 +73,8 @@ struct PhishingTermFeatureExtractor::ExtractionState {
     std::unique_ptr<base::i18n::BreakIterator> i(new base::i18n::BreakIterator(
         text, base::i18n::BreakIterator::BREAK_WORD));
 
-    if (i->Init()) {
+    if (i->Init())
       iterator = std::move(i);
-    } else {
-      DLOG(ERROR) << "failed to open iterator";
-    }
   }
 };
 
@@ -103,7 +99,8 @@ PhishingTermFeatureExtractor::PhishingTermFeatureExtractor(
 PhishingTermFeatureExtractor::~PhishingTermFeatureExtractor() {
   // The RenderView should have called CancelPendingExtraction() before
   // we are destroyed.
-  CheckNoPendingExtraction();
+  DCHECK(done_callback_.is_null());
+  DCHECK(!state_.get());
 }
 
 void PhishingTermFeatureExtractor::ExtractFeatures(
@@ -113,7 +110,8 @@ void PhishingTermFeatureExtractor::ExtractFeatures(
     DoneCallback done_callback) {
   // The RenderView should have called CancelPendingExtraction() before
   // starting a new extraction, so DCHECK this.
-  CheckNoPendingExtraction();
+  DCHECK(done_callback_.is_null());
+  DCHECK(!state_.get());
   // However, in an opt build, we will go ahead and clean up the pending
   // extraction so that we can start in a known state.
   CancelPendingExtraction();
@@ -161,7 +159,6 @@ void PhishingTermFeatureExtractor::ExtractFeaturesWithTimeout() {
       base::TimeTicks now = clock_->Now();
       if (now - state_->start_time >=
           base::TimeDelta::FromMilliseconds(kMaxTotalTimeMs)) {
-        DLOG(ERROR) << "Feature extraction took too long, giving up";
         // We expect this to happen infrequently, so record when it does.
         UMA_HISTOGRAM_COUNTS_1M("SBClientPhishing.TermFeatureTimeout", 1);
         RunCallback(false);
@@ -261,15 +258,6 @@ void PhishingTermFeatureExtractor::HandleWord(
   }
 }
 
-void PhishingTermFeatureExtractor::CheckNoPendingExtraction() {
-  DCHECK(done_callback_.is_null());
-  DCHECK(!state_.get());
-  if (!done_callback_.is_null() || state_.get()) {
-    LOG(ERROR) << "Extraction in progress, missing call to "
-               << "CancelPendingExtraction";
-  }
-}
-
 void PhishingTermFeatureExtractor::RunCallback(bool success) {
   // Record some timing stats that we can use to evaluate feature extraction
   // performance.  These include both successful and failed extractions.

chrome/renderer/safe_browsing/phishing_term_feature_extractor.h

@@ -115,11 +115,6 @@ class PhishingTermFeatureExtractor {
   // Handles a single word in the page text.
   void HandleWord(const base::StringPiece16& word);
 
-  // Helper to verify that there is no pending feature extraction.  Dies in
-  // debug builds if the state is not as expected.  This is a no-op in release
-  // builds.
-  void CheckNoPendingExtraction();
-
   // Runs |done_callback_| and then clears all internal state.
   void RunCallback(bool success);
 

chrome/renderer/safe_browsing/phishing_url_feature_extractor.cc

@@ -8,7 +8,6 @@
 #include <string>
 #include <vector>
 
-#include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_split.h"
 #include "base/strings/string_util.h"
@@ -44,8 +43,8 @@ bool PhishingUrlFeatureExtractor::ExtractFeatures(const GURL& url,
             host, net::registry_controlled_domains::EXCLUDE_UNKNOWN_REGISTRIES,
             net::registry_controlled_domains::EXCLUDE_PRIVATE_REGISTRIES);
 
+    // Check if TLD exists for host.
     if (registry_length == 0 || registry_length == std::string::npos) {
-      DVLOG(1) << "Could not find TLD for host: " << host;
       return false;
     }
     DCHECK_LT(registry_length, host.size()) << "Non-zero registry length, but "
@@ -59,8 +58,8 @@ bool PhishingUrlFeatureExtractor::ExtractFeatures(const GURL& url,
     host.erase(tld_start - 1);
     std::vector<std::string> host_tokens = base::SplitString(
         host, ".", base::KEEP_WHITESPACE, base::SPLIT_WANT_NONEMPTY);
+    // Check if domain exists for host.
     if (host_tokens.empty()) {
-      DVLOG(1) << "Could not find domain for host: " << host;
       return false;
     }
     if (!features->AddBooleanFeature(features::kUrlDomainToken +

chrome/renderer/safe_browsing/scorer.cc

@@ -58,14 +58,12 @@ Scorer::~Scorer() {}
 Scorer* Scorer::Create(const base::StringPiece& model_str) {
   std::unique_ptr<Scorer> scorer(new Scorer());
   ClientSideModel& model = scorer->model_;
+  // Parse the phishing model.
   if (!model.ParseFromArray(model_str.data(), model_str.size())) {
-    DLOG(ERROR) << "Unable to parse phishing model.  This Scorer object is "
-                << "invalid.";
     RecordScorerCreationStatus(SCORER_FAIL_MODEL_PARSE_ERROR);
     return NULL;
   } else if (!model.IsInitialized()) {
-    DLOG(ERROR) << "Unable to parse phishing model.  The model is missing "
-                << "some required fields.  Maybe the .proto file changed?";
+    // The model may be missing some required fields.
     RecordScorerCreationStatus(SCORER_FAIL_MODEL_MISSING_FIELDS);
     return NULL;
   }