Bug 184846 – Handle more JSON stringify OOM

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[JF Bastien]

JSON stringinfication can OOM easily. Here's another case.

I'm working on a separate patch which teaches StringBuilder that it's fallible, but that's way bigger and more complex, so I'll commit it separately. It'll be much more comprehensive though!

[JF Bastien]

<rdar://problem/39390672>

[JF Bastien]

Created attachment 338473 [details]
patch

[EWS Watchlist]

Attachment 338473 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/text/StringBuilderJSON.cpp:100:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mark Lam]

Comment on attachment 338473 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=338473&action=review

r=me with fixes.

> Source/WTF/ChangeLog:9
> +        JSON stringinfication can OOM easily. Here's another case.

typo: /stringinfication/stringification/.

> Source/WTF/ChangeLog:13
> +        I'm working on a separate patch which teaches StringBuilder that
> +        it's fallible, but that's way bigger and more complex, so I'll
> +        commit it separately. It'll be much more comprehensive though!

I don't think you need to say this.

> Source/WTF/wtf/text/StringBuilderJSON.cpp:98
> +    if (is8Bit() && !string.is8Bit()) {

You don't need the curly brace here.

> Source/WTF/wtf/text/StringBuilderJSON.cpp:100
> +    } else

Ditto.

[JF Bastien]

Created attachment 338478 [details]
patch

[WebKit Commit Bot]

Comment on attachment 338478 [details]
patch

Clearing flags on attachment: 338478

Committed r230863: <https://trac.webkit.org/changeset/230863>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

The test added with this change is failing on the 32-bit JSC bot:

stress/json-stringified-overflow-2.js.default: Exception: Error: Out of memory

https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1718

[JF Bastien]

