Remove a DCHECK in PassiveLogCollector which was possible to trigger once the maximum number of sources per tracker was exceeded.

BUG=45801

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

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

3 files changed, 159 insertions, 19 deletions

diff --git a/chrome/browser/net/passive_log_collector.cc b/chrome/browser/net/passive_log_collector.cc
index 921dfa2..c1e6c9b 100644
--- a/chrome/browser/net/passive_log_collector.cc
+++ b/chrome/browser/net/passive_log_collector.cc
@@ -17,7 +17,6 @@ namespace {
 // TODO(eroman): Do something with the truncation count.
 
 const size_t kMaxNumEntriesPerLog = 30;
-const size_t kMaxSourcesPerTracker = 200;
 
 void AddEntryToSourceInfo(const PassiveLogCollector::Entry& entry,
                           PassiveLogCollector::SourceInfo* out_info) {
@@ -131,8 +130,12 @@ std::string PassiveLogCollector::SourceInfo::GetURL() const {
 //----------------------------------------------------------------------------
 
 PassiveLogCollector::SourceTracker::SourceTracker(
-    size_t max_graveyard_size, PassiveLogCollector* parent)
-    : max_graveyard_size_(max_graveyard_size), parent_(parent) {
+    size_t max_num_sources,
+    size_t max_graveyard_size,
+    PassiveLogCollector* parent)
+    : max_num_sources_(max_num_sources),
+      max_graveyard_size_(max_graveyard_size),
+      parent_(parent) {
 }
 
 PassiveLogCollector::SourceTracker::~SourceTracker() {}
@@ -162,7 +165,7 @@ void PassiveLogCollector::SourceTracker::OnAddEntry(const Entry& entry) {
     }
   }
 
-  if (sources_.size() > kMaxSourcesPerTracker) {
+  if (sources_.size() > max_num_sources_) {
     // This is a safety net in case something went wrong, to avoid continually
     // growing memory.
     LOG(WARNING) << "The passive log data has grown larger "
@@ -230,16 +233,23 @@ void PassiveLogCollector::SourceTracker::AdjustReferenceCountForSource(
   // In general it is invalid to call AdjustReferenceCountForSource() on
   // source that doesn't exist. However, it is possible that if
   // SourceTracker::Clear() was previously called this can happen.
-  // TODO(eroman): Add a unit-test that exercises this case.
   SourceIDToInfoMap::iterator it = sources_.find(source_id);
-  if (it == sources_.end())
+  if (it == sources_.end()) {
+    LOG(WARNING) << "Released a reference to nonexistent source.";
     return;
+  }
 
   SourceInfo& info = it->second;
   DCHECK_GE(info.reference_count, 0);
-  DCHECK_GE(info.reference_count + offset, 0);
   info.reference_count += offset;
 
+  if (info.reference_count < 0) {
+    // In general this shouldn't happen, however it is possible to reach this
+    // state if SourceTracker::Clear() was called earlier.
+    LOG(WARNING) << "Released unmatched reference count.";
+    info.reference_count = 0;
+  }
+
   if (!info.is_alive) {
     if (info.reference_count == 1 && offset == 1) {
       // If we just added a reference to a dead source that had no references,
@@ -275,7 +285,7 @@ void PassiveLogCollector::SourceTracker::AddReferenceToSourceDependency(
 void
 PassiveLogCollector::SourceTracker::ReleaseAllReferencesToDependencies(
     SourceInfo* info) {
-  // Release all references |info| was holding to dependent sources.
+  // Release all references |info| was holding to other sources.
   for (SourceDependencyList::const_iterator it = info->dependencies.begin();
        it != info->dependencies.end(); ++it) {
     const net::NetLog::Source& source = *it;
@@ -297,11 +307,12 @@ PassiveLogCollector::SourceTracker::ReleaseAllReferencesToDependencies(
 // ConnectJobTracker
 //----------------------------------------------------------------------------
 
+const size_t PassiveLogCollector::ConnectJobTracker::kMaxNumSources = 100;
 const size_t PassiveLogCollector::ConnectJobTracker::kMaxGraveyardSize = 15;
 
 PassiveLogCollector::ConnectJobTracker::ConnectJobTracker(
     PassiveLogCollector* parent)
-    : SourceTracker(kMaxGraveyardSize, parent) {
+    : SourceTracker(kMaxNumSources, kMaxGraveyardSize, parent) {
 }
 
 PassiveLogCollector::SourceTracker::Action
@@ -328,10 +339,11 @@ PassiveLogCollector::ConnectJobTracker::DoAddEntry(const Entry& entry,
 // SocketTracker
 //----------------------------------------------------------------------------
 
+const size_t PassiveLogCollector::SocketTracker::kMaxNumSources = 200;
 const size_t PassiveLogCollector::SocketTracker::kMaxGraveyardSize = 15;
 
 PassiveLogCollector::SocketTracker::SocketTracker()
-    : SourceTracker(kMaxGraveyardSize, NULL) {
+    : SourceTracker(kMaxNumSources, kMaxGraveyardSize, NULL) {
 }
 
 PassiveLogCollector::SourceTracker::Action
@@ -359,10 +371,11 @@ PassiveLogCollector::SocketTracker::DoAddEntry(const Entry& entry,
 // RequestTracker
 //----------------------------------------------------------------------------
 
+const size_t PassiveLogCollector::RequestTracker::kMaxNumSources = 100;
 const size_t PassiveLogCollector::RequestTracker::kMaxGraveyardSize = 25;
 
 PassiveLogCollector::RequestTracker::RequestTracker(PassiveLogCollector* parent)
-    : SourceTracker(kMaxGraveyardSize, parent) {
+    : SourceTracker(kMaxNumSources, kMaxGraveyardSize, parent) {
 }
 
 PassiveLogCollector::SourceTracker::Action
@@ -395,11 +408,12 @@ PassiveLogCollector::RequestTracker::DoAddEntry(const Entry& entry,
 // InitProxyResolverTracker
 //----------------------------------------------------------------------------
 
+const size_t PassiveLogCollector::InitProxyResolverTracker::kMaxNumSources = 20;
 const size_t PassiveLogCollector::InitProxyResolverTracker::kMaxGraveyardSize =
     3;
 
 PassiveLogCollector::InitProxyResolverTracker::InitProxyResolverTracker()
-    : SourceTracker(kMaxGraveyardSize, NULL) {
+    : SourceTracker(kMaxNumSources, kMaxGraveyardSize, NULL) {
 }
 
 PassiveLogCollector::SourceTracker::Action
@@ -418,10 +432,11 @@ PassiveLogCollector::InitProxyResolverTracker::DoAddEntry(
 // SpdySessionTracker
 //----------------------------------------------------------------------------
 
+const size_t PassiveLogCollector::SpdySessionTracker::kMaxNumSources = 50;
 const size_t PassiveLogCollector::SpdySessionTracker::kMaxGraveyardSize = 10;
 
 PassiveLogCollector::SpdySessionTracker::SpdySessionTracker()
-    : SourceTracker(kMaxGraveyardSize, NULL) {
+    : SourceTracker(kMaxNumSources, kMaxGraveyardSize, NULL) {
 }
 
 PassiveLogCollector::SourceTracker::Action
diff --git a/chrome/browser/net/passive_log_collector.h b/chrome/browser/net/passive_log_collector.h
index 7fcc281..b927c0c 100644
--- a/chrome/browser/net/passive_log_collector.h
+++ b/chrome/browser/net/passive_log_collector.h
@@ -94,7 +94,14 @@ class PassiveLogCollector : public ChromeNetLog::Observer {
   // URLRequests/SocketStreams/ConnectJobs.
   class SourceTracker {
    public:
-    SourceTracker(size_t max_graveyard_size, PassiveLogCollector* parent);
+    // Creates a SourceTracker that will track at most |max_num_sources|.
+    // Up to |max_graveyard_size| unreferenced sources will be kept around
+    // before deleting them for good. |parent| may be NULL, and points to
+    // the owning PassiveLogCollector (it is used when adding references
+    // to other sources).
+    SourceTracker(size_t max_num_sources,
+                  size_t max_graveyard_size,
+                  PassiveLogCollector* parent);
 
     virtual ~SourceTracker();
 
@@ -158,6 +165,7 @@ class PassiveLogCollector : public ChromeNetLog::Observer {
     // (It includes both the "live" sources, and the "dead" ones.)
     SourceIDToInfoMap sources_;
 
+    size_t max_num_sources_;
     size_t max_graveyard_size_;
 
     // FIFO queue for entries in |sources_| that are no longer alive, and
@@ -173,6 +181,7 @@ class PassiveLogCollector : public ChromeNetLog::Observer {
   // Specialization of SourceTracker for handling ConnectJobs.
   class ConnectJobTracker : public SourceTracker {
    public:
+    static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     explicit ConnectJobTracker(PassiveLogCollector* parent);
@@ -186,6 +195,7 @@ class PassiveLogCollector : public ChromeNetLog::Observer {
   // Specialization of SourceTracker for handling Sockets.
   class SocketTracker : public SourceTracker {
    public:
+    static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     SocketTracker();
@@ -200,6 +210,7 @@ class PassiveLogCollector : public ChromeNetLog::Observer {
   // Specialization of SourceTracker for handling URLRequest/SocketStream.
   class RequestTracker : public SourceTracker {
    public:
+    static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     explicit RequestTracker(PassiveLogCollector* parent);
@@ -215,6 +226,7 @@ class PassiveLogCollector : public ChromeNetLog::Observer {
   // SOURCE_INIT_PROXY_RESOLVER.
   class InitProxyResolverTracker : public SourceTracker {
    public:
+    static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     InitProxyResolverTracker();
@@ -229,6 +241,7 @@ class PassiveLogCollector : public ChromeNetLog::Observer {
   // Tracks the log entries for the last seen SOURCE_SPDY_SESSION.
   class SpdySessionTracker : public SourceTracker {
    public:
+    static const size_t kMaxNumSources;
     static const size_t kMaxGraveyardSize;
 
     SpdySessionTracker();
@@ -264,6 +277,8 @@ class PassiveLogCollector : public ChromeNetLog::Observer {
  private:
   FRIEND_TEST_ALL_PREFIXES(PassiveLogCollectorTest,
                            HoldReferenceToDependentSource);
+  FRIEND_TEST_ALL_PREFIXES(PassiveLogCollectorTest,
+                           HoldReferenceToDeletedSource);
 
   ConnectJobTracker connect_job_tracker_;
   SocketTracker socket_tracker_;
diff --git a/chrome/browser/net/passive_log_collector_unittest.cc b/chrome/browser/net/passive_log_collector_unittest.cc
index aab7886..fcef0fb 100644
--- a/chrome/browser/net/passive_log_collector_unittest.cc
+++ b/chrome/browser/net/passive_log_collector_unittest.cc
@@ -89,6 +89,27 @@ static const int kMaxNumLoadLogEntries = 1;
 
 }  // namespace
 
+// Test that once the tracker contains a total maximum amount of data
+// (graveyard + live requests), it resets itself to avoid growing unbounded.
+TEST(RequestTrackerTest, DropsAfterMaximumSize) {
+  RequestTracker tracker(NULL);
+
+  // Fill the source tracker with as many sources as it can hold.
+  for (size_t i = 0; i < RequestTracker::kMaxNumSources; ++i)
+    tracker.OnAddEntry(MakeStartLogEntry(i));
+
+  EXPECT_EQ(RequestTracker::kMaxNumSources, GetLiveSources(tracker).size());
+
+  // Add 5 more -- this should cause it to exceed its expected peak, and
+  // therefore reset all of its data.
+  for (size_t i = 0; i < 5u; ++i) {
+    tracker.OnAddEntry(
+        MakeStartLogEntry(i + RequestTracker::kMaxNumSources));
+  }
+
+  EXPECT_EQ(4u, GetLiveSources(tracker).size());
+}
+
 TEST(RequestTrackerTest, BasicBounded) {
   RequestTracker tracker(NULL);
   EXPECT_EQ(0u, GetLiveSources(tracker).size());
@@ -336,11 +357,11 @@ TEST(PassiveLogCollectorTest, HoldReferenceToDependentSource) {
   // |source_url_request| to be freed, which in turn should release the final
   // reference to |source_socket| cause it to be freed as well.
   for (size_t i = 0; i < RequestTracker::kMaxGraveyardSize; ++i) {
-      log.OnAddEntry(NetLog::TYPE_REQUEST_ALIVE,
-                     base::TimeTicks(),
-                     NetLog::Source(NetLog::SOURCE_URL_REQUEST, next_id++),
-                     NetLog::PHASE_END,
-                     NULL);
+    log.OnAddEntry(NetLog::TYPE_REQUEST_ALIVE,
+                   base::TimeTicks(),
+                   NetLog::Source(NetLog::SOURCE_URL_REQUEST, next_id++),
+                   NetLog::PHASE_END,
+                   NULL);
   }
 
   EXPECT_EQ(0u, GetLiveSources(log.url_request_tracker_).size());
@@ -351,3 +372,92 @@ TEST(PassiveLogCollectorTest, HoldReferenceToDependentSource) {
   EXPECT_EQ(SocketTracker::kMaxGraveyardSize,
             GetDeadSources(log.socket_tracker_).size());
 }
+
+// Have a URL_REQUEST hold a reference to a SOCKET. Then cause the SOCKET to
+// get evicted (by exceeding maximum sources limit). Now the URL_REQUEST is
+// referencing a non-existant SOCKET. Lastly, evict the URL_REQUEST so it
+// tries to drop all of its references. Make sure that in releasing its
+// non-existant reference it doesn't trip any DCHECKs.
+TEST(PassiveLogCollectorTest, HoldReferenceToDeletedSource) {
+  PassiveLogCollector log;
+
+  EXPECT_EQ(0u, GetLiveSources(log.url_request_tracker_).size());
+  EXPECT_EQ(0u, GetLiveSources(log.socket_tracker_).size());
+
+  uint32 next_id = 0;
+  NetLog::Source socket_source(NetLog::SOURCE_SOCKET, next_id++);
+  NetLog::Source url_request_source(NetLog::SOURCE_URL_REQUEST, next_id++);
+
+  // Start a SOURCE_SOCKET.
+  log.OnAddEntry(NetLog::TYPE_SOCKET_ALIVE,
+                 base::TimeTicks(),
+                 socket_source,
+                 NetLog::PHASE_BEGIN,
+                 NULL);
+
+  EXPECT_EQ(0u, GetLiveSources(log.url_request_tracker_).size());
+  EXPECT_EQ(1u, GetLiveSources(log.socket_tracker_).size());
+
+  // Start a SOURCE_URL_REQUEST.
+  log.OnAddEntry(NetLog::TYPE_REQUEST_ALIVE,
+                 base::TimeTicks(),
+                 url_request_source,
+                 NetLog::PHASE_BEGIN,
+                 NULL);
+
+  // Associate the SOURCE_SOCKET with the SOURCE_URL_REQUEST.
+  log.OnAddEntry(NetLog::TYPE_SOCKET_POOL_BOUND_TO_SOCKET,
+                 base::TimeTicks(),
+                 url_request_source,
+                 NetLog::PHASE_NONE,
+                 new net::NetLogSourceParameter("x", socket_source));
+
+  // Check that an associate was made -- the SOURCE_URL_REQUEST should have
+  // added a reference to the SOURCE_SOCKET.
+  ASSERT_EQ(1u, GetLiveSources(log.url_request_tracker_).size());
+  {
+    PassiveLogCollector::SourceInfo info =
+        GetLiveSources(log.url_request_tracker_)[0];
+    EXPECT_EQ(0, info.reference_count);
+    EXPECT_EQ(1u, info.dependencies.size());
+    EXPECT_EQ(socket_source.id, info.dependencies[0].id);
+  }
+  ASSERT_EQ(1u, GetLiveSources(log.socket_tracker_).size());
+  {
+    PassiveLogCollector::SourceInfo info =
+        GetLiveSources(log.socket_tracker_)[0];
+    EXPECT_EQ(1, info.reference_count);
+    EXPECT_EQ(0u, info.dependencies.size());
+  }
+
+  // Add lots of sources to the socket tracker. This is just enough to cause
+  // the tracker to reach its peak, and reset all of its data as a safeguard.
+  for (size_t i = 0; i < SocketTracker::kMaxNumSources; ++i) {
+    log.OnAddEntry(NetLog::TYPE_SOCKET_ALIVE,
+                   base::TimeTicks(),
+                   NetLog::Source(NetLog::SOURCE_SOCKET, next_id++),
+                   NetLog::PHASE_BEGIN,
+                   NULL);
+  }
+  ASSERT_EQ(0u, GetLiveSources(log.socket_tracker_).size());
+
+  // End the original request. Then saturate the graveyard with enough other
+  // requests to cause it to be deleted. Once that source is deleted, it will
+  // try to give up its reference to the SOCKET. However that socket_id no
+  // longer exists -- should not DCHECK().
+  log.OnAddEntry(NetLog::TYPE_REQUEST_ALIVE,
+                 base::TimeTicks(),
+                 url_request_source,
+                 NetLog::PHASE_END,
+                 NULL);
+  for (size_t i = 0; i < RequestTracker::kMaxGraveyardSize; ++i) {
+    log.OnAddEntry(NetLog::TYPE_REQUEST_ALIVE,
+                   base::TimeTicks(),
+                   NetLog::Source(NetLog::SOURCE_URL_REQUEST, next_id++),
+                   NetLog::PHASE_END,
+                   NULL);
+  }
+  EXPECT_EQ(RequestTracker::kMaxGraveyardSize,
+            GetDeadSources(log.url_request_tracker_).size());
+}
+