758872 - crash in nsPluginInstanceOwner::DispatchFocusToPlugin @ nsNPAPIPluginInstance::HandleEvent

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Scoobidiver (away)]

There are 13 crashes in 14.0b2.
STR are in bug 620794 comment 10.

Signature 	nsNPAPIPluginInstance::HandleEvent More Reports Search
UUID	ab6a1a0f-ce62-4d52-ac96-977ff2120525
Date Processed	2012-05-25 21:07:29
Uptime	2141
Install Age	3.0 days since version was first installed.
Install Time	2012-05-22 21:14:26
Product	FennecAndroid
Version	14.0
Build ID	20120515171426
Release Channel	beta
OS	Linux
OS Version	0.0.0 Linux 2.6.32.9-perf #1 PREEMPT Mon Sep 19 08:03:47 2011 armv7l
Build Architecture	arm
Build Architecture Info	
Crash Reason	SIGSEGV
Crash Address	0x20
App Notes 	
AdapterVendorID: semc, AdapterDeviceID: R800i.
AdapterDescription: 'Model: 'R800i', Product: 'R800i_1247-6199', Manufacturer: 'Sony Ericsson', Hardware: 'semc''.
Sony Ericsson R800i
SEMC/R800i_1247-6199/R800i:2.3.4/4.0.2.A.0.42/1f_v3w:user/release-keys
EMCheckCompatibility	True

Frame 	Module 	Signature 	Source
0 	libxul.so 	nsNPAPIPluginInstance::HandleEvent 	dom/plugins/base/nsNPAPIPluginInstance.cpp:591
1 	libxul.so 	nsPluginInstanceOwner::DispatchFocusToPlugin 	dom/plugins/base/nsPluginInstanceOwner.cpp:1810
2 	libxul.so 	nsPluginInstanceOwner::HandleEvent 	dom/plugins/base/nsPluginInstanceOwner.cpp:2017
3 	libxul.so 	nsEventListenerManager::HandleEventSubType 	content/events/src/nsEventListenerManager.cpp:819
4 	libxul.so 	nsEventListenerManager::HandleEventInternal 	content/events/src/nsEventListenerManager.cpp:876
5 	libxul.so 	nsEventTargetChainItem::HandleEvent 	content/events/src/nsEventListenerManager.h:169
6 	libxul.so 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:372
7 	libxul.so 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:684
8 	libxul.so 	FocusBlurEvent::Run 	dom/base/nsFocusManager.cpp:1893
9 	libxul.so 	nsContentUtils::AddScriptRunner 	content/base/src/nsContentUtils.cpp:4750
10 	libxul.so 	nsFocusManager::SendFocusOrBlurEvent 	dom/base/nsFocusManager.cpp:1952
11 	libxul.so 	nsFocusManager::Blur 	dom/base/nsFocusManager.cpp:1613
12 	libxul.so 	nsFocusManager::SetFocusInner 	dom/base/nsFocusManager.cpp:1269
13 	libxul.so 	nsFocusManager::SetFocus 	dom/base/nsFocusManager.cpp:467
14 	libxul.so 	nsGenericElement::PostHandleEventForLinks 	content/base/src/nsGenericElement.cpp:5967
15 	libxul.so 	nsGenericHTMLElement::PostHandleEventForAnchors 	content/html/content/src/nsGenericHTMLElement.cpp:1267
16 	libxul.so 	nsHTMLAnchorElement::PostHandleEvent 	content/html/content/src/nsHTMLAnchorElement.cpp:353
17 	libxul.so 	nsEventTargetChainItem::PostHandleEvent 	content/events/src/nsEventDispatcher.cpp:294
18 	libxul.so 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:352
19 	libxul.so 	nsEventTargetChainItem::HandleEventTargetChain 	content/events/src/nsEventDispatcher.cpp:404
20 	libxul.so 	nsEventDispatcher::Dispatch 	content/events/src/nsEventDispatcher.cpp:684
21 	libxul.so 	PresShell::HandleEventInternal 	layout/base/nsPresShell.cpp:6621
22 	libxul.so 	PresShell::HandlePositionedEvent 	layout/base/nsPresShell.cpp:6293
23 	libxul.so 	PresShell::HandleEvent 	layout/base/nsPresShell.cpp:6093
24 	libxul.so 	nsDOMWindowUtils::SendMouseEventCommon 	dom/base/nsDOMWindowUtils.cpp:562
25 	libxul.so 	nsDOMWindowUtils::SendMouseEventToWindow 	dom/base/nsDOMWindowUtils.cpp:481
26 	libxul.so 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/unix/xptcinvoke_arm.cpp:194
27 	libxul.so 	XPCWrappedNative::CallMethod 	js/xpconnect/src/XPCWrappedNative.cpp:3111
28 	libxul.so 	XPC_WN_CallMethod 	js/xpconnect/src/XPCWrappedNativeJSOps.cpp:1554
29 	libxul.so 	js::Interpret 	js/src/jscntxtinlines.h:314
30 	libxul.so 	js::RunScript 	js/src/jsinterp.cpp:475
31 	libxul.so 	js::Invoke 	js/src/jsinterp.cpp:535
32 	libxul.so 	JS_CallFunctionValue 	js/src/jsapi.cpp:5416
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsNPAPIPluginInstance%3A%3AHandleEvent

