Josh Matthews [:jdm] Reporter
	Description • 6 years ago

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).

	Jan de Mooij [:jandem] Assignee
	Comment 1 • 6 years ago

|(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.

	Jan de Mooij [:jandem] Assignee
	Comment 2 • 6 years ago

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

	Ted Campbell [:tcampbell]
	Comment 3 • 6 years ago • Edited

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

	Jan de Mooij [:jandem] Assignee
	Comment 4 • 6 years ago

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.

	Jan de Mooij [:jandem] Assignee
	Comment 5 • 6 years ago • Edited

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();
---

	Jan de Mooij [:jandem] Assignee
	Comment 7 • 6 years ago

Depends on D12877

	Jan de Mooij [:jandem] Assignee
	Comment 8 • 6 years ago

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

	Pulsebot
	Comment 9 • 6 years ago

Pushed by jdemooij@mozilla.com:
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

	Jan de Mooij [:jandem] Assignee
	Comment 10 • 6 years ago

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).

	Steve Fink [:sfink] [:s:]
	Comment 11 • 6 years ago

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!

	Razvan Maries
	Comment 13 • 6 years ago
bugherder

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

	Chris Mills [:cmills]
	Comment 14 • 5 years ago

(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.