Skip to content

Mouse events crash when SAFE_HEAP is enabled #20992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
paradust7 opened this issue Jan 3, 2024 · 2 comments · Fixed by #21429
Closed

Mouse events crash when SAFE_HEAP is enabled #20992

paradust7 opened this issue Jan 3, 2024 · 2 comments · Fixed by #21429

Comments

@paradust7
Copy link

paradust7 commented Jan 3, 2024

I just upgraded emsdk and started getting a new crash.

Version of emscripten/emsdk:

emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.51 (c0c2ca1)
clang version 18.0.0git (https://github.com/llvm/llvm-project f2464ca317bfeeedddb7cbdea3c2c8ec487890bb)
Target: wasm32-unknown-emscripten
Thread model: posix
InstalledDir: /home/mtuser/minetest-wasm/emsdk/upstream/bin

Here's an example of the stack trace I got:

Uncaught RuntimeError: Aborted(Assertion failed: attempt to write non-integer (undefined) into integer heap)
at abort (minetest.js:613:40)
at assert (minetest.js:375:3)
at checkInt (minetest.js:887:2)
at checkInt32 (minetest.js:898:27)
at setValue_safe (minetest.js:2238:3)
at SAFE_HEAP_STORE (minetest.js:322:2)
at Object.uiEventHandlerFunc [as handlerFunc] (minetest.js:10551:3)
at jsEventHandler (minetest.js:6507:17)

Another example:

Uncaught RuntimeError: Aborted(Assertion failed: attempt to write non-integer (1364.4119319915771) into integer heap)
at abort (minetest.js:613:40)
at assert (minetest.js:375:3)
at checkInt (minetest.js:887:2)
at checkInt32 (minetest.js:898:27)
at setValue_safe (minetest.js:2238:3)
at SAFE_HEAP_STORE (minetest.js:322:2)
at fillMouseEventData (minetest.js:10427:2)
at Object.mouseEventHandlerFunc [as handlerFunc] (minetest.js:10436:3)
at HTMLCanvasElement.jsEventHandler (minetest.js:6507:17)

These are the lines triggering a crash:

HEAP32[idx + {{{ C_STRUCTS.EmscriptenMouseEvent.canvasX / 4 }}}] = e.clientX - rect.left;
HEAP32[idx + {{{ C_STRUCTS.EmscriptenMouseEvent.canvasY / 4 }}}] = e.clientY - rect.top;

HEAP32[idx + {{{ C_STRUCTS.EmscriptenMouseEvent.targetX / 4 }}}] = e.clientX - rect.left;
HEAP32[idx + {{{ C_STRUCTS.EmscriptenMouseEvent.targetY / 4 }}}] = e.clientY - rect.top;

{{{ makeSetValue('uiEvent', C_STRUCTS.EmscriptenUiEvent.detail, 'e.detail', 'i32') }}};

HEAP32[idx + {{{ C_STRUCTS.EmscriptenTouchPoint.targetX / 4}}}] = t.clientX - targetRect.left;
HEAP32[idx + {{{ C_STRUCTS.EmscriptenTouchPoint.targetY / 4}}}] = t.clientY - targetRect.top;

This list may not be exhaustive. Applying Math.round to the values being set made the errors stop. (except for e.detail, which sometimes not a number(?), so needed to be ignored in those cases)

@sbc100
Copy link
Collaborator

sbc100 commented Jan 4, 2024

Hmm.. this seems a little unfortunate since assigning to a typed array will automatically convert to an integer, but I think it worth keeping these checks around as long as there are only a few places we need to inject these explicit truncations.

I think will be slightly better to use >> 0 than Math.round since that is that behavior of setting an element of a typed array view IIUC.

@cwoffenden
Copy link
Contributor

cwoffenden commented Feb 26, 2024

This is caused by a combination of two things: mouse events are registered on a canvas (not the built-in targets, e.g. EMSCRIPTEN_EVENT_TARGET_DOCUMENT) and the browser is applying a zoom or other scaling factor.

A fix is in #21429 with an example of what happens with a zoom applied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants