Closed Bug 1572878 Opened 4 months ago Closed 3 months ago

Voice-change-O-matic crashes tab

Categories

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

69 Branch
Unspecified
Linux
defect

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- unaffected
firefox68 --- unaffected
firefox69 --- wontfix
firefox70 + verified
firefox71 --- verified

People

(Reporter: vicenzi.alexandre, Assigned: padenot)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file, 4 obsolete files)

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.

Component: Untriaged → Audio/Video
Product: Firefox → Core

Thanks for the report! I can reproduce.

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

Assignee: nobody → apehrson
Status: UNCONFIRMED → ASSIGNED
Has Regression Range: --- → no
Has STR: --- → yes
Ever confirmed: true
Keywords: regression
Priority: -- → P1

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?

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?

Assignee: apehrson → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(padenot)
OS: Unspecified → Linux
Priority: P1 → P2
Regressed by: 1429847
Version: 70 Branch → 69 Branch

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.

Assignee: nobody → padenot
Flags: needinfo?(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

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.

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

Attachment #9087092 - Attachment is obsolete: true
Pushed by padenot@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Can you request uplift to beta? Thanks!

Flags: needinfo?(padenot)

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

Flags: needinfo?(vicenzi.alexandre)

(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.

Flags: needinfo?(vicenzi.alexandre)

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:
Flags: needinfo?(padenot)
Attachment #9088191 - Flags: approval-mozilla-beta?
Attachment #9087091 - Flags: approval-mozilla-beta?

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.

Attachment #9088191 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9087091 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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!

Flags: needinfo?(padenot)
Flags: needinfo?(nbeleuzu)
Flags: needinfo?(cbrindusan)
QA Whiteboard: [qa-triaged]

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)?

Flags: needinfo?(padenot)

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

Flags: needinfo?(padenot)

Let's update the flags accordingly.

Flags: needinfo?(cbrindusan)
Attachment #9092414 - Attachment is obsolete: true
Flags: needinfo?(nbeleuzu)
Attachment #9088191 - Attachment is obsolete: true
Attachment #9087091 - Attachment is obsolete: true

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.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.