dawn.node: Implement WebIDL rules to reject Promise with exception
WebIDL requires that functions returning promises don't throw exceptions
and instead reject the promise with the exception. Implement this
behavior in idlgen and templates.
This simplifies the previous code that was introduced to manually do
this in Converter.h/.cpp.
Bug: None
Change-Id: Ie216f5182443fbd4a27104eb077f96a829c7eca4
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/159925
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: Ben Clayton <bclayton@google.com>
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
diff --git a/src/dawn/node/binding/Converter.cpp b/src/dawn/node/binding/Converter.cpp
index 1a5a1a7..0491b5f 100644
--- a/src/dawn/node/binding/Converter.cpp
+++ b/src/dawn/node/binding/Converter.cpp
@@ -44,12 +44,6 @@
for (auto& free : free_) {
free();
}
- assert(exception == nullptr);
-}
-
-Napi::Error Converter::AcquireException() {
- assert(exception != nullptr);
- return std::move(exception);
}
bool Converter::HasFeature(wgpu::FeatureName feature) {
@@ -1648,13 +1642,7 @@
}
bool Converter::Throw(Napi::Error&& error) {
- if (retainException) {
- assert(exception == nullptr);
- exception = std::move(error);
- } else {
- error.ThrowAsJavaScriptException();
- }
-
+ error.ThrowAsJavaScriptException();
return false;
}
diff --git a/src/dawn/node/binding/Converter.h b/src/dawn/node/binding/Converter.h
index 0630648..e577890 100644
--- a/src/dawn/node/binding/Converter.h
+++ b/src/dawn/node/binding/Converter.h
@@ -82,8 +82,8 @@
class Converter {
public:
explicit Converter(Napi::Env e) : env(e) {}
- Converter(Napi::Env e, wgpu::Device extensionDevice, bool retainExceptionIn = false)
- : env(e), device(std::move(extensionDevice)), retainException(retainExceptionIn) {}
+ Converter(Napi::Env e, wgpu::Device extensionDevice)
+ : env(e), device(std::move(extensionDevice)) {}
~Converter();
// Conversion function. Converts the interop type IN to the Dawn type OUT.
@@ -108,10 +108,6 @@
// Returns the Env that this Converter was constructed with.
inline Napi::Env Env() const { return env; }
- // Returns the Napi::Error stored from previous conversion and clears it. (Converter checks any
- // retained exception is acquired)
- Napi::Error AcquireException();
-
// BufferSource is the converted type of interop::BufferSource.
struct BufferSource {
void* data;
@@ -445,9 +441,6 @@
Napi::Env env;
wgpu::Device device = nullptr;
- bool retainException = false;
- Napi::Error exception;
-
bool HasFeature(wgpu::FeatureName feature);
// Function to be used when throwing a JavaScript exception because of issues during the
@@ -457,9 +450,6 @@
// if (some error case) {
// return Throw("Some error case description");
// }
- //
- // A variant also exists that doesn't take a string_view, so that more specific error types
- // can be thrown.
[[nodiscard]] bool Throw(std::string&& message);
[[nodiscard]] bool Throw(Napi::Error&& error);
diff --git a/src/dawn/node/binding/GPUAdapter.cpp b/src/dawn/node/binding/GPUAdapter.cpp
index 500d64f..dda3081 100644
--- a/src/dawn/node/binding/GPUAdapter.cpp
+++ b/src/dawn/node/binding/GPUAdapter.cpp
@@ -124,7 +124,6 @@
Napi::Env env,
interop::GPUDeviceDescriptor descriptor) {
wgpu::DeviceDescriptor desc{}; // TODO(crbug.com/dawn/1133): Fill in.
- interop::Promise<interop::Interface<interop::GPUDevice>> promise(env, PROMISE_INFO);
Converter conv(env);
std::vector<wgpu::FeatureName> requiredFeatures;
@@ -134,15 +133,13 @@
// requiredFeatures is a "sequence<GPUFeatureName>" so a Javascript exception should be
// thrown if one of the strings isn't one of the known features.
if (!conv(feature, required)) {
- Napi::Error::New(env, "invalid value for GPUFeatureName").ThrowAsJavaScriptException();
- return promise;
+ return {env, interop::kUnusedPromise};
}
requiredFeatures.emplace_back(feature);
}
if (!conv(desc.label, descriptor.label)) {
- Napi::Error::New(env, "invalid value for label").ThrowAsJavaScriptException();
- return promise;
+ return {env, interop::kUnusedPromise};
}
wgpu::RequiredLimits limits;
@@ -154,6 +151,7 @@
FOR_EACH_LIMIT(COPY_LIMIT)
#undef COPY_LIMIT
+ interop::Promise<interop::Interface<interop::GPUDevice>> promise(env, PROMISE_INFO);
for (auto [key, _] : descriptor.requiredLimits) {
promise.Reject(binding::Errors::OperationError(env, "Unknown limit \"" + key + "\""));
return promise;
diff --git a/src/dawn/node/binding/GPUBuffer.cpp b/src/dawn/node/binding/GPUBuffer.cpp
index add9700..0a0a765 100644
--- a/src/dawn/node/binding/GPUBuffer.cpp
+++ b/src/dawn/node/binding/GPUBuffer.cpp
@@ -56,12 +56,10 @@
interop::GPUSize64 offset,
std::optional<interop::GPUSize64> size) {
// Convert the mapMode and reject with the TypeError if it happens.
- Converter conv(env, device_, true);
+ Converter conv(env, device_);
wgpu::MapMode mode;
if (!conv(mode, modeIn)) {
- auto promise = interop::Promise<void>(env, PROMISE_INFO);
- promise.Reject(conv.AcquireException());
- return promise;
+ return {env, interop::kUnusedPromise};
}
// Early rejection when there is already a mapping pending.
diff --git a/src/dawn/node/binding/GPUDevice.cpp b/src/dawn/node/binding/GPUDevice.cpp
index 008f2b7..aad9bf4 100644
--- a/src/dawn/node/binding/GPUDevice.cpp
+++ b/src/dawn/node/binding/GPUDevice.cpp
@@ -377,13 +377,11 @@
interop::GPUComputePipelineDescriptor descriptor) {
using Promise = interop::Promise<interop::Interface<interop::GPUComputePipeline>>;
- Converter conv(env, device_, true);
+ Converter conv(env, device_);
wgpu::ComputePipelineDescriptor desc{};
if (!conv(desc, descriptor)) {
- Promise promise(env, PROMISE_INFO);
- promise.Reject(conv.AcquireException());
- return promise;
+ return {env, interop::kUnusedPromise};
}
struct Context {
@@ -422,13 +420,11 @@
interop::GPURenderPipelineDescriptor descriptor) {
using Promise = interop::Promise<interop::Interface<interop::GPURenderPipeline>>;
- Converter conv(env, device_, true);
+ Converter conv(env, device_);
wgpu::RenderPipelineDescriptor desc{};
if (!conv(desc, descriptor)) {
- Promise promise(env, PROMISE_INFO);
- promise.Reject(conv.AcquireException());
- return promise;
+ return {env, interop::kUnusedPromise};
}
struct Context {
diff --git a/src/dawn/node/interop/Core.h b/src/dawn/node/interop/Core.h
index 4e758be..0c4d3d5 100644
--- a/src/dawn/node/interop/Core.h
+++ b/src/dawn/node/interop/Core.h
@@ -31,6 +31,7 @@
#ifndef SRC_DAWN_NODE_INTEROP_CORE_H_
#define SRC_DAWN_NODE_INTEROP_CORE_H_
+#include <cassert>
#include <cstdint>
#include <limits>
#include <optional>
@@ -261,6 +262,10 @@
};
} // namespace detail
+// A tag used in a Promise constructor to say it won't be used and doesn't need to be initialized.
+struct UnusedPromiseTag {};
+static constexpr UnusedPromiseTag kUnusedPromise;
+
// Promise<T> is a templated wrapper around a JavaScript promise, which can
// resolve to the template type T.
template <typename T>
@@ -268,6 +273,9 @@
public:
// Constructor
Promise(Napi::Env env, const PromiseInfo& info) : PromiseBase(env, info) {}
+ Promise(Napi::Env env, UnusedPromiseTag) : PromiseBase(env, PROMISE_INFO) {
+ Reject("Unused Promise, you should never see this!");
+ }
// Resolve() fulfills the promise with the given value.
void Resolve(T&& value) const {
@@ -281,6 +289,9 @@
public:
// Constructor
Promise(Napi::Env env, const PromiseInfo& info) : PromiseBase(env, info) {}
+ Promise(Napi::Env env, UnusedPromiseTag) : PromiseBase(env, PROMISE_INFO) {
+ Reject("Unused Promise, you should never see this!");
+ }
// Resolve() fulfills the promise.
void Resolve() const { PromiseBase::Resolve(state_->deferred.Env().Undefined()); }
@@ -771,6 +782,25 @@
}
}
+// Calls a promise-returning function f, catching exceptions and returning them into a rejected
+// promise instead. This is used to implement the WebIDL semantics for exceptions in
+// promise-returning functions.
+template <typename F>
+Napi::Value CatchExceptionIntoPromise(Napi::Env env, F&& f) {
+ Napi::Value result = f();
+ assert(result.IsEmpty() || result.IsPromise());
+ if (!env.IsExceptionPending()) {
+ return result;
+ }
+
+ // For some reason these two calls MUST be in this order or Node crashes in Reject().
+ Napi::Error error = env.GetAndClearPendingException();
+ Napi::Promise::Deferred deferred = Napi::Promise::Deferred::New(env);
+
+ deferred.Reject(error.Value());
+ return deferred.Promise();
+}
+
} // namespace wgpu::interop
#endif // SRC_DAWN_NODE_INTEROP_CORE_H_
diff --git a/src/dawn/node/interop/WebGPU.cpp.tmpl b/src/dawn/node/interop/WebGPU.cpp.tmpl
index da134f0..d2190bd 100644
--- a/src/dawn/node/interop/WebGPU.cpp.tmpl
+++ b/src/dawn/node/interop/WebGPU.cpp.tmpl
@@ -197,7 +197,14 @@
}
{{- end}}
{{- range $m := $methods}}
+{{- $implSuffix := ""}}
+{{- if ReturnsPromise $m}}
+{{- $implSuffix = "Inner"}}
Napi::Value {{$m.Name}}(const Napi::CallbackInfo& info) {
+ return CatchExceptionIntoPromise(info.Env(), [&]() -> auto { return {{$m.Name}}Inner(info); });
+ }
+{{- end}}
+ Napi::Value {{$m.Name}}{{$implSuffix}}(const Napi::CallbackInfo& info) {
std::string error;
{{- range $overload_idx, $o := $m.Overloads}}
{{- $overloaded := gt (len $m.Overloads) 1}}
diff --git a/tools/src/cmd/idlgen/main.go b/tools/src/cmd/idlgen/main.go
index 000b063..4ddf628 100644
--- a/tools/src/cmd/idlgen/main.go
+++ b/tools/src/cmd/idlgen/main.go
@@ -149,6 +149,7 @@
"IsUnionType": is(ast.UnionType{}),
"Lookup": g.lookup,
"MethodsOf": methodsOf,
+ "ReturnsPromise": returnsPromise,
"SetlikeOf": setlikeOf,
"Title": strings.Title,
}
@@ -709,6 +710,31 @@
}
}
+// isPromiseType returns true if the type is 'Promise<T>'
+func isPromiseType(ty ast.Type) bool {
+ if ty, ok := ty.(*ast.ParametrizedType); ok {
+ return ty.Name == "Promise"
+ }
+ return false
+}
+
+// returnsPromise returns true if the ast.Method returns a Promise.
+func returnsPromise(obj interface{}) bool {
+ method, ok := obj.(*Method)
+ if !ok {
+ panic("Unhandled AST node type in hasAnnotation")
+ }
+
+ firstIsPromise := isPromiseType(method.Overloads[0].Type)
+ for _, o := range method.Overloads {
+ if isPromiseType(o.Type) != firstIsPromise {
+ panic("IDL has overloads that are not consistently returning Promises")
+ }
+ }
+
+ return firstIsPromise
+}
+
// setlikeOf returns the setlike ast.Pattern, if obj is a setlike interface.
func setlikeOf(obj interface{}) *ast.Pattern {
iface, ok := obj.(*ast.Interface)
@@ -784,4 +810,4 @@
// Do not modify this file directly
////////////////////////////////////////////////////////////////////////////////
-`
\ No newline at end of file
+`
