Closed notgull closed 1 year ago
Could we expose the regular web-sys types (HtmlCanvasElement
and OffscreenCanvas
) instead? FromWasmAbi
and IntoWasmAbi
transfer ownership of the JavaScript object, so it's really easy to get this wrong and drop the window, ending up with a use-after-free by accident.
Could we expose the regular web-sys types (
HtmlCanvasElement
andOffscreenCanvas
) instead?FromWasmAbi
andIntoWasmAbi
transfer ownership of the JavaScript object, so it's really easy to get this wrong and drop the window, ending up with a use-after-free by accident.
This would require importing the web-sys
crate, which is bad for a couple of reasons.
cfg
guards on the window handle itself, which is something I'm trying to avoid (#63)web-sys
is a very heavy dependency for a crate that's supposed to be lean and mean.For what it's worth wgpu tried to use WasmAbi
functions for a similar use case and ended up having to back them out because of the ownership issues (https://github.com/gfx-rs/wgpu/issues/3430 has some more context if you're interested).
It requires us to have
cfg
guards on the window handle itself
Could we avoid this by making the web-sys handles type aliases or opaque types that use ()
on other targets?
web-sys
is a very heavy dependency for a crate that's supposed to be lean and mean.
Agreed but web-sys
will most likely be in use already for people using this target, and we could still use cfg/feature guards if anyone would like to opt-out.
For what it's worth wgpu tried to use
WasmAbi
functions for a similar use case and ended up having to back them out because of the ownership issues (gfx-rs/wgpu#3430 has some more context if you're interested).
We could likely solve this on our end by making the FromWasmAbi
results ManuallyDrop
. This is how it's done in other places where raw pointers are used like this, like in Waker
s.
Could we avoid this by making the web-sys handles type aliases or opaque types that use
()
on other targets?
This adds a layer of cfg
complexity I'm uncomfortable having.
At least anecdotally based on my experience removing the use of WasmAbi
in wgpu, I wouldn't feel confident accepting these handles and converting them back into web_sys
types. For example, if one raw window handle is reused to create two HtmlCanvasElement
from the u32
representation then this would break in a subtle way.
At least anecdotally based on my experience removing the use of
WasmAbi
in wgpu, I wouldn't feel confident accepting these handles and converting them back intoweb_sys
types. For example, if one raw window handle is reused to create twoHtmlCanvasElement
from theu32
representation then this would break in a subtle way.
Unless there's a subtlety I'm missing it seems like just wrapping the result of FromWasmAbi
in ManuallyDrop
should be enough to prevent this double-drop from occurring.
My understanding is that it goes something like this because of the ownership transfer that happens when you go through IntoWasmAbi
/FromWasmAbi
:
HtmlCanvasElement
(e.g., winit) calls into_abi
and gets a value like 10u32
and puts it into a raw window handle10u32
is converted back to HtmlCanvasElement
(optionally wrapped with ManuallyDrop
) and it's used for some initial setup code, eventually dropped10u32
isn't actually valid to be used for the next from_abi
call
- then the raw window handle is handed to another crate (e.g., wgpu) and
10u32
is converted back toHtmlCanvasElement
(optionally wrapped withManuallyDrop
) and it's used for some initial setup code, eventually dropped
Emphasis mine. As long as the canvas element is never dropped by the other crate there isn't a double drop. It can be either forgotten or wrapped in ManuallyDrop
as I did above. As long as the original window source is kept alive you can be sure that the canvas reference is valid, and this can now be done thanks to borrowed window handles.
In my example, wgpu wouldn't need the handle anymore so wgpu would drop it, but I thought this would invalidate the ABI handle for future reuse (e.g. passing it to wgpu or another crate). Is my understanding wrong?
In my example, wgpu wouldn't need the handle anymore so wgpu would drop it, but I thought this would invalidate the ABI handle for future reuse (e.g. passing it to wgpu or another crate).
I didn't follow the exact discussion so far, but if you drop the JsValue
, any "pointers" to it will become invalid indeed. A trait implementer could avoid this by cloning the JsValue
internally and return the "pointer" to that.
To sum up what was discussed in the Matrix room: the idea is that ownership isn't really passed through the idx
value to wgpu
/glutin
/softbuffer
/etc. The winit::Window
or whatever is what really owns the Canvas
; when the Window
is dropped the Canvas
is dropped.
The renderer, then, only owns a "reference" to that Canvas
. It would call FromWasmAbi
to get a Canvas
, place that in a ManuallyDrop
, and then never drop it. Semantically it's similar to a borrow. Once the renderer is dropped, the Canvas
isn't dropped with it; it's just left for the windowing system to drop it.
In chat this was compared to Arc::from_ptr()
and Arc::into_ptr()
. This strategy is actually used somewhat frequently in cases where pointers are being passed around. See this code from smol
for an example.
@daxpedda Does this implementation look right? As you're the resident expert on wasm-bindgen
I'll defer to you on this before I merge it.
As far as I understood the explained implementation, it is correct. It would all be unsafe though until you somehow introduce a lifetime that binds it to the owner of Canvas
.
It would all be unsafe though until you somehow introduce a lifetime that binds it to the owner of
Canvas
.
I think this is going to miss a common use case then unfortunately. I create a canvas and want to pass it to the graphics system, but I can't guarantee my reference will outlive the usage by the graphics system, so I'll either have to leak it or drop it and break the graphics system. Instead I'd want it to be refcounted like regular JsValue
s so this is handled internally.
Instead I'd want it to be refcounted like regular
JsValue
s so this is handled internally.
I know I said:
"bump the reference counter" by cloning the value
... in IRC, but this is misleading. When you clone the JsValue
, you bump the JS reference counter of the handle, if it is one, which is not useful in the context of this discussion. The cloned JsValue
has a different "pointer" then the one it was cloned from, they are two distinct JsValue
s at this point.
In our case, with a canvas, it just happens that what they both represent in JS turns out to be the same exact thing: the canvas.
I create a canvas and want to pass it to the graphics system, but I can't guarantee my reference will outlive the usage by the graphics system, so I'll either have to leak it or drop it and break the graphics system.
I don't know much about RWH, but my impression so far was that this is the same issue other backends have as well.
Yes, as @daxpedda said the "thing can be double dropped if you aren't careful" problem would not be unique to these handles. Pretty much every resource contained in these window handles have resources associated with them that can't be dropped twice. It's up the the windowing system to own it and the rendering system to not drop it.
I don't know much about RWH, but my impression so far was that this is the same issue other backends have as well.
That's true, although temporary canvases are a common pattern for wasm projects (in contrast to native windows which are typically long-lived), especially projects that aren't using a windowing system like winit. Those projects won't be able to use raw-window-handle using this approach without leaking. This would work well if we used web-sys types instead.
That's true, although temporary canvases are a common pattern for wasm projects (in contrast to native windows which are typically long-lived), especially projects that aren't using a windowing system like winit. Those projects won't be able to use raw-window-handle using this approach without leaking.
Using the new borrowed handle system it should be possible to parameterize rendering types by the object managing the canvas. This should make it easy from a user perspective to write a wrapper around temporary canvases to work in this manner.
This would work well if we used web-sys types instead.
One of the main sticking points for raw-window-handle
is its lack of cfg
guards and dependencies. I find the current solution, as hacky as it may be, preferable to going back on both of these points.
Given that there have been no comments on this thread for a while, and that the Web maintainer for winit
has signed off on this, it seems to me that there are no more open issues and that this can be merged.
I'll give it a week before I do just in case anyone else has any comments.
Using the new borrowed handle system it should be possible to parameterize rendering types by the object managing the canvas. This should make it easy from a user perspective to write a wrapper around temporary canvases to work in this manner.
In this case you'd want the rendering types to have a strong reference to the temporary canvas in this case, so the rendering types would effectively have to become the owner.
One of the main sticking points for
raw-window-handle
is its lack ofcfg
guards and dependencies. I find the current solution, as hacky as it may be, preferable to going back on both of these points.
I think a cfg
guard to alias web types internally is different than the concerns raised in #63 The point is that you have to be on the wasm target in order to use web-sys types anyway, so these are already guarded at some level (either at the crate level or wasm-specific paths in on both sides of handle creation/usage). In other words, downstream crates already have to guard these paths, so it shouldn't affect the ergonomics negatively as raised in #63
It's not possible to create or use these handles without web-sys already being somewhere in the dependency tree on this target, so while it's a nice goal to have as few dependencies as possible, it doesn't add dependencies if you actually want to use the handle. Almost any crate targeting the web/wasm will already have web-sys as a dependency.
I appreciate trying to be consistent in exposing these handles, and we can proceed with this if people believe it's the best path forward. I'd just like to reiterate that using these ABI handles is:
It would also be unfortunate to add this complexity back into wgpu after recently removing it because of the ownership/borrowing/refcounting challenges the ABI functions caused.
The alternative of wrapping web-sys types is what downstream crates would naturally expect, and these types are what people already pass today (if they're not going through raw-window-handle). I feel like the most pragmatic solution is to wrap the web-sys types and having a target-gated dependency, even though it's not consistent with how other handles are exposed1.
1 Although I'd argue other handles would also try to use something better than *mut c_void
if platforms/libraries could've historically agreed on a handle type that was stronger than that, like the handles web-sys provides in this case.
I'll propose another solution: Expose the JsValue
only through cfg
-gated methods, and enable the methods based on a feature flag (instead of a specific platform).
Something like the following (showing an extension to the hypothetical future of a web-sys v0.4
).
# Cargo.toml
[dependencies]
web-sys-0-3 = { package = "web-sys", version = "0.3" }
web-sys-0-4 = { package = "web-sys", version = "0.4" }
pub struct Wbg02CanvasWindowHandle(Inner);
// Private
enum Inner {
#[cfg(feature = "web-sys-0-3")]
V0_3(web_sys_0_3::JsValue),
#[cfg(feature = "web-sys-0-4")]
V0_4(web_sys_0_4::JsValue),
}
impl Wbg02CanvasWindowHandle {
#[cfg(feature = "web-sys-0-3")]
pub fn new_0_3(js_value: web_sys_0_3::JsValue) -> Self {
Self(Inner::V0_3(js_value))
}
#[cfg(feature = "web-sys-0-4")]
pub fn new_0_4(js_value: web_sys_0_4::JsValue) -> Self {
Self(Inner::V0_4(js_value))
}
#[cfg(feature = "web-sys-0-3")]
pub fn value_0_3(&self) -> Option<js_value: web_sys_0_3::JsValue> {
match self.inner {
Inner::V0_3(value) => Some(value),
_ => None
}
}
#[cfg(feature = "web-sys-0-4")]
pub fn value_0_4(&self) -> Option<js_value: web_sys_0_4::JsValue> {
match self.inner {
Inner::V0_4(value) => Some(value),
_ => None
}
}
}
This turns any version mismatch into a runtime error in the consuming library, which may or may not be the better option.
If we were to go this direction, I would prefer to use JsValue
instead of web-sys
types directly in order to avoid potential version churn.
I've just realized that the additional downside of including types like that would be that it would make it impossible to release raw-window-handle
version 1.0, and therefore other 1.0 releases of other crates (like, say, glutin
). Unless wasm-bindgen
is planning on bumping from v0.2 to v1.0 anytime soon it would essentially trap the entire Rust GUI ecosystem into an unstable version.
I'll still re-iterate my past points, as even with the proposed model it still becomes increasingly more complicated to construct handles with little added benefit (as the downstream consumer is expected to be using unsafe
anyways, I would hope that they closely read the documentation).
Getting this to 1.0 is absolutely a goal. However, we don't need to get the whole crate to 1.0 all at once. Even within rust itself it's common for some parts to stabilize before others (eg: inline asm).
We could gate the entire browser handle system behind optional dependencies/features and document it as still experimental even while the rest of the crate for desktop/mobile is moved up to 1.0. If necessary the feature can be called something more generic like browser
if we want to avoid favoring a particular crate.
I would like to avoid using a feature in this case. If we hit 1.0, then wasm-bindgen
or whatever hits 1.0, we'll have a hanging feature that does nothing that we can't get rid of. In this case I would prefer a cfg
directive, like --cfg rwh_semver_exempt_web_handles
.
Unless
wasm-bindgen
is planning on bumping from v0.2 to v1.0 anytime soon it would essentially trap the entire Rust GUI ecosystem into an unstable version.
For what it's worth, this is already the case in most crates that supporting this target, e.g. winit, wgpu, glow, egui's eframe, leptos, etc. all expose web_sys
types in their public API for the same reasons I've tried to outline - it's much more convenient to work with these typed handles than requiring them to go through the wasm ABI or similar, and the dependency will already be in-tree for this target.
I don't think a special cfg instead of a cargo feature is very user friendly. Users expect to be able to look at the features list and have it really be the list without secret extra options.
As to having a feature that eventually does nothing: It's really not a big deal. If we really wanted we could even remove the feature (since we're not promising the feature is part of our stable API in the first place) if it was really so offense to the sensibilities, but it's the best user experience to just let an opt-in feature "do nothing" once it's eventually part of the default setup.
Yeah, now that you put it that way. I'll rewrite this PR to use wasm-bindgen
then, if everyone else is fine with it.
Ah wait, JsValue
is not Copy
. That kind of throws a wrench in the entire thing, as we need RawWindowHandle
to be Copy
.
So, I think the current implementation of this PR is still the best way of doing it.
Ah that's too bad. Opened #138 to see if it's worthwhile to consider lifting that.
I've now replaced the wasm-bindgen handles such that they use actual wasm-bindgen objects. This also solves the Copy
issue mentioned in #142
cc @grovesNL
Another argument for keeping raw-window-handle
completely dependency-free: It can more easily be integrated into other projects, e.g. maybe web-sys
should be the one depending on raw-window-handle
?
Just a thought, I've at least thought about adding support for it in icrate
, but have held off because raw-window-handle
doesn't guarantee it won't have any dependencies (which is very important for low-level libraries to avoid a cyclic dependency in the future).
[..] maybe
web-sys
should be the one depending onraw-window-handle
?
It is unlikely to happen, web-sys
is supposed to be bindings only, even Serde support was removed/deprecated. It's a *-sys
crate after all.
Just a thought, I've at least thought about adding support for it in
icrate
, but have held off becauseraw-window-handle
doesn't guarantee it won't have any dependencies (which is very important for low-level libraries to avoid a cyclic dependency in the future).
The dependency is only present in web targets; I am committed to maintaining raw-window-handle
's dependency-free status for desktop and mobile platforms. So it should still be fine to add to icrate
.
It's a *-sys crate after all.
Even a sys crate usually declares types, and thus can implement appropriate traits on those types. Not that it has to depend on us and do that, but it would be reasonable
Even a sys crate usually declares types, and thus can implement appropriate traits on those types. Not that it has to depend on us and do that, but it would be reasonable
Wasn't a question of "can", but of "should". My impression so far is that *-sys
crates tend to have as few dependencies as possible and let others depend on them instead of the other way around.
This isn't a rule somewhere, so I don't think it's totally unreasonable.
Would anybody be interested to make a PR (when we get there) to web-sys
so that I can review it and discuss it with the others in Rustwasm? Myself I'm not familiar at all with raw-window-handle
.
Closes #102 by adding two new window handles. One represents a
web_sys::HtmlCanvasElement
registered intowasm-bindgen
through its index and the other represents aweb_sys::OffscreenCanvas
in the same way. This allows the handles to contain arbitrary canvases without needing to rely on theid
hack.The main issue with this PR is that the underlying representation for
wasm-bindgen
objects might change to something aside from the current index system. This was raised in https://github.com/rustwasm/wasm-bindgen/issues/1766. However, the index system has remained in place for five years and according to @daxpedda it isn't going anywhere soon. In addition a new system would likely require a breaking change. Therefore we should be fine with this system for the foreseeable future.