search

kleisauke / wasm-vips

libvips for the browser and Node.js, compiled to WebAssembly with Emscripten.
https://kleisauke.github.io/wasm-vips/
MIT License
463 stars 25 forks source link

`wasm-vips` is unstable on Node `18.14.2+` and `19.4.0+` #45

Closed atjn closed 1 year ago

atjn commented 1 year ago

Node has recently made a change to the WASM interpreter that is causing errors in wasm-vips. The errors seem completely random and affect many modules, but the most affected module seems to be libspng which cannot decode even a simple png file.

The issue was introduced in Node 18.14.2 and 19.4.0. I was not able to reproduce this error in the Chromium browser, version 110.0.5481.177 and 112.0.5615.20. The issue does not affect wasm-vips version 0.0.4, although I am not sure when exactly the issue was "introduced". Maybe with the switch to Meson for libvips? I know for sure that b24ca0b and later is affected. I hope this is not OS-specific, but FYI I have only run these tests on Ubuntu 22.04, on an Intel 10th-gen x64 chip.

I noticed the errors while working on EWAB, but the test suite also reports errors on these versions:

Full test suite report

``` 1) colour cmyk: vips::Error: unable to call colourspace vips__file_open_read: unable to open file "cmyk" for reading unix error: No such file or directory profile_load: unable to load profile "cmyk" at Wc (file:///home/anton/Documents/GitHub/wasm-vips/lib/vips-node.mjs:125:428) at wasm://wasm/0152235e:wasm-function[4260]:0x1ae91f at Nd.Image$colourspace (eval at Ge (file:///home/anton/Documents/GitHub/wasm-vips/lib/vips-node.mjs:162:242), :10:10) at a. [as colourspace] (file:///home/anton/Documents/GitHub/wasm-vips/lib/vips-node.mjs:152:259) at Context. (file:///home/anton/Documents/GitHub/wasm-vips/test/unit/test_colour.js:212:21) at process.processImmediate (node:internal/timers:475:21) 2) connection image writeToTarget custom: AssertionError: expected 242 to equal +0 + expected - actual -242 +0 at Context. (file:///home/anton/Documents/GitHub/wasm-vips/test/unit/test_connection.js:161:55) at process.processImmediate (node:internal/timers:475:21) 3) foreign png: AssertionError: expected +0 to be close to 38671 +/- 0.0001 at file:///home/anton/Documents/GitHub/wasm-vips/test/unit/helpers.js:181:33 at Array.forEach () at Module.assertAlmostEqualObjects (file:///home/anton/Documents/GitHub/wasm-vips/test/unit/helpers.js:180:19) at pngValid (file:///home/anton/Documents/GitHub/wasm-vips/test/unit/test_foreign.js:396:15) at fileLoader (file:///home/anton/Documents/GitHub/wasm-vips/test/unit/test_foreign.js:79:5) at Context. (file:///home/anton/Documents/GitHub/wasm-vips/test/unit/test_foreign.js:402:5) at process.processImmediate (node:internal/timers:475:21) 4) foreign svgload: vips::Error: unable to load from file ./images/logo.svgz VipsForeignLoad: "./images/logo.svgz" is not a known file format at Wc (file:///home/anton/Documents/GitHub/wasm-vips/lib/vips-node.mjs:125:428) at wasm://wasm/0152235e:wasm-function[1301]:0x7e5ec at Function.Image$newFromFile (eval at Ge (file:///home/anton/Documents/GitHub/wasm-vips/lib/vips-node.mjs:162:242), :8:10) at a. [as newFromFile] (file:///home/anton/Documents/GitHub/wasm-vips/lib/vips-node.mjs:152:259) at fileLoader (file:///home/anton/Documents/GitHub/wasm-vips/test/unit/test_foreign.js:80:21) at Context. (file:///home/anton/Documents/GitHub/wasm-vips/test/unit/test_foreign.js:883:5) at process.processImmediate (node:internal/timers:475:21) 5) resample thumbnail: AssertionError: expected 48.23111979166666 to be below 1 + expected - actual -48.23111979166666 +1 at Context. (file:///home/anton/Documents/GitHub/wasm-vips/test/unit/test_resample.js:268:82) at process.processImmediate (node:internal/timers:475:21) ```

Pending fixes:

kleisauke commented 1 year ago

I was aware of this and reported this a couple of weeks ago at https://github.com/nodejs/node/pull/46446#issuecomment-1441578595. Unfortunately, that particular v8 commit is not cherry-picked at the time of writing.

This issue manifests with zlib-ng (both with v2.0.6 and the upcoming v2.0.7) when decompressing a zlib stream via inflate(), which means that wasm-vips v0.0.4 is also affected. Fortunately, building zlib-ng from the develop branch somehow(?) doesn't reproduce this.

I just pushed commit 7f4de7af9f494e4a17ba38f0d897120433b5caa0 as a workaround, but a better fix would be for Node.js to cherry-pick that particular v8 commit, as this may affect other Wasm projects as well.

atjn commented 1 year ago

I see, thank you! I will keep this bug open for tracking a more permanent fix :)

kleisauke commented 1 year ago

I just opened PR https://github.com/nodejs/node/pull/47092 for this.

atjn commented 1 year ago

Haha, I am currently compiling Node with the patch applied, with the intent to create that PR. I guess I will spare my poor CPU.

atjn commented 1 year ago

I am guessing that you are already well into getting a [v19.x] PR ready as well, but let me know if you want me to do anything. After all, I do have the patch ready 😄

kleisauke commented 1 year ago

I had a PR ready for v19.x, but I wasn't sure if commits from v18.x are cherry-picked to v19.x and/or whether there was a plan to update V8 there. Both would make that possible PR redundant.

I just asked at https://github.com/nodejs/node/pull/47092#issuecomment-1492948854 if it's needed/appropriate to submit a PR for that.

atjn commented 1 year ago

I could be wrong, but I believe the common workflow is to commit the fix directly to main, from where it will automatically be included in a 19.x (maybe 20.x now) release, and then also commit it to every LTS release that is no longer coupled to main, in this case just v18.x-staging.

kleisauke commented 1 year ago

In that case, commit https://github.com/nodejs/node/commit/f226350fcbebd4449fb0034fdaffa147e4de28ea would solve this as well. The corresponding PR of that commit does not have the dont-land-on-v19.x label, so hopefully that will be included in v19.x.

kleisauke commented 1 year ago

Node 18.16.0 has been released with a fix for this.

This issue still affects Node v19.x, I just opened PR https://github.com/nodejs/node/pull/47535 for that (commit https://github.com/nodejs/node/commit/f226350fcbebd4449fb0034fdaffa147e4de28ea was supposed to be included in the upcoming Node v20 release).

kleisauke commented 1 year ago

According to https://github.com/nodejs/release#end-of-life-releases Node v19.x has been EOLed, so I think v19.10.0 will not be released.

I'll close, thanks for reporting this!

atjn commented 1 year ago

It seems to me like they will release it at some point, but as you say, it really isn't important as long as all supported versions are fixed :)