1033522 - Windows Debug build hits Assert when using Gecko Media Plugin

Status: RESOLVED FIXED

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

Description

[Ethan Hugg [:ehugg]]

Windows release build works OK, but the debug build hits this assertion.  Not sure yet why the threading would be different than it is on Linux/OSX.

Preferences.cpp:439

bool
Preferences::InitStaticMembers()
{
#ifndef MOZ_B2G
  MOZ_ASSERT(NS_IsMainThread());
#endif

Stack:
>	xul.dll!mozilla::Preferences::InitStaticMembers() Line 443	C++
 	xul.dll!mozilla::Preferences::GetBool(const char * aPref, bool * aResult) Line 1386	C++
 	xul.dll!mozilla::Preferences::GetBool(const char * aPref, bool aDefault) Line 108	C++
 	xul.dll!mozilla::widget::WinTaskbar::GetAppUserModelID(nsAString_internal & aDefaultGroupId) Line 253	C++
 	xul.dll!mozilla::widget::WinTaskbar::GetDefaultGroupId(nsAString_internal & aDefaultGroupId) Line 326	C++
 	xul.dll!mozilla::ipc::GeckoChildProcessHost::InitWindowsGroupID() Line 310	C++
 	xul.dll!mozilla::ipc::GeckoChildProcessHost::PrepareLaunch() Line 262	C++
 	xul.dll!mozilla::ipc::GeckoChildProcessHost::SyncLaunch(std::vector<std::basic_string<char,std::char_traits<char>,std::allocator<char> >,std::allocator<std::basic_string<char,std::char_traits<char>,std::allocator<char> > > > aExtraOpts, int aTimeoutMs, base::ProcessArchitecture arch) Line 325	C++
 	xul.dll!mozilla::gmp::GMPProcessParent::Launch(int aTimeoutMs) Line 44	C++
 	xul.dll!mozilla::gmp::GMPParent::LoadProcess() Line 66	C++
 	xul.dll!mozilla::gmp::GMPParent::EnsureProcessLoaded() Line 198	C++
 	xul.dll!mozilla::gmp::GMPParent::GetGMPVideoEncoder(mozilla::gmp::GMPVideoEncoderParent * * aGMPVE) Line 228	C++
 	xul.dll!mozilla::gmp::GeckoMediaPluginService::GetGMPVideoEncoder(nsTArray<nsCString> * aTags, const nsAString_internal & aOrigin, GMPVideoHost * * aOutVideoHost, GMPVideoEncoder * * aGMPVE) Line 250	C++
 	xul.dll!mozilla::WebrtcGmpVideoEncoder::InitEncode_g(const webrtc::VideoCodec * aCodecSettings, int aNumberOfCores, unsigned int aMaxPayloadSize) Line 131	C++
 	xul.dll!mozilla::runnable_args_m_3_ret<mozilla::WebrtcGmpVideoEncoder *,int (__thiscall mozilla::WebrtcGmpVideoEncoder::*)(webrtc::VideoCodec const *,int,unsigned int),webrtc::VideoCodec const *,int,unsigned int,int>::Run() Line 303	C++
 	xul.dll!nsThreadSyncDispatch::Run() Line 991	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 766	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 284	C++
 	xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate * aDelegate) Line 337	C++
 	xul.dll!MessageLoop::RunInternal() Line 230	C++
 	xul.dll!MessageLoop::RunHandler() Line 223	C++
 	xul.dll!MessageLoop::Run() Line 197	C++
 	xul.dll!nsThread::ThreadFunc(void * aArg) Line 353	C++
 	nss3.dll!_PR_NativeRunThread(void * arg) Line 397	C
 	nss3.dll!pr_root(void * arg) Line 90	C

Comment 1

[Ethan Hugg [:ehugg]]

This call to InitWindowsGroupID() is unique to the Windows build:

From GeckoChildProcessHost:cpp:251

GeckoChildProcessHost::PrepareLaunch()
{
#ifdef MOZ_CRASHREPORTER
  if (CrashReporter::GetEnabled()) {
    CrashReporter::OOPInit();
  }
#endif

#ifdef XP_WIN
  InitWindowsGroupID();
#endif
  CacheGreDir();
}

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Work around

Comment 3

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

Comment on attachment 8451991 [details] [diff] [review]
Work around

Chris: is this workaround good to land?  If so, can we put it up for review?

Comment 4

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Randell Jesup [:jesup] from comment #3)
> Comment on attachment 8451991 [details] [diff] [review]
> Work around
> 
> Chris: is this workaround good to land?  If so, can we put it up for review?

Sure.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Actually, I'll fix bug 1035653 first, as they may affect this bug.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Based on looking at the comments in GeckoChildProcessHost::CacheGreDir(), which is called by GeckoChildProcessHost::PrepareLaunch() which is on the stack when we hit this assert, we should only be launching child processes when we're on the main thread.

http://mxr.mozilla.org/mozilla-central/source/ipc/glue/GeckoChildProcessHost.cpp#279

We should probably proxy the GeckoChildProcessHost::SyncLaunch() call to the main thread, rather than papering over the assert like my work around does.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: proxy call to GeckoChildProcessHost::SyncLaunch to main thread

I think we should be proxying the creation of the GMP child process to the main thread, based on the comments in GeckoChildProcessHost.

This also prevents the assertion failure that I originally filed this bug about.

Comment 8

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

Comment on attachment 8453505 [details] [diff] [review]
Patch: proxy call to GeckoChildProcessHost::SyncLaunch to main thread

per IRC, changing review to nical.  Thanks!

Comment 9

[Nicolas Silva [:nical]]

Comment on attachment 8453505 [details] [diff] [review]
Patch: proxy call to GeckoChildProcessHost::SyncLaunch to main thread

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

