From ba08f5c19c62633e67ea5888671bb45051fcc935 Mon Sep 17 00:00:00 2001
From: Brett Wilson <brettw@chromium.org>
Date: Thu, 31 Aug 2017 22:28:30 +0000
Subject: [PATCH] circular_deque: Fix erasing empty ranges.

Previously circular_deque's range erase function would DCHECK if the caller
erased an empty range, because it would try to move the items on top of
themselves to make room for nothing.

This patch early exits in the erase-nothing case.

Change-Id: I772d1fcfaf5a85b1c79656274e44eac960a9c6e1
Reviewed-on: https://chromium-review.googlesource.com/646529
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499048}
---
 base/containers/circular_deque.h           | 10 +++++++---
 base/containers/circular_deque_unittest.cc | 16 ++++++++++++++--
 2 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/base/containers/circular_deque.h b/base/containers/circular_deque.h
index 8dda6993c388b1..87d4bba8357505 100644
--- a/base/containers/circular_deque.h
+++ b/base/containers/circular_deque.h
@@ -772,8 +772,14 @@ class circular_deque {
     ValidateIterator(first);
     ValidateIterator(last);
 
+    IncrementGeneration();
+
     // First, call the destructor on the deleted items.
-    if (first.index_ < last.index_) {
+    if (first.index_ == last.index_) {
+      // Nothing deleted. Need to return early to avoid falling through to
+      // moving items on top of themselves.
+      return iterator(this, first.index_);
+    } else if (first.index_ < last.index_) {
       // Contiguous range.
       buffer_.DestructRange(&buffer_[first.index_], &buffer_[last.index_]);
     } else {
@@ -783,8 +789,6 @@ class circular_deque {
       buffer_.DestructRange(&buffer_[0], &buffer_[last.index_]);
     }
 
-    IncrementGeneration();
-
     if (last.index_ == begin_) {
       // This deletion is from the beginning. Nothing needs to be copied, only
       // begin_ needs to be updated.
diff --git a/base/containers/circular_deque_unittest.cc b/base/containers/circular_deque_unittest.cc
index 1b5ecd2177a9f1..5c192e96a36463 100644
--- a/base/containers/circular_deque_unittest.cc
+++ b/base/containers/circular_deque_unittest.cc
@@ -728,6 +728,9 @@ TEST(CircularDeque, InsertFill) {
 TEST(CircularDeque, InsertEraseRange) {
   circular_deque<int> q;
 
+  // Erase nothing from an empty deque should work.
+  q.erase(q.begin(), q.end());
+
   // Loop index used below to shift the used items in the buffer.
   for (int i = 0; i < 10; i++) {
     circular_deque<int> source;
@@ -768,11 +771,20 @@ TEST(CircularDeque, InsertEraseRange) {
     EXPECT_EQ(1, q[6]);
     EXPECT_EQ(2, q[7]);
 
-    // Now erase the inserted ranges.
-    auto result = q.erase(q.begin(), q.begin() + 2);
+    // Now erase the inserted ranges. Try each subsection also with no items
+    // being erased, which should be a no-op.
+    auto result = q.erase(q.begin(), q.begin());  // No-op.
+    EXPECT_EQ(q.begin(), result);
+    result = q.erase(q.begin(), q.begin() + 2);
     EXPECT_EQ(q.begin(), result);
+
+    result = q.erase(q.begin() + 1, q.begin() + 1);  // No-op.
+    EXPECT_EQ(q.begin() + 1, result);
     result = q.erase(q.begin() + 1, q.begin() + 3);
     EXPECT_EQ(q.begin() + 1, result);
+
+    result = q.erase(q.end() - 2, q.end() - 2);  // No-op.
+    EXPECT_EQ(q.end() - 2, result);
     result = q.erase(q.end() - 2, q.end());
     EXPECT_EQ(q.end(), result);