Comment 1

[Scoobidiver (away)]

There are two crashes from two users in today's build.

Comment 2

[Scoobidiver (away)]

More reports also at:
https://crash-stats.mozilla.com/report/list?signature=nsNPAPIPluginInstance%3A%3AHandleEvent%28void*%2C+short*%29

Comment 3

[Scoobidiver (away)]

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsNPAPIPluginInstance%3A%3AHandleEvent%28void*%2C+short*%2C+NSPluginCallReentry%29

Comment 4

[Georg Fritzsche [:gfritzsche]]

Attachment: Fix missing null check for plugin instance

Peeking into the crash reports, this is apparently just a missing null-check here:
http://hg.mozilla.org/releases/mozilla-aurora/annotate/9ff2584b47a5/dom/plugins/base/nsPluginInstanceOwner.cpp#l1810
... leading to a crash on accessing nsNPAPIPluginInstance::mRunning here:
http://hg.mozilla.org/releases/mozilla-aurora/annotate/9ff2584b47a5/dom/plugins/base/nsNPAPIPluginInstance.cpp#l591

Comment 5

[Benjamin Smedberg]

Comment on attachment 732298 [details] [diff] [review]
Fix missing null check for plugin instance

As a bandaid I guess this is ok. But we really shouldn't be registered to receive these events at all when there is no instance: nsPluginInstanceOwner::Destroy should already have been called. Perhaps johns already fixed this on trunk with the teardown refactoring? If so, make this assert before you null-check.

Comment 6

[John Schoenick [:johns]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Comment on attachment 732298 [details] [diff] [review]
> Fix missing null check for plugin instance
> 
> As a bandaid I guess this is ok. But we really shouldn't be registered to
> receive these events at all when there is no instance:
> nsPluginInstanceOwner::Destroy should already have been called. Perhaps
> johns already fixed this on trunk with the teardown refactoring? If so, make
> this assert before you null-check.

We could leave dangling instance owners around prior to my patch, so that would make sense.

Comment 7

[Georg Fritzsche [:gfritzsche]]

(In reply to John Schoenick [:johns] from comment #6)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> > As a bandaid I guess this is ok. But we really shouldn't be registered to
> > receive these events at all when there is no instance:
> > nsPluginInstanceOwner::Destroy should already have been called. Perhaps
> > johns already fixed this on trunk with the teardown refactoring? If so, make
> > this assert before you null-check.
> 
> We could leave dangling instance owners around prior to my patch, so that
> would make sense.

Right, that explains the guards on mInstance in other places. So "would make sense" to land this with the assertions?

Comment 8

[John Schoenick [:johns]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> Right, that explains the guards on mInstance in other places. So "would make
> sense" to land this with the assertions?

We should never leave dangling instance owners so an assertion makes sense, yeah. Bonus points if it uncovers a bunch of bugs :-P

Comment 9

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ccf83ffd75a

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3ccf83ffd75a

Comment 11

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 732298 [details] [diff] [review]
Fix missing null check for plugin instance

[Approval Request Comment]
Bug caused by (feature/regressing bug #): -
User impact if declined: plugin related crashes in Fennec
Testing completed (on m-c, etc.): no crashes since landing on m-c
Risk to taking this patch (and alternatives if risky): low-risk, just a null check, worst case of moving to another crash-signature
String or IDL/UUID changes made by this patch: none.

Comment 12

[bhavana bajaj [:bajaj]]

Comment on attachment 732298 [details] [diff] [review]
Fix missing null check for plugin instance

Null check, low risk patch. We are still early in our Beta cycle to take low risk fixes and do a backout if needed in case this leads to any unforeseen worser crashes.
Please make sure to look if this is resolved or led to morphed signatures once it lands on aurora/beta. Thanks !

Comment 13

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e85b222aaf5e
https://hg.mozilla.org/releases/mozilla-beta/rev/f66fc9067d0d

Comment 14

[Georg Fritzsche [:gfritzsche]]

(In reply to bhavana bajaj [:bajaj] from comment #12)
> Please make sure to look if this is resolved or led to morphed signatures
> once it lands on aurora/beta. Thanks !

Setting need-info to not lose track.

Comment 15

[Georg Fritzsche [:gfritzsche]]

I don't see any crashes with a signature containing nsNPAPIPluginInstance::HandleEvent on >=21 for the last two weeks, so this seems fine.

Comment 16

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old