Test that DOMExceptions from WebGPU always have stacks

Issue: 3103
gpuweb · Oct 27, 2023 · 32d4eba · 32d4eba
1 parent 6e21caa
commit 32d4eba
Show file tree

Hide file tree

Showing 21 changed files with 363 additions and 194 deletions.
diff --git a/src/common/framework/fixture.ts b/src/common/framework/fixture.ts
@@ -1,6 +1,6 @@
 import { TestCaseRecorder } from '../internal/logging/test_case_recorder.js';
 import { JSONWithUndefined } from '../internal/params_utils.js';
-import { assert, unreachable } from '../util/util.js';
+import { assert, ExceptionCheckOptions, unreachable } from '../util/util.js';
 
 export class SkipTestCase extends Error {}
 export class UnexpectedPassError extends Error {}
@@ -237,16 +237,26 @@ export class Fixture<S extends SubcaseBatchState = SubcaseBatchState> {
   }
 
   /** Expect that the provided promise rejects, with the provided exception name. */
-  shouldReject(expectedName: string, p: Promise<unknown>, msg?: string): void {
+  shouldReject(
+    expectedName: string,
+    p: Promise<unknown>,
+    { checkForStackProperty = false, message }: ExceptionCheckOptions = {}
+  ): void {
     this.eventualAsyncExpectation(async niceStack => {
-      const m = msg ? ': ' + msg : '';
+      const m = message ? ': ' + message : '';
       try {
         await p;
         niceStack.message = 'DID NOT REJECT' + m;
         this.rec.expectationFailed(niceStack);
       } catch (ex) {
-        niceStack.message = 'rejected as expected' + m;
+        niceStack.message = 'rejected as expected, but wrong name' + m;
         this.expectErrorValue(expectedName, ex, niceStack);
+        if (checkForStackProperty) {
+          if (!(ex instanceof Error && typeof ex.stack === 'string')) {
+            niceStack.message = 'rejected as expected, but missing stack' + m;
+            this.rec.expectationFailed(niceStack);
+          }
+        }
       }
     });
   }
@@ -257,8 +267,12 @@ export class Fixture<S extends SubcaseBatchState = SubcaseBatchState> {
    *
    * MAINTENANCE_TODO: Change to `string | false` so the exception name is always checked.
    */
-  shouldThrow(expectedError: string | boolean, fn: () => void, msg?: string): void {
-    const m = msg ? ': ' + msg : '';
+  shouldThrow(
+    expectedError: string | boolean,
+    fn: () => void,
+    { checkForStackProperty = false, message }: ExceptionCheckOptions = {}
+  ) {
+    const m = message ? ': ' + message : '';
     try {
       fn();
       if (expectedError === false) {
@@ -271,6 +285,11 @@ export class Fixture<S extends SubcaseBatchState = SubcaseBatchState> {
         this.rec.expectationFailed(new Error('threw unexpectedly' + m));
       } else {
         this.expectErrorValue(expectedError, ex, new Error(m));
+        if (checkForStackProperty) {
+          if (!(ex instanceof Error && typeof ex.stack === 'string')) {
+            this.rec.expectationFailed(new Error('missing stack' + m));
+          }
+        }
       }
     }
   }

diff --git a/src/common/util/util.ts b/src/common/util/util.ts
@@ -47,15 +47,26 @@ export function assertOK<T>(value: Error | T): T {
   return value;
 }
 
+/** Options for assertReject, shouldReject, and friends. */
+export type ExceptionCheckOptions = { checkForStackProperty?: boolean; message?: string };
+
 /**
  * Resolves if the provided promise rejects; rejects if it does not.
  */
-export async function assertReject(p: Promise<unknown>, msg?: string): Promise<void> {
+export async function assertReject(
+  expectedName: string,
+  p: Promise<unknown>,
+  { checkForStackProperty = false, message }: ExceptionCheckOptions = {}
+): Promise<void> {
   try {
     await p;
-    unreachable(msg);
+    unreachable(message);
   } catch (ex) {
-    // Assertion OK
+    // Asserted as expected
+    if (checkForStackProperty) {
+      const m = message ? ` (${message})` : '';
+      assert(ex instanceof Error && typeof ex.stack === 'string', 'missing stack property' + m);
+    }
   }
 }
 

diff --git a/src/stress/memory/oom.spec.ts b/src/stress/memory/oom.spec.ts
@@ -134,10 +134,10 @@ mapAsync on it should reject and produce a validation error. `
     if (unmapBeforeResolve) {
       // Should reject with abort error because buffer will be unmapped
       // before validation check finishes.
-      t.shouldReject('AbortError', promise!);
+      t.shouldReject('AbortError', promise!, { checkForStackProperty: true });
     } else {
       // Should also reject in addition to the validation error.
-      t.shouldReject('OperationError', promise!);
+      t.shouldReject('OperationError', promise!, { checkForStackProperty: true });
 
       // Wait for validation error before unmap to ensure validation check
       // ends before unmap.
@@ -151,9 +151,13 @@ mapAsync on it should reject and produce a validation error. `
 
     // Should throw an OperationError because the buffer is not mapped.
     // Note: not a RangeError because the state of the buffer is checked first.
-    t.shouldThrow('OperationError', () => {
-      errorBuffer.getMappedRange();
-    });
+    t.shouldThrow(
+      'OperationError',
+      () => {
+        errorBuffer.getMappedRange();
+      },
+      { checkForStackProperty: true }
+    );
 
     // Should't be a validation error even if the buffer failed to be mapped.
     errorBuffer.unmap();

diff --git a/src/unittests/loaders_and_trees.spec.ts b/src/unittests/loaders_and_trees.spec.ts
@@ -703,7 +703,7 @@ async function testIterateCollapsed(
     subqueriesToExpand: expectations,
   });
   if (expectedResult === 'throws') {
-    t.shouldReject('Error', treePromise, 'loadTree should have thrown Error');
+    t.shouldReject('Error', treePromise);
     return;
   }
   const tree = await treePromise;

diff --git a/src/unittests/test_group.spec.ts b/src/unittests/test_group.spec.ts
@@ -206,7 +206,7 @@ g.test('invalid_test_name').fn(t => {
       () => {
         g.test(name).fn(() => {});
       },
-      name
+      { message: name }
     );
   }
 });
@@ -353,7 +353,8 @@ g.test('shouldReject').fn(async t0 => {
     'TypeError',
     (async () => {
       throw new TypeError();
-    })()
+    })(),
+    { checkForStackProperty: true }
   );
 
   const g = makeTestGroupForUnitTesting(UnitTest);
@@ -363,7 +364,8 @@ g.test('shouldReject').fn(async t0 => {
       'Error',
       (async () => {
         throw new TypeError();
-      })()
+      })(),
+      { checkForStackProperty: true }
     );
   });
 

diff --git a/src/webgpu/api/operation/adapter/requestDevice.spec.ts b/src/webgpu/api/operation/adapter/requestDevice.spec.ts
@@ -118,25 +118,31 @@ g.test('stale')
         // Cause a type error by requesting with an unknown feature.
         if (awaitInitialError) {
           await assertReject(
-            adapter.requestDevice({ requiredFeatures: ['unknown-feature' as GPUFeatureName] })
+            'TypeError',
+            adapter.requestDevice({ requiredFeatures: ['unknown-feature' as GPUFeatureName] }),
+            { checkForStackProperty: true }
           );
         } else {
           t.shouldReject(
             'TypeError',
-            adapter.requestDevice({ requiredFeatures: ['unknown-feature' as GPUFeatureName] })
+            adapter.requestDevice({ requiredFeatures: ['unknown-feature' as GPUFeatureName] }),
+            { checkForStackProperty: true }
           );
         }
         break;
       case 'OperationError':
         // Cause an operation error by requesting with an alignment limit that is not a power of 2.
         if (awaitInitialError) {
           await assertReject(
-            adapter.requestDevice({ requiredLimits: { minUniformBufferOffsetAlignment: 255 } })
+            'OperationError',
+            adapter.requestDevice({ requiredLimits: { minUniformBufferOffsetAlignment: 255 } }),
+            { checkForStackProperty: true }
           );
         } else {
           t.shouldReject(
             'OperationError',
-            adapter.requestDevice({ requiredLimits: { minUniformBufferOffsetAlignment: 255 } })
+            adapter.requestDevice({ requiredLimits: { minUniformBufferOffsetAlignment: 255 } }),
+            { checkForStackProperty: true }
           );
         }
         break;
@@ -184,7 +190,8 @@ g.test('features,unknown')
 
     t.shouldReject(
       'TypeError',
-      adapter.requestDevice({ requiredFeatures: ['unknown-feature' as GPUFeatureName] })
+      adapter.requestDevice({ requiredFeatures: ['unknown-feature' as GPUFeatureName] }),
+      { checkForStackProperty: true }
     );
   });
 
@@ -208,7 +215,7 @@ g.test('features,known')
       const device = await promise;
       t.expect(device.features.has(feature), 'Device should include the required feature');
     } else {
-      t.shouldReject('TypeError', promise);
+      t.shouldReject('TypeError', promise, { checkForStackProperty: true });
     }
   });
 
@@ -225,7 +232,9 @@ g.test('limits,unknown')
 
     const requiredLimits: Record<string, number> = { unknownLimitName: 9000 };
 
-    t.shouldReject('OperationError', adapter.requestDevice({ requiredLimits }));
+    t.shouldReject('OperationError', adapter.requestDevice({ requiredLimits }), {
+      checkForStackProperty: true,
+    });
   });
 
 g.test('limits,supported')
