1786834 - Make SharedImmutableStringsCache globally accessible singleton

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task, P1)

Description

[Tooru Fujisawa [:arai]]

parser internally uses SharedImmutableStringsCache, to allocate strings for ScriptSource.

frontend::ParserBase::setSourceMapInfo
  > ScriptSource::setDisplayURL
    > ScriptSource::setDisplayURL
      > ScriptSource::getOrCreateStringZ
        > GetOrCreateStringZ

https://searchfox.org/mozilla-central/rev/d01591796d5faccf762adb09a311d8ee12f7ca7f/js/src/vm/JSScript.cpp#1910-1912,1916

template <typename SharedT, typename CharT>
static SharedT GetOrCreateStringZ(JSContext* cx, ErrorContext* ec,
                                  UniquePtr<CharT[], JS::FreePolicy>&& str) {
...
      rt->sharedImmutableStrings().getOrCreate(std::move(str), lengthWithNull);

SharedImmutableStringsCache itself is thread-safe, so possible option is to expose SharedImmutableStringsCache and let the embedding pass it to FrontendContext, and use it inside frontend code.

Comment 1

[Tooru Fujisawa [:arai]]

Given that SharedImmutableStringsCache is already singleton held by top-level JSRuntime,
we can make it globally accessible singleton and make it independent from JSRuntime.

Comment 2

[Tooru Fujisawa [:arai]]

Also, same can be done for frontend::WellKnownParserAtoms, maybe in separate bug.

Comment 3

[Tooru Fujisawa [:arai]]

Attachment: Bug 1786834 - Part 1: Move SharedImmutableStringsCache out of JSRuntime and make it singleton. r?nbp!

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: Bug 1786834 - Part 2: Remove unnecessary JSContext parameters. r?nbp!

Depends on D160204

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba594d0b12b0
Part 1: Move SharedImmutableStringsCache out of JSRuntime and make it singleton. r=nbp
https://hg.mozilla.org/integration/autoland/rev/3280c912570a
Part 2: Remove unnecessary JSContext parameters. r=nbp

Comment 6

[Serban Stanca [:SerbanS]]

Backed out for causing build bustages on ParserAtom.cpp.

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/js/src/frontend/ParserAtom.cpp:1316:3: error: use of undeclared identifier 'initialized_'

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/20f28bf6c511
Part 1: Move SharedImmutableStringsCache out of JSRuntime and make it singleton. r=nbp
https://hg.mozilla.org/integration/autoland/rev/d56b9abc6a5c
Part 2: Remove unnecessary JSContext parameters. r=nbp

Comment 8

[Atila Butkovits]

Backed out for causing failures at ParserAtom.cpp.

Backout link: https://hg.mozilla.org/integration/autoland/rev/b7164776589657b7d7fd40d32268b2a489eed789

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=MHkCnzCRQdWrth8EqcXjBg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=a3ef21e747880d0a471d1e528ded67d5187a7ae1

Failure log: https://treeherder.mozilla.org/logviewer?job_id=396256209&repo=autoland&lineNumber=2452

Comment 9

[Atila Butkovits]

We also found this: https://treeherder.mozilla.org/logviewer?job_id=396256257&repo=autoland&lineNumber=3109

Comment 10

[Tooru Fujisawa [:arai]]

Apparently, PAC doesn't meed the assumption, and there can be multiple top-level JSRuntimes at the same time.
if that's the case, we should make the singleton initialization/free thread-safe and have ref-count.

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Tooru Fujisawa [:arai] from comment #10)

Apparently, PAC doesn't meed the assumption, and there can be multiple top-level JSRuntimes at the same time.
if that's the case, we should make the singleton initialization/free thread-safe and have ref-count.

Can we initialize the singleton in InitWithFailureDiagnostic and destroy in JS_ShutDown? That's what we do for other process-wide singletons.

Comment 12

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #11)

(In reply to Tooru Fujisawa [:arai] from comment #10)

Apparently, PAC doesn't meed the assumption, and there can be multiple top-level JSRuntimes at the same time.
if that's the case, we should make the singleton initialization/free thread-safe and have ref-count.

Can we initialize the singleton in InitWithFailureDiagnostic and destroy in JS_ShutDown? That's what we do for other process-wide singletons.

Sounds good :)
I'll move the init/free code there.

Comment 13

[Tooru Fujisawa [:arai]]

I'm seeing "self-hosted" string gets leaked on reftest:

https://treeherder.mozilla.org/jobs?repo=try&revision=4396f720d77a91bf170905221be6aa15f33feb13&selectedTaskRun=Xy3YUzKsTUWIkbuAtf8NuA.0

it looks like the filename of self-hosted JS.

https://searchfox.org/mozilla-central/rev/d7d2cc647772de15c4c5aa47f74d25d0e379e404/js/src/vm/SelfHosting.cpp#2298

options.setFileAndLine("self-hosted", 1);

and actually, the leak happens locally even without the patch

Comment 14

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #13)

and actually, the leak happens locally even without the patch

without the patch, it leaks JSRuntime.

WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and everything alive inside it, that is) AT JS_ShutDown TIME.  FIX THIS!

