Skip to content
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

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

Closed
4 of 5 tasks
atjn opened this issue Mar 13, 2023 · 11 comments
Closed
4 of 5 tasks

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

atjn opened this issue Mar 13, 2023 · 11 comments
Labels
bug Something isn't working

Comments

@atjn
Copy link
Contributor

atjn commented Mar 13, 2023

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), <anonymous>:10:10)
      at a.<computed> [as colourspace] (file:///home/anton/Documents/GitHub/wasm-vips/lib/vips-node.mjs:152:259)
      at Context.<anonymous> (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.<anonymous> (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 (<anonymous>)
      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.<anonymous> (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), <anonymous>:8:10)
      at a.<computed> [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.<anonymous> (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.<anonymous> (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 kleisauke added the bug Something isn't working label Mar 14, 2023
kleisauke added a commit that referenced this issue Mar 14, 2023
To fix compatibility with Node v18.14.2+ and v19.4.0+.
@kleisauke
Copy link
Owner

I was aware of this and reported this a couple of weeks ago at nodejs/node#46446 (comment). 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 7f4de7a 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
Copy link
Contributor Author

atjn commented Mar 14, 2023

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

@kleisauke
Copy link
Owner

I just opened PR nodejs/node#47092 for this.

@atjn
Copy link
Contributor Author

atjn commented Mar 14, 2023

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
Copy link
Contributor Author

atjn commented Mar 14, 2023

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
Copy link
Owner

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 nodejs/node#47092 (comment) if it's needed/appropriate to submit a PR for that.

@atjn
Copy link
Contributor Author

atjn commented Apr 3, 2023

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
Copy link
Owner

In that case, commit nodejs/node@f226350 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
Copy link
Owner

Node 18.16.0 has been released with a fix for this.

This issue still affects Node v19.x, I just opened PR nodejs/node#47535 for that (commit nodejs/node@f226350 was supposed to be included in the upcoming Node v20 release).

@kleisauke
Copy link
Owner

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
Copy link
Contributor Author

atjn commented Jul 13, 2023

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants