1181735 - crash in mozilla::AudioMixer::FinishMixing()

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[(Away)]

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

Comment 2

[Paul Adenot (:padenot)]

Attachment: patch

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.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 4

[Robert Kaiser]

(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 5

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ and other approvals given.

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25820d96ddf8

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25820d96ddf8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/bd50a25b0ff3

Comment 11

[Paul Adenot (:padenot)]

(clearing NI)

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Any noticeable change in crash rate since this landed? Should we consider a belts-and-suspenders backport to esr38 as well?

Comment 13

[Paul Adenot (:padenot)]

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?

Comment 14

[Florin Mezei, QA (:FlorinMezei)]

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.

Comment 15

[Paul Adenot (:padenot)]

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.

Comment 16

[Paul Adenot (:padenot)]

Sylvestre is still in PTO. Ritu, would you mind having a look ?

After further analysis, I'm rather confident bug 1181097 will help.

Comment 17

[(Away)]

(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

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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.

Comment 19

[Maire Reavy [:mreavy]]

(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!

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Lawrence approved the patch for bug 1181097. Clearing n-i.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 22

[Robert Kaiser]

(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/

Comment 23

[Robert Kaiser]

(In reply to Robert Kaiser (:[e-mail]) 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 24

[Matt Wobensmith [:mwobensmith][:matt:]]

Comment 23 sounds like a verification to me!