Closed
Bug 1046000
Opened 10 years ago
Closed 10 years ago
Shut down wifi to avoid leaking at shutdown
Categories
(Firefox OS Graveyard :: Wifi, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: khuey, Assigned: khuey)
References
Details
(Keywords: memory-leak, perf, Whiteboard: [MemShrink][c=memory p= s= u=])
Attachments
(1 file)
(deleted),
patch
|
vchang
:
review+
|
Details | Diff | Splinter Review |
Wifi not shutting down is most of the shutdown leaks I see in the parent process on the emulator today.
Attachment #8464475 -
Flags: review?(vchang)
Assignee | ||
Comment 1•10 years ago
|
||
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•10 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) 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•10 years ago
|
||
(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...
Assignee | ||
Comment 4•10 years ago
|
||
(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•10 years ago
|
||
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?
Attachment #8464475 -
Flags: review?(vchang) → review+
Assignee | ||
Comment 6•10 years ago
|
||
After I land Bug 1038943 the automation will catch issues such as this in debug builds.
Updated•10 years ago
|
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/299094e66ea0
in-testsuite+ since this will soon be covered by bug 1038943.
Flags: in-testsuite+
Comment 8•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•