Remove minidump analysis aides from URLRequestThrottlerManager. Leave

in guard against null values.

We are no longer getting any new information from more minidumps for
this particular crash, so the aides are no longer necessary and I'm
leaving the guard in place to minimize the impact on our users.

BUG=71721
TEST=net_unittests

Review URL: http://codereview.chromium.org/6698033

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@78552 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 24 insertions, 110 deletions

diff --git a/net/url_request/url_request_throttler_manager.cc b/net/url_request/url_request_throttler_manager.cc
index baeeb7e..635c716 100644
--- a/net/url_request/url_request_throttler_manager.cc
+++ b/net/url_request/url_request_throttler_manager.cc
@@ -4,36 +4,9 @@
 
 #include "net/url_request/url_request_throttler_manager.h"
 
-#include <list>
-
 #include "base/logging.h"
 #include "base/string_util.h"
 
-namespace {
-
-// TODO(joi): Remove after crbug.com/71721 is fixed.
-struct IteratorHistory {
-  // Copy of 'this' pointer at time of access; this helps both because
-  // the this pointer is often obfuscated (at least for this particular
-  // stack trace) in fully optimized builds, and possibly to detect
-  // changes in the this pointer during iteration over the map (e.g.
-  // from another thread overwriting memory).
-  net::URLRequestThrottlerManager* self;
-
-  // Copy of URL key.
-  char url[256];
-
-  // Not a refptr, we don't want to change behavior by keeping it alive.
-  net::URLRequestThrottlerEntryInterface* entry;
-
-  // Set to true if the entry gets erased. Helpful to verify that entries
-  // with 0 refcount (since we don't take a refcount above) have been
-  // erased from the map.
-  bool was_erased;
-};
-
-}  // namespace
-
 namespace net {
 
 const unsigned int URLRequestThrottlerManager::kMaximumNumberOfEntries = 1500;
@@ -45,7 +18,7 @@ URLRequestThrottlerManager* URLRequestThrottlerManager::GetInstance() {
 
 scoped_refptr<URLRequestThrottlerEntryInterface>
     URLRequestThrottlerManager::RegisterRequestUrl(const GURL &url) {
-  CHECK(being_tested_ || thread_checker_.CalledOnValidThread());
+  DCHECK(being_tested_ || CalledOnValidThread());
 
   // Normalize the url.
   std::string url_id = GetIdFromUrl(url);
@@ -58,9 +31,6 @@ scoped_refptr<URLRequestThrottlerEntryInterface>
   if (entry.get() == NULL)
     entry = new URLRequestThrottlerEntry();
 
-  // TODO(joi): Demote CHECKs in this file to DCHECKs (or remove them) once
-  // we fully understand crbug.com/71721
-  CHECK(entry.get());
   return entry;
 }
 
@@ -94,21 +64,17 @@ URLRequestThrottlerManager::URLRequestThrottlerManager()
   // Construction/destruction is on main thread (because BrowserMain
   // retrieves an instance to call InitializeOptions), but is from then on
   // used on I/O thread.
-  thread_checker_.DetachFromThread();
+  DetachFromThread();
 
   url_id_replacements_.ClearPassword();
   url_id_replacements_.ClearUsername();
   url_id_replacements_.ClearQuery();
   url_id_replacements_.ClearRef();
-
-  // TODO(joi): Remove after crbug.com/71721 is fixed.
-  base::strlcpy(magic_buffer_1_, "MAGICZZ", arraysize(magic_buffer_1_));
-  base::strlcpy(magic_buffer_2_, "GOOGYZZ", arraysize(magic_buffer_2_));
 }
 
 URLRequestThrottlerManager::~URLRequestThrottlerManager() {
   // Destruction is on main thread (AtExit), but real use is on I/O thread.
-  thread_checker_.DetachFromThread();
+  DetachFromThread();
 
   // Delete all entries.
   url_entries_.clear();
@@ -119,8 +85,7 @@ std::string URLRequestThrottlerManager::GetIdFromUrl(const GURL& url) const {
     return url.possibly_invalid_spec();
 
   GURL id = url.ReplaceComponents(url_id_replacements_);
-  // TODO(joi): Remove "GOOGY/MONSTA" stuff once crbug.com/71721 is done
-  return StringPrintf("GOOGY%sMONSTA", StringToLowerASCII(id.spec()).c_str());
+  return StringToLowerASCII(id.spec()).c_str();
 }
 
 void URLRequestThrottlerManager::GarbageCollectEntriesIfNecessary() {
@@ -133,47 +98,21 @@ void URLRequestThrottlerManager::GarbageCollectEntriesIfNecessary() {
 }
 
 void URLRequestThrottlerManager::GarbageCollectEntries() {
-  // TODO(joi): Remove these crash report aids once crbug.com/71721
-  // is figured out.
-  IteratorHistory history[32] = { { 0 } };
-  size_t history_ix = 0;
-  history[history_ix++].self = this;
-
-  int nulls_found = 0;
   UrlEntryMap::iterator i = url_entries_.begin();
   while (i != url_entries_.end()) {
-    if (i->second == NULL) {
-      ++nulls_found;
-    }
-
-    // Keep a log of the first 31 items accessed after the first
-    // NULL encountered (hypothesis is there are multiple NULLs,
-    // and we may learn more about pattern of memory overwrite).
-    // We also log when we access the first entry, to get an original
-    // value for our this pointer.
-    if (nulls_found > 0 && history_ix < arraysize(history)) {
-      history[history_ix].self = this;
-      base::strlcpy(history[history_ix].url, i->first.c_str(),
-                    arraysize(history[history_ix].url));
-      history[history_ix].entry = i->second.get();
-      history[history_ix].was_erased = false;
-      ++history_ix;
-    }
-
-    // TODO(joi): Remove first i->second check when crbug.com/71721 is fixed.
+    // TODO(joi): We know, as per crbug.com/71721, that values here
+    // can sometimes be NULL because of a memory overwrite coming
+    // from somewhere else.  Minidumps of the crash are at this point
+    // not giving us any new information so adding the [i->second ==
+    // NULL] check lessens the impact on our users (it seems to
+    // generally avoid the crash).
     if (i->second == NULL || (i->second)->IsEntryOutdated()) {
       url_entries_.erase(i++);
-
-      if (nulls_found > 0 && (history_ix - 1) < arraysize(history)) {
-        history[history_ix - 1].was_erased = true;
-      }
     } else {
       ++i;
     }
   }
 
-  CHECK(nulls_found == 0);
-
   // In case something broke we want to make sure not to grow indefinitely.
   while (url_entries_.size() > kMaximumNumberOfEntries) {
     url_entries_.erase(url_entries_.begin());
diff --git a/net/url_request/url_request_throttler_manager.h b/net/url_request/url_request_throttler_manager.h
index 1cd2b5c..c48e917 100644
--- a/net/url_request/url_request_throttler_manager.h
+++ b/net/url_request/url_request_throttler_manager.h
@@ -12,7 +12,7 @@
 #include "base/basictypes.h"
 #include "base/scoped_ptr.h"
 #include "base/singleton.h"
-#include "base/threading/thread_checker_impl.h"
+#include "base/threading/non_thread_safe.h"
 #include "googleurl/src/gurl.h"
 #include "net/url_request/url_request_throttler_entry.h"
 
@@ -28,12 +28,10 @@ namespace net {
 // clean out outdated entries. URL ID consists of lowercased scheme, host, port
 // and path. All URLs converted to the same ID will share the same entry.
 //
-// NOTE: All usage of the singleton object of this class should be on the same
-// thread.
-//
-// TODO(joi): Switch back to NonThreadSafe (and remove checks in release builds)
-// once crbug.com/71721 has been tracked down.
-class URLRequestThrottlerManager {
+// NOTE: All usage of this singleton object must be on the same thread,
+// although to allow it to be used as a singleton, construction and destruction
+// can occur on a separate thread.
+class URLRequestThrottlerManager : public base::NonThreadSafe {
  public:
   static URLRequestThrottlerManager* GetInstance();
 
@@ -94,20 +92,10 @@ class URLRequestThrottlerManager {
   // Number of requests that will be made between garbage collection.
   static const unsigned int kRequestsBetweenCollecting;
 
-  // Constructor copies the string "MAGICZZ\0" into this buffer; using it
-  // to try to detect memory overwrites affecting url_entries_ in the wild.
-  // TODO(joi): Remove once crbug.com/71721 is figured out.
-  char magic_buffer_1_[8];
-
   // Map that contains a list of URL ID and their matching
   // URLRequestThrottlerEntry.
   UrlEntryMap url_entries_;
 
-  // Constructor copies the string "GOOGYZZ\0" into this buffer; using it
-  // to try to detect memory overwrites affecting url_entries_ in the wild.
-  // TODO(joi): Remove once crbug.com/71721 is figured out.
-  char magic_buffer_2_[8];
-
   // This keeps track of how many requests have been made. Used with
   // GarbageCollectEntries.
   unsigned int requests_since_last_gc_;
@@ -127,8 +115,6 @@ class URLRequestThrottlerManager {
   // workaround.
   bool being_tested_;
 
-  base::ThreadCheckerImpl thread_checker_;
-
   DISALLOW_COPY_AND_ASSIGN(URLRequestThrottlerManager);
 };
 
diff --git a/net/url_request/url_request_throttler_unittest.cc b/net/url_request/url_request_throttler_unittest.cc
index 4aed38c..febc69c 100644
--- a/net/url_request/url_request_throttler_unittest.cc
+++ b/net/url_request/url_request_throttler_unittest.cc
@@ -333,26 +333,25 @@ TEST(URLRequestThrottlerManager, IsUrlStandardised) {
   MockURLRequestThrottlerManager manager;
   GurlAndString test_values[] = {
       GurlAndString(GURL("http://www.example.com"),
-                    std::string("GOOGYhttp://www.example.com/MONSTA"),
+                    std::string("http://www.example.com/"),
                     __LINE__),
       GurlAndString(GURL("http://www.Example.com"),
-                    std::string("GOOGYhttp://www.example.com/MONSTA"),
+                    std::string("http://www.example.com/"),
                     __LINE__),
       GurlAndString(GURL("http://www.ex4mple.com/Pr4c71c41"),
-                    std::string("GOOGYhttp://www.ex4mple.com/pr4c71c41MONSTA"),
+                    std::string("http://www.ex4mple.com/pr4c71c41"),
+                    __LINE__),
+      GurlAndString(GURL("http://www.example.com/0/token/false"),
+                    std::string("http://www.example.com/0/token/false"),
                     __LINE__),
-      GurlAndString(
-          GURL("http://www.example.com/0/token/false"),
-          std::string("GOOGYhttp://www.example.com/0/token/falseMONSTA"),
-          __LINE__),
       GurlAndString(GURL("http://www.example.com/index.php?code=javascript"),
-                    std::string("GOOGYhttp://www.example.com/index.phpMONSTA"),
+                    std::string("http://www.example.com/index.php"),
                     __LINE__),
       GurlAndString(GURL("http://www.example.com/index.php?code=1#superEntry"),
-                    std::string("GOOGYhttp://www.example.com/index.phpMONSTA"),
+                    std::string("http://www.example.com/index.php"),
                     __LINE__),
       GurlAndString(GURL("http://www.example.com:1234/"),
-                    std::string("GOOGYhttp://www.example.com:1234/MONSTA"),
+                    std::string("http://www.example.com:1234/"),
                     __LINE__)};
 
   for (unsigned int i = 0; i < arraysize(test_values); ++i) {
@@ -390,13 +389,3 @@ TEST(URLRequestThrottlerManager, IsHostBeingRegistered) {
 
   EXPECT_EQ(3, manager.GetNumberOfEntries());
 }
-
-#if defined(GTEST_HAS_DEATH_TEST)
-TEST(URLRequestThrottlerManager, NullHandlingTest) {
-  MockURLRequestThrottlerManager manager;
-  manager.OverrideEntryForTests(GURL("http://www.foo.com/"), NULL);
-  ASSERT_DEATH({
-      manager.DoGarbageCollectEntries();
-  }, "");
-}
-#endif  // defined(GTEST_HAS_DEATH_TEST)