I assume that the main thread is never sending sync messages to the GMPParent thread (I looked around and didn't find any evidence that this can happen, but I don't know this code very well). It'd be nice to add a comment somewhere explaining that the main thread can't dispatch synchronous messages to the GMPParent thread without risking to deadlock.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Nicolas Silva [:nical] from comment #9)
> I assume that the main thread is never sending sync messages to the
> GMPParent thread

We do a sync dispatch from main thread to GMP thread in our xpcom shutdown observer, which will run on the main thread, here:

http://mxr.mozilla.org/mozilla-central/source/content/media/gmp/GMPService.cpp#133

I don't understand if/why that's unsafe though. Could you explain?

We should wrap the GMP thread with decorator that asserts that sync dispatches never come from the main thread.

Comment 11

[Benjamin Smedberg]

Comment on attachment 8453505 [details] [diff] [review]
Patch: proxy call to GeckoChildProcessHost::SyncLaunch to main thread

This is really not acceptable. SyncLaunch by design does all sorts of I/O and waits for the child process to start. We don't want to incur that penalty on the main thread.

Assuming that we do want to synchronously launch plugin children, it really does have to be from the GMP thread.

InitWindowsGroupID isn't needed for GMP children at all, and so we should only use it for child processes of certain types: probably just plugins. That code dates back to when plugin children were the only kind.

The CacheGREDir code is more problematic, but we really ought to just save off the GRE dir at XPCOM init so that we don't need to worry about it after that.

Comment 12

[Ethan Hugg [:ehugg]]

Here's another one.  It doesn't happen right away.  I don't know yet if they're related.  Only happens on Windows Debug builds and happens after seeing a bit of video:

xul.dll!RealBreak() Line 504	C++
 	xul.dll!Break(const char * aMsg) Line 579	C++
 	xul.dll!NS_DebugBreak(unsigned int aSeverity, const char * aStr, const char * aExpr, const char * aFile, int aLine) Line 464	C++
 	xul.dll!mozilla::ipc::MessageChannel::SyncStackFrame::~SyncStackFrame() Line 660	C++
 	xul.dll!mozilla::ipc::MessageChannel::DispatchInterruptMessage(const IPC::Message & aMsg, unsigned int stackDepth) Line 1232	C++
 	xul.dll!mozilla::ipc::MessageChannel::DispatchMessageW(const IPC::Message & aMsg) Line 1062	C++
 	xul.dll!mozilla::ipc::MessageChannel::OnMaybeDequeueOne() Line 1050	C++
 	xul.dll!DispatchToMethod<mozilla::ipc::MessageChannel,bool (__thiscall mozilla::ipc::MessageChannel::*)(void)>(mozilla::ipc::MessageChannel * obj, bool (void) * method, const Tuple0 & arg) Line 384	C++
 	xul.dll!RunnableMethod<mozilla::ipc::MessageChannel,bool (__thiscall mozilla::ipc::MessageChannel::*)(void),Tuple0>::Run() Line 307	C++
 	xul.dll!mozilla::ipc::MessageChannel::RefCountedTask::Run() Line 390	C++
 	xul.dll!mozilla::ipc::MessageChannel::DequeueTask::Run() Line 407	C++
 	xul.dll!MessageLoop::RunTask(Task * task) Line 358	C++
 	xul.dll!MessageLoop::DeferOrRunPendingTask(const MessageLoop::PendingTask & pending_task) Line 368	C++
 	xul.dll!MessageLoop::DoWork() Line 443	C++
 	xul.dll!mozilla::ipc::DoWorkRunnable::Run() Line 234	C++
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 766	C++
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 256	C++
 	xul.dll!mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate * aDelegate) Line 355	C++
 	xul.dll!MessageLoop::RunInternal() Line 230	C++
 	xul.dll!MessageLoop::RunHandler() Line 223	C++
 	xul.dll!MessageLoop::Run() Line 197	C++
 	xul.dll!nsThread::ThreadFunc(void * aArg) Line 353	C++
 	nss3.dll!_PR_NativeRunThread(void * arg) Line 397	C
 	nss3.dll!pr_root(void * arg) Line 90	C

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #11)

So, to be clear, you propose that we:
1. Only call InitWindowsGroupID() if GeckoChildProcessHost::mProcessType==GeckoProcessType_Plugin, and
2. cache the GRE dir in some XPCOM init path and make that cached value visible here, and
3. launch the child process on the GMP thread.

Is that correct? In what function should we cache the GRE dir, and how should we make it visible to GeckoChildProcessHost::SyncLaunch ()?

Thanks!

Comment 14

[Benjamin Smedberg]

Correct. You can cache the GRE dir from NS_InitXPCOM and make a mozilla::GetGREDir API from within nsXPComInit.cpp/nsXPCOMPrivate.h

Comment 15

[Benjamin Smedberg]

Attachment: bug1033522-launch-asserts

Jim, asking you for review because this touches the windows groupID code which you wrote a long time ago; I'm pretty sure that we only need that for plugin processes, but I thought I'd check with you.

Comment 16

[Benjamin Smedberg]

Attachment: bug1033522-w

Comment 17

[Jim Mathies [:jimm]]

Comment on attachment 8456463 [details] [diff] [review]
bug1033522-w

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

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +134,3 @@
>  #else
>        nsCString path;
> +    NS_CopyUnicodeToNative(gGREir, path);

This looks like a typo, should this be gGREPath?

@@ +256,1 @@
>    InitWindowsGroupID();

AFAIK plugins are the only child processes that create their own native UI so this seems fine.

Comment 18

[Jim Mathies [:jimm]]

Comment on attachment 8456449 [details] [diff] [review]
bug1033522-launch-asserts

looks good to me, assuming that typo gets sorted out.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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