1509542 - String length is limited to ~256mb

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Josh Matthews [:jdm]]

I'm trying to use Gecko's reftest analyzer (layout/tools/reftest/reftest-analyzer-structured.xhtml) to analyze an 800mb log file. The page uses the FileReader readAsText API, which makes the result of reading the file available as a JS string. Opening my log file gives me "allocation size overflow" on the line that tries to access the string result; but Chrome is able to load the log without any issue (apart from the speed of parsing 800mb of JSON).

Comment 1

[Jan de Mooij [:jandem]]

|(1 << 28) - 1| limit is from when we used 4 bits for flags, but we no longer do that.

It's probably safe to bump to |(1 << 30) - 1|, 4 times as big. More than that migth be more prone to overflow.

JSC has MaxLength INT32_MAX.

Comment 2

[Jan de Mooij [:jandem]]

V8 has the following max string length [0]:

* 32-bit: (1 << 28) - 16
* 64-bit: internal::kSmiMaxValue / 2 - 24

kSmiMaxValue is 2^31 or so on 64-bit, so their max string length will be a bit less than 1 << 30 on 64-bit.

Moving to (1 << 30) - 1 seems reasonable and would align us with Chrome.

[0] https://github.com/v8/v8/blob/96a039aab14b71069e88c4c04543e6bc9514a0d6/include/v8.h#L2650-L2652

Comment 3

[Ted Campbell [:tcampbell]]

I can't think of any objections. 2**30 seems reasonable to avoid re-auditing for overflow issues.

Comment 4

[Jan de Mooij [:jandem]]

There are three places in builtin/String.cpp where we would need fixes because static_assert failures:

* https://searchfox.org/mozilla-central/rev/20df68a5f5b5e078a11fa62a681f09debda61d79/js/src/builtin/String.cpp#146
* https://searchfox.org/mozilla-central/rev/20df68a5f5b5e078a11fa62a681f09debda61d79/js/src/builtin/String.cpp#979
* https://searchfox.org/mozilla-central/rev/20df68a5f5b5e078a11fa62a681f09debda61d79/js/src/builtin/String.cpp#1402

Seems fixable.

If I comment out these static_asserts, ",".repeat(2 ** 30 - 1) works fine in the shell.

Comment 5

[Jan de Mooij [:jandem]]

This successfully allocates 100-110 GB in the shell:
---
var arr = [];
for (var i = 0; i < 100; i++) {
    var s = String.fromCharCode(i).repeat(2 ** 30 - 1);
    arr.push(s);
    s.indexOf(",");
}
readline();
---

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Bug 1509542 part 1 - Fix Escape to not rely on result.length <= JSString::MAX_LENGTH. r?jwalden

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Bug 1509542 part 2 - Fix two static_asserts in Intl code. r?jwalden

Depends on D12877

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Bug 1509542 part 3 - Increase JSString max length from |2**28 - 1| to |2**30 - 2|. r?jwalden

We use |2**30 - 2| to ensure the size of a null-terminated char16_t buffer
still fits in int32_t.

The patch adds some tests. I tried to add similar tests for toUpperCase() and
toLocaleUpperCase("lt") (calling into ICU) but it makes the test very slow in debug builds.

Depends on D12878

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6867843444f
part 1 - Fix Escape to not rely on result.length <= JSString::MAX_LENGTH. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/b3f239eccb59
part 2 - Fix two static_asserts in Intl code. r=jwalden
https://hg.mozilla.org/integration/autoland/rev/cafc30a40d9f
part 3 - Increase JSString max length from |2**28 - 1| to |2**30 - 2|. r=jwalden

Comment 10

[Jan de Mooij [:jandem]]

Setting dev-doc-needed because it's probably worth mentioning in the "Firefox 65 for developers" release notes.

JS strings now have a maximum length of |2**30 - 2| (~1 GB) instead of |2**28 - 1| (~256 MB).

Comment 11

[Steve Fink [:sfink] [:s:]]

Hey, this is great! I had no idea this was in the works, or is now this straightforward. The hazard analysis was hitting this limit when reading JSON corresponding to js::wasm::BaseCompiler::emitBody() because of a massive number of temporaries. This change provides a bunch of breathing room. Thank you!

Comment 13

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/e6867843444f
https://hg.mozilla.org/mozilla-central/rev/b3f239eccb59
https://hg.mozilla.org/mozilla-central/rev/cafc30a40d9f

Comment 14

[Chris Mills [:cmills]]

(In reply to Jan de Mooij [:jandem] from comment #10)
> Setting dev-doc-needed because it's probably worth mentioning in the
> "Firefox 65 for developers" release notes.
> 
> JS strings now have a maximum length of |2**30 - 2| (~1 GB) instead of
> |2**28 - 1| (~256 MB).

I've added this to the Fx56 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/65#JavaScript

I'm not sure if we want to make a note of this anywhere in the actual docs, so I'll leave the dev-doc-needed open for now.

Comment 15

[Florian Scholz (Open Web Docs)]

I've also added this to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/length#Description

Does this look alright to you, :jandem?

Comment 16

[Jan de Mooij [:jandem]]

(In reply to Florian Scholz [:fscholz] (MDN) from comment #15)

I've also added this to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/length#Description

Does this look alright to you, :jandem?

Looks good. Thanks!