From 9d99f90c7d5a8a025e14fb9089aa88e7204f2070 Mon Sep 17 00:00:00 2001 From: Corentin Wallez Date: Thu, 28 Mar 2019 14:10:01 +0000 Subject: [PATCH] CommandEncoder: prevent calls even after a failed finish Finish validation acquires the command allocator but didn't set the finished state so further top-level would still try to record in the allocator, causing an ASSERT to fire. BUG=chromium:939969 Change-Id: I334878098e6b824c2c4cef4fccb75472d3b63bbe Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/6041 Reviewed-by: Kai Ninomiya Reviewed-by: Austin Eng Commit-Queue: Kai Ninomiya --- src/dawn_native/CommandEncoder.cpp | 8 ++++++++ .../validation/CommandBufferValidationTests.cpp | 16 ++++++++++++++++ 2 files changed, 24 insertions(+) diff --git a/src/dawn_native/CommandEncoder.cpp b/src/dawn_native/CommandEncoder.cpp index 1ac0790d9e..21b9992327 100644 --- a/src/dawn_native/CommandEncoder.cpp +++ b/src/dawn_native/CommandEncoder.cpp @@ -808,6 +808,9 @@ namespace dawn_native { CommandBufferBase* CommandEncoderBase::Finish() { if (GetDevice()->ConsumedError(ValidateFinish())) { + // Even if finish validation fails, it is now invalid to call any encoding commands on + // this object, so we set its state to finished. + mEncodingState = EncodingState::Finished; return CommandBufferBase::MakeError(GetDevice()); } ASSERT(!IsError()); @@ -833,6 +836,11 @@ namespace dawn_native { } void CommandEncoderBase::PassEnded() { + // This function may still be called when the command encoder is finished, just do nothing. + if (mEncodingState == EncodingState::Finished) { + return; + } + if (mEncodingState == EncodingState::ComputePass) { mAllocator.Allocate(Command::EndComputePass); } else { diff --git a/src/tests/unittests/validation/CommandBufferValidationTests.cpp b/src/tests/unittests/validation/CommandBufferValidationTests.cpp index da2a246fc1..3353390e7b 100644 --- a/src/tests/unittests/validation/CommandBufferValidationTests.cpp +++ b/src/tests/unittests/validation/CommandBufferValidationTests.cpp @@ -155,6 +155,22 @@ TEST_F(CommandBufferValidationTest, BeginComputePassBeforeEndPreviousPass) { } } +// Test that beginning a compute pass before ending the previous pass causes an error. +TEST_F(CommandBufferValidationTest, CallsAfterAFailedFinish) { + // A buffer that can't be used in CopyBufferToBuffer + dawn::BufferDescriptor bufferDesc; + bufferDesc.size = 16; + bufferDesc.usage = dawn::BufferUsageBit::Uniform; + dawn::Buffer buffer = device.CreateBuffer(&bufferDesc); + + dawn::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToBuffer(buffer, 0, buffer, 0, 0); + ASSERT_DEVICE_ERROR(encoder.Finish()); + + // TODO(cwallez@chromium.org): Currently this does nothing, should it be a device error? + encoder.CopyBufferToBuffer(buffer, 0, buffer, 0, 0); +} + // Test that using a single buffer in multiple read usages in the same pass is allowed. TEST_F(CommandBufferValidationTest, BufferWithMultipleReadUsage) { // Create a buffer used as both vertex and index buffer.