1871186 - Assertion failure: buffer, at gc/Nursery.cpp:1620

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect, P2)

Description

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

Attachment: stack

gc();
function f(x) {
  new Uint8Array(x);
}
f(0);
oomTest(function () {
  f(2147483647);
});

Assertion failure: buffer, at /Users/m1mini/trees/mozilla-central/js/src/gc/Nursery.cpp:1595
#01: js::Nursery::registerMallocedBuffer(void*, unsigned long)[/Users/m1mini/shell-cache/js-dbg-64-darwin-arm64-bc930191cf0d/js-dbg-64-darwin-arm64-bc930191cf0d +0xab4fac]
#02: js::Nursery::allocateZeroedBuffer(js::gc::Cell*, unsigned long, unsigned long)[/Users/m1mini/shell-cache/js-dbg-64-darwin-arm64-bc930191cf0d/js-dbg-64-darwin-arm64-bc930191cf0d +0xab5354]
#03: js::NewTypedArrayWithTemplateAndLength(JSContext*, JS::Handle<JSObject*>, int)[/Users/m1mini/shell-cache/js-dbg-64-darwin-arm64-bc930191cf0d/js-dbg-64-darwin-arm64-bc930191cf0d +0x57f580]

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/9047fbab5114
user:        Steve Fink
date:        Fri Nov 24 05:33:22 2023 +0000
summary:     Bug 1853907 - Move responsibility of registering malloc buffers with the nursery to the callers, to prepare for situations where you can't predict whether the owner will be in the nursery or not. r=jonco

Run with --no-threads --no-baseline --no-ion --blinterp-eager, compile with AR=ar sh ../configure --enable-debug --with-ccache --enable-nspr-build --enable-ctypes --enable-debug-symbols --enable-gczeal --enable-rust-simd --disable-tests, tested on m-c rev bc930191cf0d.

:sfink, is bug 1853907 a likely regressor? Setting s-s because gc is involved. This is very reliably reproducing on ARM macOS.

Comment 1

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

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

Comment 2

[Christian Holler (:decoder)]

We have had this in our system for at least a week, probably longer. I just haven't had the time to report it. Also with really short tests such as

a = []
for (;;) a.push(new Int32Array(2000))

Comment 4

[Donal Meehan [:dmeehan]]

:willyelm could this be triaged? Next week is the final week of Beta

Comment 5

[Jon Coppeard (:jonco)]

Nursery::allocateZeroedBuffer is returning |isMalloced| as true on allocation failure which confuses the caller.

Comment 6

[Jon Coppeard (:jonco)]

This is not a security issue. If this happens we will add a nullptr to the set of malloced buffers to free after minor GC, which has no effect. The caller will still return failure correctly.

Comment 7

[Jon Coppeard (:jonco)]

Attachment: Bug 1871186 - Make Nursery::allocateZeroedBuffer return |isMalloced| as false on failure r?jandem

This can currently causes an assertion failure but is not otherwise problematic.

Comment 8

[Jon Coppeard (:jonco)]

Attachment: Bug 1871186 - Make simulated alloc failure when marking less frequent r?jandem

Something I found when testing the original testcase attached to the bug was
that OOM tests involving GC can take a very long time (and sometimes time out)
because of the very frequent simulated OOM when pushing things onto the mark
stack. I added this originally to improve test coverage here but this may have
been over-enthusiastic. The patch changes this to rely on simulated alloc
failure whew the mark stack vector is resized and starts this off with a
smaller size in debug builds to ensure coverage.

Depends on D197911

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3b8b6ba44de3
Make Nursery::allocateZeroedBuffer return |isMalloced| as false on failure r=jandem

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6eecc1e8b34
Make simulated alloc failure when marking less frequent r=jandem

Comment 11

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/3b8b6ba44de3

Comment 12

[Sandor Molnar[:smolnar]]

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

Comment 13

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

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

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox122 to wontfix.

For more information, please visit BugBot documentation.

Comment 14

[Jon Coppeard (:jonco)]

Comment on attachment 9371526 [details]
Bug 1871186 - Make Nursery::allocateZeroedBuffer return |isMalloced| as false on failure r?jandem

Beta/Release Uplift Approval Request

• User impact if declined: No user impact but requesting uplift to help fuzzers.
• 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: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is a very simple change that adds a null check.
• String changes made/needed:
• Is Android affected?: Yes

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9371526 [details]
Bug 1871186 - Make Nursery::allocateZeroedBuffer return |isMalloced| as false on failure r?jandem

Approved for 122.0b9

Comment 16

[Donal Meehan [:dmeehan]]

Comment on attachment 9371527 [details]
Bug 1871186 - Make simulated alloc failure when marking less frequent r?jandem

Approved for 122.0b9

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/1da362dc8d7d
https://hg.mozilla.org/releases/mozilla-beta/rev/72d5576aafc4