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

chrome/browser/loader/safe_browsing_resource_throttle.cc

Index: chrome/browser/loader/safe_browsing_resource_throttle.cc
diff --git a/chrome/browser/loader/safe_browsing_resource_throttle.cc b/chrome/browser/loader/safe_browsing_resource_throttle.cc
index 23a27088ef4fe9f2a774c7f7516457b8d5fddca2..e7cf11b8c924102a4e326771c5c8b98a2bff799d 100644
--- a/chrome/browser/loader/safe_browsing_resource_throttle.cc
+++ b/chrome/browser/loader/safe_browsing_resource_throttle.cc
@@ -7,6 +7,7 @@
 #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"
@@ -233,9 +234,21 @@ void SafeBrowsingResourceThrottle::OnCheckBrowseUrlResult(
     safe_browsing::SBThreatType threat_type,
     const safe_browsing::ThreatMetadata& metadata) {
   CHECK_EQ(state_, STATE_CHECKING_URL);
+  // TODO(vakh): The following base::debug::Alias() and CHECK calls should be
+  // removed after http://crbug.com/660293 is fixed.
   CHECK(url.is_valid());
   CHECK(url_being_checked_.is_valid());
-  CHECK_EQ(url, url_being_checked_);
+  if (url != url_being_checked_) {
+    char debug_url_buf[2000], debug_url_checked_buf[2000];
+    base::strlcpy(debug_url_buf, url.spec().c_str(), arraysize(debug_url_buf));
+    base::debug::Alias(&debug_url_buf);
+    base::strlcpy(debug_url_checked_buf, url_being_checked_.spec().c_str(),
+                  arraysize(debug_url_checked_buf));
+    base::debug::Alias(&debug_url_checked_buf);

[Scott Hess - ex-Googler, 2016/12/02 23:21:13]

Suggestion: Put something easy to search for in the buffer.  Then instead of
having to decode things, you can just open the minidump in an editor capable of
reading binaries, and search for your tag (or run strings on the minidump and
grep).  So something like "sbt::ocbur:<spec>" would work fine.

I don't know what the thresholds are on minidump sizes, but I'm pretty sure the
bigger they are, the fewer you'll get.  In your case, you really only need a few
to tell you the flavor, so maybe it doesn't matter, but I'd go with a single
buffer to start with, and circle back if you get crashes where the minidump
shows an overflow on the buf.

Together those add up to something like:

char buf[2000];
snprintf(buf, sizeof(buf), "sbt::ocbur:\n%s\n%s\n", url.spec().c_str(),
other_url.spec().c_str());
base::debug::Alias(buf);

[vakh (use Gerrit instead), 2016/12/02 23:30:37]

Done.

+    DVLOG(1) << "debug_url_buf: " << debug_url_buf;
+    DVLOG(1) << "debug_url_checked_buf: " << debug_url_checked_buf;

[Scott Hess - ex-Googler, 2016/12/02 23:21:13]

Do you run a long-term browser with settings that would show these?  I wouldn't
be surprised if this was a race condition which will only happen in particular
edge cases.  Honestly, I'd just go with LOG(ERROR).  I mean, you're going to
crash anyhow, so I wouldn't worry about polluting logfiles at that point.

Actually, would the original CHECK_EQ() manage logging and crashing all
together?  I think that's fine.

[vakh (use Gerrit instead), 2016/12/02 23:30:37]

Yes, was planning on running the browser but I also do agree that this is some
sort of a race condition. Still having the LOG won't hurt. Changed the string to
be logged on CHECK fail.

+    CHECK(false);
+  }
 
   timer_.Stop();  // Cancel the timeout timer.
   threat_type_ = threat_type;