Add wasm-bindgen v0.2 handles for web_sys::HtmlCanvasElement and web_sys::OffscreenCanvas

[notgull]

Closes #102 by adding two new window handles. One represents a web_sys::HtmlCanvasElement registered into wasm-bindgen through its index and the other represents a web_sys::OffscreenCanvas in the same way. This allows the handles to contain arbitrary canvases without needing to rely on the id 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 rustwasm/wasm-bindgen#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.

[grovesNL]

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.

[notgull]

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.

This would require importing the web-sys crate, which is bad for a couple of reasons.

• It requires us to have cfg guards on the window handle itself, which is something I'm trying to avoid (Proposal: Remove all the cfg guards. #63)
• web-sys is a very heavy dependency for a crate that's supposed to be lean and mean.

[grovesNL]

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).

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.

[notgull]

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 Wakers.

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.

[grovesNL]

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.

[notgull]

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.

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.

[grovesNL]

My understanding is that it goes something like this because of the ownership transfer that happens when you go through IntoWasmAbi/FromWasmAbi:

• the owner of HtmlCanvasElement (e.g., winit) calls into_abi and gets a value like 10u32 and puts it into a raw window handle
• then the raw window handle is handed to another crate (e.g., wgpu) and 10u32 is converted back to HtmlCanvasElement (optionally wrapped with ManuallyDrop) and it's used for some initial setup code, eventually dropped
• then the raw window handle is used again (e.g., same crate as before or another) but 10u32 isn't actually valid to be used for the next from_abi call

[notgull]

• then the raw window handle is handed to another crate (e.g., wgpu) and 10u32 is converted back to HtmlCanvasElement (optionally wrapped with ManuallyDrop) 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.

[grovesNL]

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?

[daxpedda]

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.

[notgull]

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.

[daxpedda]

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.

[grovesNL]

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 JsValues so this is handled internally.

[daxpedda]

Instead I'd want it to be refcounted like regular JsValues 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 JsValues 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.

[notgull]

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.

[grovesNL]

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.

[notgull]

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.

[notgull]

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.

[grovesNL]

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 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.

1. I think a cfg guard to alias web types internally is different than the concerns raised in Proposal: Remove all the cfg guards. #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 Proposal: Remove all the cfg guards. #63

2. 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:

• unsafe and error-prone because it relies on the under-documented ABI conversion features of wasm-bindgen which in turn makes the ownership story unclear and easy to accidentally cause a use-after-free or leak canvases
• problematic trying to support common web use cases with a temporary canvas being handed off to the renderer

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 exposed¹.

¹ 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.

[madsmtm]

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.

[notgull]

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).

[Lokathor]

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.

[notgull]

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.

[grovesNL]

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.

[Lokathor]

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.

[notgull]

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.

[notgull]

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.

[grovesNL]

Ah that's too bad. Opened #138 to see if it's worthwhile to consider lifting that.

[notgull]

cc @grovesNL

[madsmtm]

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).

[daxpedda]

[..] maybe web-sys should be the one depending on raw-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.

[notgull]

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).

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.

[Lokathor]

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

[daxpedda]

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.

[madsmtm]

Nice, thanks @notgull!

[madsmtm]

@daxpedda: Do you have a reference for this? How guaranteed is it that there will never be two wasm-bindgen?

[daxpedda]

It is not 100% guaranteed.

But generally speaking the way wasm-bindgen works prevents this, because it encodes "stuff" into the Wasm binary that needs to be interpreted by the CLI tool, so multiple versions will always be incompatible unless wasm-bindgen becomes a completely different beast.

That said, it was discussed in the past to move wasm-bindgen to the component model (which isn't stable yet) and drop the CLI completely at some point. This would probably allow multiple versions of wasm-bindgen to co-exist in the same binary, but old versions will still be unusable. But considering that wasm-bindgen is not actively developed anymore and there are now things like wit-bindgen, I doubt this will ever happen.

    // the binary will simply fail to compile!

I believe it should compile, but you won't be able to use it. Without the CLI tool it won't work on any runtime and the CLI tool will fail if you have multiple versions.

[notgull]

+1 to all of your changes @madsmtm

I'd be fine with merging now and bikeshedding later