dawn_wire: Gracefully handle all invalid and unknown sTypes

This CL makes the wire gracefully handle all invalid and unknown
sTypes. All unknown sType structs are serialized and deserialized
as the base WGPUChainedStruct with sType Invalid.

Bug: dawn:369, dawn:654
Change-Id: Ia2571df81fc96e2c672d3ea13c03237a2d5fa5c1
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/39460
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>

generator/templates/dawn_wire/WireCmd.cpp

@@ -15,6 +15,7 @@
 #include "dawn_wire/WireCmd_autogen.h"
 
 #include "common/Assert.h"
+#include "common/Log.h"
 #include "dawn_wire/Wire.h"
 
 #include <algorithm>
@@ -529,11 +530,9 @@ namespace dawn_wire {
                     {% endfor %}
                     default:
                         // Invalid enum. Reserve space just for the transfer header (sType and hasNext).
-                        // Stop iterating because this is an error.
-                        // TODO(crbug.com/dawn/369): Unknown sTypes are silently discarded.
-                        ASSERT(chainedStruct->sType == WGPUSType_Invalid);
                         result += sizeof(WGPUChainedStructTransfer);
-                        return result;
+                        chainedStruct = chainedStruct->next;
+                        break;
                 }
             }
             return result;
@@ -567,13 +566,19 @@ namespace dawn_wire {
                     default: {
                         // Invalid enum. Serialize just the transfer header with Invalid as the sType.
                         // TODO(crbug.com/dawn/369): Unknown sTypes are silently discarded.
-                        ASSERT(chainedStruct->sType == WGPUSType_Invalid);
+                        if (chainedStruct->sType != WGPUSType_Invalid) {
+                            dawn::WarningLog() << "Unknown sType " << chainedStruct->sType << " discarded.";
+                        }
+
                         WGPUChainedStructTransfer* transfer = reinterpret_cast<WGPUChainedStructTransfer*>(*buffer);
                         transfer->sType = WGPUSType_Invalid;
-                        transfer->hasNext = false;
+                        transfer->hasNext = chainedStruct->next != nullptr;
 
                         *buffer += sizeof(WGPUChainedStructTransfer);
-                        return;
+
+                        // Still move on in case there are valid structs after this.
+                        chainedStruct = chainedStruct->next;
+                        break;
                     }
                 }
             } while (chainedStruct != nullptr);
@@ -615,8 +620,27 @@ namespace dawn_wire {
                             hasNext = transfer->chain.hasNext;
                         } break;
                     {% endfor %}
-                    default:
+                    default: {
-                        return DeserializeResult::FatalError;
+                        // Invalid enum. Deserialize just the transfer header with Invalid as the sType.
+                        // TODO(crbug.com/dawn/369): Unknown sTypes are silently discarded.
+                        if (sType != WGPUSType_Invalid) {
+                            dawn::WarningLog() << "Unknown sType " << sType << " discarded.";
+                        }
+
+                        const volatile WGPUChainedStructTransfer* transfer = nullptr;
+                        DESERIALIZE_TRY(GetPtrFromBuffer(buffer, size, 1, &transfer));
+
+                        WGPUChainedStruct* outStruct = nullptr;
+                        DESERIALIZE_TRY(GetSpace(allocator, sizeof(WGPUChainedStruct), &outStruct));
+                        outStruct->sType = WGPUSType_Invalid;
+                        outStruct->next = nullptr;
+
+                        // Still move on in case there are valid structs after this.
+                        *outChainNext = outStruct;
+                        outChainNext = &outStruct->next;
+                        hasNext = transfer->hasNext;
+                        break;
+                    }
                 }
             } while (hasNext);
 

generator/templates/dawn_wire/client/ApiProcs.cpp

@@ -101,11 +101,10 @@ namespace dawn_wire { namespace client {
                     case WGPUSType_Invalid:
                         break;
                     default:
-                        UNREACHABLE();
                         dawn::WarningLog()
                             << "All objects may not be from the same client. "
-                            << "Unknown sType " << chainedStruct->sType << " discarded.";
+                            << "Unknown sType " << chainedStruct->sType << " ignored.";
-                        return false;
+                        break;
                 }
                 chainedStruct = chainedStruct->next;
             }

src/tests/unittests/validation/BindGroupValidationTests.cpp

