853998 - Use SyncRunnable in WebRTC

Status: RESOLVED FIXED

(Core :: WebRTC, defect, P1)

Description

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

We have a problem with upconverting threads to nsThreads to do DISPATCH_SYNC.

Given this:
on a pthread or other non-nsThread:
   Dispatch(..., DISPATCH_SYNC);

internally Dispatch will call GetCurrentThread(), which "upconverts" the pthread to an nsThread.(See nsThreadManager::GetCurrentThread()).

Then Dispatch() creates an nsThreadSyncDispatch(this_nsthread, runnable), and sends it to the target thread, and it sits in this loop:

    while (wrapper->IsPending())
      NS_ProcessNextEvent(thread);

Note that IsPending() is this:

  bool IsPending() {
    return mSyncTask != nullptr;
  }

and nsThreadSyncDispatch::Run() is this:

nsThreadSyncDispatch::Run()
{
  if (mSyncTask) {
    mResult = mSyncTask->Run();
    mSyncTask = nullptr;
    // unblock the origin thread
    mOrigin->Dispatch(this, NS_DISPATCH_NORMAL);
  }
  return NS_OK;
}

Note that it sets mSyncTask to nullptr *in the target task* and before returning the event/wrapper.

So the calling Dispatch() can exit without calling ProcessNextEvent().  This leaves the wrapper event in the nsThread's event queue, unprocessed - and it holds a reference to the nsThread itself!  So, later, when the pthread exits, and we drop the ref to the nsThread (from TLS storage), the count doesn't go to 0 and it leaks.

This only happens sometimes; usually (and always when run locally) it ends up waiting and clearing the event queue.

