549388 - nsProxyObject::Release uses proxy object manager lock after releasing proxy object manager

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Erik Staats]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en-US; rv:1.9.0.18) Gecko/2010020219 Firefox/3.0.18
Build Identifier: trunk

This bug results in a crash in Songbird when exiting (see http://bugzilla.songbirdnest.com/show_bug.cgi?id=20276).  If an nsProxyObject holds the last reference to the proxy object manager, both Release and LockedRelease will attempt to use the proxy object manager lock after the manager has been released and destroyed, resulting in a crash.

The fix is to change Release to reference the proxy object manager before calling LockedRelease.  Release then should not release the manager until after finishing with its lock.

Reproducible: Always

Steps to Reproduce:
1. Launch Songbird 1.7.0b1, Build 1531 with a clean profile
2. Make sure not to install any extensions
3. Exit Songbird and relaunch
4. Import some media into your library
5. Exit Songbird


Actual Results:  
Crash.

Expected Results:  
Songbird exits successfully.

Comment 1

[Erik Staats]

Attachment: Changed the xulrunner proxy object to hold an extra proxy object manager reference while holding the manager lock in Release.

Comment 2

[timeless]

we generally use nsRefPtr<> instead of manually reference counting.

Comment 3

[Erik Staats]

(In reply to comment #2)
> we generally use nsRefPtr<> instead of manually reference counting.

I wasn't sure on that since nsProxyObject manually ref counted the proxy object manager.

Comment 4

[Wladimir Palant]

Bumping to critical given that this is a crashing bug and confirming.

Erik, I think the manual ref counting is used because it cannot be easily replaced with smart pointers - not the case here, using nsRefPtr<> should make the patch quite a bit simpler.

Comment 5

[Erik Staats]

Attachment: Fixed the xulrunner proxy object to hold a reference to the proxy object manager while using its lock during release.

This patch uses nsRefPtr.

Comment 6

[Benjamin Smedberg]

Comment on attachment 430417 [details] [diff] [review]
Fixed the xulrunner proxy object to hold a reference to the proxy object manager while using its lock during release.

The extra addref/release pair on the proxy object manager are going to show up. Why don't we instead addref the proxyobjectmanager when it is created and don't release it until nsProxyObjectManager::Shutdown, which very late in XPCOM shutdown.

Comment 7

[Erik Staats]

(In reply to comment #6)
> (From update of attachment 430417 [details] [diff] [review])
> The extra addref/release pair on the proxy object manager are going to show up.
> Why don't we instead addref the proxyobjectmanager when it is created and don't
> release it until nsProxyObjectManager::Shutdown, which very late in XPCOM
> shutdown.

I think just declaring mInstance as "static nsRefPtr<nsProxyObjectManager>" in nsProxyObjectManager would do that.

I think, at least for correctness, nsProxyObject::Release should still hold a reference to nsProxyObjectManager until it's done using its lock.

Comment 8

[Benjamin Smedberg]

There is no need to be uber-correct here when there are known performance consequences. Two atomic operations and a test per release of a proxy is a pretty steep price to pay.

Comment 9

[Erik Staats]

(In reply to comment #6)
> (From update of attachment 430417 [details] [diff] [review])
> The extra addref/release pair on the proxy object manager are going to show up.
> Why don't we instead addref the proxyobjectmanager when it is created and don't
> release it until nsProxyObjectManager::Shutdown, which very late in XPCOM
> shutdown.

If we did that, nsProxyObject shouldn't have to addref the POM at all.  So how about making mInstance an nsRefPtr<nsProxyObjectManager> and taking out the POM addref/release from nsProxyObject?

Doing so would fix another problem we've run into.  If a PO is released after POM shutdown, nsProxyObjectManager::GetInstance will create a new POM.  The PO will then release the new POM even though it addref'ed the original POM.  This results in mismatched ref counts and crashes.

Comment 11

[Josh Matthews [:jdm]]

I think the fix in comment 9 is the way to go.  I can't see any way to force the service to be released absolutely last, since it's just one of many in the services hashtable.

Comment 12

[Benjamin Smedberg]

"the service"? The POM is shut down explicitly at http://mxr.mozilla.org/mozilla-central/source/xpcom/build/nsXPComInit.cpp#814 after all services are freed.

Comment 13

[Josh Matthews [:jdm]]

I direct you to this stack trace as exhibit A:


(gdb) bt
#0  nsProxyObjectManager::Release (this=0x93ed4e8) at /home/t_mattjo/src/firefox/electrolysis/xpcom/proxy/src/nsProxyObjectManager.cpp:101
#1  0x017e51da in nsCOMPtr_base::assign_assuming_AddRef (this=0x93af814, newPtr=0x0) at ../../../../dist/include/nsCOMPtr.h:456
#2  0x02304620 in nsCOMPtr_base::assign_with_AddRef (this=0x93af814, rawPtr=0x0) at nsCOMPtr.cpp:89
#3  0x0105a0b1 in nsCOMPtr<nsISupports>::operator= (this=0x93af814, rhs=0x0) at ../../../../dist/include/nsCOMPtr.h:975
#4  0x02366cc8 in FreeServiceContractIDEntryEnumerate (aTable=0x939e818, aHdr=0x93a5790, aNumber=515, aData=0x0)
    at /home/t_mattjo/src/firefox/electrolysis/xpcom/components/nsComponentManager.cpp:1733
#5  0x02302620 in PL_DHashTableEnumerate (table=0x939e818, etor=0x2366c8c <FreeServiceContractIDEntryEnumerate(PLDHashTable*, PLDHashEntryHdr*, PRUint32, void*)>, arg=0x0)
    at pldhash.c:754
#6  0x02366d59 in nsComponentManagerImpl::FreeServices (this=0x939e7d0) at /home/t_mattjo/src/firefox/electrolysis/xpcom/components/nsComponentManager.cpp:1746
#7  0x023185d5 in mozilla::ShutdownXPCOM (servMgr=0x0) at /home/t_mattjo/src/firefox/electrolysis/xpcom/build/nsXPComInit.cpp:811
#8  0x0231822f in NS_ShutdownXPCOM_P (servMgr=0x0) at /home/t_mattjo/src/firefox/electrolysis/xpcom/build/nsXPComInit.cpp:723
#9  0x00d94e74 in NS_ShutdownXPCOM (svcMgr=0x0) at /home/t_mattjo/src/firefox/electrolysis/xpcom/stub/nsXPComStub.cpp:179
#10 0x0804e330 in main (argc=19, argv=0xbf9501f0, envp=0xbf950240) at /home/t_mattjo/src/firefox/electrolysis/js/src/xpconnect/shell/xpcshell.cpp:1975

Comment 14

[timeless]

Attachment: change GetInstance and Shutdown

Comment 15

[Erik Staats]

(In reply to comment #14)
> Created an attachment (id=443341) [details]
> change GetInstance and Shutdown

This won't address the case where a proxy object exists after POM Shutdown.  If a proxy object is released after shutdown, it will crash when trying to use the POM lock after releasing the last reference on the POM.

Comment 16

[Benjamin Smedberg]

If you do any releases of XPCOM objects after XPCOM shutdown you're likely to crash. That's why we have xpcom-shutdown, so you stop doing that stuff!

Comment 17

[Benjamin Smedberg]

Attachment: Don't refcount nsProxyObjectManager

Comment 18

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/11b2042dee1b