1429666 - cubeb_resampler_speex calls data callback while draining

Status: RESOLVED FIXED

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

Description

[Karl Tomlinson (:karlt)]

https://github.com/kinetiknz/cubeb/pull/398

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

Filing a separate bug from 1418820 because patches have already landed for
1418820.

This is a suspected common cause of crashes reported in bug 1418820 and possibly many other MSG crashes on NT and Android.  The bug was reproduced with a gtest in the pull request.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: bug 1429666 cubeb_resampler_speex: don't call data callback while draining

Review commit: https://reviewboard.mozilla.org/r/211942/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/211942/

Comment 2

[Karl Tomlinson (:karlt)]

Comment on attachment 8941722 [details]
bug 1429666 cubeb_resampler_speex: don't call data callback while draining

https://reviewboard.mozilla.org/r/211942/#review217726

::: media/libcubeb/README_MOZILLA:5
(Diff revision 1)
> +and the following patches, which may be overwritten when
> +included upstream.
> +https://github.com/kinetiknz/cubeb/pull/398/commits/c8e66dee61a35e6a6d54e3630e1668bdbd6984b4
> +https://github.com/kinetiknz/cubeb/pull/398/commits/2ed979bc891cf1a7822799947a357d4d3b625964

This intentionally only applies the changesets in
https://github.com/kinetiknz/cubeb/pull/398 rather than pulling libcubeb from
upstream, because we'll want to uplift this change to other branches.

I'm requesting review now, before receiving upstream review, in case I can get
review by tomorrow, because I don't expect to be available for some time after
that.

Comment 3

[Paul Adenot (:padenot)]

Comment on attachment 8941722 [details]
bug 1429666 cubeb_resampler_speex: don't call data callback while draining

https://reviewboard.mozilla.org/r/211942/#review217766

Comment 4

[Karl Tomlinson (:karlt)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bc1e8ff74fab218a7e55d0096b8eda104e99ad8

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1ad55edca07d
cubeb_resampler_speex: don't call data callback while draining r=padenot

Comment 6

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/1ad55edca07d

Comment 7

[Paul Adenot (:padenot)]

Comment on attachment 8941722 [details]
bug 1429666 cubeb_resampler_speex: don't call data callback while draining

Approval Request Comment
[Feature/Bug causing the regression]: bug 1008079
[User impact if declined]: Crashes on windows
[Is this code covered by automated tests?]: Yes, to be landed later (they are in https://github.com/kinetiknz/cubeb/pull/398, that we'll import in gecko soon).
[Has the fix been verified in Nightly?]: Yes, very clear (see https://github.com/mozilla/crashstop results)
[Needs manual test from QE? If yes, steps to reproduce]: No, this is covered by a unit test
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: This is basically just a big if() around the problematic code, to restore an invariant that was documented all along.
[String changes made/needed]: None.

Comment 8

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

Comment on attachment 8941722 [details]
bug 1429666 cubeb_resampler_speex: don't call data callback while draining

Hi Gerry, Philipp mentioned this would be a good candidate to include in the next RC build. Moving the nom from m-b to m-r for your review for next RC build.

Comment 9

[Gerry Chang [:gchang]]

Comment on attachment 8941722 [details]
bug 1429666 cubeb_resampler_speex: don't call data callback while draining

Fix a crash. Beta58+.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/63bf6cd0005459718a8a4a51bc3a7670aad5cdd2 (FIREFOX_58b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/571ae115f2de