There are two obvious solutions: Don't up-promote threads to nsThread if they're not processing their event queues  (upconverting seems inherently dangerous, though with this fixed it might not be.  (I checked, a simple run of the browser showed no up-conversions other than from media/webrtc/signaling, but I didn't even browse around a little.)  Or fix DISPATCH_SYNC to not leave "to-be-cleaned-up" events on the queue.

This would be something like:

nsThreadSyncDispatch::Run()
{
  if (!mRun) {
    MOZ_ASSERT(mSyncTask);
    mResult = mSyncTask->Run();
    mRun = true;
    // unblock the origin thread
    mOrigin->Dispatch(this, NS_DISPATCH_NORMAL);
  } else {
    // We must be on the originating thread (mOrigin) which still holds a ref to us
    mSyncTask = nullptr;
  }
  return NS_OK;
}

A side-effect would be you could include RefPtrs/COMPtrs in the DISPATCH_SYNC runnable that aren't threadsafe (since they'd be released on the originating thread).

Comment 1

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

Attachment: make DISPATCH_SYNC safe from up-promoted threads

Comment 2

[Ed Morley [:emorley]]

jesup++ for tracking this down :-D

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: make DISPATCH_SYNC safe from up-promoted threads (Solution B)

An alternative solution. Randell asked me to post it.

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Try with original solution - https://tbpl.mozilla.org/?tree=Try&rev=4c6d14a0178e

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Try with alternate solution - https://tbpl.mozilla.org/?tree=Try&rev=eb3425727c0e

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: make DISPATCH_SYNC safe from up-promoted threads (Solution B)

Whoops, meant to null pointer, not forget it (why is there a forget anyway)?

Try with alternate solution - https://tbpl.mozilla.org/?tree=Try&rev=750d9b157221

I think I vote for Randell's though fwiw.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: make DISPATCH_SYNC safe from up-promoted threads (Solution B)

Try with alternate solution - https://tbpl.mozilla.org/?tree=Try&rev=b73e00f3e692

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Actually I don't think there's a thread-safe way to do solution B, so please ignore.

Comment 9

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

Comment on attachment 728417 [details] [diff] [review]
make DISPATCH_SYNC safe from up-promoted threads

Marking for review - if the decision is to remove upconversion, we can revert this at that time (and that will require WebRTC to replace upconversion first, which we're working on anyways).

FYI, I think jib's latest patch may also be safe in fact, though I haven't thought deeply on it.  His patch will allow short-term run-ups of wrappers waiting to be destroyed, though in normal cases this won't happen or will be very rare, and I doubt more than a few can accumulate temporarily outside of abusive test situations.  His patch does cause less change in behavior of DISPATCH_SYNC, though I don't know that this matters at all.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

> FYI, I think jib's latest patch may also be safe in fact,

On looking again, I agree it is.

Comment 11

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

(In reply to Jan-Ivar Bruaroey [:jib] from comment #4)
> Try with original solution -
> https://tbpl.mozilla.org/?tree=Try&rev=4c6d14a0178e

I've had about 150ish retriggers of M2 and C across all desktop platforms with no leaks.  Not definitive given the random nature, but looking quite good.

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Looks great!

Odd, my try of solution B is not looking good at all. Might I have made a mistake pushing it? Trying again: https://tbpl.mozilla.org/?tree=Try&rev=0334f791d163

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Solution B still leaks the runnable itself:

00:02:48 WARNING - TEST-UNEXPECTED-FAIL | leakcheck | 60 bytes leaked (nsRunnable) 

Makes sense since the underlying GSM prthread is gone, and the nsThread destructor apparently does not clean up events, but asserts:

00:02:44     INFO -  [Parent 3716] ###!!! ASSERTION: Non-empty event queue being destroyed; events being leaked.: 'IsEmpty()', file e:/builds/moz2_slave/try-w32-d-00000000000000000000/build/xpcom/threads/nsEventQueue.cpp, line 39

So Solution B would require more changes.

Comment 14

[(no longer active)]

Comment on attachment 728417 [details] [diff] [review]
make DISPATCH_SYNC safe from up-promoted threads

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

Looks good, except that it might still be racy on mHasRun, right?

And this is the type of thing that bsmedberg should review.  Although if I were going to review it, I'd ask you to add some comments about what mHasRun is being used for.

Comment 15

[(no longer active)]

Comment on attachment 728520 [details] [diff] [review]
make DISPATCH_SYNC safe from up-promoted threads (Solution B)

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

I don't think this is the right fix.

Comment 16

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> Comment on attachment 728417 [details] [diff] [review]
> make DISPATCH_SYNC safe from up-promoted threads
> 
> Review of attachment 728417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, except that it might still be racy on mHasRun, right?

mHasRun isn't asynchronously referenced from another thread; it's basically a state var that says that the primary Dispatch is done, and next time it passes through the event queue is should die.  It had been implicitly using mSyncTask for this before.

> And this is the type of thing that bsmedberg should review.  Although if I
> were going to review it, I'd ask you to add some comments about what mHasRun
> is being used for.

Sure.  bsmedberg was already drinking wine per IRC, and you had indicated interest, so I started with you.  :-)

Comment 17

[Adam Roach [:abr]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> Comment on attachment 728417 [details] [diff] [review]
> make DISPATCH_SYNC safe from up-promoted threads
> 
> Review of attachment 728417 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, except that it might still be racy on mHasRun, right?


I'm not seeing the race you are. mHasRun is set in the constructor (on the dispatching thread). Once it gets dispatched, mHasRun's exclusive ownership then transfers to the dispatched-to thread, which reads and sets it. The same runnable is then dispatched back to the original thread, at which point mHasRun's exclusive ownership is transferred back to the original dispatching thread.

What problem do you envision?

Comment 18

[Benjamin Smedberg]

Comment on attachment 728417 [details] [diff] [review]
make DISPATCH_SYNC safe from up-promoted threads

As noted on IRC: if we had to use NS_DISPATCH_SYNC, this would be ok. But in this case we don't need the loop-spinning deadlock-detecting behavior of DISPATCH_SYNC and can instead use a simpler runnable:

Please instead make webrtc use this base class for its runnables: http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/PSMRunnable.h#17

You can either move that class somewhere shared (xpcom/threads) or copy its impl into webrtc for now. Right now it only supports dispatch to the main thread, so if you need to dispatch to other threads you may just want to copy it and add an explicit target parameter.

I would like us to stop supporting upconversion of threads, but for now we can just MOZ_ASSERT when upconversion happens and make sure that we don't do it in our testsuites...

Comment 19

[Benjamin Smedberg]

Attachment: SyncRunnable helper, rev. 1

You should be able to use this to wrap your existing runnables with minimal code change.

Comment 20

[Eric Rescorla (:ekr)]

Attachment: test (WIP)

Comment 21

[Eric Rescorla (:ekr)]

Attachment: Add a common "SyncRunnable" class which dispatches events totally synchronously, avoiding event loop spinning or automatic promotion of this event to a nsThread,

Comment 22

[Eric Rescorla (:ekr)]

Attachment: Add a common "SyncRunnable" class which dispatches events totally synchronously, avoiding event loop spinning or automatic promotion of this event to a nsThread,

Comment 23

[Eric Rescorla (:ekr)]

The above patches are WIP, the tests particularly so. I've been fighting with the build system and can't get it to build my gtest builds, so I temporarily stuffed the tests in mtransport/test, where I know how to make gtest work.

jesup, abr: this patch should be good enough to test to see if it fixes the bug.

bsmedberg: comments welcome on naming

ted: any chance you could hoist the test into xpcom/tests and update the patch?

Comment 24

[(not currently active) Ted Mielczarek]

I haven't actually done any work with the in-tree GTest stuff. BenWa did all that work.

Comment 25

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

CCing benwa for help with the gtest tests

Comment 26

[Benoit Girard (:BenWa)]

(In reply to Eric Rescorla (:ekr) from comment #23)
> The above patches are WIP, the tests particularly so. I've been fighting
> with the build system and can't get it to build my gtest builds, so I
> temporarily stuffed the tests in mtransport/test, where I know how to make
> gtest work.
> 

Can you tell me what problems you ran into?

Comment 27

[Benoit Girard (:BenWa)]

Comment on attachment 730014 [details] [diff] [review]
test (WIP)

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

::: media/mtransport/test/Makefile.in
@@ +110,5 @@
>    nrappkit_unittest.cpp \
>    sockettransportservice_unittest.cpp \
>    transport_unittests.cpp \
>    runnable_utils_unittest.cpp \
> +  TestSyncRunnable.cpp \

You want GTEST_CPPSRCS = TestSyncRunnable.cpp like in gfx/layers/Makefile.in.

Note that when bug 844288 lands we will need a nested test directory for GTEST sources sadly because of build restriction.

Comment 28

[Benoit Girard (:BenWa)]

Comment on attachment 730014 [details] [diff] [review]
test (WIP)

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

Make sure to build with enable-gtest (temporary)

::: media/mtransport/test/TestSyncRunnable.cpp
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 12; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

I was told we could CC0 Tests. I'm not sure if it's preferred.

@@ +63,5 @@
> +
> +#include "mtransport_test_utils.h"
> +MtransportTestUtils *test_utils;
> +
> +int main(int argc, char **argv)

You actually don't want a main method. We use XRE_main to call RUN_ALL_TESTS. TEST_F will register the test.

Comment 29

[Eric Rescorla (:ekr)]

Attachment: Proper unit tests

Comment 30

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Since not everyone here is CC'ed on both. From bug 833769, comment 1402:
> FYI, bug 590422 increased the frequency of these failures. The patch there changes
> (improves) timing/timeouts accuracy. Might be a hint that the failures here depend
> on timing.
>
> Please fix this, it brings spamzilla to new levels...

Comment 31

[Adam Roach [:abr]]

Attachment: Convert sync dispatches from SIPCC threads to use SyncRunnable (WIP, bustage)

Comment 32

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

Comment on attachment 730284 [details] [diff] [review]
Convert sync dispatches from SIPCC threads to use SyncRunnable (WIP, bustage)

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

r+ with nits

::: media/webrtc/signaling/signaling.gyp
@@ +51,5 @@
>          './src/sipcc/cpr/include',
>          '../../../ipc/chromium/src',
>          '../../../ipc/chromium/src/base/third_party/nspr',
>          '../../../xpcom/base',
> +        '../../../xpcom/threads',

remove

@@ +269,5 @@
>          '../../../dom/base',
>          '../trunk/third_party/libsrtp/srtp/include',
>          '../trunk/third_party/libsrtp/srtp/crypto/include',
>          '$(DEPTH)/dist/include',
> +        '../../../xpcom/threads',

remove

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +22,5 @@
>  #include "transportlayerice.h"
>  #include "runnable_utils.h"
>  #include "cpr_stdlib.h"
>  #include "cpr_string.h"
> +#include "SyncRunnable.h"

mozilla/SyncRunnable.h

::: media/webrtc/signaling/src/peerconnection/PeerConnectionCtx.cpp
@@ +23,5 @@
>  #include "nsIObserverService.h"
>  #include "nsIObserver.h"
>  #include "mozilla/Services.h"
>  #include "StaticPtr.h"
> +#include "SyncRunnable.h"

mozilla/SyncRunnable.h

@@ +261,5 @@
>  void PeerConnectionCtx::onCallEvent(ccapi_call_event_e aCallEvent,
>                                        CSF::CC_CallPtr aCall,
>                                        CSF::CC_CallInfoPtr aInfo) {
>    // This is called on a SIPCC thread.
> +  // WARNING: Do not dispatch this with NS_DISPATCH_NORMAL.

This comment is confusing since we're using SyncRunnable

::: media/webrtc/signaling/test/Makefile.in
@@ +136,5 @@
>   -I$(topsrcdir)/media/webrtc/signaling/src/peerconnection \
>   -I$(topsrcdir)/media/webrtc/signaling/media-conduit\
>   -I$(topsrcdir)/media/webrtc/trunk/third_party/libjingle/source/ \
>   -I$(topsrcdir)/xpcom/base/ \
> + -I$(topsrcdir)/xpcom/threads/ \

remove

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +25,5 @@
>  #include "runnable_utils.h"
>  #include "transportflow.h"
>  #include "transportlayerprsock.h"
>  #include "transportlayerdtls.h"
> +#include "SyncRunnable.h"

mozilla/SyncRunnable.h

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +32,5 @@
>  #include "nsIIOService.h"
>  #include "nsIDNSService.h"
>  #include "nsWeakReference.h"
>  #include "nricectx.h"
> +#include "SyncRunnable.h"

mozilla/SyncRunnable.h

Comment 33

[Adam Roach [:abr]]

Attachment: Convert sync dispatches from SIPCC threads to use SyncRunnable (WIP, bustage)

Comment 34

[Adam Roach [:abr]]

Attachment: Add a common "SyncRunnable" class which dispatches events totally synchronously, avoiding event loop spinning or automatic promotion of this event to a nsThread,

Comment 35

[Adam Roach [:abr]]

Attachment: Convert sync dispatches from SIPCC threads to use SyncRunnable (WIP, bustage)

Comment 36

[Adam Roach [:abr]]

Attachment: Convert sync dispatches from SIPCC threads to use SyncRunnable

Comment 37

[Adam Roach [:abr]]

Comment on attachment 730332 [details] [diff] [review]
Convert sync dispatches from SIPCC threads to use SyncRunnable

Carrying forward r+ from jesup

Comment 38

[Adam Roach [:abr]]

Try push:
https://tbpl.mozilla.org/?tree=Try&rev=b1e7a6e5c29d

Comment 39

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

Attachment: Test for SyncRunnable using gtest framework (not working)

Comment 40

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

Comment on attachment 730341 [details] [diff] [review]
Test for SyncRunnable using gtest framework (not working)

xpcom/tests/gtest test is now on bug 855474

Comment 41

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

Comment on attachment 730014 [details] [diff] [review]
test (WIP)

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

::: media/mtransport/test/TestSyncRunnable.cpp
@@ +48,5 @@
> +{
> +  nsRefPtr<TestRunnable> r(new TestRunnable());
> +  nsRefPtr<SyncRunnable> s(new SyncRunnable(r));
> +  s->DispatchToThread(gThread);
> +  

ws

Comment 42

[Adam Roach [:abr]]

Attachment: Test for SyncRunnable

Comment 43

[Adam Roach [:abr]]

Comment on attachment 730488 [details] [diff] [review]
Test for SyncRunnable

Carrying forward r+ from jesup

Comment 44

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9a04c4a506df
https://hg.mozilla.org/integration/mozilla-inbound/rev/687b479332b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d507ab8a0019

Comment 45

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ba09dac7d26
Bustage fix for warnings-as-errors

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a04c4a506df
https://hg.mozilla.org/mozilla-central/rev/687b479332b3
https://hg.mozilla.org/mozilla-central/rev/d507ab8a0019
https://hg.mozilla.org/mozilla-central/rev/8ba09dac7d26

Comment 47

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Resummarizing so that this bug reflects what was changed.