1542517 - Crash in [@ mozilla::AudioSink::CheckIsAudible]

Status: RESOLVED WORKSFORME

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

Description

[[:philipp]]

This bug is for crash report bp-72257d75-af57-40ab-b247-fe8940190327.

Top 10 frames of crashing thread:

0  @0x32d80b56 
1 xul.dll void mozilla::AudioSink::CheckIsAudible dom/media/mediasink/AudioSink.cpp:302
2 xul.dll mozilla::AudioSink::PopFrames dom/media/mediasink/AudioSink.cpp:282
3 xul.dll void mozilla::AudioStream::GetUnprocessed dom/media/AudioStream.cpp:564
4 xul.dll long mozilla::AudioStream::DataCallback dom/media/AudioStream.cpp:649
5 xul.dll static long mozilla::AudioStream::DataCallback_S dom/media/AudioStream.h:265
6 xul.dll long cubeb_resampler_speex<float, cubeb_resampler_speex_one_way<float>, cubeb_resampler_speex_one_way<float> >::fill_internal_output media/libcubeb/src/cubeb_resampler.cpp:160
7 xul.dll cubeb_resampler_fill media/libcubeb/src/cubeb_resampler.cpp:335
8 xul.dll static long `anonymous namespace'::refill media/libcubeb/src/cubeb_wasapi.cpp:477
9 xul.dll static bool `anonymous namespace'::refill_callback_output media/libcubeb/src/cubeb_wasapi.cpp:809

this is a very low volume crash signature hanging around for a while already, mainly coming from 32bit installations on windows 7.
most urls point towards video playback on youtube while the crash is taking place.

Comment 1

[Jean-Yves Avenard [:jya]]

I would have thought that this crash could no longer occur following the work done in bug 1524890 and that made assertions even stronger in bug 1530322 :(

Comment 2

[Nils Ohlmeier [:drno]]

Not a single crash in 68 so far...

Comment 3

[Daniel Veditz [:dveditz]]

... and all the 67 crashes are from builds prior to the bug-fixes in comment 1. For trunk we can call this worksforme or fixed.

exploitable-looking crashes still happen on ESR-60, though at a very low rate (3 in the last month). bp-bac43a84-e8c0-4eb7-98c9-eb0760190422 The fixes look too big to take as a whole (and a bit feature-ish). Is there an easy band-aide solution lurking in there that would be appropriate?

Comment 4

[Nils Ohlmeier [:drno]]

I agree that the patches on bug 1524890 are too big for an uplift to ESR. Jean-Yves do you have any idea of what exactly in your patches has fixed this crash and if you could extract just that bit for an uplift?

Comment 5

[Jean-Yves Avenard [:jya]]

(In reply to Nils Ohlmeier [:drno] from comment #4)

I agree that the patches on bug 1524890 are too big for an uplift to ESR. Jean-Yves do you have any idea of what exactly in your patches has fixed this crash and if you could extract just that bit for an uplift?

Following bug 1524890 we no longer use raw pointers when passing audio buffers around. We also now dynamically calculate how many audio frames a memory buffer actually contains.
in the AudioSink we no longer pass a raw pointer without its size, previously the code always assumed that some XX amount of frames were there making it quite easy to trigger out of bound memory access.

As such, I can't pinpoint exactly which part will fix this bug.

If the volume is low, I think we should just let this ride the train.

Comment 6

[Jean-Yves Avenard [:jya]]

(In reply to Daniel Veditz [:dveditz] from comment #3)

... and all the 67 crashes are from builds prior to the bug-fixes in comment 1. For trunk we can call this worksforme or fixed.

Are you sure? I still see crashes with those fixes in.

Comment 7

[Nils Ohlmeier [:drno]]

Looks like there was still a very low number of crashes in 67. jya: is there anything we can do to investigate this, or is the only option now to call this 'stalled'?

Comment 8

[Jean-Yves Avenard [:jya]]

I have no explanation for those crashes.
:padenot maybe?

Comment 9

[Paul Adenot (:padenot)]

I don't have a clue, sorry.

Comment 10

[Liz Henry (:lizzard)]

Dan, sounds like we should declare this stalled - unless you have any other ideas.
jya, worth one more look at the more recent crashes before we mark it stalled?

Comment 11

[Daniel Veditz [:dveditz]]

(In reply to Jean-Yves Avenard [:jya] from comment #6)

(In reply to Daniel Veditz [:dveditz] from comment #3)

... and all the 67 crashes are from builds prior to the bug-fixes in comment 1. For trunk we can call this worksforme or fixed.
Are you sure? I still see crashes with those fixes in.

History has proved me wrong: there are still _EXEC violations in 68.0.x and ESR-68.

(In reply to Nils Ohlmeier [:drno] from comment #7)

Looks like there was still a very low number of crashes in 67. jya: is there anything we can do to investigate this, or is the only option now to call this 'stalled'?

What's the fuzzing coverage of this code (cubeb and audio stream)? Are the functions in these stacks covered? Are any of the non-RELEASE asserts triggered in debug builds? If the fuzzing coverage is low then more fuzzing might turn up the problem. One problem doing that might be that I think we only fuzz 64bit builds and all the dangerous crashes are on 32-bit. Would 64-bit builds show the problem but just happen to handle it gracefully, or is some 32bit limit integral to whatever's going wrong here?

Comment 12

[Jean-Yves Avenard [:jya]]

I can't add anything to this bug at this stage.

Comment 13

[Liz Henry (:lizzard)]

drno, any thoughts on comment 11?

Comment 14

[Nils Ohlmeier [:drno]]

I don't think that we have any fuzzing coverage for cubeb, as it is a library which interacts with OS APIs (which I think makes fuzzing super hard).

This appears to be stalled to me at this point.

Comment 15

[Daniel Veditz [:dveditz]]

This looks like a "worksforme" -- fixed by something after 68. Uplifting to 68 seemed problematic (comment 5) and given the low volume we can probably wontfix it for ESR.

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.