Closed Bug 1181735 Opened 4 years ago Closed 4 years ago
crash in mozilla::Audio
This bug was filed from the Socorro interface and is report bp-6fbd247f-9d3a-479d-b6b6-6c3272150705. ============================================================= Top crash in 40 beta 1. Looks like a dead mixer callback. Paul can you take a look? 00 0e85f9a8 63eba191 xul!mozilla::AudioMixer::FinishMixing+0x1c 01 0e85f9d8 63eb81e8 xul!mozilla::MediaStreamGraphImpl::Process+0x174 02 0e85fa20 63e98904 xul!mozilla::MediaStreamGraphImpl::OneIteration+0x150 03 0e85fa88 63e98cbd xul!mozilla::AudioCallbackDriver::DataCallback+0x22d 04 0e85fa94 6452c04b xul!mozilla::AudioCallbackDriver::DataCallback_s+0x11 05 0e85faa8 6452c631 xul!noop_resampler::fill+0x11 06 0e85fad4 6452d1c7 xul!`anonymous namespace'::refill+0x34 07 0e85fb14 6f5fc01d xul!`anonymous namespace'::wasapi_stream_render_loop+0x17f
Let's try this, being a bit more tight when removing callbacks. Let's then observe the crash numbers. I have another lead to explore if this is not it.
Attachment #8631585 - Flags: review?(rjesup)
Assignee: nobody → padenot
Status: NEW → ASSIGNED
Attachment #8631585 - Flags: review?(rjesup) → review+
To see impact, we'll likely need a reproducible case, or to get it into beta (aurora may show enough signal to read if this is helping, though). Kairo? We'll need s-a to land, and aurora and/or beta approval
(In reply to Randell Jesup [:jesup] from comment #3) > To see impact, we'll likely need a reproducible case, or to get it into beta > (aurora may show enough signal to read if this is helping, though). Kairo? I don't know of a reproducible case, but Dev Edition is showing a volume of multiple crashes per day so should give us some insight at least. In the end, we'll need this on beta both to verify with data that it works and to actually fix the crash there. :)
Comment on attachment 8631585 [details] [diff] [review] patch [Security approval request comment] How easily could an exploit be constructed based on the patch? Hard based on the patch. Perhaps easier if someone can link the patch back to the crashstats reports that reference this bug and show 0x5a*/etc addresses. Would still need a POC to cause those semi-at-will. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No Which older supported branches are affected by this flaw? 34.*, 35.*, apparently 36 No crashes seen in 37-39 (so far) in the last month 40, 41, 42 affected. If not all supported branches, which bug introduced the flaw? Likely the main cubeb/MediaStreamGraph refactor last fall to drive the graph from callbacks. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Trivial; should apply easily. How likely is this patch to cause regressions; how much testing does it need? Very unlikely to cause regressions; only basic smoketesting needed.
sec-approval+ and other approvals given.
Target Milestone: --- → mozilla42
https://hg.mozilla.org/releases/mozilla-aurora/rev/0d125537bfbc Based on the sec-approval comment, should we consider taking this on esr38 as well to cover our bases?
Any noticeable change in crash rate since this landed? Should we consider a belts-and-suspenders backport to esr38 as well?
I can't find a good graph that would show if this has fixed the issue. This is what I did: click on the link in comment 0, click on "more reports", click on "graph", change the channel to beta. It says that it does not fine any occurrence. What am I doing wrong?
Flags: needinfo?(padenot) → needinfo?(dmajor)
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3AAudioMixer%3A%3AFinishMixing() - expanding the "Product" area still shows right now: Firefox 40.0b3 51.23 % 643 Firefox 40.0b2 30.68 % 385 Firefox 40.0b4 8.37 % 105 Firefox 40.0b1 4.86 % 61 Beta 4 (released ~2 days ago) still looks affected.
I'm thinking maybe 1181097 will help here. It's currently waiting for for a+. NI Sylvestre so he knows it's more important than what is stated in the bug.
Sylvestre is still in PTO. Ritu, would you mind having a look ? After further analysis, I'm rather confident bug 1181097 will help.
Flags: needinfo?(sledru) → needinfo?(rkothari)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10) > https://hg.mozilla.org/releases/mozilla-beta/rev/bd50a25b0ff3 (In reply to Paul Adenot (:padenot) from comment #13) > I can't find a good graph that would show if this has fixed the issue. > > This is what I did: click on the link in comment 0, click on "more reports", > click on "graph", change the channel to beta. It says that it does not fine > any occurrence. What am I doing wrong? I never use the graph (I don't remember if it's because I don't trust it, or some other reason). Here's a supersearch that says this still happens a lot on 40b4: https://crash-stats.mozilla.com/search/?signature=~FinishMixing&release_channel=beta&product=Firefox&platform=Windows+NT&_facets=signature&_facets=build_id&_facets=version&_facets=release_channel&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-build_id
Paul, I have approved the patch for bug 1181097 to be uplifted to Aurora. I am not comfortable approving for Beta. Let's give it a few days to ensure it doesn't have a negative impact.
(In reply to Ritu Kothari (:ritu) from comment #18) > Paul, I have approved the patch for bug 1181097 to be uplifted to Aurora. I > am not comfortable approving for Beta. Let's give it a few days to ensure it > doesn't have a negative impact. Sorry, I needinfo'd you in bug 1181097, not realizing the discussion was happening here. I can clear the needinfo there, but I feel strongly that we need to get this patch on Beta as soon as we can -- before the next go-to Beta (Beta 4). In this particular case, we're not going to gain much by waiting, and we stand to lose valuable time if this patch doesn't fix the crash here. I'm happy to talk about this more, but I need Beta approval today or tomorrow if we're going to get this into Beta 4 which I believe goes to build on Monday. Thanks!
Lawrence approved the patch for bug 1181097. Clearing n-i.
Per IRC discussion with Maire, setting Gecko <40 to unaffected as the fix that landed here is not applicable even though similar crashes have existed earlier.
(In reply to Ritu Kothari (:ritu) from comment #20) > Lawrence approved the patch for bug 1181097. Clearing n-i. Looks like we are not seeing this in 40.0b6 so far. \o/
(In reply to Robert Kaiser (:email@example.com) from comment #22) > (In reply to Ritu Kothari (:ritu) from comment #20) > > Lawrence approved the patch for bug 1181097. Clearing n-i. > > Looks like we are not seeing this in 40.0b6 so far. \o/ Yes, the signature is gone in everything after beta 4, we're good. :)
Comment 23 sounds like a verification to me!
You need to log in before you can comment on or make changes to this bug.