Skip to content

Commit

Permalink
Return an error instead of panicking when canvas context is unavailab…
Browse files Browse the repository at this point in the history
…le (#3052)

* Low-level error handling in canvas context creation (for WebGPU and WebGL 2).

Part of fixing #3017. This commit changes the handling of `web_sys`
errors and nulls from `expect()` to returning `Err`, but it doesn't
actually affect the public API — that will be done in the next commit.

* Breaking: Change type of `create_surface()` functions to expose canvas errors.

Part of fixing #3017.
  • Loading branch information
kpreid authored Dec 1, 2022
1 parent 7960c5e commit 2209463
Show file tree
Hide file tree
Showing 10 changed files with 193 additions and 81 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ Additionally `Surface::get_default_config` now returns an Option and returns Non
+ let config = surface.get_default_config(&adapter).expect("Surface unsupported by adapter");
```

#### Fallible surface creation

`Instance::create_surface()` now returns `Result<Surface, CreateSurfaceError>` instead of `Surface`. This allows an error to be returned instead of panicking if the given window is a HTML canvas and obtaining a WebGPU or WebGL 2 context fails. (No other platforms currently report any errors through this path.) By @kpreid in [#3052](https://github.com/gfx-rs/wgpu/pull/3052/)

### Changes

#### General
Expand Down
40 changes: 24 additions & 16 deletions wgpu-core/src/instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -502,45 +502,53 @@ impl<G: GlobalIdentityHandlerFactory> Global<G> {
&self,
canvas: &web_sys::HtmlCanvasElement,
id_in: Input<G, SurfaceId>,
) -> SurfaceId {
) -> Result<SurfaceId, hal::InstanceError> {
profiling::scope!("Instance::create_surface_webgl_canvas");

let surface = Surface {
presentation: None,
gl: self.instance.gl.as_ref().map(|inst| HalSurface {
raw: {
inst.create_surface_from_canvas(canvas)
.expect("Create surface from canvas")
},
}),
gl: self
.instance
.gl
.as_ref()
.map(|inst| {
Ok(HalSurface {
raw: inst.create_surface_from_canvas(canvas)?,
})
})
.transpose()?,
};

let mut token = Token::root();
let id = self.surfaces.prepare(id_in).assign(surface, &mut token);
id.0
Ok(id.0)
}

#[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))]
pub fn create_surface_webgl_offscreen_canvas(
&self,
canvas: &web_sys::OffscreenCanvas,
id_in: Input<G, SurfaceId>,
) -> SurfaceId {
) -> Result<SurfaceId, hal::InstanceError> {
profiling::scope!("Instance::create_surface_webgl_offscreen_canvas");

let surface = Surface {
presentation: None,
gl: self.instance.gl.as_ref().map(|inst| HalSurface {
raw: {
inst.create_surface_from_offscreen_canvas(canvas)
.expect("Create surface from offscreen canvas")
},
}),
gl: self
.instance
.gl
.as_ref()
.map(|inst| {
Ok(HalSurface {
raw: inst.create_surface_from_offscreen_canvas(canvas)?,
})
})
.transpose()?,
};

let mut token = Token::root();
let id = self.surfaces.prepare(id_in).assign(surface, &mut token);
id.0
Ok(id.0)
}

#[cfg(dx12)]
Expand Down
61 changes: 41 additions & 20 deletions wgpu-hal/src/gles/web.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,32 +33,53 @@ impl Instance {
&self,
canvas: &web_sys::HtmlCanvasElement,
) -> Result<Surface, crate::InstanceError> {
let webgl2_context = canvas
.get_context_with_context_options("webgl2", &Self::create_context_options())
.expect("Cannot create WebGL2 context")
.and_then(|context| context.dyn_into::<web_sys::WebGl2RenderingContext>().ok())
.expect("Cannot convert into WebGL2 context");

*self.webgl2_context.lock() = Some(webgl2_context.clone());

Ok(Surface {
webgl2_context,
srgb_present_program: None,
swapchain: None,
texture: None,
presentable: true,
})
self.create_surface_from_context(
canvas.get_context_with_context_options("webgl2", &Self::create_context_options()),
)
}

pub fn create_surface_from_offscreen_canvas(
&self,
canvas: &web_sys::OffscreenCanvas,
) -> Result<Surface, crate::InstanceError> {
let webgl2_context = canvas
.get_context_with_context_options("webgl2", &Self::create_context_options())
.expect("Cannot create WebGL2 context")
.and_then(|context| context.dyn_into::<web_sys::WebGl2RenderingContext>().ok())
.expect("Cannot convert into WebGL2 context");
self.create_surface_from_context(
canvas.get_context_with_context_options("webgl2", &Self::create_context_options()),
)
}

/// Common portion of public `create_surface_from_*` functions.
///
/// Note: Analogous code also exists in the WebGPU backend at
/// `wgpu::backend::web::Context`.
fn create_surface_from_context(
&self,
context_result: Result<Option<js_sys::Object>, wasm_bindgen::JsValue>,
) -> Result<Surface, crate::InstanceError> {
let context_object: js_sys::Object = match context_result {
Ok(Some(context)) => context,
Ok(None) => {
// <https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext-dev>
// A getContext() call “returns null if contextId is not supported, or if the
// canvas has already been initialized with another context type”. Additionally,
// “not supported” could include “insufficient GPU resources” or “the GPU process
// previously crashed”. So, we must return it as an `Err` since it could occur
// for circumstances outside the application author's control.
return Err(crate::InstanceError);
}
Err(js_error) => {
// <https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext>
// A thrown exception indicates misuse of the canvas state. Ideally we wouldn't
// panic in this case, but for now, `InstanceError` conveys no detail, so it
// is more informative to panic with a specific message.
panic!("canvas.getContext() threw {js_error:?}")
}
};

// Not returning this error because it is a type error that shouldn't happen unless
// the browser, JS builtin objects, or wasm bindings are misbehaving somehow.
let webgl2_context: web_sys::WebGl2RenderingContext = context_object
.dyn_into()
.expect("canvas context is not a WebGl2RenderingContext");

*self.webgl2_context.lock() = Some(webgl2_context.clone());

Expand Down
8 changes: 6 additions & 2 deletions wgpu/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ wgsl = ["wgc?/wgsl"]
trace = ["serde", "wgc/trace"]
replay = ["serde", "wgc/replay"]
angle = ["wgc/angle"]
webgl = ["wgc"]
webgl = ["hal", "wgc"]
emscripten = ["webgl"]
vulkan-portability = ["wgc/vulkan-portability"]
expose-ids = []
Expand All @@ -100,9 +100,13 @@ optional = true
[dependencies.wgt]
workspace = true

[target.'cfg(any(not(target_arch = "wasm32"), target_os = "emscripten"))'.dependencies.hal]
[target.'cfg(not(target_arch = "wasm32"))'.dependencies.hal]
workspace = true

[target.'cfg(target_arch = "wasm32")'.dependencies.hal]
workspace = true
optional = true

[dependencies]
arrayvec.workspace = true
log.workspace = true
Expand Down
5 changes: 3 additions & 2 deletions wgpu/examples/framework.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,7 @@ async fn setup<E: Example>(title: &str) -> Setup {
let size = window.inner_size();

#[cfg(any(not(target_arch = "wasm32"), target_os = "emscripten"))]
let surface = instance.create_surface(&window);
let surface = instance.create_surface(&window).unwrap();
#[cfg(all(target_arch = "wasm32", not(target_os = "emscripten")))]
let surface = {
if let Some(offscreen_canvas_setup) = &offscreen_canvas_setup {
Expand All @@ -174,7 +174,8 @@ async fn setup<E: Example>(title: &str) -> Setup {
} else {
instance.create_surface(&window)
}
};
}
.unwrap();

(size, surface)
};
Expand Down
2 changes: 1 addition & 1 deletion wgpu/examples/hello-triangle/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ use winit::{
async fn run(event_loop: EventLoop<()>, window: Window) {
let size = window.inner_size();
let instance = wgpu::Instance::new(wgpu::Backends::all());
let surface = unsafe { instance.create_surface(&window) };
let surface = unsafe { instance.create_surface(&window) }.unwrap();
let adapter = instance
.request_adapter(&wgpu::RequestAdapterOptions {
power_preference: wgpu::PowerPreference::default(),
Expand Down
2 changes: 1 addition & 1 deletion wgpu/examples/hello-windows/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ struct Viewport {

impl ViewportDesc {
fn new(window: Window, background: wgpu::Color, instance: &wgpu::Instance) -> Self {
let surface = unsafe { instance.create_surface(&window) };
let surface = unsafe { instance.create_surface(&window) }.unwrap();
Self {
window,
background,
Expand Down
28 changes: 17 additions & 11 deletions wgpu/src/backend/direct.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,24 +219,30 @@ impl Context {
pub fn instance_create_surface_from_canvas(
self: &Arc<Self>,
canvas: &web_sys::HtmlCanvasElement,
) -> Surface {
let id = self.0.create_surface_webgl_canvas(canvas, ());
Surface {
) -> Result<Surface, crate::CreateSurfaceError> {
let id = self
.0
.create_surface_webgl_canvas(canvas, ())
.map_err(|hal::InstanceError| crate::CreateSurfaceError {})?;
Ok(Surface {
id,
configured_device: Mutex::default(),
}
})
}

#[cfg(all(target_arch = "wasm32", feature = "webgl", not(feature = "emscripten")))]
pub fn instance_create_surface_from_offscreen_canvas(
self: &Arc<Self>,
canvas: &web_sys::OffscreenCanvas,
) -> Surface {
let id = self.0.create_surface_webgl_offscreen_canvas(canvas, ());
Surface {
) -> Result<Surface, crate::CreateSurfaceError> {
let id = self
.0
.create_surface_webgl_offscreen_canvas(canvas, ())
.map_err(|hal::InstanceError| crate::CreateSurfaceError {})?;
Ok(Surface {
id,
configured_device: Mutex::default(),
}
})
}

#[cfg(target_os = "windows")]
Expand Down Expand Up @@ -943,13 +949,13 @@ impl crate::Context for Context {
&self,
display_handle: raw_window_handle::RawDisplayHandle,
window_handle: raw_window_handle::RawWindowHandle,
) -> Self::SurfaceId {
Surface {
) -> Result<Self::SurfaceId, crate::CreateSurfaceError> {
Ok(Surface {
id: self
.0
.instance_create_surface(display_handle, window_handle, ()),
configured_device: Mutex::new(None),
}
})
}

fn instance_request_adapter(
Expand Down
52 changes: 40 additions & 12 deletions wgpu/src/backend/web.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1026,23 +1026,51 @@ impl Context {
pub fn instance_create_surface_from_canvas(
&self,
canvas: &web_sys::HtmlCanvasElement,
) -> <Self as crate::Context>::SurfaceId {
let context: wasm_bindgen::JsValue = match canvas.get_context("webgpu") {
Ok(Some(ctx)) => ctx.into(),
_ => panic!("expected to get context from canvas"),
};
create_identified(context.into())
) -> Result<<Self as crate::Context>::SurfaceId, crate::CreateSurfaceError> {
self.create_surface_from_context(canvas.get_context("webgpu"))
}

pub fn instance_create_surface_from_offscreen_canvas(
&self,
canvas: &web_sys::OffscreenCanvas,
) -> <Self as crate::Context>::SurfaceId {
let context: wasm_bindgen::JsValue = match canvas.get_context("webgpu") {
Ok(Some(ctx)) => ctx.into(),
_ => panic!("expected to get context from canvas"),
) -> Result<<Self as crate::Context>::SurfaceId, crate::CreateSurfaceError> {
self.create_surface_from_context(canvas.get_context("webgpu"))
}

/// Common portion of public `instance_create_surface_from_*` functions.
///
/// Note: Analogous code also exists in the WebGL2 backend at
/// `wgpu_hal::gles::web::Instance`.
fn create_surface_from_context(
&self,
context_result: Result<Option<js_sys::Object>, wasm_bindgen::JsValue>,
) -> Result<<Self as crate::Context>::SurfaceId, crate::CreateSurfaceError> {
let context: js_sys::Object = match context_result {
Ok(Some(context)) => context,
Ok(None) => {
// <https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext-dev>
// A getContext() call “returns null if contextId is not supported, or if the
// canvas has already been initialized with another context type”. Additionally,
// “not supported” could include “insufficient GPU resources” or “the GPU process
// previously crashed”. So, we must return it as an `Err` since it could occur
// for circumstances outside the application author's control.
return Err(crate::CreateSurfaceError {});
}
Err(js_error) => {
// <https://html.spec.whatwg.org/multipage/canvas.html#dom-canvas-getcontext>
// A thrown exception indicates misuse of the canvas state. Ideally we wouldn't
// panic in this case ... TODO
panic!("canvas.getContext() threw {js_error:?}")
}
};
create_identified(context.into())

// Not returning this error because it is a type error that shouldn't happen unless
// the browser, JS builtin objects, or wasm bindings are misbehaving somehow.
let context: web_sys::GpuCanvasContext = context
.dyn_into()
.expect("canvas context is not a GPUCanvasContext");

Ok(create_identified(context))
}

pub fn queue_copy_external_image_to_texture(
Expand Down Expand Up @@ -1141,7 +1169,7 @@ impl crate::Context for Context {
&self,
_display_handle: raw_window_handle::RawDisplayHandle,
window_handle: raw_window_handle::RawWindowHandle,
) -> Self::SurfaceId {
) -> Result<Self::SurfaceId, crate::CreateSurfaceError> {
let canvas_attribute = match window_handle {
raw_window_handle::RawWindowHandle::Web(web_handle) => web_handle.id,
_ => panic!("expected valid handle for canvas"),
Expand Down
Loading

0 comments on commit 2209463

Please sign in to comment.