String length is limited to ~256mb

RESOLVED FIXED in Firefox 65

Status

()

defect
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: jdm, Assigned: jandem)

Tracking

({dev-doc-complete})

unspecified
mozilla65
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(3 attachments)

Reporter

Description

6 months 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).
Assignee

Comment 1

6 months 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.
Assignee

Comment 2

6 months 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
I can't think of any objections. 2**30 seems reasonable to avoid re-auditing for overflow issues.
Assignee

Comment 5

6 months ago
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();
---
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Assignee

Comment 8

6 months 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
Assignee

Updated

6 months ago
Blocks: 1313624

Comment 9

6 months 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
Assignee

Comment 10

6 months 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).
Keywords: dev-doc-needed
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!
Assignee

Updated

6 months ago
Duplicate of this bug: 1313624

Comment 13

6 months 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
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
(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.
Flags: needinfo?(jdemooij)
Assignee

Comment 16

4 months ago

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

Flags: needinfo?(jdemooij)
Depends on: 1518440
You need to log in before you can comment on or make changes to this bug.