520639 - Topcrash at [@ RtlpWaitForCriticalSection][@ RtlpWaitForCriticalSection | RtlEnterCriticalSection][@RtlDeleteCriticalSection ][@ NPSWF32.dll@0x154517 ]

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[Justin Dolske [:Dolske]]

Public tracking bug for the topcrash with this signature. Further discussion in bug 511757, which is security sensitive (and thus annoyingly doesn't get linked on the Socorro topcrash summary).

Comment 1

[IU]

Since I can't see bug 511757, I'll comment here--even though my signature from today's crash has an additional component: [@RtlpWaitForCriticalSection | RtlEnterCriticalSection ].

I've seen this type of crash (related to flash) over several months now.  It appears to happen when transitioning from a flash video.

For example, I got the this crash when an online video I was watching was transitioning to a commercial break: http://crash-stats.mozilla.com/report/index/507016d1-15d7-47a9-acb6-02c302091127

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	ntdll.dll 	RtlpWaitForCriticalSection 	
1 	ntdll.dll 	RtlEnterCriticalSection 	
2 	wininet.dll 	wininet.dll@0x12c96 	
3 	NPSWF32.dll 	NPSWF32.dll@0x14e7a4 	
4 	NPSWF32.dll 	NPSWF32.dll@0x5eab5 	
5 	NPSWF32.dll 	NPSWF32.dll@0x5f0cf 	
6 	NPSWF32.dll 	NPSWF32.dll@0x5f1d3 	
7 	NPSWF32.dll 	NPSWF32.dll@0x5f479 	


Interestingly, two days ago I had a crash that had only the [@RtlDeleteCriticalSection ] signature.  That one is here: http://crash-stats.mozilla.com/report/index/bp-fc23a8db-ba7d-492a-b06a-0b3c12091125

Crashing Thread
Frame 	Module 	Signature [Expand] 	Source
0 	ntdll.dll 	RtlDeleteCriticalSection 	
1 	NPSWF32.dll 	NPSWF32.dll@0x154517 	
2 	NPSWF32.dll 	NPSWF32.dll@0x1334d7 	
3 	xul.dll 	nsPluginTag::TryUnloadPlugin 	modules/plugin/base/src/nsPluginHost.cpp:975
4 	xul.dll 	nsPluginTag::~nsPluginTag 	modules/plugin/base/src/nsPluginHost.cpp:771
5 	xul.dll 	nsPluginTag::`scalar deleting destructor' 	
6 	xul.dll 	nsPluginTag::Release 	modules/plugin/base/src/nsPluginHost.cpp:796
7 	xul.dll 	nsRefPtr<nsPrivateTextRangeList>::assign_assuming_AddRef 	obj-firefox/dist/include/nsAutoPtr.h:957
8 	xul.dll 	nsRefPtr<PluginWindowEvent>::assign_with_AddRef 	obj-firefox/dist/include/nsAutoPtr.h:941
9 	xul.dll 	nsPluginHost::ReloadPlugins 	modules/plugin/base/src/nsPluginHost.cpp:2574
10 	xul.dll 	nsPluginArray::Refresh 	dom/base/nsPluginArray.cpp:214
11 	xul.dll 	NS_InvokeByIndex_P 	xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp:102
12 	xul.dll 	XPCWrappedNative::CallMethod 	js/src/xpconnect/src/xpcwrappednative.cpp:2727
13 	xul.dll 	XPC_WN_CallMethod 	js/src/xpconnect/src/xpcwrappednativejsops.cpp:1756
14 	mozjs.dll 	js_Invoke 	js/src/jsinterp.cpp:1376
15 	mozjs.dll 	js_Interpret 	js/src/jsops.cpp:2305
16 	mozjs.dll 	js_Invoke 	js/src/jsinterp.cpp:1384
17 	mozjs.dll 	js_fun_call 	js/src/jsfun.cpp:1950
18 	mozjs.dll 	js_Interpret 	js/src/jsops.cpp:2271
19 	mozjs.dll 	js_Execute 	js/src/jsinterp.cpp:1618
20 	mozjs.dll 	JS_EvaluateUCScriptForPrincipals 	js/src/jsapi.cpp:5058
21 	xul.dll 	nsJSContext::EvaluateString 	dom/base/nsJSEnvironment.cpp:1713
22 	xul.dll 	nsScriptLoader::EvaluateScript 	content/base/src/nsScriptLoader.cpp:713
23 	xul.dll 	nsScriptLoader::ProcessRequest 	content/base/src/nsScriptLoader.cpp:626
24 	xul.dll 	nsCOMArray_base::RemoveObject 	obj-firefox/xpcom/build/nsCOMArray.cpp:129
25 	xul.dll 	nsScriptLoader::OnStreamComplete 	content/base/src/nsScriptLoader.cpp:97

Comment 2

[IU]

Oops.  Ignore the portion of my post detailing the second signature.  I misread RtlEnterCriticalSection as RtlDeleteCriticalSection.  Sorry

Comment 3

[Samuel Sidler (old account; do not CC)]

dolske: Any reason we don't just close these bugs as dupes of the security-sensitive bugs? They'll still show on the topcrash list, but I guess they will have a line through them, signifying closed. Hm...

Comment 4

[Justin Dolske [:Dolske]]

ss: I don't particularly care either way, though having them crossed out is unfortunate. We just don't have a good way to tag crashes that are security sensitive, maybe we need to add a simple lookup table (manually edited) to Socorro for such a purpose.

Comment 6

[Johnny Stenback (:jst)]

The underlying problem here, and in bug 521558, is that we're trying to unload plugins from the nsPluginTag destructor, which used to be fine until we started exposing the nsPluginTag objects to the blocklinsting code, which completely changes the lifetime of these objects. I think the fix is basically as simple as not calling TryUnloadPlugin() from the nsPluginTag destructor as I really don't see why we'd ever need to do it there in the first place. Plugins are generally unloaded from nsPluginInstanceTagList::remove(), which is called when a plugin is torn down on a webpage as well as for every plugin tag remaining at shutdown, or at least every one that could have something to unload. I'm testing this out on try now, and I'll post a patch once I see the results there...

Comment 7

[Johnny Stenback (:jst)]

Oh, and the reason this got worse recently in 3.6 was the fact that we started handing out the plugin tag to JS by calling into the blocklisting code when loading a plugin in the fix for bug 514327. We need to block on this IMO.

Comment 8

[Johnny Stenback (:jst)]

Attachment: Likely fix.

I believe this will fix this crash. My first attempt at this showed leaks on the try servers, but I believe that leaks are gone with this patch. Waiting for confirmation from the try servers now...

Comment 9

[(not currently active) Ted Mielczarek]

If you can link me a try server build I'll be happy to test it. I could reproduce this crash with alarming frequency with Flashblock enabled.

Comment 10

[Peter Van der Beken [:peterv]]

(In reply to comment #9)
> If you can link me a try server build I'll be happy to test it. I could
> reproduce this crash with alarming frequency with Flashblock enabled.

Looks like https://build.mozilla.org/tryserver-builds/[e-mail]-try-bf5ad34e9e7d/

Comment 11

[Johnny Stenback (:jst)]

Yup, what peterv said.

Comment 12

[Josh Aas]

Comment on attachment 415356 [details] [diff] [review]
Likely fix.

With this patch we'll never properly unload plugins by default. There are only two places we call TryUnloadPlugin - one never happens unless "plugins.unloadASAP" is set and the other one is killed off in this patch. We need to properly unload from somewhere.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 415356 [details] [diff] [review]
Likely fix.

Yup, this is no good. We'll not crash with this, and we won't "leak", but we'll never unload plugin libraries. Not that that's necessarily a bad thing, come to think of it...

Josh said he knew what needed to be done here, and is looking into this one further (since I was unable to do so today).

Comment 14

[Johnny Stenback (:jst)]

Seems like Marcia found steps to reproduce this bug! See bug 530989, comment c16.

Comment 15

[Martijn Wargers (dead)]

Fwiw, I posted a zipped up testcase in bug 521558.

Comment 16

[Josh Aas]

Attachment: fix v1.1

This is closer to what we want but I need to finish testing some things.

Comment 17

[Johnny Stenback (:jst)]

Attachment: fix v1.2jst

This is a followup version to Josh's previous patch, with the additional change to make nsPluginHost::Destroy() also unload plugins. W/o this we'll never unload plugins at XPCOM shuttdown.

Josh, can you think of anything else we'd need to worry about here? I've stared at this code a fair bit by now and I'm feeling pretty confident about this change...

Comment 18

[Johnny Stenback (:jst)]

Oh, and I pushed this to try...

Comment 19

[Josh Aas]

Attachment: fix v1.3

Shutdown unloading is what I still needed to figure out (had to run for a bit) and your patch looks good. This iteration just removes more unnecessary stuff from ::Destroy.

Comment 20

[Johnny Stenback (:jst)]

Try server builds are available at:

https://build.mozilla.org/tryserver-builds/[e-mail]-try-c81eb74f4ca5/

and I'll be landing Josh's latest patch shortly.

Comment 21

[Johnny Stenback (:jst)]

Attachment: fix v1.3 for 1.9.2

This is the same patch made to apply to 1.9.2. Few things didn't apply, but nothing tricky to merge.

Comment 22

[Johnny Stenback (:jst)]

Fixed on trunk and 1.9.2.

http://hg.mozilla.org/mozilla-central/rev/5be08dcdc276
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/3777f1168748

We need a variant of this for at least 1.9.1 as well...

Comment 23

[Peter Van der Beken [:peterv]]

This seems to have stuck on 1.9.2 with a followup leak fix:

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cb4ffebe9f4c

Comment 24

[Daniel Veditz [:dveditz]]

Can we get a 1.9.1 patch that includes the followup leak fix?

Comment 25

[Black_Ps`]

Is this the same as bp-583413d2-1e5f-41d1-abdf-37d662091217	[@ RtlDeleteCriticalSection ]	17/12/2009	11:09
↳ Caused by a plugin: Shockwave Flash (npswf32.dll)‎ ? sorry for posting it here but i don't want to report a existing bug already

Comment 26

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

These crashes don't seem to have gone down a whole lot in Firefox 3.6b5.  Does another bug need to be filed?  Did this fix some known subset of the crashes?

Comment 27

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

This did fix bug 530989, though.

Comment 28

[chris hofmann]

(In reply to comment #26)
> These crashes don't seem to have gone down a whole lot in Firefox 3.6b5.  Does
> another bug need to be filed?  Did this fix some known subset of the crashes?

#2 top crash in 3.6 rc1 too.

bug 511757 turned out not to be security sensitive so its open now.  we could use that bug to track further work if its useful, or open a new one.

Comment 29

[Daniel Veditz [:dveditz]]

If this patch did not fix the topcrash do we need to block on it?

Comment 30

[Johnny Stenback (:jst)]

I suspect that there's far more causes for this than what can be covered in any one bug, and I really don't think we can realistically block on this, no matter how much we'd like it fixed.

Comment 31

[Martijn Wargers (dead)]

Nobody changed this bug, who is Nobody?

Comment 32

[Robert Kaiser]

(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #31)
> Nobody changed this bug, who is Nobody?

An automated change we did a while ago when the Crash Signature field was new. We imported all the [@ ...] stuff from summaries into that field but suppressed bugmail specifically for that. Now that there was some other change affecting this bug (closing a dependent bug), the bugmail included the note about that automated change from some time ago.