Closed Bug 640901 Opened 13 years ago Closed 13 years ago

Crash [@ mozilla::plugins::PPluginIdentifierParent::CreateSharedMemory(unsigned int, mozilla::ipc::SharedMemory::SharedMemoryType, bool, int*) ] [@ mozilla::dom::PAudioParent::CreateSharedMemory(unsigned int, mozilla::ipc::SharedMemory::SharedMemoryType,

Categories

(Core Graveyard :: Plug-ins, defect)

x86
Windows 7
defect
Not set
critical

Tracking

(blocking2.0 Macaw+, status2.0 .1-fixed)

RESOLVED FIXED
Tracking Status
blocking2.0 --- Macaw+
status2.0 --- .1-fixed

People

(Reporter: scoobidiver, Assigned: bent.mozilla)

References

Details

(Keywords: crash, regression, relnote, Whiteboard: [watching crash-stats][fx4-rc-ridealong])

Crash Data

Attachments

(4 files, 4 obsolete files)

It is a new crash signature that first appeared in 4.0 RC1.
It starts showing up as #38 top crasher in 4.0 RC1.

Every comments talk about opening PDF with Acrobat Reader X.

Signature	mozilla::plugins::PPluginIdentifierParent::CreateSharedMemory(unsigned int, mozilla::ipc::SharedMemory::SharedMemoryType, bool, int*)
UUID	4429fd9a-fe37-4fb4-a21b-575cf2110310
Time 	2011-03-10 19:16:41.509271
Uptime	1656
Install Age	87925 seconds (1.0 days) since version was first installed.
Product	Firefox
Version	4.0
Build ID	20110303194838
Branch	2.0
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
CPU	x86
CPU Info	GenuineIntel family 6 model 37 stepping 2
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x1
User Comments	Adobe Reader crashed, but this should not cause Firefox to crash! Add-ons and plugins should be in seperate processes, if they are not already.
App Notes 	AdapterVendorID: 8086, AdapterDeviceID: 0042, AdapterDriverVersion: 8.15.10.2202
D2D? D2D+
DWrite? DWrite+
D3D10 Layers? D3D10 Layers+

Frame 	Module 	Signature [Expand] 	Source
0 	xul.dll 	mozilla::plugins::PPluginIdentifierParent::CreateSharedMemory 	obj-firefox/ipc/ipdl/PLayersChild.cpp:385
1 	xul.dll 	mozilla::plugins::PPluginInstanceParent::RemoveManagee 	obj-firefox/ipc/ipdl/PPluginInstanceParent.cpp:2061
2 	xul.dll 	mozilla::plugins::PPluginScriptableObjectParent::OnMessageReceived 	obj-firefox/ipc/ipdl/PPluginScriptableObjectParent.cpp:613
3 	xul.dll 	mozilla::plugins::PPluginModuleParent::OnMessageReceived 	obj-firefox/ipc/ipdl/PPluginModuleParent.cpp:528
4 	xul.dll 	mozilla::ipc::AsyncChannel::OnDispatchMessage 	ipc/glue/AsyncChannel.cpp:262
5 	xul.dll 	mozilla::ipc::RPCChannel::Call 	ipc/glue/RPCChannel.cpp:244
6 	xul.dll 	mozilla::plugins::PPluginScriptableObjectParent::CallInvalidate 	obj-firefox/ipc/ipdl/PPluginScriptableObjectParent.cpp:119
7 	xul.dll 	mozilla::plugins::PluginScriptableObjectParent::ScriptableInvalidate 	dom/plugins/PluginScriptableObjectParent.cpp:117
8 	xul.dll 	NPObjWrapperPluginDestroyedCallback 	modules/plugin/base/src/nsJSNPRuntime.cpp:1925
9 	xul.dll 	PL_DHashTableEnumerate 	obj-firefox/xpcom/build/pldhash.c:754
10 	xul.dll 	nsJSNPRuntime::OnPluginDestroy 	modules/plugin/base/src/nsJSNPRuntime.cpp:1992
11 	xul.dll 	nsNPAPIPluginInstance::InitializePlugin 	modules/plugin/base/src/nsNPAPIPluginInstance.cpp:426
12 	xul.dll 	nsNPAPIPluginInstance::Initialize 	modules/plugin/base/src/nsNPAPIPluginInstance.cpp:150
13 	xul.dll 	nsPluginHost::TrySetUpPluginInstance 	modules/plugin/base/src/nsPluginHost.cpp:1383
14 	xul.dll 	nsPluginHost::SetUpPluginInstance 	modules/plugin/base/src/nsPluginHost.cpp:1264

More reports at:
https://crash-stats.mozilla.com/report/list?range_value=4&range_unit=weeks&signature=mozilla%3A%3Aplugins%3A%3APPluginIdentifierParent%3A%3ACreateSharedMemory%28unsigned%20int%2C%20mozilla%3A%3Aipc%3A%3ASharedMemory%3A%3ASharedMemoryType%2C%20bool%2C%20int*%29
cjones, if you're done with Fennec blockers could you look? It appears that a plugin returned a failure code from NPP_New, but then we end up in the weeds?
I have no idea what might be happening there.  Ben, is the ScriptableObject stuff doing what's supposed to?
There's something wrong with the stack... Frame 1 can't possibly call frame 0 unless there's some kind of corruption.
I believe what's happening is:

nsNPAPIPluginInstance::InitializePlugin calls PluginModuleParent::NPP_New
either acrobat is crashing or NPP_New is returning a failure NPError code

which ends up at either http://hg.mozilla.org/mozilla-central/annotate/4512b571e48e/dom/plugins/PluginModuleParent.cpp#l880 or #l891

At that point, the plugin has already created NPObjects. We proceed to destroy those actors, and then the PPluginInstance (or it is already destroyed), but somehow the ParentNPObject here doesn't realize that and is referencing a dead actor.
moving up the daily ranking pretty fast, #36 on thursday, #24 yesterday.

more stats, test urls, and other info in Bug 641218
carrying chofmann's nomination over from duped bug.
blocking2.0: --- → ?
Whiteboard: [watching crash-stats]
update on stats
        mozilla::plugins::PPluginIdentifierParent::CreateSharedMemory.unsigned.int,.mozilla::ipc::SharedMemory::SharedMemoryType,.bool,.int..

date     total    breakdown by build
         crashes  count build, count build, ...


20110310 67 4.02011030319 	67 , 
20110311 156 4.02011030319 	156 , 
20110312 131 4.02011030319 	131 , 
20110313 201 4.02011030319 	201 , 

still a long tail of sites where this is seen.

   2 http://www.tepco.co.jp/images/tokyo.pdf
   2 http://www.pizarreno.cl/upload/pizarreno/2008616123440_siding.pdf
   2 http://dreamprogram.com/wp-content/uploads/2010/04/DreamToDestiny-Introduction-1.pdf
   1 https://www.bestsecret.com/medias/sys_master/8487611646533440.pdf
   1 https://ssl.fimmg.org/privacy/documenti/6466/20110313180802-6466.pdf
   1 https://kundenshop.1und1.de/modules/frontend-consumer/pdf/handbuch_fritzbox_fon_wlann_7390.pdf
   1 https://jobs.utah.gov/ui/Forms/form682.pdf
Attached patch TestSplinter Review
This test reproduces the error.
Attached patch Patch, v1 (obsolete) — Splinter Review
I think this should fix us. Basically calling a constructor that fails will cause the actor to be deleted, but not its subtree.
Assignee: nobody → bent.mozilla
Status: NEW → ASSIGNED
Attachment #519432 - Flags: review?(jones.chris.g)
Can we get a brief description of the type of user actions that would cause this crash?
Clicking on a link to a PDF file is the known cause. In this case, Acrobat is crashing (we don't know why), and we have a bug where if a plugin crashes while you are creating a plugin instance, and it has created any scriptable objects, then we crash in this way.
searching though module correlation data nothing sands out.

I did notice avast! Antivirus (snxhk.dll) showing up in several reports but its just a bit more than the usual pct.  maybe some combination of other incompatible modules that adds up 100%? 

19% (49/255) vs.  12% (9532/78012) snxhk.dll
19% (48/255) vs.  12% (8975/78012) dhcpcsvc6.DLL
18% (47/255) vs.   8% (6148/78012) cryptdll.dll
18% (45/255) vs.   9% (6963/78012) sensapi.dll
17% (44/255) vs.  11% (8881/78012) SensApi.dll
17% (44/255) vs.   5% (4006/78012) cabinet.dll
15% (39/255) vs.   9% (6842/78012) ATL80.dll
15% (39/255) vs.   3% (2090/78012) googletoolbar-ff4.dll
15% (39/255) vs.   3% (2090/78012) frozen.dll
15% (38/255) vs.   9% (6723/78012) GrooveUtil.DLL
15% (38/255) vs.   9% (6723/78012) GrooveNew.DLL
15% (38/255) vs.   9% (6720/78012) GrooveShellExtensions.dll
14% (36/255) vs.   5% (4261/78012) msi.dll
12% (31/255) vs.   4% (2759/78012) wuapi.dll
12% (30/255) vs.   6% (4321/78012) wevtapi.dll
12% (30/255) vs.   6% (4321/78012) Wpc.dll
11% (29/255) vs.   6% (4743/78012) ntdsapi.dll
11% (29/255) vs.   5% (4098/78012) browseui.dll
10% (25/255) vs.   5% (3643/78012) GrooveIntlResource.dll
9% (22/255) vs.   4% (2791/78012) MpOAV.dll
Comment on attachment 519432 [details] [diff] [review]
Patch, v1

>diff --git a/ipc/ipdl/ipdl/lower.py b/ipc/ipdl/ipdl/lower.py
>--- a/ipc/ipdl/ipdl/lower.py
>+++ b/ipc/ipdl/ipdl/lower.py
>-        
>+        # make sure that we aren't still hanging around in the manager's hash
>+        if ptype.isManaged():

This is unnecessary.

>     def failCtorIf(self, md, cond):
>         actorvar = md.actorDecl().var()
>+        type = md.decl.type.constructedType()

The dtorEpilogue() helper is doing exactly this already.  These two should share this logic, factoring it into a new helper would be fine.  The only difference between the two is that dtorEpilogue() dispatches ActorDestroy(why==Deletion), and failed constructors are dispatching with why==AbnormalShutdown.  That's not what we want; it specifically means crashes.  Instead, let's add FailedConstructor to enum ActorDestroyReason, dispatch that to the actor whose constructor failed, and then dispatch AncestorDeletion to managees. 

This is a fundamental semantic change, you need tests ;).
Attachment #519432 - Flags: review?(jones.chris.g) → review-
We've asked :cww to get in touch with some users who are suffering from this crash to see if we can determine why it's not universally reproducible.
The early feedback from users indicates:

 - doesn't happen all the time
 - they installed Adobe Reader X the "moment it was available"
 - they have been running betas for a while

No observable pattern with plugins, add-ons or graphics drivers.
blocking2.0: ? → .x+
Keywords: relnote
Whiteboard: [watching crash-stats] → [watching crash-stats][fx4-rc-ridealong]
Attached patch Patch, v2 (obsolete) — Splinter Review
How's this?
Attachment #519432 - Attachment is obsolete: true
Attachment #520245 - Flags: review?(jones.chris.g)
Comment on attachment 520245 [details] [diff] [review]
Patch, v2

Looks better, thanks.

>diff --git a/ipc/glue/ProtocolUtils.h b/ipc/glue/ProtocolUtils.h
>     enum ActorDestroyReason {
>         Deletion,
>         AncestorDeletion,
>         NormalShutdown,
>-        AbnormalShutdown
>+        AbnormalShutdown,
>+        FailedCtor
>     };

Tiny nits: s/FailedCtor/FailedConstructor/, and I'd prefer to list FailedConstructor before Deletion here.  Kinda hard to explain; there's an ordering of these in my head, in which FailedConstructor goes first :S.

>diff --git a/ipc/ipdl/ipdl/lower.py b/ipc/ipdl/ipdl/lower.py
>+        # make sure that we aren't still hanging around in the manager's hash

Nit: this isn't "making sure we've been removed", which implies the code is the backstop for something that may or may not have happened by now.  Instead, this code is *the* entity responsible for unregistering the actor's ID.  I'm not sure you really need a python comment here given the C++ one just below it, the wording of which I prefer.

>+        if type.isManager():
>+            failif.addifstmts([
>+                StmtExpr(self.callActorDestroy(
>+                             actorvar, why=_DestroyReason.FailedCtor)),
>+                StmtExpr(self.callDeallocSubtree(md, actorvar))
>             ])

This is wrong, because leaf protocols still need ActorDestroy() to be called on them.  DeallocSubtree() is a no-op for leaf protocols.  But see below.

>+        failif.addifstmts([
>+            StmtExpr(self.callRemoveActor(actorvar, ipdltype=type)),
>+            StmtReturn(ExprLiteral.NULL),
>+        ])

This code is still doing exactly the same thing as |dtorEpilogue()|; why not save pain and share the impl?

As we discussed on IRC, dtorEpilogue() is now doing an |unregisterActor()| made redundant by the changes to DestroySubtree().

Also, this version didn't address "dispatch [FailedConstructor] to the actor whose constructor failed, and then dispatch AncestorDeletion to managees."  If we don't do that, there won't be a way for managees to distinguish their own constructor failing from an ancestor's constructor failing after the descendent was successfully constructed.  See the code that munges Deletion->AncestorDeletion at http://mxr.mozilla.org/mozilla-central/source/ipc/ipdl/ipdl/lower.py#2967; we need another case for FailedConstructor->AncestorDeletion.  I don't think descendents really need to care about failed ctors of ancestors, so re-using AncestorDeletion ought to be fine to dispatch to descendents' ActorDestroy()s (instead of, say, creating a new AncestorFailedConstructor destroy reason).

How's the test coming along?  Can I help?
Attachment #520245 - Flags: review?(jones.chris.g)
Attached patch Patch, v3Splinter Review
Attachment #520245 - Attachment is obsolete: true
Attachment #520685 - Flags: review?(jones.chris.g)
 Bug 643515 adds these stacks and signature.

....Signature number:183-

mozilla::dom::PAudioParent::CreateSharedMemoryunsignedint,mozilla::ipc::SharedMemory::SharedMemoryType,bool,int

______ distribution of 25 different stacks, looking at top 10 frames
     24  stacks like
0|0|xul.dll|mozilla::dom::PAudioParent::CreateSharedMemory(unsigned int,mozilla::ipc::SharedMemory::SharedMemoryType,bool,int *)
0|1|xul.dll|mozilla::plugins::PPluginInstanceParent::RemoveManagee(int,mozilla::ipc::RPCChannel::RPCListener *)
0|2|xul.dll|mozilla::plugins::PPluginScriptableObjectParent::OnMessageReceived(IPC::Message const &)
0|3|xul.dll|mozilla::plugins::PPluginModuleParent::OnMessageReceived(IPC::Message const &)
0|4|xul.dll|mozilla::ipc::AsyncChannel::OnDispatchMessage(IPC::Message const &)
0|5|xul.dll|mozilla::ipc::RPCChannel::Call(IPC::Message *,IPC::Message *)
0|6|xul.dll|mozilla::plugins::PPluginScriptableObjectParent::CallInvalidate()
0|7|xul.dll|mozilla::plugins::PluginScriptableObjectParent::ScriptableInvalidate(NPObject *)
0|8|xul.dll|NPObjWrapperPluginDestroyedCallback
0|9|xul.dll|PL_DHashTableEnumerate

      1  stacks like
0|0|xul.dll|mozilla::dom::PAudioParent::CreateSharedMemory(unsigned int,mozilla::ipc::SharedMemory::SharedMemoryType,bool,int *)
0|1|xul.dll|mozilla::plugins::PPluginInstanceParent::RemoveManagee(int,mozilla::ipc::RPCChannel::RPCListener *)
0|2|xul.dll|mozilla::plugins::PPluginScriptableObjectParent::OnMessageReceived(IPC::Message const &)
0|3|xul.dll|mozilla::plugins::PPluginModuleParent::OnMessageReceived(IPC::Message const &)
0|4|xul.dll|mozilla::ipc::AsyncChannel::OnDispatchMessage(IPC::Message const &)
0|5|xul.dll|mozilla::ipc::RPCChannel::OnMaybeDequeueOne()
0|6|xul.dll|MessageLoop::RunTask(Task *)
0|7|xul.dll|MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const &)
0|8|xul.dll|MessageLoop::DoWork()
0|9|xul.dll|mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate *)
Summary: Crash [@ mozilla::plugins::PPluginIdentifierParent::CreateSharedMemory(unsigned int, mozilla::ipc::SharedMemory::SharedMemoryType, bool, int*) ] → Crash [@ mozilla::plugins::PPluginIdentifierParent::CreateSharedMemory(unsigned int, mozilla::ipc::SharedMemory::SharedMemoryType, bool, int*) ] [@ mozilla::dom::PAudioParent::CreateSharedMemory(unsigned int, mozilla::ipc::SharedMemory::SharedMemoryType,
Comment on attachment 520685 [details] [diff] [review]
Patch, v3

Looks good.  There are some problems with this patch, but r=me with the upcoming fixes tacked on.
Attachment #520685 - Flags: review?(jones.chris.g) → review+
Attached patch Test for bug 640901, v1.1 (obsolete) — Splinter Review
Whups, forgot to remove some dead code from the previous version.
Attachment #520830 - Attachment is obsolete: true
Whups, forgot to qref.  Sigh.
Attachment #520831 - Attachment is obsolete: true
I was trying to land this bug, but there is no way to figure out which patch(es) should be pushed.
Since it does not have checkin-needed, I wouldn't try to push it. Let the bug owners do it!
Pushed 7279fb5423bd to try.
blocking2.0: .x+ → ?
blocking2.0: ? → Macaw+
Crash Signature: [@ mozilla::plugins::PPluginIdentifierParent::CreateSharedMemory(unsigned int, mozilla::ipc::SharedMemory::SharedMemoryType, bool, int*) ]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.