[test] Test configureAll #87

tlively · 2026-01-23T00:05:29Z

No description provided.

tlively · 2026-01-23T00:37:00Z

test/js-api/custom-descriptors/configure-all.any.js

+  // An empty data array will cause a trap.
+  assert_throws_js(WebAssembly.RuntimeError, () => {


V8 currently throws a CompileError here. I think trapping makes more sense because the initial read is out of the array bounds.

I chose a CompileError because of symmetry with decoding wire bytes. Changing the type of error is a one-liner so I don't care much.

That said, it's convenient to throw the same kind of error for all possible failures, because then various failure cases can all fall through to the same bit of code that allocates and throws the actual error object. That seems to be what this PR suggests, so I'm fine with that.

tlively · 2026-01-23T00:37:20Z

test/js-api/custom-descriptors/configure-all.any.js

+  // Extra prototypes will cause a trap.
+  assert_throws_js(WebAssembly.RuntimeError, () => {


V8 does not currently raise any error on extra contents in the arrays, but I think it's worth doing. See #86.

Sure, no problem.

tlively · 2026-01-23T00:40:22Z

test/js-api/custom-descriptors/configure-all.any.js

+  configureAll(makeProtosArray([proto]),
+               makeMethodsArray([null, null, null]),
+               makeDataArray(data),
+               null);


V8 currently traps on this, but I don't think it should be necessary to inspect the value of the methods or prototypes before trying to install them.

@jakobkummerow pointed out that requiring non-null values here allows the optimized method wrappers to skip a null check, so I'll go ahead and make that change.

Thanks. Also, for the record, we do need to inspect the value before installing it, because if it's a funcref then we have to fetch or allocate the JS wrapper for it. Conceptually this is a point where values transition from the Wasm world to the JS world (like extern.convert_any).

My personal preference would be to statically require an array of non-nullable (ref func).

tlively · 2026-01-23T00:41:46Z

test/js-api/custom-descriptors/configure-all.any.js

+  for (let disallowed of [null, undefined, nonextensible, unwritable, 10, "foo"]) {
+    const protosArray = makeProtosArray([disallowed]);
+    assert_throws_js(TypeError, () => {
+      configureAll(protosArray, methodsArray, dataArray, null);
+    });
+  }


V8 currently throws on unwritable, but not most of the other disallowed values. It also traps instead of throwing a TypeError when writing to null.

tlively · 2026-01-23T00:44:11Z

test/js-api/custom-descriptors/configure-all.any.js

+  for (disallowed of [null, undefined, 10, "foo", nonextensible, unwritable]) {
+    assert_throws_js(TypeError, () => {
+      configureAll(makeProtosArray([{}]),
+                   makeMethodsArray([makeStructWithProto]),
+                   makeDataArray(data),
+                   disallowed);
+    });


V8 generally has the same mismatches in behavior here as when failing to write properties to a prototype object.

tlively · 2026-01-23T00:44:50Z

test/js-api/custom-descriptors/configure-all.any.js

+  configureAll(makeProtosArray([proto]),
+               makeMethodsArray([makeStructWithProto, null, null, null]),
+               makeDataArray(data),
+               constructors);


Just like with the non-static methods, V8 traps on these null methods.

tlively · 2026-01-23T00:46:01Z

test/js-api/custom-descriptors/configure-all.any.js

+    0, // parent prototype index 0
+  ];
+
+  for (let allowed of [null, {}, Object.getPrototypeOf({}), new Proxy({}, {})]) {


V8 does not allow null or the Proxy here, but I don't think there's any reason to restrict what values are allowed as prototypes beyond the normal rules.

tlively · 2026-01-23T00:46:36Z

test/js-api/custom-descriptors/configure-all.any.js

+    0, // parent prototype index 0
+  ];
+
+  assert_throws_js(WebAssembly.RuntimeError, () => {


V8 throws a CompileError here instead of trapping and similar for other cases of invalid data. I don't feel too strongly about what kind of error we throw for this kind of error.

tlively · 2026-01-23T00:48:42Z

test/js-api/custom-descriptors/configure-all.any.js

+    1, // one method
+    0x00, // normal method
+    0x2, // two bytes in name
+    0b11011111, 0b00000000, // invalid UTF-8


V8 does not currently raise an error on invalid UTF-8. Instead it seems to take the raw byte stream as the name somehow.

The current implementation happens to use an existing helper function that silently inserts replacement characters (0xFFFD) for invalid UTF-8. There's no particular reason for that, and it's easy to fix.

rmahdav

LGTM, thanks Thomas!

rmahdav · 2026-01-23T18:32:39Z

test/js-api/custom-descriptors/configure-all.any.js

+    1, // one prototype
+    1, // one constructor
+    ...stringToBytes("Foo"),
+    3, // three static method configs


What will happen if we specify 3 as the count of method configs here but define less (2 methods) or more (4 methods) in the next lines? Could this also be one example of unconsumed elements?

Yes, exactly. The case where there are extra method configs is similar to the "extra data" test and the case where there are not enough method configs is similar to the "early end of data" test.

Got it, thanks!

jakobkummerow

LGTM with comments

jakobkummerow · 2026-01-23T14:25:44Z

test/js-api/custom-descriptors/configure-all.any.js

+  // Extra prototypes will cause a trap.
+  assert_throws_js(WebAssembly.RuntimeError, () => {


Sure, no problem.

jakobkummerow · 2026-01-23T14:29:21Z

test/js-api/custom-descriptors/configure-all.any.js

+  // An empty data array will cause a trap.
+  assert_throws_js(WebAssembly.RuntimeError, () => {


I chose a CompileError because of symmetry with decoding wire bytes. Changing the type of error is a one-liner so I don't care much.

That said, it's convenient to throw the same kind of error for all possible failures, because then various failure cases can all fall through to the same bit of code that allocates and throws the actual error object. That seems to be what this PR suggests, so I'm fine with that.

jakobkummerow · 2026-01-23T23:34:42Z

test/js-api/custom-descriptors/configure-all.any.js

+  configureAll(makeProtosArray([proto]),
+               makeMethodsArray([null, null, null]),
+               makeDataArray(data),
+               null);


Thanks. Also, for the record, we do need to inspect the value before installing it, because if it's a funcref then we have to fetch or allocate the JS wrapper for it. Conceptually this is a point where values transition from the Wasm world to the JS world (like extern.convert_any).

My personal preference would be to statically require an array of non-nullable (ref func).

jakobkummerow · 2026-01-23T23:35:25Z

test/js-api/custom-descriptors/configure-all.any.js

+  for (let disallowed of [null, undefined, nonextensible, unwritable, 10, "foo"]) {
+    const protosArray = makeProtosArray([disallowed]);
+    assert_throws_js(TypeError, () => {
+      configureAll(protosArray, methodsArray, dataArray, null);
+    });
+  }


jakobkummerow · 2026-01-23T23:37:46Z

test/js-api/custom-descriptors/configure-all.any.js

+    1, // one method
+    0x00, // normal method
+    0x2, // two bytes in name
+    0b11011111, 0b00000000, // invalid UTF-8


The current implementation happens to use an existing helper function that silently inserts replacement characters (0xFFFD) for invalid UTF-8. There's no particular reason for that, and it's easy to fix.

[test] Test configureAll

73e9c65

tlively requested review from jakobkummerow and rmahdav January 23, 2026 00:05

tlively commented Jan 23, 2026

View reviewed changes

rmahdav approved these changes Jan 23, 2026

View reviewed changes

jakobkummerow approved these changes Jan 23, 2026

View reviewed changes

		// An empty data array will cause a trap.
		assert_throws_js(WebAssembly.RuntimeError, () => {

		// Extra prototypes will cause a trap.
		assert_throws_js(WebAssembly.RuntimeError, () => {

[test] Test configureAll #87

Are you sure you want to change the base?

[test] Test configureAll #87

Uh oh!

Conversation

tlively commented Jan 23, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmahdav left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobkummerow left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants