Closed Bug 1332402 Opened 7 years ago Closed 7 years ago

Shutdown abort after using BrowserStack, with fatal "Assertion failure: !mThread (SingletonThreads should be Released and shut down before exit!), at media/mtransport/nr_socket_prsock.cpp:181"

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- wontfix
firefox-esr52 --- wontfix
firefox53 --- wontfix
firefox54 --- fixed

People

(Reporter: dholbert, Assigned: bwc)

References

()

Details

(Keywords: assertion)

Attachments

(3 files)

STR:
 1. Be using a debug build of mozilla-central. (Mine is disable-optimize enable-debug.)

 2. Visit http://browserstack.com/ , log in, and start a remote browser session there (e.g. with Windows 10 & Edge 14).  You can create a free trial account if you don't have an account already. (They'll require you to install a restartless browser extension the first time, too.)

 3. Click the BrowserStack "Stop" button to end your session.

 4. Quit Firefox.

ACTUAL RESULTS:
Shutdown abort, with this fatal assertion:
Assertion failure: !mThread (SingletonThreads should be Released and shut down before exit!), at media/mtransport/nr_socket_prsock.cpp:181


Backtrace:
==========
#0  0x00007fecce3e5e02 in mozilla::SingletonThreadHolder::~SingletonThreadHolder() (this=0x7fecab84a700)
    at $SRC/media/mtransport/nr_socket_prsock.cpp:181
#1  0x00007fecce3e5d4e in mozilla::SingletonThreadHolder::Release() (this=0x7fecab84a700)
    at $SRC/media/mtransport/nr_socket_prsock.cpp:192
#2  0x00007fecce3e5c86 in mozilla::StaticRefPtr<mozilla::SingletonThreadHolder>::AssignAssumingAddRef(mozilla::SingletonThreadHolder*) (this=0x7fecd7003da8 <mozilla::sThread>, aNewPtr=0x0)
    at $OBJ/dist/include/mozilla/StaticPtr.h:161
#3  0x00007fecce3e5b8c in mozilla::StaticRefPtr<mozilla::SingletonThreadHolder>::AssignWithAddref(mozilla::SingletonThreadHolder*) (this=0x7fecd7003da8 <mozilla::sThread>, aNewPtr=0x0)
    at $OBJ/dist/include/mozilla/StaticPtr.h:153
#4  0x00007fecce3e581f in mozilla::StaticRefPtr<mozilla::SingletonThreadHolder>::operator=(mozilla::SingletonThreadHolder*) (this=0x7fecd7003da8 <mozilla::sThread>, aRhs=0x0)
    at $OBJ/dist/include/mozilla/StaticPtr.h:106
#5  0x00007fecce3e6180 in mozilla::ClearOnShutdown_Internal::PointerClearer<mozilla::StaticRefPtr<mozilla::SingletonThreadHolder> >::Shutdown() (this=0x7fecab96b520)
    at $OBJ/dist/include/mozilla/ClearOnShutdown.h:81
#6  0x00007feccd0b6251 in mozilla::KillClearOnShutdown(mozilla::ShutdownPhase) (aPhase=mozilla::ShutdownPhase::ShutdownFinal)
    at $SRC/xpcom/base/ClearOnShutdown.cpp:37
#7  0x00007feccd20939d in mozilla::ShutdownXPCOM(nsIServiceManager*) (aServMgr=0x0)
    at $SRC/xpcom/build/XPCOMInit.cpp:965
#8  0x00007feccd208fe5 in NS_ShutdownXPCOM(nsIServiceManager*) (aServMgr=0x0)
    at $SRC/xpcom/build/XPCOMInit.cpp:853
#9  0x00007fecd234dc7c in XRE_TermEmbedding() ()
    at $SRC/toolkit/xre/nsEmbedFunctions.cpp:216
#10 0x00007feccdad7a62 in mozilla::ipc::ScopedXREEmbed::Stop() (this=0x7fece0025998)
    at $SRC/ipc/glue/ScopedXREEmbed.cpp:117


