Closed Bug 1033522 Opened 6 years ago Closed 5 years ago

Windows Debug build hits Assert when using Gecko Media Plugin

Categories

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

x86_64
Windows 8.1
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33

People

(Reporter: ehugg, Assigned: benjamin)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files, 2 obsolete files)

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
Blocks: OpenH264
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();
}
Attached patch Work around (obsolete) — Splinter Review
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?
Flags: needinfo?(cpearce)
(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.
Flags: needinfo?(cpearce)
Actually, I'll fix bug 1035653 first, as they may affect this bug.
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.
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.
Assignee: nobody → cpearce
Attachment #8451991 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8453505 - Flags: review?(bent.mozilla)
Comment on attachment 8453505 [details] [diff] [review]
Patch: proxy call to GeckoChildProcessHost::SyncLaunch to main thread

per IRC, changing review to nical.  Thanks!
Attachment #8453505 - Flags: review?(bent.mozilla) → review?(nical.bugzilla)
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.
Attachment #8453505 - Flags: review?(nical.bugzilla) → review+
(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 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.
Attachment #8453505 - Flags: review+ → review-
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
(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!
Correct. You can cache the GRE dir from NS_InitXPCOM and make a mozilla::GetGREDir API from within nsXPComInit.cpp/nsXPCOMPrivate.h
Assignee: cpearce → benjamin
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.
Attachment #8456449 - Flags: review?(jmathies)
Attached patch bug1033522-wSplinter Review
Attachment #8453505 - Attachment is obsolete: true
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 on attachment 8456449 [details] [diff] [review]
bug1033522-launch-asserts

looks good to me, assuming that typo gets sorted out.
Attachment #8456449 - Flags: review?(jmathies) → review+
https://hg.mozilla.org/mozilla-central/rev/be05446d4fd2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.