1209987 - webrtc.org Engine creation and destruction should happen on the WebRTC threads

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

<jesup> gcp: The CamerasParent code blows up new thread-safety checks added in 43... :-(  It verifies that you're calling AllocateCaptureDevice() from the ViECaptureThread... and we're calling it from a CamerasParent thread (VideoCapture)
<jesup> gcp: I think module_process_thread_ is created in ViESharedData::ViESharedData() (via ProcessThread::Create())
<jesup> yes, pbackground: CamerasParent.cpp:275   helper->mEngine = webrtc::VideoEngine::Create(helper->mConfig);

What this means is that the creation and destruction of the webrtc.org engines should be done on the same thread as the one the actual calls are dispatched to.

We currently don't do this, consequently they're on PBackground:
https://dxr.mozilla.org/mozilla-central/source/dom/media/systemservices/CamerasParent.cpp#236

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Attachment: webrtc.org Engine creation and destruction should happen on the WebRTC threads

This also slightly simplifies the shutdown sequence to make it clear
what happens where. Quite a bit of thread and ref juggling but no way
around that.

Comment 2

[Gian-Carlo Pascutto [:gcp]]

So it looks like we can get rid of all the locking if we can run everything on the WebRTC thread. 

Unfortunately there's this:
https://bugzilla.mozilla.org/show_bug.cgi?id=1070216#c32

I'm going to see if that is still needed.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8670739 [details] [diff] [review]
webrtc.org Engine creation and destruction should happen on the WebRTC threads

Review of attachment 8670739 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/media/systemservices/CamerasParent.cpp
@@ +513,5 @@
>  CamerasParent::RecvGetCaptureDevice(const int& aCapEngine,
>                                      const int& aListNumber)
>  {
>    LOG((__PRETTY_FUNCTION__));
> +  LOG(("RecvGetCaptureDevice"));

redundant

@@ +575,3 @@
>        int error = -1;
> +      if (!self->EnsureInitialized(aCapEngine)) {
> +        LOG(("Fails to initialize"));

How about (for all of these) LOG(("Failed to initialize in " __FUNCTION__)) or "Failed to initialize in CamerasParent"

@@ +744,5 @@
> +              self->mEngines[aCapEngine].mEngineIsRunning = false;
> +            }
> +          }
> +          MutexAutoLock lock(self->mCallbackMutex);
> +          for (unsigned int i = 0; i < self->mCallbacks.Length(); i++) {

size_t perhaps

@@ +851,5 @@
> +
> +  // We need to shut down the video capture thread, but in
> +  // all likelyhood we're running on it (when the runnable
> +  // in ActorDestroy derefs self). Dispatch the stop to the main
> +  // thread (PBackground which started it is likely gone by now).

Thread shutdown has to occur from the starting thread I think, at least on some OSes (check with froyd)

Comment 4

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c6f62b302aa

Comment 5

[Gian-Carlo Pascutto [:gcp]]

Attachment: webrtc.org Engine creation and destruction should happen on the WebRTC threads

Removed locks. Addressed review comments.

Comment 6

[Gian-Carlo Pascutto [:gcp]]

(In reply to Randell Jesup [:jesup] from comment #3)
> Thread shutdown has to occur from the starting thread I think, at least on
> some OSes (check with froyd)

The VideoCaptureThread (base::thread) is created when PBackground creates the CamerasParent class. I presume that (the creation) is on the PBackground thread. When ActorDestroy gets called, we need to shut down the WebRTC stuff on the VideoCaptureThread. To do this the relevant shutdown runnable needs to take a reference to CamerasParent, and when the shutdown sequence chair-dance ends, it's going to be the one holding the last ref to it because PBackground is gone by then. So ~CamerasParent is now invoked on the VideoCaptureThread and somehow has to get rid of itself. Oops.

This is tricky. I can think of 3 ways (that I dare post here):

1) Synchronously dispatch the WebRTC shutdown tasks to the VideoCaptureThread, while sitting on the PBackground thread in ActorDestroy, and then do the destruction of the thread on PBackground. This is a little evil, and I haven't tried if it works, but I suspect it might. This causes PBackground to block on WebRTC shutdown.
2) Do the thread creation on PBackground, and in the destructor Dispatch the thread stop/deletion to the mainthread. This is the cleanest. But as stated above jesup isn't sure it's allowed, I'm not either, and froydnj responded "dare I ask why you even want to know this?"
3) Somehow do the thread creation on mainthread. You really want to have the thread up by the time the CamerasParent ctr finishes though, because otherwise all the incoming PBackground traffic must check if that thread is up (and hold if it's not done yet? that brings us back to 1).

Comment 7

[Gian-Carlo Pascutto [:gcp]]

Attachment: webrtc.org Engine creation and destruction should happen on the WebRTC threads

I ended up implementing (3). I think it's what I'll least regret later.

Comment 8

[Gian-Carlo Pascutto [:gcp]]

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

Comment 9

[Gian-Carlo Pascutto [:gcp]]

Attachment: Assert that Cameras are not used on the main thread

Need to test how much (hopefully nothing) breaks after bug Bug 1070216 lands.

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8670839 [details] [diff] [review]
webrtc.org Engine creation and destruction should happen on the WebRTC threads

Review of attachment 8670839 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the mVideoCaptureThread fully within the monitor lock

::: dom/media/systemservices/CamerasParent.cpp
@@ +629,2 @@
>          cbh = self->mCallbacks.AppendElement(
> +            new CallbackHelper(static_cast<CaptureEngine>(aCapEngine), capnum, self));

Is this just extra indent?

@@ +812,5 @@
> +  if (mVideoCaptureThread) {
> +    base::Thread *thread = mVideoCaptureThread;
> +    {
> +      MonitorAutoLock lock(mThreadMonitor);
> +      mVideoCaptureThread = nullptr;

*thread = mVideoCaptureThread; 
That should be within the lock, no?  And the test.
If mVideoCaptureThread is referenced from multiple threads (and the lock says it is), then all access must be under lock or otherwise guaranteed not to have TSAN issues -- or make it Atomic, which might be a pain perhaps.

Comment 11

[Gian-Carlo Pascutto [:gcp]]

(In reply to Randell Jesup [:jesup] from comment #10)
> @@ +812,5 @@
> > +  if (mVideoCaptureThread) {
> > +    base::Thread *thread = mVideoCaptureThread;
> > +    {
> > +      MonitorAutoLock lock(mThreadMonitor);
> > +      mVideoCaptureThread = nullptr;
> 
> *thread = mVideoCaptureThread; 
> That should be within the lock, no?  And the test.
> If mVideoCaptureThread is referenced from multiple threads (and the lock
> says it is), then all access must be under lock or otherwise guaranteed not
> to have TSAN issues -- or make it Atomic, which might be a pain perhaps.

Well, the thread that this is on is the only one writing to it, so I think the *reads* in the same thread can safely go outside the lock.

Comment 12

[Gian-Carlo Pascutto [:gcp]]

Failing on try:

 10:21:34     INFO -  [1882] ###!!! ASSERTION: Failed NS_DispatchToMainThread() in shutdown; leaking: 'false', file /builds/slave/try-lx-d-000000000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp, line 192
 10:21:34     INFO -  #01: NS_DispatchToMainThread(already_AddRefed<nsIRunnable>&&, unsigned int) [xpcom/glue/nsThreadUtils.cpp:192]
 10:21:34     INFO -  #02: NS_DispatchToMainThread(nsIRunnable*, unsigned int) [xpcom/glue/nsThreadUtils.cpp:209]
 10:21:34     INFO -  #03: mozilla::camera::CamerasParent::~CamerasParent() [dom/media/systemservices/CamerasParent.cpp:826]
 10:21:34     INFO -  #04: mozilla::camera::CamerasParent::~CamerasParent() [memory/mozalloc/mozalloc.h:210]
 10:21:34     INFO -  #05: mozilla::camera::CamerasParent::Release() [dom/media/systemservices/CamerasParent.h:78]
 10:21:34     INFO -  #06: mozilla::media::LambdaRunnable<mozilla::camera::CamerasParent::ActorDestroy(mozilla::ipc::IProtocolManager<mozilla::ipc::IProtocol>::ActorDestroyReason)::<lambda()> >::~LambdaRunnable [xpcom/glue/nsThreadUtils.h:230]

The comments in nsThreadUtils.cpp seem to suggest that this Is A Bad Idea.

Comment 13

[Gian-Carlo Pascutto [:gcp]]

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

Comment 14

[Gian-Carlo Pascutto [:gcp]]

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

Comment 15

[Gian-Carlo Pascutto [:gcp]]

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

Comment 16

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2dfc0c051845

Comment 17

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f433098d038

Comment 18

[Gian-Carlo Pascutto [:gcp]]

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

Comment 19

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99d05c4dfbee

Comment 20

[Gian-Carlo Pascutto [:gcp]]

Attachment: webrtc.org Engine creation and destruction should happen on the WebRTC threads

Sorry for yet another review, but as you can see above all the 
past approaches didn't actually work properly.

We now observe for xpcom-shutdown, dispatch the WebRTC engine
closing to the WebRTC thread, and sit on a monitor (i.e. it's a blocking,
sync dispatch) for that to finish, after which we dispatch the WebRTC
thread shutdown to the main thread (which might not be a real dispatch
if the shutdown came from xpcom-shutdown since we're in the main thread then,
but we must also handle the case where PBackground shuts down our actor). 
We must block hard because once in xpcom-shutdown, the runnable in the WebRTC
thread might  not get a chance to dispatch the thread close to the main thread
before we're in xpcom-shutdown-threads, at which point the main thread is no 
longer  accessible, causing an inevitable thread leak.

The proper machinery has been added to distinguish the two additional
states, i.e. "WebRTC thread still being spun up" and "WebRTC is being
shut down, thread may go away soon".

Comment 21

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8671955 [details] [diff] [review]
webrtc.org Engine creation and destruction should happen on the WebRTC threads

Review of attachment 8671955 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming there's no issue with the booleans and RemoveObserver() is added (or point out that it's not needed for some reason), but we may be introducing races depending on the answer about the booleans.  Likely that requires at least Atomic<> or a lock.

::: dom/media/systemservices/CamerasParent.cpp
@@ +151,5 @@
> +                       const char16_t *aData)
> +{
> +  MOZ_ASSERT(!strcmp(aTopic, NS_XPCOM_WILL_SHUTDOWN_OBSERVER_ID));
> +  StopVideoCapture();
> +  return NS_OK;

Don't you need to remove the Observer here?

@@ +399,5 @@
> +      LOG(("Being closed down while engine %d is running!", i));
> +    }
> +    if (mEngines[i].mPtrViERender) {
> +      mEngines[i].mPtrViERender->Release();
> +      mEngines[i].mPtrViERender = nullptr;

Perhaps not critical for this patch landing (and an existing issue), but wouldn't it be easier/safer to use ScopedCustomReleasePtr<webrtc::ViEBase>/etc like VideoConduit/AudioConduit do?  Then these become ...mPtrViERender = nullptr;/etc

::: dom/media/systemservices/CamerasParent.h
@@ +142,5 @@
>  
>    // Shutdown handling
>    bool mChildIsAlive;
>    bool mDestroyed;
> +  bool mWebRTCAlive;

What's the thread access story for these three?  What threads can access? Is there locking? (I think no) Perhaps these should be some form of Atomic<bool> (not sure the level of consistency across the set needed)

Comment 22

[Gian-Carlo Pascutto [:gcp]]

(In reply to Randell Jesup [:jesup] from comment #21)

> >    // Shutdown handling
> >    bool mChildIsAlive;
> >    bool mDestroyed;
> > +  bool mWebRTCAlive;
> 
> What's the thread access story for these three?  What threads can access? Is
> there locking? (I think no) Perhaps these should be some form of
> Atomic<bool> (not sure the level of consistency across the set needed)

mChildIsAlive and mDestroyed should be PBackground-only vars.

It looks like mWebRTCAlive needs a memory barrier in EnsureInitialized (the other access are under the monitor), so making it Atomic is probably easiest.

Comment 23

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #11)
> (In reply to Randell Jesup [:jesup] from comment #10)
> > @@ +812,5 @@
> > > +  if (mVideoCaptureThread) {
> > > +    base::Thread *thread = mVideoCaptureThread;
> > > +    {
> > > +      MonitorAutoLock lock(mThreadMonitor);
> > > +      mVideoCaptureThread = nullptr;
> > 
> > *thread = mVideoCaptureThread; 
> > That should be within the lock, no?  And the test.
> > If mVideoCaptureThread is referenced from multiple threads (and the lock
> > says it is), then all access must be under lock or otherwise guaranteed not
> > to have TSAN issues -- or make it Atomic, which might be a pain perhaps.
> 
> Well, the thread that this is on is the only one writing to it, so I think
> the *reads* in the same thread can safely go outside the lock.

Actually, one of the horrors of TSAN is that this is wrong.  Reads from another thread can corrupt the value.  Really. https://blog.mozilla.org/nnethercote/2015/02/24/fix-your-damned-data-races/ and the lastr example from https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-could-possibly-go-wrong

Comment 24

[Gian-Carlo Pascutto [:gcp]]

(In reply to Randell Jesup [:jesup] from comment #23)
> Actually, one of the horrors of TSAN is that this is wrong.  Reads from
> another thread can corrupt the value.  Really.
> https://blog.mozilla.org/nnethercote/2015/02/24/fix-your-damned-data-races/
> and the lastr example from
> https://software.intel.com/en-us/blogs/2013/01/06/benign-data-races-what-
> could-possibly-go-wrong

The relevant ones only apply to stack vars, not in-memory ones, which is guaranteed here. Of course there's the catch-all undefined behavior issue.

Note that the last patch already got rid of that code.

Comment 25

[Gian-Carlo Pascutto [:gcp]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=894263233ee3

Comment 26

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc7d1acf804f05db4c6cea3f480cbc6bccbce153
Bug 1209987 - webrtc.org Engine creation and destruction should happen on the WebRTC threads. r=jesup

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=15554576&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/730bd2d9fe31

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Hrm, this happened twice in a row, and backfilling the gap behind the first instance showed this was in fact the first instance. But then it didn't fail on a new push several pushes later (which didn't finish until I had already backed this out, assuming it was permafailing). So maybe this isn't at fault.

doing a bunch of retriggers to see if I can get it to happen earlier.

Comment 29

[Gian-Carlo Pascutto [:gcp]]

This code has a large risk of causing leaks (see all try pushes above), but the current implementation didn't leak on try, and the leak in the logs seems to originate from unrelated audio code.

Comment 30

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c88cbf86ea362ddcfa0ae692caa538aaa120640
Bug 1209987 - webrtc.org Engine creation and destruction should happen on the WebRTC threads. r=jesup

Comment 31

[Gian-Carlo Pascutto [:gcp]]

Relanded in agreement with the sheriffs.
<nigelb|sheriffduty> Out of 8 test runs, only 2 were orange.

It's possible this modifies shutdown timing so it's exposing bugs in other code.

Comment 32

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #31)
> Relanded in agreement with the sheriffs.
> <nigelb|sheriffduty> Out of 8 test runs, only 2 were orange.
> 
> It's possible this modifies shutdown timing so it's exposing bugs in other
> code.

I did a bunch of retriggers on pushes near where you landed, and it leaked just like that linked one well before you landed, so I think you're in the clear in this particular case.

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9c88cbf86ea3