RB-B P1 issue: Add debug aliases for state_ and urls to debug CHECK failures

chrome/browser/loader/safe_browsing_resource_throttle.cc

 // Copyright (c) 2012 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 "chrome/browser/loader/safe_browsing_resource_throttle.h"
 
 #include <iterator>
 #include <utility>
 
+#include "base/debug/alias.h"
 #include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/trace_event/trace_event.h"
 #include "base/values.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/prerender/prerender_contents.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "components/safe_browsing_db/util.h"
 #include "components/safe_browsing_db/v4_feature_list.h"
 #include "components/safe_browsing_db/v4_local_database_manager.h"
@@ skipping 205 matching lines @@
 const char* SafeBrowsingResourceThrottle::GetNameForLogging() const {
   return "SafeBrowsingResourceThrottle";
 }
 
 // SafeBrowsingService::Client implementation, called on the IO thread once
 // the URL has been classified.
 void SafeBrowsingResourceThrottle::OnCheckBrowseUrlResult(
     const GURL& url,
     safe_browsing::SBThreatType threat_type,
     const safe_browsing::ThreatMetadata& metadata) {
-  CHECK_EQ(state_, STATE_CHECKING_URL);
-  CHECK(url.is_valid());
-  CHECK(url_being_checked_.is_valid());
-  CHECK_EQ(url, url_being_checked_);
+  // TODO(vakh): The following base::debug::Alias() calls should be removed and
+  // CHECKs corrected after http://crbug.com/660293 is fixed.
+  base::debug::Alias(&state_);
+  base::debug::Alias(&url);
+  base::debug::Alias(&url_being_checked_);

[Scott Hess - ex-Googler, 2016/12/02 04:19:27]

I am not sure that these work.  They'll annotate to pin the main object, but not
the spec_.

Actually, looking at the Alias() implementation ... I have no idea why it works
to do what I have used it for in the past.  Like sql/connection.cc in
ReportDiagnosticInfo().  In that function, I put the thing I want to see in the
crash dump into a stack variable, then Alias() that, then potentially dump
(well, in that case I force a dump, but I've used it successfully in
potentially-dump cases before).  It may be that the stack placement is
important.  I don't know what parts of the heap come up in crash dumps.

+  std::string alias_str = base::StringPrintf(
+      "OnCheckBrowseUrlResult: state_: %d, url: %s, url_being_checked_: %s",
+      state_, url.spec().c_str(), url_being_checked_.spec().c_str());

[Scott Hess - ex-Googler, 2016/12/02 04:19:27]

In fact, I'm not even sure this will work, because I'm not sure you'll get the
string's heap part.  If you want to be safe, I'd snprintf() into a stack buffer.
 I'm not sure that's correct, but I'm sure it will work, and for something like
this you'll probably rather have results than correctness :-).

+  base::debug::Alias(&alias_str);
+  if (!url_being_checked_.is_valid()) {
+    CHECK(false);
+  }
+  if (!url.is_valid()) {
+    CHECK(false);
+  }
+  if (url != url_being_checked_) {
+    CHECK(false);
+  }
+  if (state_ != STATE_CHECKING_URL) {
+    CHECK(false);
+  }
 
   timer_.Stop();  // Cancel the timeout timer.
   threat_type_ = threat_type;
   state_ = STATE_NONE;
 
   if (defer_state_ != DEFERRED_NONE) {
     EndNetLogEvent(NetLogEventType::SAFE_BROWSING_DEFERRED, nullptr, nullptr);
   }
   EndNetLogEvent(
       NetLogEventType::SAFE_BROWSING_CHECKING_URL, "result",
@@ skipping 180 matching lines @@
                        unchecked_redirect_url_, "defer_reason",
                        "resumed_redirect");
     }
   }
 
   if (resume) {
     defer_state_ = DEFERRED_NONE;
     controller()->Resume();
   }
 }