1422631 - suspect cubeb_data_callback called from refill_callback_duplex() while stream is draining

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

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

Alex observed in https://bugzilla.mozilla.org/show_bug.cgi?id=1406772#c12
that, unlike refill_callback_output(), refill_callback_duplex() does not
return early when stm->draining.

https://searchfox.org/mozilla-central/source/media/libcubeb/include/cubeb.h#346
claims that the data callback is not called during draining:

     "Returning less than the number of frames this callback asks for or
      provides puts the stream in drain mode. This callback will not be called
      again, and [...]"

refill_callback_input() seems to be OK.  It uses the different approach of
returning false when stm->draining, so that it is not called again.

refill_callback_duplex() does not consider stm->draining except in
get_output_buffer().  Does something ensure that GetCurrentPadding() always
returns zero for duplex output clients?

Comment 1

[Karl Tomlinson (:karlt)]

If GetCurrentPadding() is not always zero when called from
refill_callback_duplex() then that would explain many of the MSG crashes.
It would be a regression from https://hg.mozilla.org/mozilla-central/rev/54534d5c938247df4873aa8c37116b8c5de42a52

Comment 2

[Alex Chronopoulos [:achronop]]

I was planning to fix that today as part of 1406772. Since we have a new bug now I will use this one.

Comment 3

[Karl Tomlinson (:karlt)]

Thank you, Alex!

Comment 4

[Mike Taylor [:miketaylr]]

Alex, Bug 1406772 is fixed now, any updates on this issue?

Comment 5

[Alex Chronopoulos [:achronop]]

I will redirect the question to Karl who did the analysis. He suggested that the fix from 1406772 will also fix this one.

Comment 6

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

No sec crashes From AudioChannelsDownMix() since 12/4 builds (matches bug 1406772)

Nullptr crashes (MSGShutdownRunnable::Run()) continue:
  mGraph->mDriver->Shutdown(); 
I doubt mGraph is null, so my guess is somehow mDriver is.

UAFs in beta (and likely UAFs in 59) crashes continue in WaitForNextIteration() on this line:
  mGraphImpl->mNeedAnotherIteration = false; // atomic

Those could be spun into separate bugs -- karl?  You added those two signatures to this.

Comment 7

[Karl Tomlinson (:karlt)]

Filed bug 1426603 for WaitForNextIteration() and 1418820 exists for MSGShutdownRunnable::Run().

https://hg.mozilla.org/mozilla-central/rev/782f14e02e9b contains the fix for this bug and so marking fixed.

Not marking duplicate, because we may want the fix on other branches.  Hard to say though because I don't know how to determine whether it made any difference to the crash rate.  (I can't see how to change the domain of the crash-stats graphs, and I can't make sense of the data points unless there are many days when we didn't produce Nightlies.) This bug was a suspected cause for these crashes, but we clearly have another cause.

Comment 8

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

This was a sec bug, but the underlying issue for bug 1406772 has been uplifted to beta, and affects nothing before that, so I think we're good on that patch

Comment 9

[Alex Chronopoulos [:achronop]]

A small clarification. This is expected to be fixed with the second part of bug 1406772 which added with the cubeb import on bug 1423901. That cubeb change is not uplifted in beta. 

To remind you the case, bug 1406772 could be solved with two independent ways. Both of them was valid improvements and implemented. One landed with bug 1406772 and uplifted to beta. The second one, which is of interest in this bug, landed with cubeb import in bug 1423901 and it's _not_ uplifted in beta.

Comment 10

[Gerry Chang [:gchang]]

Hi :achronop & Karl,
Per comment #9, do we need to uplift cubeb import of bug 1423901 to 58 or we can leave it here?

Comment 11

[Gerry Chang [:gchang]]

Track 58+ as sec-high.

Comment 12

[Alex Chronopoulos [:achronop]]

I think we can live without it. This crash signature has been solved by the uplift of Bug 1406772. I believe this fix did not help in the way that Karl had been expecting so it's not necessary to uplift. If I am missing something, Karl can correct me.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

What, if anything, should we do about ESR52 then?

Comment 14

[Karl Tomlinson (:karlt)]

I agree with Alex, but will try to clarify the situation.

I'm removing the [@ mozilla::AudioChannelsDownMix<T>] crash signature because
I think it is adding to the confusion.

The issue reported here was fixed by the cubeb import in bug 1423901.
That is on 59 but not 58.

This issue was a suspected cause of bug 1406772, bug 1426603, and bug 1418820.

The patch in bug 1406772 has avoided the crash in
[@ mozilla::AudioChannelsDownMix<T>], but appears to have moved crashes to
mozilla::SystemClockDriver::WaitForNextIteration |
mozilla::MediaStreamGraphImpl::UpdateMainThreadState, which is bug 1426603.
This is consistent with suspecting a common underlying cause.

However, we have no evidence that this issue existed in real life, nor that
the cubeb import reduced the frequency of any of these crashes, and we have
evidence that bug 1426603 and bug 1418820 are still occurring in builds with
the cubeb import.  There is therefore no clear value in uplifting the cubeb
import, and so I'll mark this wontfix on other branches.

Please keep the bug restricted to the security group, however, until the
outstanding cause of these crashes is determined.  This is tracked in bug
1426603 and bug 1418820.