crash in mozilla::plugins::PluginModuleParent::TerminateChildProcess @ PL_DHashTableOperate

VERIFIED FIXED in Firefox 27

Status

()

defect
P2
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: scoobidiver, Assigned: aklotz)

Tracking

({crash, regression})

20 Branch
mozilla28
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19 unaffected, firefox20 wontfix, firefox21 wontfix, firefox22 wontfix, firefox23 wontfix, firefox25 wontfix, firefox26 wontfix, firefox27 verified, firefox28 verified)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

6 years ago
It's #72 top browser crasher in 20.0b5 and occurs mainly on Windows XP.

Signature 	PL_DHashTableOperate | nsTHashtable<nsBaseHashtableET<nsCStringHashKey, nsCString> >::PutEntry(nsACString_internal const&) More Reports Search
UUID	53e07183-695e-405e-9190-299632130318
Date Processed	2013-03-18 14:15:46
Uptime	1923
Last Crash	2.1 weeks before submission
Install Age	36.1 minutes since version was first installed.
Install Time	2013-03-18 13:38:59
Product	Firefox
Version	20.0
Build ID	20130313170052
Release Channel	beta
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 23 stepping 10
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0xd
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x2a42, AdapterSubsysID: 17831854, AdapterDriverVersion: 8.15.10.2555
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 
Processor Notes 	sp-processor08.phx1.mozilla.com_4808:2008
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x2a42
Total Virtual Memory	2147352576
Available Virtual Memory	1108848640
System Memory Use Percentage	64
Available Page File	3495723008
Available Physical Memory	1126715392

Frame 	Module 	Signature 	Source
0 	xul.dll 	PL_DHashTableOperate 	obj-firefox/xpcom/build/pldhash.cpp:578
1 	xul.dll 	nsTHashtable<nsBaseHashtableET<nsCStringHashKey,nsCString> >::PutEntry 	obj-firefox/dist/include/nsTHashtable.h:170
2 	xul.dll 	mozilla::plugins::PluginModuleParent::TerminateChildProcess 	dom/plugins/ipc/PluginModuleParent.cpp:491
3 	xul.dll 	mozilla::plugins::PluginHangUIParent::RecvUserResponse 	dom/plugins/ipc/PluginHangUIParent.cpp:296
4 	xul.dll 	mozilla::plugins::PluginHangUIParent::OnMiniShmEvent 	dom/plugins/ipc/PluginHangUIParent.cpp:351
5 	xul.dll 	mozilla::plugins::MiniShmBase::OnEvent 	dom/plugins/ipc/hangui/MiniShmBase.h:232
6 	xul.dll 	mozilla::plugins::MiniShmParent::OnEvent 	dom/plugins/ipc/MiniShmParent.cpp:199
7 	xul.dll 	mozilla::plugins::MiniShmBase::SOnEvent 	dom/plugins/ipc/hangui/MiniShmBase.h:299
8 	ntdll.dll 	RtlpTpWaitCallback 	
9 	ntdll.dll 	TppWaitpExecuteCallback 	
10 	ntdll.dll 	TppCallbackCheckThreadBeforeCallback 	
11 	kernel32.dll 	BaseThreadInitThunk 	
12 	ntdll.dll 	__RtlUserThreadStart 	
13 	ntdll.dll 	_RtlUserThreadStart

More reports at:
https://crash-stats.mozilla.com/report/list?signature=PL_DHashTableOperate+|+nsTHashtable%3CnsBaseHashtableET%3CnsCStringHashKey%2C+nsCString%3E+%3E%3A%3APutEntry%28nsACString_internal+const%26%29
https://crash-stats.mozilla.com/report/list?signature=PL_DHashTableOperate+|+nsTHashtable%3CnsBaseHashtableET%3CnsISupportsHashKey%2C+nsCString%3E+%3E%3A%3APutEntry%28nsISupports*%29
https://crash-stats.mozilla.com/report/list?signature=PL_DHashTableOperate+|+nsTHashtable%3CnsBaseHashtableET%3CnsPtrHashKey%3CnsPIDOMWindow%3E%2C+nsAutoPtr%3CnsTArray%3Cmozilla%3A%3Adom%3A%3Aworkers%3A%3AWorkerPrivate*%3E+%3E+%3E+%3E%3A%3APutEntry%28nsPIDOMWindow*%29

Comment 1

6 years ago
aklotz, there are some unexpected stacks here; PluginHangUIParent::RecvUserResponse appears to be called even though the main thread is not blocked on a plugin hang? Perhaps the PluginHangUIParent isn't disabling itself completely when the plugin hang recovers?
Flags: needinfo?(aklotz)
Assignee

Comment 2

6 years ago
Posted patch Potential fixSplinter Review
Adds a check to ensure that a cancellation isn't already pending when processing a user response.
Attachment #726573 - Flags: review?(benjamin)
Flags: needinfo?(aklotz)

Updated

6 years ago
Attachment #726573 - Flags: review?(benjamin) → review+
Assignee

Comment 3

6 years ago
Let's see if this helps.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cad363e90ae
Whiteboard: [leave open]
Assignee

Updated

6 years ago
Assignee: nobody → aklotz
Assignee

Comment 5

