Bug 234833 - WebAssembly i32.atomic.wait timeout value incorrectly interpreted by factor 1000
Summary: WebAssembly i32.atomic.wait timeout value incorrectly interpreted by factor 1000
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebAssembly (show other bugs)
Version: WebKit Nightly Build
Hardware: Mac (Intel) macOS 11
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2022-01-03 16:28 PST by JP Posma
Modified: 2022-03-16 13:24 PDT (History)
11 users (show)

See Also:


Attachments
Zipfile containing the same example as on https://github.com/janpaul123/wasm-atomic-safari-bug (34.46 KB, application/zip)
2022-01-03 16:28 PST, JP Posma
no flags Details
Patch (1.66 KB, patch)
2022-01-03 21:44 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff
Patch (1.60 KB, patch)
2022-01-03 22:10 PST, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description JP Posma 2022-01-03 16:28:52 PST
Created attachment 448260 [details]
Zipfile containing the same example as on https://github.com/janpaul123/wasm-atomic-safari-bug

When using the i32.atomic.wait instruction in WebAssembly, the timeout value seems to be interpreted by a factor 1000 too much. So, when using 1 second, it waits for 1000 seconds, and using 1 millisecond it waits for 1 second.

Builds tested:
- r287554 on https://webkit.org/build-archives/#mac-bigsur-x86_64%20arm64 (which reports as "Version 15.2 (16612.3.6.1.8, 16612)")
- Safari 15.2 on Big Sur (which is where I discovered this initially)
- Recent versions of Chrome, Firefox, and Edge (it behaves correctly on all those browsers).

It's a bit of work to reproduce, since you need to run the WebAssembly in a Web Worker, using a SharedArrayBuffer (which requires specific headers to be set). So I created a minimal test case, available at https://github.com/janpaul123/wasm-atomic-safari-bug as well as attached in a zip file. See the README.md for how to run it.

Note that using the Javascript equivalent "Atomics.wait" behaves correctly (though be aware that it takes a timeout in milliseconds instead of nanoseconds).

Thanks!
— JP
Comment 1 JP Posma 2022-01-03 17:21:35 PST
For completeness, here is the spec for this instruction: https://github.com/WebAssembly/threads/blob/main/proposals/threads/Overview.md#wait (not sure if there's a more official W3C version somewhere, too)

Also, props to my teammate Paras Sanghavi for finding this!
Comment 2 Yusuke Suzuki 2022-01-03 21:44:43 PST
Created attachment 448265 [details]
Patch
Comment 3 Yusuke Suzuki 2022-01-03 22:10:55 PST
Created attachment 448267 [details]
Patch
Comment 4 JP Posma 2022-01-04 09:53:53 PST
Thanks for the quick fix Yusuke! That looks good to me.
Comment 5 Michael Saboff 2022-01-04 09:56:16 PST
Comment on attachment 448267 [details]
Patch

r=me
Comment 6 EWS 2022-01-04 10:11:51 PST
Committed r287575 (245706@main): <https://commits.webkit.org/245706@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 448267 [details].
Comment 7 Radar WebKit Bug Importer 2022-01-04 10:12:19 PST
<rdar://problem/87097734>
Comment 8 Sam Clegg 2022-03-15 15:31:54 PDT
We had at least one user report the effect of this bug when using pthreads under emscripten: https://github.com/emscripten-core/emscripten/issues/16499.

Do we know how many versions of safari this bug might have shipped in?   Or maybe which devices were effected?   I'm hoping we can avoid doing some kind of  mitigation (perhaps reverting back to the JS API for some period of time).  Off my a factor 1000 in usleep() is pretty serious.
Comment 9 Keith Miller 2022-03-16 13:24:17 PDT
(In reply to Sam Clegg from comment #8)
> We had at least one user report the effect of this bug when using pthreads
> under emscripten: https://github.com/emscripten-core/emscripten/issues/16499.
> 
> Do we know how many versions of safari this bug might have shipped in?   Or
> maybe which devices were effected?   I'm hoping we can avoid doing some kind
> of  mitigation (perhaps reverting back to the JS API for some period of
> time).  Off my a factor 1000 in usleep() is pretty serious.

I believe this fix shipped in Safari 15.4. So anything before then will have the issue.