@@ -306,7 +315,9 @@ g.test('limit,better_than_supported')
       [limit]: clamp(value, { min: 0, max: limitInfo[limit].maximumValue }),
     };
 
-    t.shouldReject('OperationError', adapter.requestDevice({ requiredLimits }));
+    t.shouldReject('OperationError', adapter.requestDevice({ requiredLimits }), {
+      checkForStackProperty: true,
+    });
   });
 
 g.test('limit,worse_than_default')
@@ -369,6 +380,8 @@ g.test('limit,worse_than_default')
       );
       device.destroy();
     } else {
-      t.shouldReject('OperationError', adapter.requestDevice({ requiredLimits }));
+      t.shouldReject('OperationError', adapter.requestDevice({ requiredLimits }), {
+        checkForStackProperty: true,
+      });
     }
   });
diff --git a/src/webgpu/api/operation/buffers/map_ArrayBuffer.spec.ts b/src/webgpu/api/operation/buffers/map_ArrayBuffer.spec.ts
@@ -56,7 +56,9 @@ g.test('postMessage')
     });
 
     if (transfer) {
-      t.shouldThrow('TypeError', () => mc.port1.postMessage(ab1, [ab1]));
+      t.shouldThrow('TypeError', () => mc.port1.postMessage(ab1, [ab1]), {
+        checkForStackProperty: true,
+      });
       // Wait to make sure the postMessage isn't received.
       await new Promise(resolve => timeout(resolve, 100));
     } else {
@@ -85,5 +87,7 @@ g.test('postMessage')
     t.expect(ab1.byteLength === 0, 'after unmap, ab1 should be detached');
 
     // Transferring an already-detached ArrayBuffer is a DataCloneError.
-    t.shouldThrow('DataCloneError', () => mc.port1.postMessage(ab1, [ab1]));
+    t.shouldThrow('DataCloneError', () => mc.port1.postMessage(ab1, [ab1]), {
+      checkForStackProperty: true,
+    });
   });
diff --git a/src/webgpu/api/operation/buffers/map_oom.spec.ts b/src/webgpu/api/operation/buffers/map_oom.spec.ts
@@ -39,7 +39,7 @@ g.test('mappedAtCreation')
     if (oom) {
       // getMappedRange is normally valid on OOM buffers, but this one fails because the
       // (default) range is too large to create the returned ArrayBuffer.
-      t.shouldThrow('RangeError', f);
+      t.shouldThrow('RangeError', f, { checkForStackProperty: true });
     } else {
       const buffer = f();
       const mapping = buffer.getMappedRange();

diff --git a/src/webgpu/api/validation/buffer/mapping.spec.ts b/src/webgpu/api/validation/buffer/mapping.spec.ts
@@ -45,6 +45,7 @@ class F extends ValidationTest {
         assert(expectation.rejectName === null, 'mapAsync unexpectedly passed');
       } catch (ex) {
         assert(ex instanceof Error, 'mapAsync rejected with non-error');
+        assert(typeof ex.stack === 'string', 'mapAsync rejected without a stack');
         assert(expectation.rejectName === ex.name, `mapAsync rejected unexpectedly with: ${ex}`);
         assert(
           expectation.earlyRejection === rejectedEarly,
@@ -62,9 +63,13 @@ class F extends ValidationTest {
         this.expect(data.byteLength === size);
       }
     } else {
-      this.shouldThrow('OperationError', () => {
-        buffer.getMappedRange(offset, size);
-      });
+      this.shouldThrow(
+        'OperationError',
+        () => {
+          buffer.getMappedRange(offset, size);
+        },
+        { checkForStackProperty: true }
+      );
     }
   }
 

diff --git a/src/webgpu/api/validation/capability_checks/features/query_types.spec.ts b/src/webgpu/api/validation/capability_checks/features/query_types.spec.ts
@@ -38,9 +38,13 @@ g.test('createQuerySet')
     const count = 1;
     const shouldException = type === 'timestamp' && !featureContainsTimestampQuery;
 
-    t.shouldThrow(shouldException ? 'TypeError' : false, () => {
-      t.device.createQuerySet({ type, count });
-    });
+    t.shouldThrow(
+      shouldException ? 'TypeError' : false,
+      () => {
+        t.device.createQuerySet({ type, count });
+      },
+      { checkForStackProperty: true }
+    );
   });
 
 g.test('writeTimestamp')
@@ -70,7 +74,11 @@ g.test('writeTimestamp')
     });
     const encoder = t.createEncoder('non-pass');
 
-    t.shouldThrow(featureContainsTimestampQuery ? false : 'TypeError', () => {
-      encoder.encoder.writeTimestamp(querySet, 0);
-    });
+    t.shouldThrow(
+      featureContainsTimestampQuery ? false : 'TypeError',
+      () => {
+        encoder.encoder.writeTimestamp(querySet, 0);
+      },
+      { checkForStackProperty: true }
+    );
   });