Closed Bug 903386 Opened 6 years ago Closed 6 years ago

crash in RevocableStore::Revocable::Revocable

Categories

(Core :: IPC, defect, critical)

All
Windows 7
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla27
Tracking Status
firefox26 --- verified
firefox27 --- verified

People

(Reporter: scoobidiver, Assigned: gfritzsche)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

It's a low volume crash that seems to have spiked progressively.

A comment says: "WebRtc testing on latest Nightly (2013-08-08)"

Signature 	RevocableStore::Revocable::Revocable(RevocableStore*) More Reports Search
UUID 	92e27a4e-4f84-4242-ae06-01b812130807
Date Processed	2013-08-07 05:18:22.002483
Uptime	4044
Last Crash	467650 seconds before submission
Install Age 	4044 since version was first installed.
Install Time 	2013-08-07 04:09:19
Product 	Firefox
Version 	26.0a1
Build ID 	20130806030203
Release Channel 	nightly
OS 	Windows NT
OS Version 	6.2.9200
Build Architecture 	x86
Build Architecture Info 	GenuineIntel family 6 model 42 stepping 7 | 4
Crash Reason 	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address 	0x4
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x0116, AdapterSubsysID: 1858103c, AdapterDriverVersion: 9.17.10.2963
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 

Frame 	Module 	Signature 	Source
0 	xul.dll 	RevocableStore::Revocable::Revocable(RevocableStore *) 	ipc/chromium/src/base/revocable_store.cc
1 	xul.dll 	ScopedTaskFactory<ScopedRunnableMethodFactory<mozilla::plugins::PluginModuleParent>::RunnableMethod<void ( mozilla::plugins::PluginModuleParent::*)(bool),Tuple1<bool> > >::TaskWrapper::TaskWrapper(RevocableStore *) 	ipc/chromium/src/base/task.h
2 	xul.dll 	ScopedRunnableMethodFactory<mozilla::plugins::PluginModuleParent>::NewRunnableMethod<void ( mozilla::plugins::PluginModuleParent::*)(bool),bool>(void ( mozilla::plugins::PluginModuleParent::*)(bool),bool const &) 	ipc/chromium/src/base/task.h
3 	xul.dll 	mozilla::plugins::PluginModuleParent::CleanupFromTimeout(bool) 	dom/plugins/ipc/PluginModuleParent.cpp
4 	xul.dll 	RunnableMethod<mozilla::plugins::PluginModuleParent,void ( mozilla::plugins::PluginModuleParent::*)(bool),Tuple1<bool> >::Run() 	ipc/chromium/src/base/task.h
5 	xul.dll 	MessageLoop::RunTask(Task *) 	ipc/chromium/src/base/message_loop.cc
6 	xul.dll 	MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const &) 	ipc/chromium/src/base/message_loop.cc
7 	xul.dll 	MessageLoop::DoWork() 	ipc/chromium/src/base/message_loop.cc
8 	xul.dll 	mozilla::ipc::DoWorkRunnable::Run() 	ipc/glue/MessagePump.cpp
9 	xul.dll 	nsThread::ProcessNextEvent(bool,bool *) 	xpcom/threads/nsThread.cpp
10 	xul.dll 	NS_ProcessNextEvent(nsIThread *,bool) 	obj-firefox/xpcom/build/nsThreadUtils.cpp
11 	xul.dll 	nsThread::Shutdown() 	xpcom/threads/nsThread.cpp
12 	xul.dll 	nsRunnableMethodImpl<tag_nsresult ( nsIUrlClassifierDBServiceWorker::*)(void),1>::Run() 	obj-firefox/dist/include/nsThreadUtils.h
13 	xul.dll 	nsThread::ProcessNextEvent(bool,bool *) 	xpcom/threads/nsThread.cpp
14 	xul.dll 	NS_ProcessNextEvent(nsIThread *,bool) 	obj-firefox/xpcom/build/nsThreadUtils.cpp
15 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *) 	ipc/glue/MessagePump.cpp
16 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc
17 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc
18 	xul.dll 	nsBaseAppShell::Run() 	widget/xpwidgets/nsBaseAppShell.cpp
19 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp
20 	xul.dll 	nsAppStartup::Run() 	toolkit/components/startup/nsAppStartup.cpp
21 	xul.dll 	XREMain::XRE_mainRun() 	toolkit/xre/nsAppRunner.cpp
22 	xul.dll 	XREMain::XRE_main(int,char * * const,nsXREAppData const *) 	toolkit/xre/nsAppRunner.cpp
23 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp
24 	firefox.exe 	do_main 	browser/app/nsBrowserApp.cpp
25 	firefox.exe 	NS_internal_main(int,char * *) 	browser/app/nsBrowserApp.cpp
26 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp
27 	firefox.exe 	__tmainCRTStartup 	f:/dd/vctools/crt_bld/self_x86/crt/src/crtexe.c
28 	kernel32.dll 	BaseThreadInitThunk 	
29 	ntdll.dll 	__RtlUserThreadStart 	
30 	ntdll.dll 	_RtlUserThreadStart 	

