901795 - Change the Preferences code not to leak if you register a weak pref observer on a pref which never changes

Status: RESOLVED WONTFIX

(Core :: Preferences: Backend, defect)

Description

[Justin Lebar (not reading bugmail)]

Just as the observer service leaks if you register a weak observer which never gets fired (bug 901753), the prefs service leaks if you register a weak pref observer which never runs (e.g. because the pref never changes).

The solution here may be more complicated than using a WeakSet, but I think the machinery in bug 901746 still makes this fixable.

Comment 1

[Justin Lebar (not reading bugmail)]

Wow, the prefs code is not optimal.

prefapi.cpp keeps a linked list of pref callbacks.  Every time we change a pref, we iterate over the whole thing -- because of the way the API is spec'ed, a pref observer for "foo." gets called whenever "foo.anything" is changed.  When we iterate over this big list, we don't remove any weak refs we happen upon.

Removing a pref observer is O(n) in the number of observers, which is pretty lame.

It seems to me that the right thing to do here is to merge the data structure kept by nsPrefBranch.cpp (a hashset of (nsIObserver, char*)) with the linked list kept by prefapi.cpp.

I'm not sure how much this matters, but the scenario I'm afraid of is that we have some short-lived object which registers a weak pref listener on some pref which never changes in practice (that's most or all prefs).  This could be particularly bad on B2G, as we'll build up weakrefs in this list over months, potentially, and this will slow down all other pref operations.

Comment 2

[Justin Lebar (not reading bugmail)]

It seems like optimally, we'd  use a trie or a radix tree to represent the pref observers.  I think this would make it efficient to find all observers which watch a given prefix.

Again, we should figure out how much this matters, but I'd like us not to get blindsided by a late-breaking B2G bug which tickles this.

Comment 3

[Justin Lebar (not reading bugmail)]

This isn't a simple bug, but it's a nicely self-contained project which might suit someone who enjoys working with data structures and is somewhat familiar with how our code works.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Justin what would be a simple test case for this, specifically, how do I track this leak. I'm not very familiar with B2G tooling for these things. Will it work on b2g-desktop or do I need a device?

NOTE: No immediate plans to take this bug.

Comment 5

[Justin Lebar (not reading bugmail)]

This is mostly a hypothetical thing at this point.  I don't know how to cause it, but I'm afraid that there's some path that we'll do 10,000 times in a B2G test in the future that will mess us up here.

Your question is a really good one, though.  As a prerequisite to this, we should write a memory reporter that counts the number of pref observers.  Even better would be to write a memory multi-reporter which reports separately

a) Strong pref observers.
b) Weak pref observers whose referent is alive.
c) Weak pref observers whose referent is dead.

We should do this for the observer service, too; that would have made bug 901416 obvious.

I'll file bugs for these.

Comment 6

[Miguel AF]

Hi, if it is fit for a complete newbie in the mozilla tree, I can take on this bug.

Comment 7

[Justin Lebar (not reading bugmail)]

I guess it depends on how good you are at C++ and figuring out large codebases.  :)  Maybe give it a try and let us know how it goes?  See also the discussion in bug 903285 for tips.

Comment 8

[Miguel AF]

Ok, i'll investigate it and get back to you in the next couple of days.

Comment 9

[Kamil Muszynski (:kamilm)]

Hi Miguel, are you still planning to work on this bug? If not, I would like to give it a try (I'm also a newbe :) ).

Comment 10

[Miguel AF]

Hi Kamil, go ahead. I've started to look at it a while ago but had to stop and havent been able to pick it up again and it will be a while until having free time again.
Cheers

Comment 11

[Kamil Muszynski (:kamilm)]

Turns out there is a lot I have to learn before fixing this bug, but I can do this if there is no close deadline for it.

Nikhil already asked a question about test scenario for this leak - Justin, can you give me some hint? I have latest mozilla-central build and I've found out that I can inspect the number of dead/alive weak references using about:memory page, but so far dead references counter shows 0 :) What I've tried is to change some preferences available in Edit->Preferences menu.

Comment 12

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Kamil,  jlebar unfortunately doesn't work on the project currently. Josh?

Comment 13

[Josh Matthews [:jdm]]

I apologize for taking so long to respond here. I don't know about any kind of automated test for this right now, but inside gBrowserInit.onLoad (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#739) you could create a pref listener for browser.tabs.remote and add it as a weak reference. I would expect opening up about:memory to show dead references after this (try pressing the GC/CC buttons if that's not true and see if anything changes).

Comment 14

[Kamil Muszynski (:kamilm)]

Josh, sorry it took me so long, but I needed to learn more about JS and XPCOM in general. I added the following code:

var myObserver = {

        observe : function observe(a,b,c){
            alert("Observing!");
        },

        QueryInterface  : XPCOMUtils.generateQI([Ci.nsISupportsWeakReference]),
    }
    gPrefService.addObserver("browser.tabs.remote", myObserver, true); 

to gBrowserInit.onLoad method. 

Now, when I open about:memory, I can see that there is one weak dead reference for preference service, and it stays that way even after GC/CC is executed manually - so I suppose this is the desired behavior. I will now start to figure out what prefapi.cpp does :)

Comment 15

[Josh Matthews [:jdm]]

Great; that looks right to me :) Please tag me with "Need more information from" if you have further questions!

Comment 16

[:Gijs (he/him)]

Kris, is this still current?

Comment 17

[Kris Wright :KrisWright]

It looks like the callsites mentioned don't exist anymore, and probably haven't existed in a while. We do support weak references in nsPrefBranch but I don't believe we leak them if we don't use them - this code has been largely rewritten and merged into libpref. I'm going to go ahead and resolve this as wontfix.