jesup, looks like this SingletonThreadHolder thing is from bug 1145354. Perhaps you or bwc could take a look?
It looks like we handle this gracefully in opt builds, but it's severe enough that we assert fatally in debug builds:
>  ~SingletonThreadHolder()
>  {
>    r_log(LOG_GENERIC,LOG_DEBUG,"Deleting SingletonThreadHolder");
>    MOZ_ASSERT(!mThread, "SingletonThreads should be Released and shut down before exit!");
>    if (mThread) {
>      mThread->Shutdown();
>      mThread = nullptr;
>    }
>  }
http://searchfox.org/mozilla-central/rev/30fcf167af036aeddf322de44a2fadd370acfd2f/media/mtransport/nr_socket_prsock.cpp#175,181
Keywords: assertion
Flags: needinfo?(rjesup)
Looking.
Summary: Shutdown abort after using BrowserStack, with "Assertion failure: !mThread (SingletonThreads should be Released and shut down before exit!), at media/mtransport/nr_socket_prsock.cpp:181" → Shutdown abort after using BrowserStack, with fatal "Assertion failure: !mThread (SingletonThreads should be Released and shut down before exit!), at media/mtransport/nr_socket_prsock.cpp:181"
So I guess on shutdown the usual calls to ReleaseUse aren't happening because STS or the IO thread isn't running anymore? I wonder if we can call ReleaseUse sooner, maybe even within ~NrUdpSocketIpc?
We don't *need* to assert on this; just complain.  Most likely a transport/etc is stuck in some GC wait or CC loop - which is a more general thing we should try to track down and avoid, but not on this bug.  Also, probably makes sense to clear the thread at shutdown-threads time, not afterwards.
Attachment #8828483 - Flags: review?(docfaraday)
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
Comment on attachment 8828482 [details]
Bug 1332402 - Part 1: Simplify shutdown of the IO thread.

https://reviewboard.mozilla.org/r/105866/#review106802

I think this is ok, since I believe the io_thread_'s event queue will get all processed and emptied.  We probably want this, and my patch.  (we only need my patch I think; this does simplify things a bit if all our assumptions are right.)

::: media/mtransport/nr_socket_prsock.cpp:1136
(Diff revision 1)
>    RUN_ON_THREAD(io_thread_,
>                  mozilla::WrapRunnableNM(&NrUdpSocketIpc::release_child_i,
> -                                        socket_child_.forget().take(),
> +                                        socket_child_.forget().take()),
> -                                        sts_thread_),
>                  NS_DISPATCH_NORMAL);
> +  sThread->ReleaseUse();

At minimum, this needs a comment that ReleaseUse may shut down io_thread_ after it processes the WrapRunnable.  Since Shutdown() spins the event loop until the shutdown message is processed, this should work.
Attachment #8828482 - Flags: review+
Comment on attachment 8828482 [details]
Bug 1332402 - Part 1: Simplify shutdown of the IO thread.

https://reviewboard.mozilla.org/r/105866/#review106802

> At minimum, this needs a comment that ReleaseUse may shut down io_thread_ after it processes the WrapRunnable.  Since Shutdown() spins the event loop until the shutdown message is processed, this should work.

I can add a comment, sure.
Rank: 15
Priority: -- → P1
Attachment #8828483 - Flags: review?(docfaraday) → review+
Comment on attachment 8828928 [details]
Bug 1332402 - Part 2: just complain if SingletonThread is still active at shutdown

https://reviewboard.mozilla.org/r/106146/#review107148
Attachment #8828928 - Flags: review?(docfaraday) → review+
Assignee: rjesup → docfaraday
Pushed by bcampen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/94ed8555d2ee
Part 1: Simplify shutdown of the IO thread. r=jesup
https://hg.mozilla.org/integration/autoland/rev/c1e8aed2515e
Part 2: just complain if SingletonThread is still active at shutdown r=bwc
https://hg.mozilla.org/mozilla-central/rev/94ed8555d2ee
https://hg.mozilla.org/mozilla-central/rev/c1e8aed2515e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Is this worth backporting to 53?
Flags: needinfo?(rjesup) → needinfo?(docfaraday)
For an assertion on shutdown in rare cases? Nah.
Flags: needinfo?(docfaraday)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: