385086 - null XPCOM refs in content pref services to prevent leaks

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect)

Description

[Myk Melez [:myk] [@mykmelez]]

After the content pref service got checked in, Rlk on fxdbug-linux-tbox went from 2.99KB to 3.07KB and then to 3.16KB.  It now fluctuates between the latter two numbers, whereas before it was fluctuating between the former two.

Per discussion on #developers, one issue is references to XPCOM components in the member variables of the two services defined by nsContentPrefService.js (the content pref service and the content URI grouper).  Those need to be nulled when the services are destroyed on shutdown.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: nulls XPCOM refs in content pref service

Here's a patch that nulls all those references on shutdown.  The only one it doesn't null is the _interfaces array of interfaces.  If those need to be nulled as well, then perhaps it'd be better simply to inline the interfaces in the QueryInterface method.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

Fix checked in to trunk:

Checking in toolkit/components/contentprefs/src/nsContentPrefService.js;
/cvsroot/mozilla/toolkit/components/contentprefs/src/nsContentPrefService.js,v  <--  nsContentPrefService.js
new revision: 1.2; previous revision: 1.1
done

Comment 3

[Adam Guthrie]

If this patch didn't fix the leaks, then is there any reason we shouldn't back it out?

Comment 4

[Myk Melez [:myk] [@mykmelez]]

In yesterday's conversation on IRC, some folks (brendan?) seemed to think it valuable to preemptively null references to XPCOM components even when doing so doesn't provide an immediately obvious benefit.

And at one point I think someone (sayrer?) also suggested that it would be valuable to null database statements to prevent them from hanging onto the database connection.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 268992 [details] [diff] [review]
patch v1: nulls XPCOM refs in content pref service

You might rather write:

  for (var i in this)
    this[i] = null;

I did favor this kind of code from an XPCOM-shutdown protocol, and argued (I'm a realist) that it's not superstitious given our current world of leaks.

But it would be nice to roll it up into a loop.

/be

Comment 6

[John P Baker]

I don't pretend to know enough about what is important here...
But as _observers and _genericObservers have the potential to still be referencing (large) singeleton objects that haven't had removeObserver() called on them, should they be cleared as well?

Comment 7

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #6)
> I don't pretend to know enough about what is important here...
> But as _observers and _genericObservers have the potential to still be
> referencing (large) singeleton objects that haven't had removeObserver() called
> on them, should they be cleared as well?

Yes, you're right, indeed they should.  Reopening to attach a patch for that as well.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: followup v1: removes observers, also fixes browser/ files, clarifies comments

Here's a followup patch that clears the observer data structures and also nulls XPCOM component references in the browser/base/content/ JS files that were added as part of the content prefs checkin.

This patch also modifies the comments describing the nulling to clarify that the nulling is preemptive/precautionary.

Comment 9

[Robert Sayre]

Comment on attachment 269251 [details] [diff] [review]
followup v1: removes observers, also fixes browser/ files, clarifies comments

Still leaking with this patch?

Comment 10

[Myk Melez [:myk] [@mykmelez]]

(In reply to comment #9)
> (From update of attachment 269251 [details] [diff] [review])
> Still leaking with this patch?

Yeah, unfortunately this doesn't actually fix the leak, which is expected, since at the moment the only consumer of the content prefs service is the text zoom controller, and that properly removes itself as an observer on browser shutdown.

This fix is more to make sure we don't leak if other consumers (perhaps in extensions) don't properly remove themselves in the future.

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Followup patch checked in:

Checking in toolkit/components/contentprefs/src/nsContentPrefService.js;
/cvsroot/mozilla/toolkit/components/contentprefs/src/nsContentPrefService.js,v  <--  nsContentPrefService.js
new revision: 1.3; previous revision: 1.2
done
Checking in browser/base/content/browser-contentPrefSink.js;
/cvsroot/mozilla/browser/base/content/browser-contentPrefSink.js,v  <--  browser-contentPrefSink.js
new revision: 1.2; previous revision: 1.1
done
Checking in browser/base/content/browser-textZoom.js;
/cvsroot/mozilla/browser/base/content/browser-textZoom.js,v  <--  browser-textZoom.js
new revision: 1.2; previous revision: 1.1
done

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Hrm, sorry about the churn, but somehow I missed Brendan's comment 5.  Not sure why.  I found the bugmail marked read in my Inbox, but I don't recall reading it.

I'll attach a followup patch that uses the approach he outlines.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: followup 2 v1: Brendan's suggestion

Here's a patch that implements Brendan's approach with the addition of a try-catch block to catch "setting a property that has only a getter" exceptions when the loop tries to null read-only properties.

Comment 14

[Robert Sayre]

Comment on attachment 269276 [details] [diff] [review]
followup 2 v1: Brendan's suggestion

oh, that is nicer. what a tough reviewer I am ;)

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Thanks Robert.  Hopefully this is the last iteration for this bug.  Fix checked in to trunk:

Checking in toolkit/components/contentprefs/src/nsContentPrefService.js;
/cvsroot/mozilla/toolkit/components/contentprefs/src/nsContentPrefService.js,v  <--  nsContentPrefService.js
new revision: 1.4; previous revision: 1.3
done
Checking in browser/base/content/browser-contentPrefSink.js;
/cvsroot/mozilla/browser/base/content/browser-contentPrefSink.js,v  <--  browser-contentPrefSink.js
new revision: 1.3; previous revision: 1.2
done
Checking in browser/base/content/browser-textZoom.js;
/cvsroot/mozilla/browser/base/content/browser-textZoom.js,v  <--  browser-textZoom.js
new revision: 1.3; previous revision: 1.2
done