Closed Bug 1730518 Opened 3 years ago Closed 3 years ago

Crash in [@ core::result::unwrap_failed | audioipc_client::stream::{{impl}}::process]

Categories

(Core :: Audio/Video: cubeb, defect, P1)

x86
Windows 7
defect

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox92 --- unaffected
firefox93 + fixed
firefox94 + fixed
firefox95 + fixed

People

(Reporter: pascalc, Assigned: kinetik)

References

(Regression)

Details

(Keywords: crash, regression, topcrash)

Crash Data

Attachments

(1 file, 1 obsolete file)

Crash report: https://crash-stats.mozilla.org/report/index/51613d17-0063-4fcd-b3e4-43d070210909

MOZ_CRASH Reason: Client failed to set up shmem: Error(Io(Os { code: 8, kind: OutOfMemory, message: "Not enough storage is available to process this command." }), State { next_error: None })

Top 10 frames of crashing thread:

0 xul.dll RustMozCrash mozglue/static/rust/wrappers.cpp:17
1 xul.dll mozglue_static::panic_hook mozglue/static/rust/lib.rs:91
2 xul.dll core::ops::function::Fn::call<fn ../a178d0322ce20e33eac124758e837cbd80a6f633/library/core/src/ops/function.rs:227
3 xul.dll std::panicking::rust_panic_with_hook ../a178d0322ce20e33eac124758e837cbd80a6f633//library/std/src/panicking.rs:626
4 xul.dll std::panicking::begin_panic_handler::{{closure}} ../a178d0322ce20e33eac124758e837cbd80a6f633//library/std/src/panicking.rs:519
5 xul.dll std::sys_common::backtrace::__rust_end_short_backtrace<closure-0, !> ../a178d0322ce20e33eac124758e837cbd80a6f633//library/std/src/sys_common/backtrace.rs:141
6 xul.dll std::panicking::begin_panic_handler ../a178d0322ce20e33eac124758e837cbd80a6f633//library/std/src/panicking.rs:515
7 xul.dll core::panicking::panic_fmt ../a178d0322ce20e33eac124758e837cbd80a6f633//library/core/src/panicking.rs:92
8 xul.dll core::result::unwrap_failed ../a178d0322ce20e33eac124758e837cbd80a6f633//library/core/src/result.rs:1355
9 xul.dll audioipc_client::stream::{{impl}}::process third_party/rust/audioipc-client/src/stream.rs:201

This signature is spiking on beta since beta 2

Matthew, Chun-Min, a new one from the recent shmem changes.

Flags: needinfo?(kinetik)
Flags: needinfo?(cchang)

Fixed in bug 1725749, will need to uplift to 93 beta.

Status: NEW → RESOLVED
Closed: 3 years ago
Flags: needinfo?(kinetik)
Flags: needinfo?(cchang)
Resolution: --- → DUPLICATE

(In reply to Matthew Gregan [:kinetik] from comment #2)

Fixed in bug 1725749, will need to uplift to 93 beta.

Sorry, I'm in error here. This is a different issue. Reopening.

Assignee: nobody → kinetik
Status: RESOLVED → REOPENED
Component: Audio/Video → Audio/Video: cubeb
Resolution: DUPLICATE → ---
Regressed by: 1724141
Has Regression Range: --- → yes
Status: REOPENED → NEW

Matthew, RC week is next week, will you have a fix for uplift before we ship 93?

Flags: needinfo?(kinetik)

This is Windows 32-bits only OOM. This is failing doing a 2MB mmap. 2MB is a bit big, about 5.5s of stereo audio at 48k. We can lower it in a lot of cases I guess. Chun-Min, what do you think?

Flags: needinfo?(cchang)

IIRC, We've used 2 MB per input or output scope for the audio stream for a long time, but I am not aware that it causes trouble in the past. We've always use 2 MB for input-only or output-only and 4 MB for duplex stream. However, bug 1724141 changes how we allocate the memory. Now we allocate 2 MB for input data in a duplex stream by Vec::with_capacity [1] instead of asking the same size for shmem by CreateFileMappingA [2]. I suspect Vec::with_capacity causes a memory fragmentation issue. Cubeb issue #124 may solve this properly. But I agree that we can reduce the size since it's obviously too big.

Test on tryserver: https://treeherder.mozilla.org/jobs?repo=try&revision=e3485dec231c058c17840560e6c85575386a4e0b

[1] https://searchfox.org/mozilla-central/rev/50c3cf7a3c931409b54efa009795b69c19383541/third_party/rust/audioipc-client/src/stream.rs#207
[2] https://searchfox.org/mozilla-central/rev/50c3cf7a3c931409b54efa009795b69c19383541/third_party/rust/audioipc/src/shm.rs#282

Flags: needinfo?(cchang)

This is our biggest new crash on 93 beta, we need a patch as soon as possible. Our last beta is tomorrow, we build RC early next week. Thanks

Severity: S2 → S1
Priority: -- → P1
Pushed by padenot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/dd075a074e45 Use less memory for the AudioIPC shmem on Window 32-bits. r=bryce

Comment on attachment 9242707 [details]
Bug 1730518 - Use less memory for the AudioIPC shmem on Window 32-bits. r?chunmin

Beta/Release Uplift Approval Request

  • User impact if declined: Top crasher on Windows 32bits
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce: Hard to reproduce (OOM because of not enough adress space).
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): I've tested personally on a variety of configuration it works fine, and hopefully it's effective. We can roll back with a pref change.
  • String changes made/needed:
