Crash in [@ core::result::unwrap_failed | audioipc_client::stream::{{impl}}::process]
Categories
(Core :: Audio/Video: cubeb, defect, P1)
Tracking
()
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)
(deleted),
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details |
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
Comment 1•3 years ago
|
||
Matthew, Chun-Min, a new one from the recent shmem changes.
Assignee | ||
Comment 2•3 years ago
|
||
Fixed in bug 1725749, will need to uplift to 93 beta.
Updated•3 years ago
|
Assignee | ||
Comment 3•3 years ago
|
||
(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 | ||
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Updated•3 years ago
|
Reporter | ||
Comment 4•3 years ago
|
||
Matthew, RC week is next week, will you have a fix for uplift before we ship 93?
Reporter | ||
Updated•3 years ago
|
Comment 5•3 years ago
|
||
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?
Comment 6•3 years ago
|
||
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
Reporter | ||
Comment 7•3 years ago
|
||
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
Comment 8•3 years ago
|
||
https://treeherder.mozilla.org/jobs?repo=try&revision=1f24e23de4bab0da2b4a446a1f4946c9215d325d is a bit like the patch in comment 6 but making the change only for Win32.
https://treeherder.mozilla.org/jobs?repo=try&revision=9ada14bf2b50ce6370e63cbe61ff29d5a776148b is backing out the two commits altogether as a last-resort.
Comment 9•3 years ago
|
||
Comment 10•3 years ago
|
||
Comment 11•3 years ago
|
||
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:
Reporter | ||
Comment 12•3 years ago
|
||
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!
Reporter | ||
Comment 13•3 years ago
|
||
bugherder uplift |
Comment 14•3 years ago
|
||
bugherder |
Comment 15•3 years ago
|
||
It's better to leave this open and see if the crashes rate is decreased or not.
Comment 16•3 years ago
|
||
Backed out for causing content crashes (bug 1732479)
Reporter | ||
Comment 17•3 years ago
|
||
Good news, we stopped crashing in beta 9 (only one crash after 24h), I'll keep monitoring this for release.
Updated•3 years ago
|
Reporter | ||
Comment 18•3 years ago
|
||
The fix here seems to have caused a bigger crasher in bug 1732547.
Comment 19•3 years ago
|
||
(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.
Assignee | ||
Comment 20•3 years ago
|
||
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.
Assignee | ||
Comment 21•3 years ago
|
||
We should also back out 4954c0e83dc5 (from comment 11) since it won't have any effect once f95a1c5ef0b1 is backed out.
Reporter | ||
Comment 22•3 years ago
|
||
Backed out 1 changesets (bug 1730518) for causing content crashes on beta (bug 1732547) a=pascalc
https://hg.mozilla.org/releases/mozilla-beta/rev/e733ce0e4675
Reporter | ||
Comment 23•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 24•3 years ago
|
||
Hi Pascal, can you confirm the crash rate is decreased after backout? If so, it's safe to close this bug now I guess.
Reporter | ||
Comment 25•3 years ago
|
||
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.
Comment 26•3 years ago
|
||
(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?
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Comment 27•3 years ago
|
||
(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.
Assignee | ||
Comment 28•3 years ago
|
||
Assignee | ||
Comment 29•3 years ago
|
||
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:
Comment 30•3 years ago
|
||
Comment 31•3 years ago
|
||
bugherder |
Updated•3 years ago
|
Comment 32•3 years ago
|
||
Comment on attachment 9245839 [details]
Bug 1730518 - Update audioipc to 8381f381. r?#cubeb-reviewers
Approved for 94.0b6.
Comment 33•3 years ago
|
||
bugherder uplift |
Assignee | ||
Comment 34•3 years ago
|
||
Fix looks good - no crashes with this signature after 94.0b5.
Description
•