180366 - [regression] browser crashes on shutdown after scripting plugin

Status: RESOLVED FIXED

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

Description

[Peter Lubczynski]

Steps to reproduce:
1. Goto the testcase (sorry, it's internal):
http://jazz.mcom.com/users/shrir/publish/flashacceptancesuite/API/index.htm
2. Click on a scriptable function like Stop.
3. Quit the browser. Crashes w/:

 #0   0x0061d48c in nsCOMPtr_base::_dt(void)
>  #1   0x005500f0 in dl(void *)
>  #2   0x01e31aa8 in XPCWrappedNativeProto::_dt(void)
>  #3   0x01e2ae98 in DyingProtoKiller(JSDHashTable *, JSDHashEntryHdr *,
unsigned long, void *)
>  #4   0x01dde790 in JS_DHashTableEnumerate
>  #5   0x01e2b490 in XPCJSRuntime::GCCallback(JSContext *, JSGCStatus)
>  #6   0x025ce38c in jsds_GCCallbackProc(JSContext *, JSGCStatus)
>  #7   0x02aece50 in DOMGCCallback(JSContext *, JSGCStatus)
>  #8   0x01d9af44 in js_GC
>  #9   0x01d9a660 in js_ForceGC
>  #10  0x01d77cc0 in JS_GC
>  #11  0x02b10354 in nsDOMSOFactory::Observe(nsISupports *, char const *,
wchar_t const *)
>  #12  0x005a5780 in nsObserverService::NotifyObservers(nsISupports *, char
const *, wchar_t const *)
>  #13  0x005c8c84 in NS_ShutdownXPCOM
>  #14  0x004cad20 in main

This is a regression. I pulled a build from the 10th and it does not crash.
Here's the bonsai query I'm looking at:
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=11%2F10%2F2002+8%3A00&maxdate=11%2F11%2F2002+8%3A00&cvsroot=%2Fcvsroot

Comment 1

[Peter Lubczynski]

*** Bug 180365 has been marked as a duplicate of this bug. ***

Comment 2

[shrirang khanzode]

still crashes on win XP with today's trunk build.

Comment 3

[serge (gone)]

