Workaround for a TextDecoder bug in Safari causing a RangeError to be thrown

[bes]

TextDecoder in Safari has a limitation that causes it to throw RangeError after decoding more than 2GiB of data. This causes long running wasm programs that need to use TextDecoder to crash and start throwing RuntimeError with the message "Out of bounds memory access".

We work around the issue by tracking how much data has been decoded by any given TextDecoder, and replace it when it comes close to 2GiB, deducting a small margin of 1MiB which has been empirically shown to reduce the likelihood of miscounting (for unknown reasons) causing a RangeError to be thrown.

This commit also adds stricter handling of the kind of declaration used for TextDecoder and TextEncoder - TextDecoder always uses let because it needs to be mutable, and TextEncoder always uses const because it doesn't need to be mutable.

Fixes #4471

[bes]

I am not sure why I needed these changes. I am using rustc 1.86.0, maybe I should be using another version?

[bes]

Some unrelated clippy lints are failing, has CI rustc been updated without updating the code?

[bes]

There don't seem to be any tests (generated .js-files) that use the non-browser path of getText 🤔

[stephanemagnenat]

Some unrelated clippy lints are failing, has CI rustc been updated without updating the code?

@daxpedda do you have any input on these? The PR works around a critical bug on Safari and would be very useful for many people!

[stephanemagnenat]

@bes I opened an issue (#4482) as even main does not compile any more in CI.

[bes]

@bes I opened an issue (#4482) as even main does not compile any more in CI.

Thanks

[bes]

We've been running this fix in production for a week now, and the results are encouraging. Screenshots from Sentry.

Ignore the first column, it's low because of a quirk in how Sentry does data cutoff...

Can you guess which day the fix went out?

[stephanemagnenat]

@bes the CI has been fixed in latest main commit, could you rebase your PR please? That should help it getting merged, now that there are more active maintainers (see #4533) ;-)

[daxpedda]

I'm a tiny bit concerned about the 1MiB margin. This comment shows that it fails at 2,147,483,600 bytes and succeeds at 2,147,483,500 bytes. So the margin should be between 148 and 47 bytes.

Considering how reproducable this test looks like, it would be nice to find out the exact number it fails at. The precise number is actually not that important, it would just be nice to once and for all establish that it actually fails at a certain number and that its not flaky or anything.

In any case, until somebody finds the time to actually try this out we will leave it at the current 1MiB margin considering that there is real-world data behind it.

[bes]

I'm a tiny bit concerned about the 1MiB margin. This comment shows that it fails at 2,147,483,600 bytes and succeeds at 2,147,483,500 bytes. So the margin should be between 148 and 47 bytes.

Considering how reproducable this test looks like, it would be nice to find out the exact number it fails at. The precise number is actually not that important, it would just be nice to once and for all establish that it actually fails at a certain number and that its not flaky or anything.

In any case, until somebody finds the time to actually try this out we will leave it at the current 1MiB margin considering that there is real-world data behind it.

I tried a small margin at first but could not get it to work on my machine, so I grabbed a margin which I thought was large enough to avoid problems but small enough to not have any significance (1 MiB is 0.049% of 2GiB)

[daxpedda]

I tried a small margin at first but could not get it to work on my machine, so I grabbed a margin which I thought was large enough to avoid problems but small enough to not have any significance (1 MiB is 0.049% of 2GiB)

If you can actually try it, could you do the following:

const decoder = new TextDecoder()
const buffer1 = new ArrayBuffer(100)

for (let i = 0; i < 21474836; i++) {
    decoder.decode(buffer1)
}

const buffer2 = new ArrayBuffer(1)
let i = 2147483601

try {
    for (; i <= 2147483648; i++) {
        decoder.decode(buffer2)
    }
} catch {
    console.err(`failed at ${i} number of bytes`)
}

And check if it fails consistently at the same number of bytes.

[daxpedda]

I keep forgetting this: please add an entry to the changelog as well!

[bes]

Added a line in the changelog.

If you can actually try it, could you do the following:

I'm on vacation right now, supposed to go on a day trip, so pushing these change kinda in between packing. I would appreciate if we can skip this for now :)

[daxpedda]

I'm on vacation right now, supposed to go on a day trip, so pushing these change kinda in between packing. I would appreciate if we can skip this for now :)

Absolutely!

[stephanemagnenat]

@bes thank you so much for your work!

@daxpedda thank you for merging! Any plan for making a release soon? This fix would be very useful to us!

[daxpedda]

I not yet back to maintaining wasm-bindgen so my time is quite limited right now, I'll leave this to the other maintainers to decide.

My 2¢: making a release is relatively low cost so I favor making one as soon as requested.

[stephanemagnenat]

My 2¢: making a release is relatively low cost so I favor making one as soon as requested.

Is there an ideal way to make such a request? Should I open a new issue for that, or do other maintainers read closed PR?

[daxpedda]

You asking for it here was the request.
But please open a tracking issue so it doesn't get lost!