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 206 matching lines @@
   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);
+  // 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;
   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();
   }
 }