1732601 - Uncaught out of memory (OOM) crash with Baseline JIT enabled

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[ser.maltsev]

Attachment: firefox-out-of-memory.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:93.0) Gecko/20100101 Firefox/93.0

Steps to reproduce:

Firefox 93.0b9 (64-bit) on Windows 10.

Using JS code with set of mixins and defined Reflect properties cause "Uncaught out of memory" console crash with empty call stack.

Setting javascript.options.baselinejit to false or setting a bigger number for
javascript.options.baselinejit.threshold (like 1000) solves the issue.

The attached JS script shows the problem.
It is a bit synthetic but we face the exact same error in production code for a real project.

const
    { getPrototypeOf } = Object,
    { defineProperty } = Reflect,
    { hasOwnProperty } = Object.prototype,
    metaSymbol         = Symbol('classMetaData'),
    Mixin              = Target => class extends Target {},
    MixinFoo           = Target => class extends Target {
        get foo() { }

        set foo(value) { }
    };

class Base {
    /*
     * An object owned by this class that does not share properties with its super class.
     */
    static get $meta() {
        const me = this;
        let meta = me[metaSymbol];
        if (!hasOwnProperty.call(me, metaSymbol)) {
            me[metaSymbol] = meta = {
                class : me,
                super : getPrototypeOf(me).$meta
            };
            defineProperty(meta.class.prototype, '$meta', {
                value : meta
            });
        }
        return meta;
    }
}

class MyClass extends (Mixin(Mixin(Mixin(Mixin(Mixin(Mixin(Mixin(Mixin(Mixin(Mixin(Mixin(MixinFoo(Base))))))))))))) {}

const
    instance = new MyClass(),
    arr      = [...Array(1000).keys()].map(() => {
        return instance.foo || MyClass.$meta.foo;
    });

console.log(`OK. ${arr.length}`);

Actual results:

Crash with "Uncaught out of memory" in console.

Expected results:

Expected no error to happen.
Output OK. 1000 in console.

Comment 1

[Jan de Mooij [:jandem]]

Great find, thanks for the bug report and the great test case!

This is an error handling issue with trial inlining.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Bug 1732601 part 1 - Remove |attached| outparam from AttachBaselineCacheIRStub. r?iain!

This is set to true iff we return a non-nullptr IC stub, so it's redundant.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Bug 1732601 part 2 - Move some code into replaceICStub. r?iain!

This will simplify the next patch.

Depends on D126663

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Bug 1732601 part 3 - Return enum value from AttachBaselineCacheIRStub to fix error handling. r?iain!

Depends on D126664

Comment 5

[ser.maltsev]

Hi, Jan!
Please confirm this bug was introduced in FireFox 92.
I can't reproduce this in FireFox 91.

When this gets fixed which version will contain the fix?
Need this for our support team to inform users.
Thank you.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8065f206d34d
part 1 - Remove |attached| outparam from AttachBaselineCacheIRStub. r=iain
https://hg.mozilla.org/integration/autoland/rev/70188b9bb172
part 2 - Move some code into replaceICStub. r=iain
https://hg.mozilla.org/integration/autoland/rev/f754530ba210
part 3 - Return enum value from AttachBaselineCacheIRStub to fix error handling. r=iain

Comment 7

[Jan de Mooij [:jandem]]

(In reply to ser.maltsev from comment #5)

Please confirm this bug was introduced in FireFox 92.
I can't reproduce this in FireFox 91.

The underlying bug is present in Firefox 91, but it's possible this wasn't an issue before bug 1717438 was fixed (this landed in Firefox 92).

When this gets fixed which version will contain the fix?
Need this for our support team to inform users.

Firefox 94. Maybe Firefox 93 but it's pretty late in the release cycle so probably not.

Comment 8

[ser.maltsev]

Hi, Jan!

Thanks a lot for fixing the issue and for the information!

Comment 9

[Jan de Mooij [:jandem]]

Pascal, do you think we should uplift this to 93 at this point? The underlying bug has become easier to hit in practice since Firefox 92.

Comment 10

[Pascal Chevrel:pascalc (PTO until April 26)]

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

Pascal, do you think we should uplift this to 93 at this point? The underlying bug has become easier to hit in practice since Firefox 92.

There is no fix in 94 yet and we have already built our 93 RC so that seems unlikely. Unless it is a release blocker that would mean building an RC2, how severe is this bug for our user base?

Comment 11

[Jan de Mooij [:jandem]]

(In reply to Pascal Chevrel:pascalc from comment #10)

There is no fix in 94 yet and we have already built our 93 RC so that seems unlikely. Unless it is a release blocker that would mean building an RC2,

OK that's what I figured.

how severe is this bug for our user base?

It can break websites using a certain code pattern because we throw a JS exception where we shouldn't.

Comment 12

[Pascal Chevrel:pascalc (PTO until April 26)]

Is there a crash signature associated to this bug we could have to assess how many crashes we had in beta because of this bug that we could fix? it's not clear to me what the real world impact is from the information I have.

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Pascal Chevrel:pascalc from comment #12)

Is there a crash signature associated to this bug we could have to assess how many crashes we had in beta because of this bug that we could fix? it's not clear to me what the real world impact is from the information I have.

Unfortunately not because it's not an OOM crash, it's just a JS correctness bug. That makes it hard to determine the real-world impact... It's probably not super common because this is the only bug report I'm aware of, but it's hard to say for sure.

Comment 14

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/8065f206d34d
https://hg.mozilla.org/mozilla-central/rev/70188b9bb172
https://hg.mozilla.org/mozilla-central/rev/f754530ba210

Comment 15

[ser.maltsev]

Hi, Jan.

Could you please confirm this fix now applies to this page.
https://bryntum.com/examples/scheduler-pro/bigdataset/

Actually we have more fails for FF 92 in other demos and clients who use this product in their production report the same.

Comment 16

[Jan de Mooij [:jandem]]

(In reply to ser.maltsev from comment #15)

Could you please confirm this fix now applies to this page.
https://bryntum.com/examples/scheduler-pro/bigdataset/

Yes that page now works for me with the latest Firefox Nightly build.

If you want to try this for yourself, you can download the latest Firefox Nightly build here: https://www.mozilla.org/en-US/firefox/all/#product-desktop-nightly

Comment 17

[ser.maltsev]

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

Thanks a lot for fast and great support!
I see it works in nightly.

Comment 18

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

The patch landed in nightly and beta is affected.
:jandem, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 19

[Alexandru Ionescu (needinfo me) [:alexandrui]]

(In reply to Atila Butkovits from comment #14)

https://hg.mozilla.org/mozilla-central/rev/8065f206d34d
https://hg.mozilla.org/mozilla-central/rev/70188b9bb172
https://hg.mozilla.org/mozilla-central/rev/f754530ba210

== Change summary for alert #31565 (as of Wed, 29 Sep 2021 20:23:05 GMT) ==

Improvements:

Ratio	Test	Platform	Options	Absolute values (old vs new)
16%	fandom FirstVisualChange	linux1804-64-shippable-qr	warm webrender	236.92 -> 200.00
8%	fandom fcp	linux1804-64-shippable-qr	warm webrender	200.29 -> 184.83
6%	wikipedia FirstVisualChange	macosx1014-64-shippable-qr	warm webrender	977.93 -> 923.33
5%	wikipedia ContentfulSpeedIndex	macosx1014-64-shippable-qr	warm webrender	977.15 -> 927.00
5%	fandom fnbpaint	linux1804-64-shippable-qr	warm webrender	207.54 -> 196.96

For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=31565

Comment 20

[Ryan VanderMeulen [:RyanVM]]

We won't be building a 93.0.1 release. This change will ride 94.