1968209 - wasm: correctly handle OOMs related to BaseCompiler::patchHotnessCheck

Status: RESOLVED FIXED

(Core :: JavaScript: WebAssembly, defect, P1)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: Debug stack

function f() {
  new WebAssembly.Module(
    wasmTextToBinary(`
        (type (func (param i64 f64) (result i64)))
        (type (func (result f64)))
        (table 6 funcref)
        (func (type 0) (param i64 f64) (result i64)
            (local i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i32 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 i64 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f32 f64)
            block (result i64)
                block
                    block (result i32)
                        loop
                            block (result f64)
                                loop (result i32)
                                    (i32.const 0)
                                    (i32.const 0)
                                    (br_if 3)
                                    (local.set 2)
                                    (i32.const 0)
                                end
                                (local.tee 3)
                                (local.set 4)
                                block (result i32)
                                    (i32.const 0)
                                    if
                                        (i32.const 0)
                                        (br_if 5)
                                    end
                                    (i32.const 0)
                                end
                                (local.set 4)
                                (i32.const 0)
                                (call_indirect (type 1))
                            end
                            (local.set 523)
                        end
                        (br 1)
                    end
                    (br_if 0)
                end
                (i64.const 8)
            end
        )
    `)
  );
  oomTest(f);
}
f();

#0  0x000055555823a680 in MOZ_CrashSequence (aAddress=0x0, aLine=1116)
    at /home/msf1/shell-cache/js-dbg-64-linux-x86_64-68577999ef6b/objdir-js/dist/include/mozilla/Assertions.h:248
#1  js::jit::MacroAssembler::patchSub32FromMemAndBranchIfNegative (this=<optimized out>, offset=..., imm=...)
    at /home/msf1/trees/mozilla-central/js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp:1116
#2  0x0000555558687a09 in js::wasm::BaseCompiler::emitEnd (this=this@entry=0x7fffffff7340)
    at /home/msf1/trees/mozilla-central/js/src/wasm/WasmBaselineCompile.cpp:4189
#3  0x00005555586a7fed in js::wasm::BaseCompiler::emitBody (this=this@entry=0x7fffffff7340)
    at /home/msf1/trees/mozilla-central/js/src/wasm/WasmBaselineCompile.cpp:10523
#4  0x00005555586c04e3 in js::wasm::BaseCompiler::emitFunction (this=0x7fffffff7340)
    at /home/msf1/trees/mozilla-central/js/src/wasm/WasmBaselineCompile.cpp:12317
#5  js::wasm::BaselineCompileFunctions (codeMeta=..., compilerEnv=..., lifo=..., inputs=..., code=code@entry=0x7ffff4122c08,
    error=error@entry=0x7fffffffb250) at /home/msf1/trees/mozilla-central/js/src/wasm/WasmBaselineCompile.cpp:12495
/snip

Bisection seems to point to m-c rev 9618824194c5 from bug 1957504 as a start (landed Apr 30, 2025).

I ran with the additional shell flag --setpref=wasm_lazy_tiering=true:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/8f48c0f2c326
user:        Ryan Hunt
date:        Mon Oct 14 15:41:34 2024 +0000
summary:     Bug 1913114 - wasm: Rename lazy tiering prefs and add pref that enables lazy tiering only for wasm-gc. r=yury

I then ran bisection with the additional shell flag --setpref=wasm_experimental_compile_pipeline=true:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/e9af9e7c0cef
user:        Julian Seward
date:        Wed Sep 11 15:17:17 2024 +0000
summary:     Bug 1911591 - part 2: variable-sized downwards steps in wasm lazy tiering hotness counting.  r=yury.

Run with --fuzzing-safe --ion-eager, compile with AR=ar sh ../configure --enable-debug --enable-debug-symbols --with-ccache --enable-nspr-build --enable-ctypes --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev 68577999ef6b.

Julian, is bug 1957504 or bug 1911591 a likely regressor?

Comment 1

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1957504

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1968209-patchHotnessCheck-OOM.diff-1

A proposed fix.

Comment 3

[BugBot [:suhaib / :marco]]

Set release status flags based on info from the regressing bug 1957504

Comment 4

[Julian Seward [:jseward]]

Attachment: Bug 1968209 - wasm: correctly handle OOMs related to BaseCompiler::patchHotnessCheck. r=rhunt.

Comment 5

[Daniel Veditz [:dveditz]]

needinfo to Julian for Ryan's question in the Diff

Is this security sensitive though? If masm OOMs during baseline compilation, we will throw away everything at the end of compilation.

Comment 6

[Julian Seward [:jseward]]

If masm OOMs during baseline compilation, we will throw away everything at
the end of compilation.

That's true; but the question is, can the resulting possibly-invalid update to
the assembler buffer cause any trouble before the buffer is thrown away? I
guess not, given that neither the written address nor the written data is under
external control. Also we have a pretty good set of assertions to protect us
[1] [2] [3], although non-release on arm32.

On consideration, I would be OK with declassifying this. But I don't have
authorization to do that; can one of you do it?

[1] https://searchfox.org/mozilla-central/source/js/src/jit/x86-shared/MacroAssembler-x86-shared.cpp#1116

[2] https://searchfox.org/mozilla-central/source/js/src/jit/arm64/MacroAssembler-arm64.cpp#2152

[3] https://searchfox.org/mozilla-central/source/js/src/jit/arm/MacroAssembler-arm.cpp#4651-4652

Comment 7

[Ryan Hunt [:rhunt]]

I agree with Julian's analysis. I will declassify.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/ce1981cc4a24
https://hg.mozilla.org/integration/autoland/rev/c1abd479f7be
wasm: correctly handle OOMs related to BaseCompiler::patchHotnessCheck.  r=rhunt.

Comment 9

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/c1abd479f7be

Comment 10

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:jseward, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox140 to wontfix.

For more information, please visit BugBot documentation.

Comment 11

[Julian Seward [:jseward]]

Attachment: Bug 1968209 - wasm: correctly handle OOMs related to BaseCompiler::patchHotnessCheck. r=rhunt.

In BaseCompiler::emitEnd, for LabelKind::Loop, don't try to patch the
loop-head hotness check if we are in an OOM state.

Original Revision: https://phabricator.services.mozilla.com/D251467

Comment 12

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined: In an OOM situation, there could be either a segfault or a release-assertion failure, when in fact the OOM should have been caught and recovered from.
• Code covered by automated testing: no
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: but I said "No" in the previous box
• Risk associated with taking this patch: Minimal; does not change behaviour in a non-OOM situation.
• Explanation of risk level: as above
• String changes made/needed: none
• Is Android affected?: yes

Comment 13

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/2b681a7411df
https://hg.mozilla.org/releases/mozilla-beta/rev/93077612d93e