108046 - DNSService Shutdown uses getService; suggest registering WeakRef

Status: VERIFIED FIXED

(Core :: Networking, defect, P1)

Description

[David Epstein]

reproduced in 10/27 debug mozilla build. this is causing a warning message when
shutting down mozilla. see bug 107391.
1. Set break points in nsWarning messages in nsComponentManager.cpp;
nsComponentManagerImpl::GetService() and
nsComponentManagerImpl::GetServiceByContractID() (currently lines 1750 & 2075).
2. Launch Mozilla with F5.
3. Quit Mozilla.
4. F5 until you see stack trace including destructor for
nsIOService()
5. Doubleclick on 3 lines above ~nsIOService():
nsDNSService::RemovePrefObserver() line 1178 + 30 bytes
Result: Creates nsIPrefService object by calling do_GetService() to shutdown
DNSService service. 
Suggest registering a weakReference and using it to shutdown service.

Comment 1

[David Epstein]

Here's the source in nsDnsService.cpp for RemovePrefObserver():
nsresult
nsDNSService::RemovePrefObserver()
{
    nsresult rv, rv2;
    
    nsCOMPtr<nsIPrefService> prefs = do_GetService(NS_PREFSERVICE_CONTRACTID, &rv);
    if (NS_FAILED(rv))  return rv;
    
    nsCOMPtr<nsIPrefBranchInternal> prefInternal = do_QueryInterface(prefs, &rv);
    if (NS_FAILED(rv))  return rv;
    
    rv = prefInternal->RemoveObserver(NETWORK_ENABLEIDN, this);
    if (NS_FAILED(rv))  return rv;

    rv  = prefInternal->RemoveObserver(NETWORK_DNS_CACHE_ENTRIES, this);
    rv2 = prefInternal->RemoveObserver(NETWORK_DNS_CACHE_EXPIRATION, this);
    
    return NS_FAILED(rv) ? rv : rv2;
}

Comment 2

[Doug Turner (:dougt)]

-> gordon

Comment 3

[gordon]

Targeting for necko stability milestone 0.9.8.

Comment 4

[gordon]

Doug, according to dp's comment (#6) in bug 107391, this is correct pattern to
be using.  Can we just close this bug?

Comment 5

[Doug Turner (:dougt)]

The only problem is if we do not correct this pattern, we will have to wade
through many false positives.  

Comment 6

[gordon]

Attachment: proposed patch adding weak reference to pref service.

Comment 7

[gordon]

Darin, Doug, can you r/sr?

Comment 8

[Doug Turner (:dougt)]

Comment on attachment 69538 [details] [diff] [review]
proposed patch adding weak reference to pref service.

r=dougt

Comment 9

[gordon]

Comment on attachment 69538 [details] [diff] [review]
proposed patch adding weak reference to pref service.

sr=darin

Comment 10

[gordon]

Patch checked in.  Marking FIXED.

Comment 11

[benc]

David:

Can you try your steps again and verify if it is gone?

Comment 12

[David Epstein]

Verified patch wrt to Mozilla 0.9.9 3/19/02 debug build. Looks fine. With steps
above, nsDNSService::RemovePrefObserver()isn't in the shutdown call stack;
getService() not used for dns service shutdown.