6 years ago
Comment on attachment 726573 [details] [diff] [review]
Potential fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature, Plugin Hang UI (bug 805591)
User impact if declined: Intermittent crashes, particularly on slower machines
Testing completed (on m-c, etc.): Landed on m-c, also tested manually. Not 100% sure that this completely eliminates the crash but this is a reasonable preventative measure that won't harm anything.
Risk to taking this patch (and alternatives if risky): Low, adds an additional sanity check
String or UUID changes made by this patch: None
Attachment #726573 - Flags: approval-mozilla-beta?
Attachment #726573 - Flags: approval-mozilla-aurora?
Comment on attachment 726573 [details] [diff] [review]
Potential fix

We can take this on Aurora and analyze the data over the coming weeks to see if this does help with volume but it's too late to take a speculative fix on beta and this is not a topcrasher so I think we're OK to wait and let this bake.
Attachment #726573 - Flags: approval-mozilla-beta?
Attachment #726573 - Flags: approval-mozilla-beta-
Attachment #726573 - Flags: approval-mozilla-aurora?
Attachment #726573 - Flags: approval-mozilla-aurora+
Reporter

Updated

6 years ago
Crash Signature: , nsCOMPtr<nsIXULTemplateBuilder> > >::PutEntry(nsISupports*) ] [@ PL_DHashTableOperate | nsTHashtable<nsBaseHashtableET<nsCharPtrHashKey, nsCOMPtr<nsISupports> > >::PutEntry(char const*) ] → , nsCOMPtr<nsIXULTemplateBuilder> > >::PutEntry(nsISupports*) ] [@ PL_DHashTableOperate | nsTHashtable<nsBaseHashtableET<nsCharPtrHashKey, nsCOMPtr<nsISupports> > >::PutEntry(char const*) ] [@ PL_DHashTableOperate | nsTHashtable<nsURIHashKey>::PutEntry(…
Priority: -- → P2
Reporter

Comment 8

6 years ago
(In reply to Aaron Klotz [:aklotz] from comment #7)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/939d9a298677
There are still crashes in 21.0 Beta, 22.0a2 and 23.0a1.
See https://crash-stats.mozilla.com/report/list?signature=PL_DHashTableOperate+|+nsTHashtable%3CnsBaseHashtableET%3CnsURIHashKey%2C+nsRefPtr%3CnsXULPrototypeDocument%3E+%3E+%3E%3A%3APutEntry%28nsIURI*%29 and https://crash-stats.mozilla.com/report/list?signature=PL_DHashTableOperate+|+nsTHashtable%3CUnassociatedIconHashKey%3E%3A%3APutEntry%28nsIURI*%29
Crash Signature: , JSObject*> >::PutEntry(nsXBLPrototypeHandler*) ] → , JSObject*> >::PutEntry(nsXBLPrototypeHandler*) ] [@ PL_DHashTableOperate | nsTHashtable<nsBaseHashtableET<nsURIHashKey, nsRefPtr<nsXULPrototypeDocument> > >::PutEntry(nsIURI*) ] [@ PL_DHashTableOperate | nsTHashtable<UnassociatedIconHashKey>::PutEntry…
Assignee

Comment 9

6 years ago
Posted patch Mutual exclusion fix (obsolete) — Splinter Review
This crash is happening when the PCrashReporter sub-actor is torn down at the same time that the Plugin Hang UI calls TerminateChildProcess from another thread.

I think what we need here is to ensure that there is mutual exclusion between destruction of the PluginModuleParent's sub-actors and PluginModuleParent::TerminateChildProcess.
Attachment #819172 - Flags: review?(benjamin)
Comment on attachment 819172 [details] [diff] [review]
Mutual exclusion fix

I'm confused. What is the shared state that this lock is protecting, and why isn't the normal channel locking scheme insufficient?

Updated

6 years ago
Flags: needinfo?(aklotz)
Assignee

Comment 11

6 years ago
The shared state is the PCrashReporterParent object. If the channel ends up closing (and thus destroying the PCrashReporterParent sub-actor) concurrently with the crash report annotations being done by the Plugin Hang UI on another thread, we end up with this crash.

Unless I missed something, it doesn't look to me like the channel lock is held when OnChannelClose and OnChannelError are invoked.
Flags: needinfo?(aklotz)
Comment on attachment 819172 [details] [diff] [review]
Mutual exclusion fix

I'm worried about this. The locking surface is pretty huge with OnChannelClose/OnChannelError. Can we instead save CrashReporterParent* as a member (so that we don't have to lock around the IPDL data structures) and lock only in ActorDestroy?

In either case, I'd like a better comment explaining that this protects both CrashReporter() method itself as well as adding annotations, and why this is safe (because plugin child processes never send annotations during normal operation, presumably).
Attachment #819172 - Flags: review?(benjamin) → review-
Assignee

Comment 13

6 years ago
This version saves the CrashReporterParent* as a member and only needs to lock in DeallocPCrashReporterParent, instead of OnChannelClose and OnChannelError.
Attachment #819172 - Attachment is obsolete: true
Attachment #8341147 - Flags: review?(benjamin)

Updated

6 years ago
Attachment #8341147 - Flags: review?(benjamin) → review+
Assignee

Updated

6 years ago
Whiteboard: [leave open]
https://hg.mozilla.org/mozilla-central/rev/5f07f8e85831
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 946189
Marking verified fixed based on crash-stats.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.