The following testcase crashes on mozilla-central revision 7ffabb358c42 (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --baseline-eager --no-ion):

// Adapted from randomly chosen test: js/src/tests/test262/language/statements/class/elements/after-same-line-static-async-gen-rs-field-identifier.js
oomTest(
    new Function(`
        evaluate(\`
            class C {
                c;
                _;
                2;
                u;
                J;
                _;
            }
            t(C);
            function s() {
                e;
                if (r) {
                    0;
                }
                c1 = 1;
                t.e(
                    c
                    .a2,
                    1
                );
                s
                .s( 
                    c8,
                    1
                );
                a
                (   
                    c
                    .J
                    , 1
                );
                u
                (   
                    c
                    ._
                    , 1
                );
            }
            e.r;
        \`, {
            compileAndGo: true
        })
    `)
)

Backtrace:

#0  JS::Rooted<JSAtom*>::~Rooted (this=0x7f6e13c19040) at /home/ubuntu/shell-cache/js-dbg-64-dm-linux-x86_64-7ffabb358c42/objdir-js/dist/include/js/RootingAPI.h:1061
#1  js::frontend::BytecodeEmitter::emitClass (this=<optimized out>, classNode=<optimized out>, nameKind=js::frontend::BytecodeEmitter::ClassNameKind::BindingName, nameForAnonymousClass=...) at js/src/frontend/BytecodeEmitter.cpp:8850
#2  0x0000558d18a6077b in js::frontend::BytecodeEmitter::emitTree (this=0x7ffe1a198340, pn=0x7f6e13cde998, valueUsage=js::frontend::ValueUsage::WantValue, emitLineNote=js::frontend::BytecodeEmitter::EMIT_LINENOTE) at js/src/frontend/BytecodeEmitter.cpp:9393
#3  0x0000558d18a610a9 in js::frontend::BytecodeEmitter::emitStatementList (this=<optimized out>, stmtList=<optimized out>) at js/src/frontend/BytecodeEmitter.cpp:6726
#4  js::frontend::BytecodeEmitter::emitTree (this=0x7ffe1a198340, pn=0x7f6e13cdd020, valueUsage=js::frontend::ValueUsage::WantValue, emitLineNote=<optimized out>) at js/src/frontend/BytecodeEmitter.cpp:9030
#5  0x0000558d18a55722 in js::frontend::BytecodeEmitter::emitScript (this=0x7ffe1a198340, body=0x7f6e13cdd020) at js/src/frontend/BytecodeEmitter.cpp:2455
/snip

For detailed crash information, see attachment.

Setting s-s as a start as this seems like a Rooting / GC assertion failure.

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/0b15a4e88e9c
user: Iain Ireland
date: Tue Jun 25 17:34:50 2019 +0000
summary: Bug 1559072: Revert to old boxing format r=djvj

I don't know if this is correct, setting needinfo? from both Iain (autobisectjs points here) and Ashley (it involves classes) to talk a look.

Also, removing whitespace in the testcase seems to cause the assertion to disappear.

Well, on second thought, it involves oomTest, so probably not s-s though.

This typically indicates there's a Maybe<> somewhere that contains a Rooted so the stack can easily become unbalanced. Maybe the RootedAtom in NameOpEmitter because ClassEmitter has a Maybe<NameOpEmitter>, but that's a wild guess.

Yep, seems like it's my fault for not understanding Maybe<Rooted> is scary.

These two lines are reversed in their initialization order vs. destruction order, then. https://searchfox.org/mozilla-central/rev/f91bd38732d4a330eba4e780812274b98eb81274/js/src/frontend/BytecodeEmitter.cpp#8722-8723

(edit) Maybe. Maybe not. I'm just looking at this for fun right now, it's late and I'm not thinking things through :P

(Edit: Filed as bug 1562298)

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset: https://hg.mozilla.org/mozilla-central/rev/d96f98f974e0
user: Ashley Hauck
date: Wed Mar 20 17:26:01 2019 +0000
summary: Bug 1535166 - Implement computed field names. r=jorendorff

On the other hand, if the testcase is run with --fuzzing-safe --no-threads --no-baseline --no-ion instead, it crashes [@ js::frontend::FullParseHandler::addClassMemberDefinition] instead:

#0  js::frontend::FullParseHandler::addClassMemberDefinition (this=0x7fffffffac30, memberList=0x7ffff5de7060, member=0x0)
    at /home/ubuntu/trees/mozilla-central/js/src/frontend/FullParseHandler.h:470
