1214346 - JSON.stringify should throw a catchable exception in case of OOM

Status: RESOLVED INVALID

(Core :: JavaScript: Standard Library, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

We probably already have hidden issues (dataloss) with Session Restore due to passing huge objects to JSON.stringify. Making the error catchable will let us at least report it, and behave intelligently.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Reproducible testcase:

js -e 'oomAfterAllocations(10); try { JSON.stringify({"a": {"b": {"c": 3}}}) } catch (e) { print("caught") }; print("Yay!")'

Comment 2

[Mike de Boer [:mikedeboer]]

Bump. Jason, is this something your team is willing to pick up, perhaps?

Comment 3

[Jason Orendorff [:jorendorff]]

OOM in JSON.stringify() throws the string "out of memory".

The test case in comment 1 fails because `oomAfterAllocations` causes all attempts to allocate memory to fail, not just inside JSON.stringify but afterwards as well. So what happens -- we throw "out of memory", try to enter the catch-block, but something in the interpreter has to allocate some memory to run that code (perhaps an environment object? I didn't check), and *that* fails, so instead of running the catch block we throw "out of memory" again. The script terminates with the uncaught exception, and we error out a third time trying to format the exception to print it. That's my guess, at least.

Change it to `oomAtAllocation` and it works:

    oomAtAllocation(10);
    try {
        JSON.stringify({"a": {"b": {"c": 3}}});
    } catch (e) {
        print("caught");
    }
    print("Yay!");

Prints:
    caught
    Yay!

The `oomTest` builtin is basically like a loop around oomAtAllocation(i) or oomAfterAllocation(i):

    oomTest(
        () => JSON.stringify({"a": {"b": {"c": 3}}}),
        {
            expectExceptionOnFailure: true,
            keepFailing: true  // or false, for oomAtAllocation behavior
        }
    );
    print("ok");

This also passes.

Comment 4

[Jason Orendorff [:jorendorff]]

Sorry, this got put onto an "I should really investigate this" pile and forgotten. I think this is not really a bug and the implications mentioned in comment 0 are not happening; --> RESO INVALID.

Comment 5

[Mike de Boer [:mikedeboer]]

That's a relief to hear :) Thanks, Jason!