More reports at:
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=RevocableStore%3A%3ARevocable%3A%3ARevocable%28RevocableStore*%29
From the minidump here: http://hg.mozilla.org/mozilla-central/annotate/a0dd80f800e2/ipc/chromium/src/base/revocable_store.cc#l13

store_reference_ is null. Unless I'm misreading, it appears that `store` is non-null, but its owning_reference_ is null. I'm pretty sure we're operating on dead memory, and the only real candidate is the PluginModuleParent itself.

Reading the code, there appears to be two ways that we post CleanupFromTimeout:

http://hg.mozilla.org/mozilla-central/annotate/a0dd80f800e2/dom/plugins/ipc/PluginModuleParent.cpp#l289

mTaskFactory.NewRunnableMethod

http://hg.mozilla.org/mozilla-central/annotate/a0dd80f800e2/dom/plugins/ipc/PluginModuleParent.cpp#l501 and 507

NewRunnableMethod

The bare `NewRunnableMethod` doesn't make a scoped/revocable event, so if something else destroyed the PluginModuleParent in the meantime, it wouldn't notice that it had gone. I think that this can be fixed by making line 501/507 use mTaskFactory.NewRunnableMethod.

aklotz/gfritzsche, does this ring any bells or sound reasonable?
Flags: needinfo?(georg.fritzsche)
Flags: needinfo?(aklotz)
Was reading the branch sources: this is different on trunk due to bug 828034, which fixed the "else" half of the clause but added this comment to the top half:

        // If we're posting from a different thread we can't create
        // the task via mTaskFactory

In this case, how do we make sure that the event is cancelled properly?
Hmm, looking at the code, it looks like the whole reason why this is a problem is beacause ScopedRunnableMethodFactory::NewRunnableMethod creates a TaskWrapper object, and TaskWrapper inherits from NonThreadSafe, a class that asserts if it is destroyed on a different thread than it was created.

I don't see anything in this code that justifies inheriting from NonThreadSafe, at least for our purposes. I suppose we could create a variant of ScopedRunnableMethodFactory that doesn't use NonThreadSafe...
Flags: needinfo?(aklotz)
From reading through this comment 1 / 2 sound reasonable. 
I'd also rather have a thread-safe ScopedRunnableMethodFactory then the manual task tracking we have in a few other places - it doesn't look like too much effort to do.
Flags: needinfo?(georg.fritzsche)
Assignee: nobody → georg.fritzsche
Attached patch WIP patchSplinter Review
WIP - i still need to decide on a proper name, but it looks fine on try.

Benjamin, i'm wondering: currently this is fine without any locking (all tasks are revoked when the toplevel actor shuts down, so there are no further calls that could race).
However, we don't have any performance-relevant paths here - should we make this thread-safe to avoid possible future accidents or is explicit documentation enough?
Attachment #807216 - Flags: feedback?(benjamin)
PluginModuleParent::TerminateChildProcess is called on the hangUI thread, right? Is there any potential race there?
Right. We have two places where this can come from:
* PluginHangUIParent::SOnHangUIProcessExit()
* PluginHangUIParent::OnMiniShmEvent()

The cleanup of the PluginHangUIParent, and hence the MiniShmParent, is triggered from PluginModuleParent - per talking with Aaron this takes care of the two sources above and should be race-free.
Attachment #807216 - Flags: review?(aklotz)
Attachment #807216 - Flags: feedback?(benjamin)
Attachment #807216 - Flags: feedback+
Comment on attachment 807216 [details] [diff] [review]
WIP patch

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

::: dom/plugins/ipc/ScopedRunnableMethodFactory.h
@@ +6,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +namespace plugins {
> +

Could you please add a doc comment here describing what this is based upon and why we have added this?
Attachment #807216 - Flags: review?(aklotz) → review+
https://hg.mozilla.org/mozilla-central/rev/5e5cf3b5ea4f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment on attachment 807216 [details] [diff] [review]
WIP patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Plugin Hang UI
User impact if declined: races that can lead to crashes
Testing completed (on m-c, etc.): looking good on m-c for the last days
Risk to taking this patch (and alternatives if risky): I'd say low - worst case we would be moving the crash to a different signature. 
String or IDL/UUID changes made by this patch: None.
Attachment #807216 - Flags: approval-mozilla-aurora?
Comment on attachment 807216 [details] [diff] [review]
WIP patch

Low risk, already on trunk for over a week so willing to take this upstream to help with plugin UI hang.
Attachment #807216 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
there are no longer reports of this signature on Fx26 or Fx27
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.