and the patch moves SharedImmutableStringsCache out of JSRuntime and that results in UAF in cached string.

Comment 15

[Tooru Fujisawa [:arai]]

The same JS Runtime leak happens even without my patch on automation,
and the leak itself isn't treated as job failure.

So, possible options are:

• (a) somehow fix the leak itself
• (b) if the leak is intentional there (something related to test harness?):
  • (b-1) support the case in SharedImmutableStringsCache destructor and tweak the lifetime of strings
  • (b-2) add some workaround to the assertion and just ignore the leak

Comment 16

[Tooru Fujisawa [:arai]]

the safe option for (b-1) would be to add ref count to those singletons, and let either JS_Shutdown or destroyRuntime destruct those singletons,
instead of always destruct them in JS_Shutdown.

Comment 17

[Jan de Mooij [:jandem]]

We have code to not release certain things if there are still live runtimes at shutdown time, for example here. Maybe that works for this too? We don't support re-initializing the JS engine after a JS_Shutdown (there are assertions IIRC).

Comment 18

[Tooru Fujisawa [:arai]]

(In reply to Jan de Mooij [:jandem] from comment #17)

We have code to not release certain things if there are still live runtimes at shutdown time, for example here. Maybe that works for this too? We don't support re-initializing the JS engine after a JS_Shutdown (there are assertions IIRC).

Sounds good!
I'll revert the singleton handling and add the !JSRuntime::hasLiveRuntimes() condition.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50256d26a267
Part 1: Move SharedImmutableStringsCache out of JSRuntime and make it singleton. r=nbp
https://hg.mozilla.org/integration/autoland/rev/65fb3dcd1e52
Part 2: Remove unnecessary JSContext parameters. r=nbp

Comment 20

[Cristian Tuns]

Backed out for causing spidermonkey bustages.

• Backout link
• Push with failures
• Failure Log
• Failure line: /builds/worker/checkouts/gecko/js/src/vm/SharedImmutableStringsCache.cpp:127:8: error: incomplete type 'JSRuntime' named in nested name specifier

And also this: https://treeherder.mozilla.org/logviewer?job_id=397546713&repo=autoland

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cef93fc024cf
Part 1: Move SharedImmutableStringsCache out of JSRuntime and make it singleton. r=nbp
https://hg.mozilla.org/integration/autoland/rev/4ecae42a3ced
Part 2: Remove unnecessary JSContext parameters. r=nbp

Comment 22

[Cristian Tuns]

Backed out for causing one of the issues above(https://bugzilla.mozilla.org/show_bug.cgi?id=1786834#c20)

• Backout link
• Push with failures
• Failure Log
• Failure line: ==2796==ERROR: AddressSanitizer: attempting to call malloc_usable_size() for pointer which is not owned: 0x7fa17fae9520
  SUMMARY: AddressSanitizer: bad-malloc_usable_size /builds/worker/fetches/llvm-project/compiler-rt/lib/asan/asan_malloc_linux.cpp:119:3 in malloc_usable_size

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/07efd170adbf
Part 1: Move SharedImmutableStringsCache out of JSRuntime and make it singleton. r=nbp
https://hg.mozilla.org/integration/autoland/rev/a1f696ec2548
Part 2: Remove unnecessary JSContext parameters. r=nbp

Comment 24

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/07efd170adbf
https://hg.mozilla.org/mozilla-central/rev/a1f696ec2548