Fix Bug in RingBuffer

TEST=unit test
BUG=42859 

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

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

3 files changed, 39 insertions, 16 deletions

diff --git a/gpu/command_buffer/client/gles2_implementation.cc b/gpu/command_buffer/client/gles2_implementation.cc
index 0df4121..3886b95 100644
--- a/gpu/command_buffer/client/gles2_implementation.cc
+++ b/gpu/command_buffer/client/gles2_implementation.cc
@@ -644,8 +644,9 @@ void GLES2Implementation::ShaderBinary(
     return;
   }
   GLsizei shader_id_size = n * sizeof(*shaders);
-  void* shader_ids = transfer_buffer_.Alloc(shader_id_size);
-  void* shader_data = transfer_buffer_.Alloc(length);
+  int8* buffer = transfer_buffer_.AllocTyped<int8>(shader_id_size + length);
+  void* shader_ids = buffer;
+  void* shader_data = buffer + shader_id_size;
   memcpy(shader_ids, shaders, shader_id_size);
   memcpy(shader_data, binary, length);
   helper_->ShaderBinary(
@@ -654,9 +655,7 @@ void GLES2Implementation::ShaderBinary(
       binaryformat,
       transfer_buffer_id_, transfer_buffer_.GetOffset(shader_data),
       length);
-  int32 token = helper_->InsertToken();
-  transfer_buffer_.FreePendingToken(shader_ids, token);
-  transfer_buffer_.FreePendingToken(shader_data, token);
+  transfer_buffer_.FreePendingToken(buffer, helper_->InsertToken());
 }
 
 void GLES2Implementation::PixelStorei(GLenum pname, GLint param) {
@@ -1145,18 +1144,20 @@ void GLES2Implementation::ReadPixels(
           transfer_buffer_id_, transfer_buffer_.GetOffset(buffer),
           result_shm_id(), result_shm_offset());
       WaitForCmd();
+      if (*result != 0) {
+        // We have to copy 1 row at a time to avoid writing pad bytes.
+        const int8* src = static_cast<const int8*>(buffer);
+        for (GLint yy = 0; yy < num_rows; ++yy) {
+          memcpy(dest, src, unpadded_row_size);
+          dest += padded_row_size;
+          src += padded_row_size;
+        }
+      }
+      transfer_buffer_.FreePendingToken(buffer, helper_->InsertToken());
       // If it was not marked as successful exit.
       if (*result == 0) {
         return;
       }
-      // We have to copy 1 row at a time to avoid writing pad bytes.
-      const int8* src = static_cast<const int8*>(buffer);
-      for (GLint yy = 0; yy < num_rows; ++yy) {
-        memcpy(dest, src, unpadded_row_size);
-        dest += padded_row_size;
-        src += padded_row_size;
-      }
-      transfer_buffer_.FreePendingToken(buffer, helper_->InsertToken());
       yoffset += num_rows;
       height -= num_rows;
     }
@@ -1181,12 +1182,14 @@ void GLES2Implementation::ReadPixels(
             transfer_buffer_id_, transfer_buffer_.GetOffset(buffer),
             result_shm_id(), result_shm_offset());
         WaitForCmd();
+        if (*result != 0) {
+          memcpy(row_dest, buffer, part_size);
+        }
+        transfer_buffer_.FreePendingToken(buffer, helper_->InsertToken());
         // If it was not marked as successful exit.
         if (*result == 0) {
           return;
         }
-        memcpy(row_dest, buffer, part_size);
-        transfer_buffer_.FreePendingToken(buffer, helper_->InsertToken());
         row_dest += part_size;
         temp_xoffset += num_pixels;
         temp_width -= num_pixels;
diff --git a/gpu/command_buffer/client/ring_buffer.cc b/gpu/command_buffer/client/ring_buffer.cc
index 678110a..89da7fe 100644
--- a/gpu/command_buffer/client/ring_buffer.cc
+++ b/gpu/command_buffer/client/ring_buffer.cc
@@ -40,11 +40,13 @@ void RingBuffer::FreeOldestBlock() {
     in_use_offset_ = 0;
     free_offset_ = 0;
   }
-  blocks_.pop_back();
+  blocks_.pop_front();
 }
 
 RingBuffer::Offset RingBuffer::Alloc(unsigned int size) {
   DCHECK_LE(size, size_) << "attempt to allocate more than maximum memory";
+  DCHECK(blocks_.empty() || blocks_.back().valid)
+      << "Attempt to alloc another block before freeing the previous.";
   // Similarly to malloc, an allocation of 0 allocates at least 1 byte, to
   // return different pointers every time.
   if (size == 0) size = 1;
@@ -82,6 +84,8 @@ void RingBuffer::FreePendingToken(RingBuffer::Offset offset,
 }
 
 unsigned int RingBuffer::GetLargestFreeSizeNoWaiting() {
+  // TODO(gman): Should check what the current token is and free up to that
+  //    point.
   if (free_offset_ == in_use_offset_) {
     if (blocks_.empty()) {
       // The entire buffer is free.
diff --git a/gpu/command_buffer/client/ring_buffer_test.cc b/gpu/command_buffer/client/ring_buffer_test.cc
index a6ec71a..4ba74ac 100644
--- a/gpu/command_buffer/client/ring_buffer_test.cc
+++ b/gpu/command_buffer/client/ring_buffer_test.cc
@@ -161,6 +161,22 @@ TEST_F(RingBufferTest, TestGetLargestFreeSizeNoWaiting) {
   allocator_->FreePendingToken(offset, helper_.get()->InsertToken());
 }
 
+TEST_F(RingBufferTest, TestFreeBug) {
+  // The first and second allocations must not match.
+  const unsigned int kAlloc1 = 10;
+  const unsigned int kAlloc2 = 20;
+  RingBuffer::Offset offset = allocator_->Alloc(kAlloc1);
+  EXPECT_EQ(kBufferSize - kAlloc1, allocator_->GetLargestFreeSizeNoWaiting());
+  allocator_->FreePendingToken(offset, helper_.get()->InsertToken());
+  offset = allocator_->Alloc(kAlloc2);
+  EXPECT_EQ(kBufferSize - kAlloc1 - kAlloc2,
+            allocator_->GetLargestFreeSizeNoWaiting());
+  allocator_->FreePendingToken(offset, helper_.get()->InsertToken());
+  offset = allocator_->Alloc(kBufferSize);
+  EXPECT_EQ(0u, allocator_->GetLargestFreeSizeNoWaiting());
+  allocator_->FreePendingToken(offset, helper_.get()->InsertToken());
+}
+
 // Test fixture for RingBufferWrapper test - Creates a
 // RingBufferWrapper, using a CommandBufferHelper with a mock
 // AsyncAPIInterface for its interface (calling it directly, not through the