1426603 - Crash in mozilla::SystemClockDriver::WaitForNextIteration | mozilla::MediaStreamGraphImpl::UpdateMainThreadState

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P2)

Description

[Karl Tomlinson (:karlt)]

+++ This bug was initially created as a clone of Bug #1382366 +++

We still have reports of this crash in builds with changes for bug 1382366 and bug 1422631 for bug 1422631.

e.g. https://crash-stats.mozilla.com/report/index/c0d3d7b4-b2da-4716-92b9-6f0ee0171220

We even have a Linux crash in the same place, but with an address of 0
https://crash-stats.mozilla.com/report/index/a61f64f4-f61f-4c10-82e1-ee7150171206

Comment 1

[Karl Tomlinson (:karlt)]

Assigning to myself, because I suspect the same cause as bug 1418820, even though I don't know what that is.

Comment 2

[Jean-Yves Avenard [:jya]]

none of this would occur with bug 1425473 in

Comment 3

[Karl Tomlinson (:karlt)]

Looks like this may have become more frequent on Nightly since https://hg.mozilla.org/releases/mozilla-beta/rev/ff143543997b ~ Dec 7.
I guess previously we were crashing in AudioChannelsDownMix<T> before reaching this crash.

Comment 4

[Frederik Braun [:freddy]]

This bug has been idle for a while. Any news, Karl?

Comment 5

[Karl Tomlinson (:karlt)]

The lack of any nightly crash reports with this signature after Jan 5 is
expected, assuming the diagnostic asserts (nightly+aurora) from
https://bugzilla.mozilla.org/show_bug.cgi?id=1418820#c15 catch the problem
before this crash.

The majority of the assertion failures were caused by bug 1429666.  It's fix
is now on 58 and 59, but there hasn't been a noticeable reduction in this
signature on release or beta.

Bug 1436267 is a suspect for the few remaining diagnostic assert failures on
60 nightly and 59 aurora.

If the fix for bug 1436267 appears effective on nightly, then we can uplift to
other branches.  Due to low volume on nightly, we won't have evidence for a
positive result until week after the patch merges.

Comment 6

[Frederik Braun [:freddy]]

We have not seen any crashes for releases after 59.0b9 (build id 20180209162511).
Does this mean we want to uplift?

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: esr52 patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
sec-high

User impact if declined:
low frequency crash, potentially exploitable, but we don't have STR.

Fix Landed on Version:
60, uplifted to 59

Risk to taking this patch (and alternatives if risky):
low

String or UUID changes made by this patch:
none

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

The patch indicates that there is a problem in handling errors from cubeb.
Analysis of cubeb may be able to identify sources of errors and so steps
to generate these errors.  There remains to identify how to exploit the flaw.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The comments in the patch explain the flaw.  These are already public through
bug 1436267.  Bug 1436267 does not clearly identify a problem in 52 because
the assertion that failed never existed in 52.

Which older supported branches are affected by this flaw?

52.

If not all supported branches, which bug introduced the flaw?

This crash pre-existed as bug 1382366.  The fix for bug 1382366
introduced a new flaw while remedying a potentially existing flaw.
The consequences of each flaw are similar.

Do you have backports for the affected branches?

Yes.  This is the one backport remaining.

How likely is this patch to cause regressions; how much testing does it need?

The patch has been shipped previously on 59 and 60, and so does not need
further testing.  It is a much simpler patch, completing the one for bug
1382366 that caused this regression, and so this is unlikely to cause further
regressions.

Comment 8

[Karl Tomlinson (:karlt)]

This is really a duplicate of bug 1436267, but that information is not yet public.
Currently unresolved is a low frequency crash on ESR52.

Comment 9

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

(In reply to Karl Tomlinson (:karlt) from comment #8)
> This is really a duplicate of bug 1436267, but that information is not yet
> public.
> Currently unresolved is a low frequency crash on ESR52.

So the status of fixed in 59 and such is because of it being fixed in 1436267.

We're too late to uplift to ESR52.7, which ships on Tuesday.

Comment 10

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

Comment on attachment 8957408 [details] [diff] [review]
esr52 patch

Clearing sec-approval. This is already fixed on trunk. I'll leave the ESR52 request for the release management to approve, I assume about two weeks after we ship 52.7 next week.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Yeah, this already missed 52.7.0 unfortunately.

Right now, we won't be able to take this until the 52.8.0 release in May (shipping alongside Fx60). However, since it's such a simple patch and alignment with Fx59 would be preferable, I'm going to keep this on the radar as a 52.7.1 ride-along candidate in the event that such an opportunity arises during this cycle.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8957408 [details] [diff] [review]
esr52 patch

I discussed this with Dan and Al and neither felt this was a risky patch to take. Given that this bug has already been noted in our Fx59 advisories and has been in the tree for over a month without any known regressions, I'm taking this for the 52.7.2 release.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/ca0b92ecedee6561e0ce590eab9309c31a1ad0c5