1572878 - Voice-change-O-matic crashes tab

Status: VERIFIED FIXED

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

Description

[Alexandre Vicenzi]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:70.0) Gecko/20100101 Firefox/70.0

Steps to reproduce:

1. Visit https://mdn.github.io/voice-change-o-matic/
2. Try to play something (select something on combo box or click somewhere in the page)

Actual results:

Tab crashes.

Expected results:

Not crash.

Can reproduce on Ubuntu 18 under NIghtly 70.0a1 (2019-08-09).
It crashes when asking for Mic permission.
Tested under clean profile.
No crash report is available on about:crashes.

Comment 1

[Andreas Pehrson [:pehrsons]]

Thanks for the report! I can reproduce.

[Tracking Requested - why for this release]:
getUserMedia crash regression

Comment 2

[Andreas Pehrson [:pehrsons]]

I'm only able to reproduce this in firefox-trunk on the profile I use daily. If I hook it up with a fresh profile, or run another build with mozregression, it doesn't repro. Under rr it doesn't repro either.

I managed to repro with gdb attached to the child process - it gets a SIGKILL. But who kills it?

Comment 3

[Andreas Pehrson [:pehrsons]]

With some profile fiddling and mozregression I tracked this down to bug 1429847.

That makes sense -- we overshoot the hard realtime limit for some reason, and rtkit gives us the SIGKILL.
This does work in most cases. I'm not sure what this page is doing that is special. That - and the fact that only linux is affected makes this lower priority. Changing to P2.

Paul, can you take a look when you're back?

Comment 4

[Marcia Knous [:marcia - needinfo? me/PTO 12/6-12/12)]]]

Discussed during triage today. I missed the fact that this is Linux only. I agree that it can wait until Paul comes back to be addressed.

Comment 5

[Paul Adenot (:padenot)]

Ok this is because this page creates a convolver, and then sets an impulse that is 16 seconds long. This is about 120ms of computation on my i9-7940X. We set a max real-time quantum of 50ms on the real-time audio thread.

I'm going to do two things:

• In this bug, I'm going to move this computation to another thread
• In another bug, I'm going to implement something to be able to handle this more gracefully

Comment 6

[Paul Adenot (:padenot)]

Attachment: Bug 1572878 - Add a new method on AudioNodeStream, to be able to set a Reverb object on a ConvolverNode, from the main thread, using a ControlMessage. r?karlt

Comment 7

[Paul Adenot (:padenot)]

Attachment: Bug 1572878 - Initialize the Reverb object of ConvovlerNodes asynchronously to avoid blocking the audio rendering thread, when running on a real-time AudioContext. r?karlt

This is using the MediaDecoding thread pool because it is expected that it has
already been started shortly before creating the ConvolverNode, because the
impulse itself had to be decoded.

This is bouncing the data from the main thread to a media thread pool thread,
where the reverb object is initialized (this is the expensive part), then back
to the main thread, and then to the MSG thread.

When this process is finished, the reference on the task queue is dropped,
attempting to free the threads.

On an OfflineAudioContext, the behaviour is largely unchanged.

Comment 8

[Karl Tomlinson (back Dec 20 :karlt)]

Attachment: ConvolverNode instantaneous start testcase

Testcase for https://phabricator.services.mozilla.com/D42901?id=151346

Comment 9

[Paul Adenot (:padenot)]

Attachment: Bug 1572878 - Compute the reverb on the main thread. r?karlt

This is necessary to keep the invariant that once the setter returns, the reverb
is ready to process.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/af6f58a90573
Add a new method on AudioNodeStream, to be able to set a Reverb object on a ConvolverNode, from the main thread, using a ControlMessage. r=karlt
https://hg.mozilla.org/integration/autoland/rev/3291687c7cf7
Compute the reverb on the main thread. r=karlt

Comment 11

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/af6f58a90573
https://hg.mozilla.org/mozilla-central/rev/3291687c7cf7

Comment 12

[Liz Henry (:lizzard)]

Can you request uplift to beta? Thanks!

Comment 13

[Liz Henry (:lizzard)]

Would you mind testing with the latest Nightly to verify the issue is fixed?

Comment 14

[Alexandre Vicenzi]

(In reply to Liz Henry (:lizzard) from comment #13)

Would you mind testing with the latest Nightly to verify the issue is fixed?

Issue is fixed in 71.0a1 (2019-09-07).

https://mdn.github.io/voice-change-o-matic works as expected.

Comment 15

[Paul Adenot (:padenot)]

Comment on attachment 9088191 [details]
Bug 1572878 - Compute the reverb on the main thread. r?karlt

Beta/Release Uplift Approval Request

• User impact if declined: Crash with SIGKILL of the content process.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce: Load https://mdn.github.io/voice-change-o-matic/, accept the prompt
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is making Firefox behave like Chrome. Not a complicated patch.
• String changes made/needed:

Comment 16

[Liz Henry (:lizzard)]

Comment on attachment 9088191 [details]
Bug 1572878 - Compute the reverb on the main thread. r?karlt

Crash fix, verified in nightly, let's take this for beta 6.

Comment 17

[Cristian Brindusan [:cbrindusan]]

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

Comment 18

[Cristian Brindusan [:cbrindusan]]

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

Comment 19

[Narcis Beleuzu [:NarcisB]]

Backed out for mda failures on test_convolverNodeOOM.html

Backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/2228823d219f25bef2c69de9c58760a85f42a973
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266204400&repo=mozilla-beta&lineNumber=74629

Comment 20

[Paul Adenot (:padenot)]

Attachment: Bug 1572878 - Take a test fix from 1576656.

Comment 21

[Paul Adenot (:padenot)]

Narcis, Cristian, this is a bit of a test fix that is missing. It was landed in bug 1576656 (originally landed at the same time), but I attached just the required bit here if that's helpful. You can re-land the three together and it will work, or uplift bug 1576656 as well (it's not very big: a spec fix and an added test), to get the exact state in which central is right now. Thanks!

Comment 22

[Adrian Florinescu [:adrian_sv]]

Tried to verify the fix on Ubuntu 16.04 x64, assuming that what's left to uplift shouldn't affect the crash. However, seems that I still get a tab crash without the remaining patch uplifted:

• 71.0a1 20190912215412 - problem is fixed
• 70.0b5 20190909162732 - problem reproduces, tab crashes (expected, fix not uplifted to b5)
• 70.0b6 20190912160217 - problem still reproduces, tab crashes

Paul, is this expected? Is the remaining patch needed for this fix on b6? Also, is this affecting and should be verified on the rest of OS'es (windows/mac)?

Comment 23

[Paul Adenot (:padenot)]

it's been backed out, can you try again when this has re-landed ?

Comment 24

[Andreas Pehrson [:pehrsons]]

Let's update the flags accordingly.

Comment 25

[Bogdan Tara[:bogdan_tara]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cf977ea491dd
https://hg.mozilla.org/releases/mozilla-beta/rev/bb72b1c4113e
https://hg.mozilla.org/releases/mozilla-beta/rev/b76c6c0285e0

Comment 26

[Adrian Florinescu [:adrian_sv]]

No idea how this uplift skipped between the cracks of the uplift verification, but verified in b14 anyways on Ubuntu 16.04 using Firefox 70.0b14 20191010142853 and double checked Firefox 71.0a1 20191010214019, just to be sure.