1668335 - Crash in [@ js::frontend::ParserAtomEntry::toJSAtom]

Status: RESOLVED INCOMPLETE

(Core :: JavaScript Engine, defect, P1)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

17 crashes so far and the graphics cards indicate these are on many devices. The first crash is with Nightly 82.0a1 20200903151816..

Crash report: https://crash-stats.mozilla.org/report/index/882585bc-9cac-4631-8ae2-bec630200906

Top 10 frames of crashing thread:

0 xul.dll js::frontend::ParserAtomEntry::toJSAtom const js/src/frontend/ParserAtom.cpp:148
1 xul.dll js::frontend::EmitScriptThingsVector js/src/frontend/BytecodeSection.cpp:135
2 xul.dll static JSScript::fullyInitFromStencil js/src/vm/JSScript.cpp:3680
3 xul.dll static JSScript::fromStencil js/src/vm/JSScript.cpp:3748
4 xul.dll js::frontend::CompilationInfo::instantiateStencils js/src/frontend/Stencil.cpp:584
5 xul.dll js::frontend::InstantiateStencils js/src/frontend/BytecodeCompiler.cpp:268
6 xul.dll js::frontend::CompileGlobalScript js/src/frontend/BytecodeCompiler.cpp:324
7 xul.dll JS::CompileForNonSyntacticScope js/src/vm/CompilationAndEvaluation.cpp:126
8 xul.dll mozJSComponentLoader::ObjectForLocation js/xpconnect/loader/mozJSComponentLoader.cpp:810
9 xul.dll mozJSComponentLoader::Import js/xpconnect/loader/mozJSComponentLoader.cpp:1276

Comment 1

[Kannan Vijayan [:djvj]]

The code that's crashing here was just completely redone by tcampbell to enable constexpr small static strings:

Current code: https://searchfox.org/mozilla-central/source/js/src/frontend/ParserAtom.cpp#216
Crashing code: https://hg.mozilla.org/mozilla-central/file/aa032cbc94551a0f6e7e821d78aa0388f998e830/js/src/frontend/ParserAtom.cpp#l148

I'll pull an older revision and look into this and see if I can replicate it. Leaving needinfo on.

Comment 2

[Kannan Vijayan [:djvj]]

Repro attempts didn't work, but I noticed this in the stack trace:

js::frontend::EmitScriptThingsVector(JSContext*, js::frontend::CompilationInfo&, js::frontend::CompilationGCOutput&, mozilla::Vector<mozilla::Variant<const js::frontend::ParserAtom*, js::frontend::NullScriptThing, js::frontend::TypedIndex<js::frontend::BigIntStencil>, js::frontend::TypedIndex<js::ObjLiteralStencil>, js::frontend::TypedIndex<js::frontend::RegExpStencil>, js::frontend::TypedIndex<js::Scope>, js::frontend::TypedIndex<js::frontend::ScriptStencil>, js::frontend::EmptyGlobalScopeType>, 0, js::SystemAllocPolicy> const&, mozilla::Span<JS::GCCellPtr, 18446744073709551615>)

Specifically, the <Span> type with the suspiciously large Extent type parameter. That doesn't look right.

Comment 3

[Kannan Vijayan [:djvj]]

The span thing was a red herring - the default extent is SIZE_MAX for dynamic sizes.

Did some more digging around the code looking for something that stood out in the handling of EmitScriptThingsVector, or ScriptThingVariant, or the vector of it.. to see if a uintptr_t(-1) could slip in somewhere.

Also noticed, looking at crash reports - that the crash addresses are all over the place. This is the only one that crashes on SIZE_MAX, the rest are at addresses like 0xc, 0x8, 0x10, and nullptr, and some at random 32-bit values as well as random 64-bit values.

https://crash-stats.mozilla.org/signature/?product=Firefox&signature=js%3A%3Afrontend%3A%3AParserAtomEntry%3A%3AtoJSAtom&date=%3E%3D2020-09-24T19%3A09%3A00.000Z&date=%3C2020-10-01T19%3A09%3A00.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_columns=startup_crash&_sort=-date&page=1#reports

Adding sec-moderate to be on the safe side. There's an odd chance of some stack clobbering happening here.

Comment 4

[Ted Campbell [:tcampbell]]

In this crash, the this pointer of the ParserAtom points into the static atoms section, but has a bad pointer (XOR 0x8) that results in it decoding incorrectly and crashing.

In this crash, the this pointer is 0x80 and we obviously crash. That is a single bit flip from nullptr which is normal value to expect here.

In this crash, the ObjLiteral decoding is bad and the parsing atom crash is just the consequence. The ObjLiteral is decoding as "object", but the first value in reader stream is "Undefined" which would only be sensible for "array" mode. It only takes a single bit flip to switch between these two modes.

js::frontend::ParserAtomEntry::toJSAtom and friends are very hot pieces of code so I don't think it is surprising that it would be tripped up by memory corruption issues. I'm not sure there is anything to do here. The crash volume changes as we refactor and rename there various parser atoms functions.

Comment 5

[Ted Campbell [:tcampbell]]

Crashes seem to have cleared up and we've made a lot of changes in this area since then

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.