w2k stack trace:
nsCOMPtr<nsIClassInfo>::~nsCOMPtr<nsIClassInfo>() line 490 + 13 bytes
XPCWrappedNativeProto::~XPCWrappedNativeProto() line 83 + 11 bytes
XPCWrappedNativeProto::`scalar deleting destructor'(unsigned int 0x00000001) +
15 bytes
DyingProtoKiller(JSDHashTable * 0x01416858, JSDHashEntryHdr * 0x0205a6e8,
unsigned long 0x00000003, void * 0x00000000) line 200 + 28 bytes
JS_DHashTableEnumerate(JSDHashTable * 0x01416858, int (JSDHashTable *,
JSDHashEntryHdr *, unsigned long, void *)* 0x0175a990
DyingProtoKiller(JSDHashTable *, JSDHashEntryHdr *, unsigned long, void *), void
* 0x00000000) line 602 + 34 bytes
XPCWrappedNativeProtoMap::Enumerate(JSDHashOperator (JSDHashTable *,
JSDHashEntryHdr *, unsigned long, void *)* 0x0175a990
DyingProtoKiller(JSDHashTable *, JSDHashEntryHdr *, unsigned long, void *), void
* 0x00000000) line 639 + 27 bytes
XPCJSRuntime::GCCallback(JSContext * 0x03f32f08, JSGCStatus JSGC_FINALIZE_END)
line 485
DOMGCCallback(JSContext * 0x03f32f08, JSGCStatus JSGC_FINALIZE_END) line 1641 +
23 bytes
js_GC(JSContext * 0x03f32f08, unsigned int 0x00000000) line 1374 + 12 bytes
js_ForceGC(JSContext * 0x03f32f08, unsigned int 0x00000000) line 993 + 13 bytes
JS_GC(JSContext * 0x03f32f08) line 1659 + 11 bytes
nsDOMSOFactory::Observe(nsDOMSOFactory * const 0x01e9a4f4, nsISupports *
0x0027ddd4, const char * 0x10110aa8, const unsigned short * 0x00000000) line 227
+ 10 bytes
nsObserverService::NotifyObservers(nsObserverService * const 0x0027c028,
nsISupports * 0x0027ddd4, const char * 0x10110aa8, const unsigned short *
0x00000000) line 213
NS_ShutdownXPCOM(nsIServiceManager * 0x00000000) line 694
main(int 0x00000001, char * * 0x002776d0) line 1911 + 8 bytes
mainCRTStartup() line 338 + 17 bytes
KERNEL32! CreateProcessW + 5542 bytes

Comment 4

[David G King]

I'm seeing this all the time. WinXP with build 2002120604 (and I think a bit
earlier).

See talkback TB14869860Y for my latest crash. Upgrading Severity as this is a crash.

Comment 5

[Andrew Schultz]

David: that's bug 183901

Comment 6

[Peter Lubczynski]

Win32 moz 20021108 crashes for me :(

Shrir, can you help narrow down when this really started? Thanks!
I've got some moz Win32 daily zips archived here:
http://peterl.mcom.com/builds/archive/

Comment 7

[Peter Lubczynski]

*** Bug 185053 has been marked as a duplicate of this bug. ***

Comment 8

[shrirang khanzode]

I am trying to narrow down the build date when this started to happen ...

Comment 9

[shrirang khanzode]

this regressed between sept 4 and 5th ( builds 2002-09-04-08 and 2002-09-05-03)

Comment 10

[David G King]

I'm not totally sure about those dates (with all due respect). The two reasons I
say that are 1) someone would have reported the bug before 11/15 and 2) Comment
#0 says that the 11/10 build worked fine.

Has this problem been coming and going based on some other code/setting? Or has
a recent default setting change exposed this bug?

Shrir: Does the 11/10 build work properly for you?
 

Comment 11

[shrirang khanzode]

dgk > nope, it crashes like all other builds for me.

Comment 12

[Peter Lubczynski]

Looking in the debugger w/ breakpoints, the problem here is apparent:

we PR_UnloadLibrary during Observe("quit-application"). Sometime later, there is
a GC that we crash on when calling ->Release of the XPCOM scriptable
nsIClassInfo object.

I wonder if it would help to force a GC on Observe("quit-application") or if we
should just leak...

Comment 13

[Peter Lubczynski]

Attachment: patch v.1

This patch should stop the crash but I hope it does not effect bug 148889. It
prevents calling PR_UnloadLibrary on plugins during browser shutdown. Since
this is only at browser shutdown (nsPluginHostImlp::Destory), the OS should
unload the library shortly.

Andrei, Serge, what do you think?

Comment 14

[av (gone)]

Peter, so you suggest not to unload plugin libraries at all? This will probably
mean that bug 153001 is invalid, right?

Comment 15

[David G King]

IMHO, Bug # 153001 is very important for Windows machines. If plugins (DLL's)
aren't unloaded then you have memory used (allocated?) for no purpose.

Peter, are you sure that the OS (I'm thinking Windows in particular) will unload
the "library" by itself?

Comment 16

[av (gone)]

From MSDN help: "Every process that loads the DLL maps it into its virtual
address space. [...] The system maintains a reference count for each DLL. [...]
When the process terminates, or when the reference count goes to 0 (run-time
dynamic linking only), the DLL is unloaded from the virtual address space."

Comment 17

[Peter Lubczynski]

Attachment: patch v.2

Here's another patch that also fixes the problem but takes a completely
differnet route:

The GC that causes the crash happens on Observe("xpcom-shutdown") which gets
sent after Observe("quit-application") when we do our cleanup and unload. By
removing the Observervation of ("quit-application") and doing our unloading
later on xpcom-shutdown, the crash no longer happens.

Comment 18

[av (gone)]

Peter, do I remember correctly that 'quit-application' observation was added
because 'xpcom-shutdown' did not always come? Is this still the case?

Comment 19

[Doug Turner (:dougt)]

I haven't taken a close look at the patch (more context is always nice: try cvs
diff -u10).  The deal is that you want to ensure that your code unloads dll's
after every reference to objects in those dlls is gone.  If you have a reference
stuck in JS, you have to ensure that GC cleans it up before your code executes.
 It would makes sense, to make the xpconnect code listen to a quit application
event, and your plugin code listen to the lower level xpcom shutdown event.

The problem that I see with both xpconnect and the plugin code triggering from
the same event is that there is no explict ordering - the plugin code could
called prior to the xpconnect code on shutdown.  If both post events to finish
their cleanup, then we will be having a similar conversation about a similar crash.

david, is there any problem with killing xpconnect at the quit application time.
 Note that js components will be effected - they will no longer be notified of
XPCOM Shutdown. 

Comment 20

[David G King]

AV: Thanks for that. I feel much happier about this patch going in (well, the v1
form). I haven't studied v2 as yet.

What do people think about nominating this bug as blocking 1.3b?

Comment 21

[Doug Turner (:dougt)]

if xpcom-shutdown is not being called, lets fix that - not bandaide plugin code.
 gawd knows we have too many bandaides in the plugin code :-)

Comment 22

[Peter Lubczynski]

Plugin code always posts a PLEvent to call PR_UnloadLibrary so as long as that
PLEvent doesn't get processed synchronously and xpconnect's GC does happen
synchronous, we should be fine processing them on the same
Observe("xpcom-shutdown").

Doug is right, if we don't always get an "xpcom-shutdown" that should probably
be fixed as other code may depend on it.

However, I'm still not sure patch v.2 is better than leaking the library with
patch v.1. How can plugin code know when all references to COM objects have been
released? I don't think its possible unless we invent some way of asking the plugin?

Comment 23

[av (gone)]

>How can plugin code know when all references to COM objects have been released?
I >don't think its possible unless we invent some way of asking the plugin?

We probably need even more. A way to instruct a plugin to release everything
because we want to shutdown, and unload it or not depending on the response.

Comment 24

[Peter Lubczynski]

Attachment: patch v.3

well, adding to the API isn't going to help plugins in the field. This crash on
shutdown is really annoying and needs to be fixed. 

Here's a new patch that is a compromise between the last two:
We still call |Destory| on Observe("quit-application") but don't unload libs.
On Observe("xpcom-shutdown") we'll unload the libs and if this message doesn't
come, we'll just leak them and the OS will cleanup on app terminate.

Comment 25

[Jeff G]

How will the leak affect MacOS9?  Its been my experience that MacOS8/9 does not
like leaks and that may cause an OS crash.  If I am wrong on this...let me
know...but this bug has caused the "bomb" and reboot to occur on OS9...so I
suspect a leak may cause a similar problem. Your thoughts?

Comment 26

[av (gone)]

Comment on attachment 109668 [details] [diff] [review]
patch v.3

Do I need to do it twice? Bug in bugzilla? r=av.

Comment 27

[Patrick C. Beard]

Comment on attachment 109668 [details] [diff] [review]
patch v.3

sr=beard (twice?)

Comment 28

[Peter Lubczynski]

The patch won't leak unloading plugins on OS9 or any OS as long as
Observe("xpcom-shutdown") is recieved.

Patch in trunk, marking FIXED.