#1  0x0000555556e25cfd in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::finishClassConstructor (this=0x7fffffffa798, 
    classStmt=..., className=..., hasHeritage=<optimized out>, classStartOffset=4294945840, classEndOffset=150, numFields=6, 
    classMembers=@0x7fffffff9970: 0x7ffff5de7060) at /home/ubuntu/trees/mozilla-central/js/src/frontend/Parser.cpp:6997
#2  0x0000555556e13189 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::classDefinition (this=<optimized out>, 
    yieldHandling=<optimized out>, classContext=js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::ClassStatement, 
    defaultHandling=js::frontend::NameRequired) at /home/ubuntu/trees/mozilla-central/js/src/frontend/Parser.cpp:7134
#3  0x0000555556e0d81b in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementListItem (this=<optimized out>, 
    yieldHandling=<optimized out>, canHaveDirectives=<optimized out>) at /home/ubuntu/trees/mozilla-central/js/src/frontend/Parser.cpp:7998
#4  0x0000555556e0bb95 in js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementList (this=<optimized out>, 
    yieldHandling=js::frontend::YieldIsName) at /home/ubuntu/trees/mozilla-central/js/src/frontend/Parser.cpp:3475
#5  0x0000555556e4642d in js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::globalBody (this=0x7fffffffa798, globalsc=0x7fffffffadf8)
    at /home/ubuntu/trees/mozilla-central/js/src/frontend/Parser.cpp:1446
#6  0x0000555556e7b583 in js::frontend::ScriptCompiler<char16_t>::compileScript (this=0x7fffffffa240, info=..., environment=..., sc=<optimized out>)
    at /home/ubuntu/trees/mozilla-central/js/src/frontend/BytecodeCompiler.cpp:531
#7  0x0000555556e4a96b in CreateGlobalScript<char16_t> (info=..., srcBuf=..., sourceObjectOut=<optimized out>)
    at /home/ubuntu/trees/mozilla-central/js/src/frontend/BytecodeCompiler.cpp:208
/snip

Not sure if they are both the same issue or not.

Ah, that second stack is a typical OOM bug (unrelated to the original issue): there's no if null check here https://searchfox.org/mozilla-central/source/js/src/frontend/Parser.cpp#6995-6999

(In reply to Ashley Hauck [:khyperia] from comment #7)

Ah, that second stack is a typical OOM bug (unrelated to the original issue): there's no if null check here https://searchfox.org/mozilla-central/source/js/src/frontend/Parser.cpp#6995-6999

I've cloned it out to bug 1562298, thanks for the clarification.

Ashley is on PTO for a while, so I took a quick look at this to make sure it wasn't my fault.

The problem appears to be that we emplace a new NameOpEmitter here, which involves rooting an atom for the name. The NameOpEmitter is emplaced inside the ClassEmitter defined here. The atom that we root on the next line goes on the stack under the NameOpEmitter's atom, but is cleaned up first, which triggers the assertion.

Normally the NameOpEmitter is reset here, but if we OOM before that happens, then everything explodes. (In this particular case we OOM in this emitTree, but I think any OOM would trigger the bug.)

I'll punt the actual fix to somebody who knows this code better than I do.

I'll take this.

I'm going to change Rooted in helper classes' field to Handle, so that rooting will happen outside the Maybe.

Depends on D36695

Depends on D36696

Hi Jason, do you think you'll be able to look at this review soon? Would be nice to get this issue fixed before we ship 69.

ni? myself to ask uplift for this and bug 1562298 after merge

https://hg.mozilla.org/integration/autoland/rev/f2d6c783b476243e1d62a015fcead30d7151e3a8
https://hg.mozilla.org/integration/autoland/rev/946723ace24effc19de6b7f9f0744d2018c31185
https://hg.mozilla.org/mozilla-central/rev/f2d6c783b476
https://hg.mozilla.org/mozilla-central/rev/946723ace24e

I think bug 1555037 is more relevant, that added Maybe<NameOpEmitter>.
anyway it's also landed to firefox69.

Comment on attachment 9075549 [details]
Bug 1562102 - Use more Handle in BytecodeEmitter methods and helper classes. r?jorendorff

Beta/Release Uplift Approval Request

• User impact if declined: Possibly de-referencing random value, or use-after-free.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1562298
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Removed unnecessary rooting
• String changes made/needed:

Comment on attachment 9075549 [details]
Bug 1562102 - Use more Handle in BytecodeEmitter methods and helper classes. r?jorendorff

Fixes a JS sec bug. Approved for 69.0b10.

https://hg.mozilla.org/releases/mozilla-beta/rev/22a134bc52b0
https://hg.mozilla.org/releases/mozilla-beta/rev/7baf87d8c07f