1617283 - AudioSession COM Cleanup

Status: RESOLVED FIXED

(Core :: Widget: Win32, task, P3)

Description

[(No longer employed by Mozilla) Aaron Klotz]

I see a few COMy things in AudioSession that we should clean up:

• Remove CoInitialize and CoUninitialize calls; since mscom::ProcessRuntime is instantiated during startup, COM is always available by the time this code runs.
• Since the thread started by SpawnASCReleaseThread does not declare its apartment, it implicitly lives inside the multi-threaded apartment. Unfortunately the IAudioSessionControl interface came from the main thread's single-threaded apartment, so calling Release on it from the multi-threaded apartment actually violates the rules of COM. At the very least the interface should be wrapped in a mscom::AgileReference while passing between threads.
• We're using PR_CreateThread here; perhaps NS_DispatchBackgroundTask is better suited to this? I guess that depends on whether it is possible to block shutdown by using the latter...

Comment 1

[(No longer employed by Mozilla) Aaron Klotz]

Note that CLSID_MMDeviceEnumerator has a threading model of Both, which means that its objects are safe to use in either an STA or the MTA, but not simultaneously. Passing between apartments requires either marshaling or an agile reference.

Comment 2

[David Parks [:handyman]]

Thanks Aaron. I believe we can remove the use of AudioSession entirely from content processes entirely because of audio remoting -- I'm trying that out in bug 1430907 comment 27. But AudioSession also runs in the plugin and main processes. Does what you're saying apply there as well? I don't know if you know how the plugin COM setup is done but I'm wondering about your first 2 points there and in main (where I assume the Co(Un)Initialize calls are redundant but I don't know about the STA/MTA of it all).

Comment 3

[(No longer employed by Mozilla) Aaron Klotz]

The good news is that I did some further investigation and found out that the MMDeviceEnumerator aggregates the free-threaded marshaler, which in plain English means that the STA/MTA threading stuff isn't an issue after all.

As helpful as audio remoting is, I think we should still take care of the first and third points. But which ever background thread we run the cleanup on, the runnable should be wrapped within a mscom::MTARegion

Comment 5

[Shazib Summar [:ssummar]]

Attachment: WIP: Bug 1617283 - Removed COM violations and shifted AudioSession to MTA r=handyman

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/14ef7c10fef4
Removed COM violations and shifted AudioSession to MTA r=handyman

Comment 7

[Atila Butkovits]

Backed out for causing bustages complaining about Unified_cpp_widget_windows0.obj.

Backout link: https://hg.mozilla.org/integration/autoland/rev/b0b7509ee7a0db5bb617a1a4dc6e345be2ec9385

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=Cutl7Na0TnCDNZ_y4sT29w.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=14ef7c10fef4d5cf6590ef2fd99f82c6d6c4c64d

Failure log: https://treeherder.mozilla.org/logviewer?job_id=366889094&repo=autoland&lineNumber=31670

Comment 8

[Shazib Summar [:ssummar]]

Sorry about the backout. It's a very trivial issue. I'm working on it.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e1b946a41694
Removed COM violations and shifted AudioSession to MTA r=handyman

Comment 10

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/e1b946a41694

Comment 11

[Narcis Beleuzu [:NarcisB]]

Backed out for audio related crashes and hangs (bug 1755700, bug 1755717)
Backout link: https://hg.mozilla.org/mozilla-central/rev/1f245c0744d051d4382f2ee21dc6acadf04152d7

Comment 12

[Shazib Summar [:ssummar]]

Attachment: Bug 1617283 - Shifted AudioSession to MTA and removed COM violations r=handyman

Pointer to AudioSession object is made std::atomic to resolve crashes (bug 1755700) by atomically modifying/reading the pointer as an effort to make sure any update to the pointer is seen across other threads immediately while not using locks to maintain such concurrency and avoiding dataraces.

This patch is built upon the work done in D136377.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6fd4b72c9a6c
Shifted AudioSession to MTA and removed COM violations r=handyman

Comment 14

[Narcis Beleuzu [:NarcisB]]

Backed out for dt failure on browser_dbg-backgroundtask-debugging.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/57045ad64fb48792be9e513c55520b0a052bac22
Log link: https://treeherder.mozilla.org/logviewer?job_id=371198472&repo=autoland&lineNumber=2035

Comment 15

[Shazib Summar [:ssummar]]

Thanks for notifying. I'm looking into it.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2230bd66878c
Shifted AudioSession to MTA and removed COM violations r=handyman

Comment 17

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/2230bd66878c

Comment 18

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

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.