Closed Bug 753687 Opened 12 years ago Closed 11 years ago

nsCategoryCache implementation invites memory leaks

Categories

(Core :: XPCOM, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: jwkbugzilla, Assigned: benjamin)

References

Details

(Whiteboard: [MemShrink:P3])

Attachments

(2 files)

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.
Whiteboard: [MemShrink]
If it only occurs when an add-on is uninstalled/disabled, it's a MemShrink:P3.
Whiteboard: [MemShrink] → [MemShrink:P3]
There is a testcase for this in bug 905357
Yeah ok, this is stupid and easy to fix.
Assignee: nobody → benjamin
Priority: -- → P2
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 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 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+
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.
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.
> > +    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 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?
I'm a little worried about the risk of this patch, and I don't particularly support beta uplift. This is finicky code.
(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 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.