535321 - Manually killing mozilla-runtime.exe resulted in a crash [@ mozilla::plugins::PPluginModuleParent::DestroySubtree(mozilla::ipc::IProtocolManager<mozilla::ipc::RPCChannel::RPCListener>::ActorDestroyReason) ]

Status: RESOLVED DUPLICATE of bug 535298

(Core Graveyard :: Plug-ins, defect)

Description

[u88484]

Firefox hung so I killed one of the mozilla-runtime.exe that showed in the task manager which resulted in the crash reporter launching.  The crash report is as follows:

Signature	mozilla::plugins::PPluginModuleParent::DestroySubtree(mozilla::ipc::IProtocolManager<mozilla::ipc::RPCChannel::RPCListener>::ActorDestroyReason)
UUID	60f50cc7-deba-41f8-83d5-5ba4f2091216
Time 	2009-12-16 09:56:52.60231
Uptime	262
Last Crash	1524012 seconds before submission
Product	Firefox
Version	3.7a1pre
Build ID	20091216045436
Branch	1.9.3
OS	Windows NT
OS Version	6.1.7600
CPU	x86
CPU Info	AuthenticAMD family 15 model 104 stepping 2
Crash Reason	EXCEPTION_ACCESS_VIOLATION
Crash Address	0x61a0600
User Comments	
Processor Notes 	
Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 		@0x61a0600 	
1 	xul.dll 	mozilla::plugins::PPluginModuleParent::DestroySubtree 	obj-firefox/ipc/ipdl/PPluginModuleParent.cpp:549
2 	xul.dll 	mozilla::plugins::PPluginModuleParent::OnChannelError 	obj-firefox/ipc/ipdl/PPluginModuleParent.cpp:489
3 	xul.dll 	mozilla::ipc::AsyncChannel::NotifyMaybeChannelError 	ipc/glue/AsyncChannel.cpp:279
4 	xul.dll 	MessageLoop::RunTask 	ipc/chromium/src/base/message_loop.cc:326
5 	xul.dll 	MessageLoop::DeferOrRunPendingTask 	ipc/chromium/src/base/message_loop.cc:334
6 	xul.dll 	MessageLoop::DoWork 	ipc/chromium/src/base/message_loop.cc:434
7 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:122
8 	xul.dll 	xul.dll@0x9a5023 	
9 	xul.dll 	MessageLoop::RunHandler 	ipc/chromium/src/base/message_loop.cc:194
10 	xul.dll 	nsComponentManagerImpl::GetServiceByContractID 	xpcom/components/nsComponentManager.cpp:2199
11 	nspr4.dll 	PR_GetEnv 	
12 	xul.dll 	_IsNonwritableInCurrentImage 	
13 	xul.dll 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:168
14 	xul.dll 	nsBaseAppShell::Run 	widget/src/xpwidgets/nsBaseAppShell.cpp:174
15 	nspr4.dll 	PR_GetEnv 	
16 	xul.dll 	nsAppStartup::Run 	toolkit/components/startup/src/nsAppStartup.cpp:182
17 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3529

Comment 1

[Benjamin Smedberg]

cjones, we're getting a bunch of crashes with invalid pointers inside of mozilla::plugins::PPluginModuleParent::DestroySubtree. Is it possible that the PluginModuleParent has already been destroyed but the channel doesn't know it?

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Not unless some shutdown invariant isn't holding.  The only way this could happen is if nsNPAPI* is freeing its PluginLibrary* before the crash notification, but since PluginModuleParent notifies nsNPAPI* of the crash following this crash notification, I don't see how this could be.  Maybe it's a bad interaction with nsNPAPI* refcounting?

Comment 3

[u88484]

Ben, this could be related to what we just talked about on irc.  I had two mozilla-runtime.exes (from flash) and killed one of them since the browser was hung.

Comment 4

[Benjamin Smedberg]

Which is the invariant? The plugin host is allowed to free the nsIPlugin (nsNPAPIPlugin) at any point after all the instances have been destroyed (in fact reloading plugins does exactly this if creating a new plugin instance fails, as it would if you crash while calling NPP_New).

It seems possible to me that we could dispatch the plugin-crash runnable, call ~nsNPAPIPlugin/~PluginModuleParent/~AsyncChannel and then AsyncChannel::NotifyMaybeChannelError references a dead channel.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> Which is the invariant? The plugin host is allowed to free the nsIPlugin
> (nsNPAPIPlugin) at any point after all the instances have been destroyed (in
> fact reloading plugins does exactly this if creating a new plugin instance
> fails, as it would if you crash while calling NPP_New).
> 

PluginModuleParent expects one of two shutdown paths: NP_Shutdown() or child process crash.  If the nsNPAPIPlugin is being freed without calling NP_Shutdown(), I could see this crash happening.  It's also possible that the child could crash in a race with NP_Shutdown(), but that's pretty unlikely (though we should eventually protect against it).

> It seems possible to me that we could dispatch the plugin-crash runnable, call
> ~nsNPAPIPlugin/~PluginModuleParent/~AsyncChannel and then
> AsyncChannel::NotifyMaybeChannelError references a dead channel.

It goes the other way around, NotifyMaybeChannelError() kicks off the code that leads to nsNPAPIPlugin::PluginCrashed and then hopefully ~nsNPAPIPlugin().  Again, unless there's a behavior in nsNPAPIPlugin that I don't know about.

Comment 6

[Benjamin Smedberg]

The sequence I think may be bad is:
* User closes a tab with a plugin
* Pluginhost calls NP_Shutdown, which we dispatch to the child
* child crashes (or hangs and gets killed by the user)
* Parent dispatches the NotifyMaybeChannelError runnable
* NP_Shutdown returns, and the pluginhost calls ~nsNPAPIPlugin->~PluginModuleParent
* the runnable actually executes, but with a bad AsyncChannel `this` pointer

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I'm pretty confident this crash falls into the category fixed by bug 535298.  Please REOPEN if not.  (Want Resolved->SUBSUMED, have to settle for DUP.)