Fix a DCHECK that could fire in PassiveLogCollector for a particular stream of events.

The sequence of events needed to trigger the assertion is a little complicated (see unittest for specifics).

The general pattern goes something like this:
  (1) Add a dependency between two sources of different types. (This increments the refcount on the dependency.)
  (2) Saturate the "graveyard" containing the dependency causing it to be nuked.
  (3) Add a new entry to the original dependency, causing it to re-create the source (but now with 0 refcount).
  (4) Force the buffer containing the source in step (1) to be nuked
  (5) In releasing the dependencies, it will now try to release reference on non-existent source.

While this was crashing debug mode, I don't believe there would have been an impact on release builds (since DeleteSourceInfo() early returns if it can't find the source).

BUG=58847
TEST=PassiveLogCollectorTest.ReleaseDependencyToUnreferencedSource
  
Review URL: http://codereview.chromium.org/4960001

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

3 files changed, 71 insertions, 6 deletions

diff --git a/chrome/browser/net/passive_log_collector.cc b/chrome/browser/net/passive_log_collector.cc
index 2aca24f..84a9403 100644
--- a/chrome/browser/net/passive_log_collector.cc
+++ b/chrome/browser/net/passive_log_collector.cc
@@ -289,6 +289,15 @@ void PassiveLogCollector::SourceTracker::AddToDeletionQueue(
   }
 }
 
+void PassiveLogCollector::SourceTracker::EraseFromDeletionQueue(
+    uint32 source_id) {
+  DeletionQueue::iterator it =
+      std::remove(deletion_queue_.begin(), deletion_queue_.end(),
+                  source_id);
+  DCHECK(it != deletion_queue_.end());
+  deletion_queue_.erase(it);
+}
+
 void PassiveLogCollector::SourceTracker::AdjustReferenceCountForSource(
     int offset, uint32 source_id) {
   DCHECK(offset == -1 || offset == 1) << "invalid offset: " << offset;
@@ -306,7 +315,8 @@ void PassiveLogCollector::SourceTracker::AdjustReferenceCountForSource(
   DCHECK_GE(info.reference_count, 0);
   info.reference_count += offset;
 
-  if (info.reference_count < 0) {
+  bool released_unmatched_reference = info.reference_count < 0;
+  if (released_unmatched_reference) {
     // 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.";
@@ -317,12 +327,10 @@ void PassiveLogCollector::SourceTracker::AdjustReferenceCountForSource(
     if (info.reference_count == 1 && offset == 1) {
       // If we just added a reference to a dead source that had no references,
       // it must have been in the deletion queue, so remove it from the queue.
-      DeletionQueue::iterator it =
-          std::remove(deletion_queue_.begin(), deletion_queue_.end(),
-                      source_id);
-      DCHECK(it != deletion_queue_.end());
-      deletion_queue_.erase(it);
+      EraseFromDeletionQueue(source_id);
     } else if (info.reference_count == 0) {
+      if (released_unmatched_reference)
+        EraseFromDeletionQueue(source_id);
       // If we just released the final reference to a dead source, go ahead
       // and delete it right away.
       DeleteSourceInfo(source_id);
diff --git a/chrome/browser/net/passive_log_collector.h b/chrome/browser/net/passive_log_collector.h
index d98049d..5d46fb3 100644
--- a/chrome/browser/net/passive_log_collector.h
+++ b/chrome/browser/net/passive_log_collector.h
@@ -181,6 +181,9 @@ class PassiveLogCollector : public ChromeNetLog::Observer {
     // Adds |source_id| to the FIFO queue (graveyard) for deletion.
     void AddToDeletionQueue(uint32 source_id);
 
+    // Removes |source_id| from the |deletion_queue_| container.
+    void EraseFromDeletionQueue(uint32 source_id);
+
     // Adds/Releases a reference from the source with ID |source_id|.
     // Use |offset=-1| to do a release, and |offset=1| for an addref.
     void AdjustReferenceCountForSource(int offset, uint32 source_id);
diff --git a/chrome/browser/net/passive_log_collector_unittest.cc b/chrome/browser/net/passive_log_collector_unittest.cc
index 18530a6..e7d6074 100644
--- a/chrome/browser/net/passive_log_collector_unittest.cc
+++ b/chrome/browser/net/passive_log_collector_unittest.cc
@@ -438,3 +438,57 @@ TEST(PassiveLogCollectorTest, HoldReferenceToDeletedSource) {
             GetDeadSources(log.url_request_tracker_).size());
 }
 
+// Regression test for http://crbug.com/58847
+TEST(PassiveLogCollectorTest, ReleaseDependencyToUnreferencedSource) {
+  PassiveLogCollector log;
+
+  // If these constants are weird, the test won't be testing the right thing.
+  EXPECT_LT(PassiveLogCollector::RequestTracker::kMaxGraveyardSize,
+            PassiveLogCollector::RequestTracker::kMaxNumSources);
+
+  // Add a "reference" to a non-existant source (sourceID=1706 does not exist).
+  scoped_refptr<net::NetLog::EventParameters> params =
+      new net::NetLogSourceParameter(
+          "source_dependency",
+          net::NetLog::Source(net::NetLog::SOURCE_SOCKET, 1263));
+  log.OnAddEntry(net::NetLog::TYPE_SOCKET_POOL_BOUND_TO_SOCKET,
+                 base::TimeTicks(),
+                 net::NetLog::Source(net::NetLog::SOURCE_URL_REQUEST, 1706),
+                 net::NetLog::PHASE_NONE,
+                 params);
+
+  // At this point source 1706 has noted 1263 as a dependency. However the
+  // reference count for 1263 was not adjusted since it doesn't actually exist.
+
+  // Move source 1706 to the graveyard.
+  log.OnAddEntry(net::NetLog::TYPE_REQUEST_ALIVE,
+                 base::TimeTicks(),
+                 net::NetLog::Source(net::NetLog::SOURCE_URL_REQUEST, 1706),
+                 net::NetLog::PHASE_END,
+                 NULL);
+
+  // Now create a source entry for 1263, such that it is unreferenced and
+  // waiting to be garbage collected.
+  log.OnAddEntry(net::NetLog::TYPE_SOCKET_ALIVE,
+                 base::TimeTicks(),
+                 net::NetLog::Source(net::NetLog::SOURCE_SOCKET, 1263),
+                 net::NetLog::PHASE_END, NULL);
+
+  // Add kMaxGraveyardSize  unreferenced URL_REQUESTS, so the circular buffer
+  // containing source 1706. After adding kMaxGraveyardSize - 1 the buffer
+  // will be full. Now when we add one more more source it will now evict the
+  // oldest item, which is 1706. In doing so, 1706 will try to release the
+  // reference it *thinks* it has on 1263. However 1263 has a reference count
+  // of 0 and is already in a graveyard.
+  for (size_t i = 0;
+       i < PassiveLogCollector::RequestTracker::kMaxGraveyardSize; ++i) {
+    log.OnAddEntry(net::NetLog::TYPE_REQUEST_ALIVE,
+                   base::TimeTicks(),
+                   net::NetLog::Source(net::NetLog::SOURCE_URL_REQUEST, i),
+                   net::NetLog::PHASE_END,
+                   NULL);
+  }
+
+  // To pass, this should simply not have DCHECK-ed above.
+}
+