@@ -72,9 +72,6 @@ class BindGroupValidationTest : public ValidationTest {
 
 // Test the validation of BindGroupDescriptor::nextInChain
 TEST_F(BindGroupValidationTest, NextInChainNullptr) {
-    // TODO(crbug.com/dawn/654): Crashes with the wire. Diagnose and fix this.
-    DAWN_SKIP_TEST_IF(UsesWire());
-
     wgpu::BindGroupLayout layout = utils::MakeBindGroupLayout(device, {});
 
     wgpu::BindGroupDescriptor descriptor;
@@ -88,7 +85,7 @@ TEST_F(BindGroupValidationTest, NextInChainNullptr) {
 
     // Check that nextInChain != nullptr is an error.
     wgpu::ChainedStruct chainedDescriptor;
-    chainedDescriptor.sType = wgpu::SType::ShaderModuleWGSLDescriptor;
+    chainedDescriptor.sType = wgpu::SType::Invalid;
     descriptor.nextInChain = &chainedDescriptor;
     ASSERT_DEVICE_ERROR(device.CreateBindGroup(&descriptor));
 }

src/tests/unittests/wire/WireExtensionTests.cpp

@@ -121,7 +121,7 @@ TEST_F(WireExtensionTests, MutlipleChainedStructs) {
     FlushClient();
 }
 
-// Test that a chained struct with Invalid sType is an error.
+// Test that a chained struct with Invalid sType passes through as Invalid.
 TEST_F(WireExtensionTests, InvalidSType) {
     WGPUSamplerDescriptorDummyAnisotropicFiltering clientExt = {};
     clientExt.chain.sType = WGPUSType_Invalid;
@@ -132,7 +132,33 @@ TEST_F(WireExtensionTests, InvalidSType) {
     clientDesc.label = "sampler with anisotropic filtering";
 
     wgpuDeviceCreateSampler(device, &clientDesc);
-    FlushClient(false);
+    EXPECT_CALL(api, DeviceCreateSampler(apiDevice, NotNull()))
+        .WillOnce(Invoke([&](Unused, const WGPUSamplerDescriptor* desc) -> WGPUSampler {
+            EXPECT_EQ(desc->nextInChain->sType, WGPUSType_Invalid);
+            EXPECT_EQ(desc->nextInChain->next, nullptr);
+            return api.GetNewSampler();
+        }));
+    FlushClient();
+}
+
+// Test that a chained struct with unknown sType passes through as Invalid.
+TEST_F(WireExtensionTests, UnknownSType) {
+    WGPUSamplerDescriptorDummyAnisotropicFiltering clientExt = {};
+    clientExt.chain.sType = static_cast<WGPUSType>(-1);
+    clientExt.chain.next = nullptr;
+
+    WGPUSamplerDescriptor clientDesc = {};
+    clientDesc.nextInChain = &clientExt.chain;
+    clientDesc.label = "sampler with anisotropic filtering";
+
+    wgpuDeviceCreateSampler(device, &clientDesc);
+    EXPECT_CALL(api, DeviceCreateSampler(apiDevice, NotNull()))
+        .WillOnce(Invoke([&](Unused, const WGPUSamplerDescriptor* desc) -> WGPUSampler {
+            EXPECT_EQ(desc->nextInChain->sType, WGPUSType_Invalid);
+            EXPECT_EQ(desc->nextInChain->next, nullptr);
+            return api.GetNewSampler();
+        }));
+    FlushClient();
 }
 
 // Test that if both an invalid and valid stype are passed on the chain, it is an error.
@@ -152,7 +178,21 @@ TEST_F(WireExtensionTests, ValidAndInvalidSTypeInChain) {
     clientDesc.label = "sampler with anisotropic filtering";
 
     wgpuDeviceCreateSampler(device, &clientDesc);
-    FlushClient(false);
+    EXPECT_CALL(api, DeviceCreateSampler(apiDevice, NotNull()))
+        .WillOnce(Invoke([&](Unused, const WGPUSamplerDescriptor* desc) -> WGPUSampler {
+            const auto* ext =
+                reinterpret_cast<const WGPUSamplerDescriptorDummyAnisotropicFiltering*>(
+                    desc->nextInChain);
+            EXPECT_EQ(ext->chain.sType, WGPUSType_SamplerDescriptorDummyAnisotropicFiltering);
+            EXPECT_EQ(ext->maxAnisotropy, clientExt1.maxAnisotropy);
+
+            EXPECT_EQ(ext->chain.next->sType, WGPUSType_Invalid);
+
+            EXPECT_EQ(ext->chain.next->next, nullptr);
+
+            return api.GetNewSampler();
+        }));
+    FlushClient();
 
     // Swap the order of the chained structs.
     clientDesc.nextInChain = &clientExt2.chain;
@@ -160,7 +200,21 @@ TEST_F(WireExtensionTests, ValidAndInvalidSTypeInChain) {
     clientExt1.chain.next = nullptr;
 
     wgpuDeviceCreateSampler(device, &clientDesc);
-    FlushClient(false);
+    EXPECT_CALL(api, DeviceCreateSampler(apiDevice, NotNull()))
+        .WillOnce(Invoke([&](Unused, const WGPUSamplerDescriptor* desc) -> WGPUSampler {
+            EXPECT_EQ(desc->nextInChain->sType, WGPUSType_Invalid);
+
+            const auto* ext =
+                reinterpret_cast<const WGPUSamplerDescriptorDummyAnisotropicFiltering*>(
+                    desc->nextInChain->next);
+            EXPECT_EQ(ext->chain.sType, WGPUSType_SamplerDescriptorDummyAnisotropicFiltering);
+            EXPECT_EQ(ext->maxAnisotropy, clientExt1.maxAnisotropy);
+
+            EXPECT_EQ(ext->chain.next, nullptr);
+
+            return api.GetNewSampler();
+        }));
+    FlushClient();
 }
 
 // Test that (de)?serializing a chained struct with subdescriptors works.