From 28c1fba1c02eb0c8fedb9794645e4a2dc06d1904 Mon Sep 17 00:00:00 2001
From: Corentin Wallez <cwallez@chromium.org>
Date: Sat, 15 Dec 2018 10:34:42 +0000
Subject: [PATCH] Validate CommmandBuffers aren't ended mid pass.

Also adds regression tests.

BUG=chromium:914566
BUG=chromium:914641

Change-Id: Ic1f9f2440580c3598831c8b2d1310e81aa944133
Reviewed-on: https://dawn-review.googlesource.com/c/3321
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
---
 src/dawn_native/CommandBuffer.cpp             |  6 +++
 .../CommandBufferValidationTests.cpp          | 48 +++++++++++++++++--
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/src/dawn_native/CommandBuffer.cpp b/src/dawn_native/CommandBuffer.cpp
index c9e6c254d8..97b5678b88 100644
--- a/src/dawn_native/CommandBuffer.cpp
+++ b/src/dawn_native/CommandBuffer.cpp
@@ -364,6 +364,10 @@ namespace dawn_native {
     // Implementation of the command buffer validation that can be precomputed before submit
 
     MaybeError CommandBufferBuilder::ValidateGetResult() {
+        if (mEncodingState != EncodingState::TopLevel) {
+            return DAWN_VALIDATION_ERROR("Command buffer recording ended mid-pass");
+        }
+
         MoveToIterator();
         mIterator.Reset();
 
@@ -502,6 +506,7 @@ namespace dawn_native {
             }
         }
 
+        UNREACHABLE();
         return DAWN_VALIDATION_ERROR("Unfinished compute pass");
     }
 
@@ -609,6 +614,7 @@ namespace dawn_native {
             }
         }
 
+        UNREACHABLE();
         return DAWN_VALIDATION_ERROR("Unfinished render pass");
     }
 
diff --git a/src/tests/unittests/validation/CommandBufferValidationTests.cpp b/src/tests/unittests/validation/CommandBufferValidationTests.cpp
index 8655449c88..017d4fced3 100644
--- a/src/tests/unittests/validation/CommandBufferValidationTests.cpp
+++ b/src/tests/unittests/validation/CommandBufferValidationTests.cpp
@@ -25,10 +25,11 @@ TEST_F(CommandBufferValidationTest, Empty) {
         .GetResult();
 }
 
-// Tests for basic render pass usage
-TEST_F(CommandBufferValidationTest, RenderPass) {
-    auto renderpass = CreateSimpleRenderPass();
+// Test that a command buffer cannot be ended mid render pass
+TEST_F(CommandBufferValidationTest, EndedMidRenderPass) {
+    dawn::RenderPassDescriptor renderpass = CreateSimpleRenderPass();
 
+    // Control case, command buffer ended after the pass is ended.
     {
         dawn::CommandBufferBuilder builder = AssertWillBeSuccess(device.CreateCommandBufferBuilder());
         dawn::RenderPassEncoder pass = builder.BeginRenderPass(renderpass);
@@ -36,11 +37,52 @@ TEST_F(CommandBufferValidationTest, RenderPass) {
         builder.GetResult();
     }
 
+    // Error case, command buffer ended mid-pass.
     {
         dawn::CommandBufferBuilder builder = AssertWillBeError(device.CreateCommandBufferBuilder());
         dawn::RenderPassEncoder pass = builder.BeginRenderPass(renderpass);
         builder.GetResult();
     }
+
+    // Error case, command buffer ended mid-pass. Trying to use encoders after GetResult
+    // should fail too.
+    {
+        dawn::CommandBufferBuilder builder = AssertWillBeError(device.CreateCommandBufferBuilder());
+        dawn::RenderPassEncoder pass = builder.BeginRenderPass(renderpass);
+        builder.GetResult();
+        // TODO(cwallez@chromium.org) this should probably be a device error, but currently it
+        // produces a builder error.
+        pass.EndPass();
+    }
+}
+
+// Test that a command buffer cannot be ended mid compute pass
+TEST_F(CommandBufferValidationTest, EndedMidComputePass) {
+    // Control case, command buffer ended after the pass is ended.
+    {
+        dawn::CommandBufferBuilder builder = AssertWillBeSuccess(device.CreateCommandBufferBuilder());
+        dawn::ComputePassEncoder pass = builder.BeginComputePass();
+        pass.EndPass();
+        builder.GetResult();
+    }
+
+    // Error case, command buffer ended mid-pass.
+    {
+        dawn::CommandBufferBuilder builder = AssertWillBeError(device.CreateCommandBufferBuilder());
+        dawn::ComputePassEncoder pass = builder.BeginComputePass();
+        builder.GetResult();
+    }
+
+    // Error case, command buffer ended mid-pass. Trying to use encoders after GetResult
+    // should fail too.
+    {
+        dawn::CommandBufferBuilder builder = AssertWillBeError(device.CreateCommandBufferBuilder());
+        dawn::ComputePassEncoder pass = builder.BeginComputePass();
+        builder.GetResult();
+        // TODO(cwallez@chromium.org) this should probably be a device error, but currently it
+        // produces a builder error.
+        pass.EndPass();
+    }
 }
 
 // Test that using a single buffer in multiple read usages in the same pass is allowed.