1046000 - Shut down wifi to avoid leaking at shutdown

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect, P2)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

Wifi not shutting down is most of the shutdown leaks I see in the parent process on the emulator today.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Just to explain what's going on here, the wifi services shutdown methods are never called.  This leads to two problems:

1) We do not shutdown the threads on our own.  Instead we rely on the nsThreadManager::Shutdown's code to go through and shutdown all the threads that Gecko forgot to shutdown.  This is not a big deal, but it is undesirable.
2) The wifi event listener is rooted until the component manager is shut down (because the component manager holds a reference to the wifi services, which hold references to the wifi event listener).  But we shut down the cycle collector before we shut down the component manager, so our last chance to collect cycles cannot catch any cycles involving the wifi event listener.  This leads to the leaks.

The solution is to explicitly shut down the services during xpcom-shutdown and explicitly release the wifi event listener during shutdown.  This ensures that we shut down the threads properly and that the cycle collector has a chance to collect cycles involving the wifi event listener.

Comment 2

[Henry Chang [:hchang]]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #1)
> Just to explain what's going on here, the wifi services shutdown methods are
> never called.  This leads to two problems:
> 
> 1) We do not shutdown the threads on our own.  Instead we rely on the
> nsThreadManager::Shutdown's code to go through and shutdown all the threads
> that Gecko forgot to shutdown.  This is not a big deal, but it is
> undesirable.
> 2) The wifi event listener is rooted until the component manager is shut
> down (because the component manager holds a reference to the wifi services,
> which hold references to the wifi event listener).  But we shut down the
> cycle collector before we shut down the component manager, so our last
> chance to collect cycles cannot catch any cycles involving the wifi event
> listener.  This leads to the leaks.
> 
> The solution is to explicitly shut down the services during xpcom-shutdown
> and explicitly release the wifi event listener during shutdown.  This
> ensures that we shut down the threads properly and that the cycle collector
> has a chance to collect cycles involving the wifi event listener.

Hi Kyle,

Can we just call Shutdown() in the ~WifiService() to avoid
explicitly and manually shutting down WifiService in WifiWorker?

(since the life time of WifiService is already bound to xpcom-shutdown [1])

[1] http://hg.mozilla.org/mozilla-central/file/f61a27b00e05/dom/wifi/WifiProxyService.cpp#l186

Comment 3

[Henry Chang [:hchang]]

(In reply to Henry Chang [:henry] from comment #2)
> Hi Kyle,
> 
> Can we just call Shutdown() in the ~WifiService() to avoid
> explicitly and manually shutting down WifiService in WifiWorker?
> 
> (since the life time of WifiService is already bound to xpcom-shutdown [1])
> 

Oops it's not exact. The global reference is cleared but WifiProxyService 
is still referenced by the wifi event listener so we still have to 
explicitly break the cycle...

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Henry Chang [:henry] from comment #3)
> (In reply to Henry Chang [:henry] from comment #2)
> > Hi Kyle,
> > 
> > Can we just call Shutdown() in the ~WifiService() to avoid
> > explicitly and manually shutting down WifiService in WifiWorker?
> > 
> > (since the life time of WifiService is already bound to xpcom-shutdown [1])
> > 
> 
> Oops it's not exact. The global reference is cleared but WifiProxyService 
> is still referenced by the wifi event listener so we still have to 
> explicitly break the cycle...

Right.  The cycle must be explicitly broken at some point.  And you can't break reference counting cycles from the destructor of the objects participating in the cycle, because the cycle prevents the destructor from running.  We could add cycle collection to the wifi services but this is simpler.

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Comment on attachment 8464475 [details] [diff] [review]
Patch

Review of attachment 8464475 [details] [diff] [review]:
-----------------------------------------------------------------

Hi Kyle, thanks for your detail explanation. It's really helpful. 
I am wondering that if we have tools to help to verfiy the fix of the problem or can we 
write the test case for this kind of problems?

Comment 6

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

After I land Bug 1038943 the automation will catch issues such as this in debug builds.

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/299094e66ea0

in-testsuite+ since this will soon be covered by bug 1038943.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/299094e66ea0