Attachment #9242707 - Flags: approval-mozilla-beta?

Comment on attachment 9242707 [details]
Bug 1730518 - Use less memory for the AudioIPC shmem on Window 32-bits. r?chunmin

Approved for uplift in our last beta which hopefully will allow us to know if the fix is effective before we build RC, thanks a lot!

Attachment #9242707 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Status: NEW → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 94 Branch

It's better to leave this open and see if the crashes rate is decreased or not.

Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---

Backed out for causing content crashes (bug 1732479)

Target Milestone: 94 Branch → ---

Good news, we stopped crashing in beta 9 (only one crash after 24h), I'll keep monitoring this for release.

Regressions: 1732547

The fix here seems to have caused a bigger crasher in bug 1732547.

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

The fix here seems to have caused a bigger crasher in bug 1732547.

Maybe we should back that out, and try something else here?

Note soft freeze is the 30th. Matthew is back this week, I've pinged him on Matrix and asked that he take a look.

I have a fix for this that I'm still testing, but I think it's a little too risky/invasive to uplift this late in a beta cycle. So I think the best approach is to back out f95a1c5ef0b1 (bug 1724141) and 7c72a7afec71 (bug 1725749, followup fix for bug 1724141) from beta.

Sheriff team, can you please back those two changes out of beta? Per a local test and Paul's test in comment 8, it should revert cleanly.

Flags: needinfo?(kinetik) → needinfo?(sheriffs)

We should also back out 4954c0e83dc5 (from comment 11) since it won't have any effect once f95a1c5ef0b1 is backed out.

Audioipc update was backed out of beta in https://hg.mozilla.org/releases/mozilla-beta/rev/747a4159c441

Crashes in 93 for this signature should now be fixed by backout.

Attachment #9242707 - Attachment is obsolete: true

Hi Pascal, can you confirm the crash rate is decreased after backout? If so, it's safe to close this bug now I guess.

Flags: needinfo?(pascalc)

The crashes disappeared for 83 RC after the audioipc backout on beta, was that backed out or fixed differently from central as well? If not, then that could mean that the crashes will be back on 94 beta after the merge next week.

Flags: needinfo?(pascalc) → needinfo?(cchang)

(In reply to Matthew Gregan [:kinetik] from comment #20)

I have a fix for this that I'm still testing, but I think it's a little too risky/invasive to uplift this late in a beta cycle. So I think the best approach is to back out f95a1c5ef0b1 (bug 1724141) and 7c72a7afec71 (bug 1725749, followup fix for bug 1724141) from beta.

Sheriff team, can you please back those two changes out of beta? Per a local test and Paul's test in comment 8, it should revert cleanly.

Matthew, should we back out the code in central as well? or the new fix will be ready soon?

Flags: needinfo?(cchang) → needinfo?(kinetik)
Crash Signature: [@ core::result::unwrap_failed | audioipc_client::stream::{{impl}}::process] → [@ core::result::unwrap_failed | audioipc_client::stream::{{impl}}::process] [@ core::result::unwrap_failed | audioipc_client::stream::impl$1::process]
Flags: needinfo?(sheriffs)

(In reply to C.M.Chang[:chunmin] from comment #26)

Matthew, should we back out the code in central as well? or the new fix will be ready soon?

A smaller/safe fix is up for review now, I'd like to land that in central and uplift to beta early this week. I was working on a more comprehensive fix (reducing shmem size to the minimum required for a given stream), but it's not quite ready yet, so I'll continue working on that and land it as a follow-on improvement in central once it's ready.

Flags: needinfo?(kinetik)

Comment on attachment 9245839 [details]
Bug 1730518 - Update audioipc to 8381f381. r?#cubeb-reviewers

Beta/Release Uplift Approval Request

  • User impact if declined: Content crash in OOM situations (especially 32-bit platforms) while audio in use.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • 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): Small change, makes OOM situation non-fatal, restoring behaviour in earlier version.
  • String changes made/needed:
Attachment #9245839 - Flags: approval-mozilla-beta?
Pushed by mgregan@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/1d2d6bd66da7 Update audioipc to 8381f381. r=cubeb-reviewers,chunmin
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch

Comment on attachment 9245839 [details]
Bug 1730518 - Update audioipc to 8381f381. r?#cubeb-reviewers

Approved for 94.0b6.

Attachment #9245839 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Fix looks good - no crashes with this signature after 94.0b5.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: