Fix race in Slintpad causing preview to panic (esp. in FF)

With the following snippet there was a race:

```
if (instance === null) {
    instance = await create();
    await instance.show();
} else {
    instance.create_with_existing_window(...);
}
```

When awaiting for the promise that show() returns to settle, we would re-enter
this function (due to a broken recursion guard, that's now removed).
When re-entering, create_with_existing_window() is called, which - in order to
access the slint::Window to re-use - must create the window adapter.
Since that didn't happen from within an event loop invocation, winit would panic.

Consequently, this patch set also makes create_with_existing_window() return
a promise, as well as hide() (for the sake of it).

The broken recursion guard is easy to fix (with a .finally() handler setting
the boolean back), but that would mean we end up not always rendering
the very latest sources. Plus, the same issue exists in wasm_preview.ts.

Now, in case of subsequent re-entrancy in this code, we end up creating
a bunch of create_with_exiting_window() promises, which will eventually
settle and then we go back to the original show() call that will show the canvas.
This commit is contained in:
Simon Hausmann 2023-06-14 16:02:54 +02:00 committed by Simon Hausmann
parent 74ae79bca7
commit 26f3aa84fa
3 changed files with 44 additions and 24 deletions

View file

@ -193,8 +193,26 @@ impl WrappedCompiledComp {
pub fn create_with_existing_window( pub fn create_with_existing_window(
&self, &self,
instance: WrappedInstance, instance: WrappedInstance,
) -> Result<WrappedInstance, JsValue> { ) -> Result<InstancePromise, JsValue> {
Ok(WrappedInstance(self.0.create_with_existing_window(instance.0.window()).unwrap())) Ok(JsValue::from(js_sys::Promise::new(&mut |resolve, reject| {
let comp = send_wrapper::SendWrapper::new(self.0.clone());
let instance = send_wrapper::SendWrapper::new(instance.0.clone_strong());
let resolve = send_wrapper::SendWrapper::new(resolve);
if let Err(e) = slint_interpreter::invoke_from_event_loop(move || {
let instance =
WrappedInstance(comp.take().create_with_existing_window(instance.take().window()).unwrap());
resolve.take().call1(&JsValue::UNDEFINED, &JsValue::from(instance)).unwrap_throw();
}) {
reject
.call1(
&JsValue::UNDEFINED,
&JsValue::from(
format!("internal error: Failed to queue closure for event loop invocation: {e}"),
),
)
.unwrap_throw();
}
})).unchecked_into::<InstancePromise>())
} }
} }
@ -212,17 +230,35 @@ impl WrappedInstance {
/// Marks this instance for rendering and input handling. /// Marks this instance for rendering and input handling.
#[wasm_bindgen] #[wasm_bindgen]
pub fn show(&self) -> Result<js_sys::Promise, JsValue> { pub fn show(&self) -> Result<js_sys::Promise, JsValue> {
self.invoke_from_event_loop_wrapped_in_promise(|instance| instance.show())
}
/// Hides this instance and prevents further updates of the canvas element.
#[wasm_bindgen]
pub fn hide(&self) -> Result<js_sys::Promise, JsValue> {
self.invoke_from_event_loop_wrapped_in_promise(|instance| instance.hide())
}
fn invoke_from_event_loop_wrapped_in_promise(
&self,
callback: impl FnOnce(
&slint_interpreter::ComponentInstance,
) -> Result<(), slint_interpreter::PlatformError>
+ 'static,
) -> Result<js_sys::Promise, JsValue> {
let callback = std::cell::RefCell::new(Some(callback));
Ok(js_sys::Promise::new(&mut |resolve, reject| { Ok(js_sys::Promise::new(&mut |resolve, reject| {
let inst_weak = self.0.as_weak(); let inst_weak = self.0.as_weak();
if let Err(e) = slint_interpreter::invoke_from_event_loop({ if let Err(e) = slint_interpreter::invoke_from_event_loop({
let resolve = send_wrapper::SendWrapper::new(resolve); let resolve = send_wrapper::SendWrapper::new(resolve);
let reject = send_wrapper::SendWrapper::new(reject.clone()); let reject = send_wrapper::SendWrapper::new(reject.clone());
let callback = send_wrapper::SendWrapper::new(callback.take().unwrap());
move || { move || {
let resolve = resolve.take(); let resolve = resolve.take();
let reject = reject.take(); let reject = reject.take();
let callback = callback.take();
match inst_weak.upgrade() { match inst_weak.upgrade() {
Some(instance) => match instance.show() { Some(instance) => match callback(&instance) {
Ok(()) => { Ok(()) => {
resolve.call0(&JsValue::UNDEFINED).unwrap_throw(); resolve.call0(&JsValue::UNDEFINED).unwrap_throw();
} }
@ -231,7 +267,7 @@ impl WrappedInstance {
.call1( .call1(
&JsValue::UNDEFINED, &JsValue::UNDEFINED,
&JsValue::from(format!( &JsValue::from(format!(
"Calling show() on ComponentInstance failed: {e}" "Invocation on ComponentInstance from within event loop failed: {e}"
)), )),
) )
.unwrap_throw(); .unwrap_throw();
@ -242,7 +278,7 @@ impl WrappedInstance {
.call1( .call1(
&JsValue::UNDEFINED, &JsValue::UNDEFINED,
&JsValue::from(format!( &JsValue::from(format!(
"Calling show() on ComponentInstance failed because instance was deleted too soon" "Invocation on ComponentInstance failed because instance was deleted too soon"
)), )),
) )
.unwrap_throw(); .unwrap_throw();
@ -261,11 +297,6 @@ impl WrappedInstance {
} }
})) }))
} }
/// Hides this instance and prevents further updates of the canvas element.
#[wasm_bindgen]
pub fn hide(&self) -> Result<(), JsValue> {
self.0.hide().map_err(|e| -> JsValue { format!("{e}").into() })
}
/// THIS FUNCTION IS NOT PART THE PUBLIC API! /// THIS FUNCTION IS NOT PART THE PUBLIC API!
/// Highlights instances of the requested component /// Highlights instances of the requested component

View file

@ -237,7 +237,7 @@ function getPreviewHtml(slint_wasm_interpreter_url: Uri): string {
if (component !== undefined) { if (component !== undefined) {
document.getElementById("slint_error_div").innerHTML = ""; document.getElementById("slint_error_div").innerHTML = "";
if (current_instance !== null) { if (current_instance !== null) {
current_instance = component.create_with_existing_window(current_instance); current_instance = await component.create_with_existing_window(current_instance);
} else { } else {
try { try {
slint.run_event_loop(); slint.run_event_loop();

View file

@ -120,7 +120,6 @@ class PreviewerBackend {
#canvas_id: string | null = null; #canvas_id: string | null = null;
#instance: slint_preview.WrappedInstance | null = null; #instance: slint_preview.WrappedInstance | null = null;
#to_highlight: HighlightInfo = { file: "", offset: 0 }; #to_highlight: HighlightInfo = { file: "", offset: 0 };
#is_rendering = false;
#picker_mode = false; #picker_mode = false;
constructor(client_port: MessagePort, lsp_port: MessagePort) { constructor(client_port: MessagePort, lsp_port: MessagePort) {
@ -142,15 +141,6 @@ class PreviewerBackend {
} }
if (m.data.command === "render") { if (m.data.command === "render") {
const port = m.ports[0]; const port = m.ports[0];
if (this.#is_rendering) {
port.postMessage({
type: "Error",
data: "Already rendering",
});
port.close();
return;
}
this.#is_rendering = true;
this.render( this.render(
m.data.style, m.data.style,
@ -192,8 +182,7 @@ class PreviewerBackend {
.catch((e) => { .catch((e) => {
port.postMessage({ type: "Error", data: e }); port.postMessage({ type: "Error", data: e });
port.close(); port.close();
}); })
this.#is_rendering = false;
} }
} catch (e) { } catch (e) {
client_port.postMessage({ type: "Error", data: e }); client_port.postMessage({ type: "Error", data: e });
@ -290,7 +279,7 @@ class PreviewerBackend {
this.#instance = await component.create(this.canvas_id!); // eslint-disable-line this.#instance = await component.create(this.canvas_id!); // eslint-disable-line
await this.#instance.show(); await this.#instance.show();
} else { } else {
this.#instance = component.create_with_existing_window( this.#instance = await component.create_with_existing_window(
this.#instance, this.#instance,
); );
this.configure_picker_mode(); this.configure_picker_mode();