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

Throw exception when text encode alloc memory fail. #334

Open
xu-ms opened this issue Aug 8, 2024 · 3 comments
Open

Throw exception when text encode alloc memory fail. #334

xu-ms opened this issue Aug 8, 2024 · 3 comments

Comments

@xu-ms
Copy link

xu-ms commented Aug 8, 2024

What is the issue with the Encoding Standard?

In Edge and Chrome, we have found that TextEncoder Encoding Standard (whatwg.org) may encounter OOM (Out of Memory) errors when encode text, not only due to large memory allocation but also for some unknown reasons.
Our current strategy is to crash the renderer process. We now want to optimize this behavior by throwing an exception when memory allocation fails.

I tested encode with a long string in Firefox and Safari browsers.
Firefox will throw a exception "out of memory".
image
Safari will return a empty UInt8Array.

I noticed that the current standards do not define how to handle OOM (Out of Memory) situations.Do we need to establish such a standard?

@annevk
Copy link
Member

annevk commented Aug 12, 2024

Crashing the website process does actually seem preferable over having to define for each API what OOM means.

There's currently no standards effort that I'm aware of around standardizing OOM, but if anything it should probably be discussed within TC39 to start with.

cc @littledan @ljharb @codehag

@ljharb
Copy link

ljharb commented Aug 13, 2024

@ricea
Copy link
Collaborator

ricea commented Aug 13, 2024

In practice browsers have already abandoned consistent behaviour here, as some OOM failures cause more pain for users than others. XHR is particularly terrible in Blink, as it just silently replaces the response with an empty response in case of OOM.

In this specific case, there's no danger of leaving the user agent in an inconsistent state, as we have to do the allocation before anything else. However, user JavaScript may still be in an inconsistent state. In general, the exception will stop it continuing with erroneous calculations, but if it happens to be catching and discarding exceptions when encode() is called that could be dangerous.

I recognize the wisdom of the oom-fails-fast proposal, but it doesn't reflect what engines actually do. I've had to fix security holes in Blink caused by stack overflow throwing an exception in places that the standard says should never throw.

My philosophy is that OOM should continue to crash in most cases, but we need to pragmatically carve out exceptions in cases that cause particular user pain. As such, I'd like to attempt convergence between browsers on those exceptions, and one way to do that is to specifically mention them in the relevant standards.

aarongable pushed a commit to chromium/chromium that referenced this issue Aug 13, 2024
In the current encode method, DOMArrayBuffer::Create is used to create
a DOMUint8Array. When creation fails, it directly triggers an
OOM crash.

There are a large number of crashes here, so we want to mitigate them 
by using DOMUint8Array::CreateOrNull and throwing an exception when 
creation fails.

Bug: 355018938
Change-Id: I3976ff1b330b5611ded8f8cbfadfef93f2d39e37
Blink-Chat: https://groups.google.com/a/chromium.org/g/blink-dev/c/duKL2xRvIaw/m/0yhaqk5EBAAJ
Encoding-Github: whatwg/encoding#334
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5742992
Reviewed-by: Marijn Kruisselbrink <[email protected]>
Commit-Queue: xu ms <[email protected]>
Reviewed-by: Leon Han <[email protected]>
Reviewed-by: Adam Rice <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1340854}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants