Closed
Bug 753687
Opened 12 years ago
Closed 11 years ago
nsCategoryCache implementation invites memory leaks
Categories
(Core :: XPCOM, defect, P2)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: jwkbugzilla, Assigned: benjamin)
References
Details
(Whiteboard: [MemShrink:P3])
Attachments
(2 files)
2.27 KB,
application/x-xpinstall
|
Details | |
17.28 KB,
patch
|
froydnj
:
review+
lsblakk
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
The way nsCategoryCache currently removes its entries invites memory leaks, particularly when dealing with bootstrapped extensions. The obvious way to unregister a component when the extension is disabled/uninstalled is this: for each (let category in this.xpcom_categories) catMan.deleteCategoryEntry(category, this.classDescription, false); registrar.unregisterFactory(this.classID, this); First remove the category entries, then remove the factory of the component, should work - but doesn't. If there is an nsCategoryCache instance watching this category it will not remove this component and it will leak. Reason: the notification about category entry deletion happens asynchronously so that nsCategoryCache::EntryRemoved gets to run after nsIComponentRegistrar::unregisterFactory. But it doesn't "remember" the component and calls do_GetService - which fails because the component is no longer registered. So the component stays in the list. See attached extension for example. It implements nsIContentPolicy and adds itself to the content-policy category. Install and uninstall it - its content policy implementation has been removed (enumerating content-policy category confirms that) yet it is still active (dumping all content policy calls to console). Also, about:compartments shows a leaked compartment for testpolicy@adblockplus.org/bootstrap.js. I think that the solution would be to avoid the do_GetService call - nsCategoryCache needs to remember which component corresponds to which entry and remove it without looking it up again.
Updated•12 years ago
|
Whiteboard: [MemShrink]
Comment 1•12 years ago
|
||
If it only occurs when an add-on is uninstalled/disabled, it's a MemShrink:P3.
Whiteboard: [MemShrink] → [MemShrink:P3]
Comment 3•11 years ago
|
||
There is a testcase for this in bug 905357
Assignee | ||
Comment 4•11 years ago
|
||
Yeah ok, this is stupid and easy to fix.
Assignee: nobody → benjamin
Priority: -- → P2
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #814546 -
Flags: review?(nfroyd)
Reporter | ||
Comment 6•11 years ago
|
||
Comment on attachment 814546 [details] [diff] [review] nsCategoryCache implementation doesn't free old category entries if their contract mapping is removed using .unregisterFactory. Store the factory objects directly in the map, instead of keeping both a map and a separate list. Review of attachment 814546 [details] [diff] [review]: ----------------------------------------------------------------- Nice cleanup, only a nit from my end. ::: xpcom/glue/nsCategoryCache.cpp @@ +35,5 @@ > + while (NS_SUCCEEDED(strings->HasMore(&more)) && more) { > + nsAutoCString entryName; > + strings->GetNext(entryName); > + > + nsCString entryValue; Shouldn't this be nsAutoCString as well?
Comment 7•11 years ago
|
||
Comment on attachment 814546 [details] [diff] [review] nsCategoryCache implementation doesn't free old category entries if their contract mapping is removed using .unregisterFactory. Store the factory objects directly in the map, instead of keeping both a map and a separate list. Review of attachment 814546 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/nsCategoryCache.h @@ +20,5 @@ > > #include "nsXPCOM.h" > > +class NS_COM_GLUE nsCategoryObserver MOZ_FINAL : public nsIObserver > +{ Do we really need separate classes for the Observer and the Cache? Why not just have nsCategoryCache implement nsIObserver directly, with a flag to indicate that the delayed initialization has occurred? @@ +64,2 @@ > > + void GetEntries(nsCOMArray<T>& result) { Since all but one of the consumers of GetEntries() immediately iterate through the result, why don't we just return an iterator? (Let me guess... hash iterators aren't stable and an entry could end up being a callback that modifies the hash while we're iterating?)
Comment 8•11 years ago
|
||
Comment on attachment 814546 [details] [diff] [review] nsCategoryCache implementation doesn't free old category entries if their contract mapping is removed using .unregisterFactory. Store the factory objects directly in the map, instead of keeping both a map and a separate list. Review of attachment 814546 [details] [diff] [review]: ----------------------------------------------------------------- WFM.
Attachment #814546 -
Flags: review?(nfroyd) → review+
Comment 9•11 years ago
|
||
Thinking about this a little more, I think the underlying asynchronous inconsistency between CategoryManager and CategoryCache is the real problem here. I'm not sure if there's a reasonable way to redesign this, but I'm really uncomfortable with having an unpredictable amount of event handling go on between a client removing an entry from a category, and that change being reflected in a corresponding CategoryCache.
Assignee | ||
Comment 10•11 years ago
|
||
It is currently the way it is because the category manager is supposedly a threadsafe service. We could probably build the category cache directly into the internal platform and avoid deadlocks, but the arbitrary-notification problem makes that a poor idea in general. If we really wanted to fix this "right", we'd stop using categories and just have specific extensibility APIs. Or we'd make the category manager main-thread-only, but I don't know what side effects that would have. Anyway, I don't think any of that should block this change, which is pretty small in scope and unlikely to cause regressions.
Assignee | ||
Comment 11•11 years ago
|
||
> > + nsCString entryValue; > > Shouldn't this be nsAutoCString as well? No. entryValue is assigned via getter_Copies and so it won't ever use the stack buffer. > > +class NS_COM_GLUE nsCategoryObserver MOZ_FINAL : public nsIObserver > > +{ > > Do we really need separate classes for the Observer and the Cache? Why not > just have nsCategoryCache implement nsIObserver directly, with a flag to > indicate that the delayed initialization has occurred? I don't know why it is this way. Perhaps people reguarly create category observers but don't use them? > (Let me guess... hash iterators aren't stable and an entry could end up > being a callback that modifies the hash while we're iterating?) Precisely.
https://hg.mozilla.org/mozilla-central/rev/94bd7574eff1
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment 13•11 years ago
|
||
Comment on attachment 814546 [details] [diff] [review] nsCategoryCache implementation doesn't free old category entries if their contract mapping is removed using .unregisterFactory. Store the factory objects directly in the map, instead of keeping both a map and a separate list. [Approval Request Comment] Bug caused by (feature/regressing bug #): 853388 User impact if declined: May need to restart Firefox to complete Adblock Plus add-on upgrade; see bug 924340 Testing completed (on m-c, etc.): Has been on FF 27 for three weeks Risk to taking this patch (and alternatives if risky): Low String or IDL/UUID changes made by this patch: None
Attachment #814546 -
Flags: approval-mozilla-beta?
Assignee | ||
Comment 14•11 years ago
|
||
I'm a little worried about the risk of this patch, and I don't particularly support beta uplift. This is finicky code.
Reporter | ||
Comment 15•11 years ago
|
||
(In reply to :Irving Reid from comment #13) > User impact if declined: May need to restart Firefox to complete Adblock > Plus add-on upgrade; see bug 924340 Current Adblock Plus version already worked around the issue so updates will always succeed (even from versions without the fix). I don't really see the point of uplifting this - this bug has been sitting around for 17 months, it can wait another six weeks.
Comment 16•11 years ago
|
||
Comment on attachment 814546 [details] [diff] [review] nsCategoryCache implementation doesn't free old category entries if their contract mapping is removed using .unregisterFactory. Store the factory objects directly in the map, instead of keeping both a map and a separate list. Given the concern about risk here, the workaround, and that this is an old issue - let's let it ride the trains.
Attachment #814546 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
You need to log in
before you can comment on or make changes to this bug.
Description
•