(In reply to Ryan Haddad from comment #8)
> The test added with this change is failing on the 32-bit JSC bot:
> 
> stress/json-stringified-overflow-2.js.default: Exception: Error: Out of
> memory
> 
> https://build.webkit.org/builders/Apple%20High%20Sierra%2032-
> bit%20JSC%20%28BuildAndTest%29/builds/1718

Shouldn't these bots not run tests marked with:

//@ skip if $memoryLimited

?

[Mark Lam]

(In reply to JF Bastien from comment #9)
> (In reply to Ryan Haddad from comment #8)
> > The test added with this change is failing on the 32-bit JSC bot:
> > 
> > stress/json-stringified-overflow-2.js.default: Exception: Error: Out of
> > memory
> > 
> > https://build.webkit.org/builders/Apple%20High%20Sierra%2032-
> > bit%20JSC%20%28BuildAndTest%29/builds/1718
> 
> Shouldn't these bots not run tests marked with:
> 
> //@ skip if $memoryLimited
> 
> ?

No.  The failing bot is a High Sierra bot which is not memory limited.  The test should catch the OOM error and verify that it did catch the error.

[Mark Lam]

(In reply to Mark Lam from comment #10)
> (In reply to JF Bastien from comment #9)
> > (In reply to Ryan Haddad from comment #8)
> > > The test added with this change is failing on the 32-bit JSC bot:
> > > 
> > > stress/json-stringified-overflow-2.js.default: Exception: Error: Out of
> > > memory
> > > 
> > > https://build.webkit.org/builders/Apple%20High%20Sierra%2032-
> > > bit%20JSC%20%28BuildAndTest%29/builds/1718
> > 
> > Shouldn't these bots not run tests marked with:
> > 
> > //@ skip if $memoryLimited
> > 
> > ?
> 
> No.  The failing bot is a High Sierra bot which is not memory limited.  The
> test should catch the OOM error and verify that it did catch the error.

I can reproduce this on a 32-bit build but only if I don't run jsc from inside lldb.  I'll just land a test fix to get the bots unstuck.

[Mark Lam]

Test fix landed in r230972: <http://trac.webkit.org/r230972>.

[JF Bastien]

(In reply to Mark Lam from comment #12)
> Test fix landed in r230972: <http://trac.webkit.org/r230972>.

That fix is incorrect. It's handling OOM from string *and* stringify. We only want to test OOM from stringify.

It seems to me like 32-bit bots should be marked as memory limited.

[Mark Lam]

(In reply to JF Bastien from comment #13)
> (In reply to Mark Lam from comment #12)
> > Test fix landed in r230972: <http://trac.webkit.org/r230972>.
> 
> That fix is incorrect. It's handling OOM from string *and* stringify. We
> only want to test OOM from stringify.
> 
> It seems to me like 32-bit bots should be marked as memory limited.

I disagree.  padStart() can throw an OutOfMemoryError, and it did in this case.  I understand that the test is to check if JSON.stringify() throws an OOM.  But what is the correct behavior for the test then?  I think if padStart() throws an OOM, then the test is over ... which is what I changed it to do.

How would you do this differently?

[JF Bastien]

(In reply to Mark Lam from comment #14)
> (In reply to JF Bastien from comment #13)
> > (In reply to Mark Lam from comment #12)
> > > Test fix landed in r230972: <http://trac.webkit.org/r230972>.
> > 
> > That fix is incorrect. It's handling OOM from string *and* stringify. We
> > only want to test OOM from stringify.
> > 
> > It seems to me like 32-bit bots should be marked as memory limited.
> 
> I disagree.  padStart() can throw an OutOfMemoryError, and it did in this
> case.  I understand that the test is to check if JSON.stringify() throws an
> OOM.  But what is the correct behavior for the test then?  I think if
> padStart() throws an OOM, then the test is over ... which is what I changed
> it to do.
> 
> How would you do this differently?

Mark 32-bit bots as memory limited, otherwise we're deluding ourselves that we're testing stringify on *any* bot (64-bit bots aren't testing stringify either as far as we know).

[Mark Lam]

(In reply to JF Bastien from comment #15)
> (In reply to Mark Lam from comment #14)
> > (In reply to JF Bastien from comment #13)
> > > (In reply to Mark Lam from comment #12)
> > > > Test fix landed in r230972: <http://trac.webkit.org/r230972>.
> > > 
> > > That fix is incorrect. It's handling OOM from string *and* stringify. We
> > > only want to test OOM from stringify.
> > > 
> > > It seems to me like 32-bit bots should be marked as memory limited.
> > 
> > I disagree.  padStart() can throw an OutOfMemoryError, and it did in this
> > case.  I understand that the test is to check if JSON.stringify() throws an
> > OOM.  But what is the correct behavior for the test then?  I think if
> > padStart() throws an OOM, then the test is over ... which is what I changed
> > it to do.
> > 
> > How would you do this differently?
> 
> Mark 32-bit bots as memory limited, otherwise we're deluding ourselves that
> we're testing stringify on *any* bot (64-bit bots aren't testing stringify
> either as far as we know).

The 32-bit bots are not memory limited by any stretch of the imagination.  I'm reproducing this on my iMac which tons of memory.  We could skip the tests for 32-bit builds, but I think that's not really fixing the core issue.  From my cursory look at the code briefly, I think padStart() could just as well throw an OOM on 6-bit as well.  

I filed https://bugs.webkit.org/show_bug.cgi?id=184941 for us to re-visit these tests later.  For now, the tests do get some coverage (thought not guaranteed to be the specific coverage we want) and the bots are green.

[Mark Lam]

(In reply to Mark Lam from comment #16)
> The 32-bit bots are not memory limited by any stretch of the imagination. 
> I'm reproducing this on my iMac which tons of memory.  We could skip the
> tests for 32-bit builds, but I think that's not really fixing the core
> issue.  From my cursory look at the code briefly, I think padStart() could
> just as well throw an OOM on 6-bit as well.  
> 
> I filed https://bugs.webkit.org/show_bug.cgi?id=184941 for us to re-visit
> these tests later.  For now, the tests do get some coverage (thought not
> guaranteed to be the specific coverage we want) and the bots are green.

By "6-bit", I meant 64-bit.