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!
<rdar://problem/39390672>
Created attachment 338473 [details] patch
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.
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.
Created attachment 338478 [details] patch
Comment on attachment 338478 [details] patch Clearing flags on attachment: 338478 Committed r230863: <https://trac.webkit.org/changeset/230863>
All reviewed patches have been landed. Closing bug.
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
(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 ?
(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.
(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.
Test fix landed in r230972: <http://trac.webkit.org/r230972>.
(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.
(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?
(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).
(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.
(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.