993203 - Report counts of settings observers in about:memory

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

This would have made finding https://bugzilla.mozilla.org/show_bug.cgi?id=990837#c15 much easier.

njn, if you're not comfortable reviewing this bounce it to Olli.

Comment 1

[Nicholas Nethercote [inactive]]

Comment on attachment 8403024 [details] [diff] [review]
Patch

Review of attachment 8403024 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/settings/SettingsManager.js
@@ +388,5 @@
> +      let path;
> +      if (length < 20) {
> +        path = "settings-observers";
> +      } else {
> +        path = "settings-observers-suspect/referent(topic=" + topic + ")";

I've been considering removing this suspect/non-suspect heuristic in the existing reporters, and just reporting everything. Because it's just so crude. What do you think?

@@ +414,5 @@
>    classID: Components.ID("{c40b1c70-00fb-11e2-a21f-0800200c9a66}"),
>    contractID: "@mozilla.org/settingsManager;1",
>    QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports,
> +                                         Ci.nsIDOMGlobalPropertyInitializer,
> +                                         Ci.nsIObserver,

I don't understand why you added Ci.nsIObserver.

Comment 2

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Nicholas Nethercote [:njn] from comment #1)
> Comment on attachment 8403024 [details] [diff] [review]
> Patch
> 
> Review of attachment 8403024 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/settings/SettingsManager.js
> @@ +388,5 @@
> > +      let path;
> > +      if (length < 20) {
> > +        path = "settings-observers";
> > +      } else {
> > +        path = "settings-observers-suspect/referent(topic=" + topic + ")";
> 
> I've been considering removing this suspect/non-suspect heuristic in the
> existing reporters, and just reporting everything. Because it's just so
> crude. What do you think?

There are a lot of different settings observers.  I didn't want to clutter up about:memory that much for the non-leaking case.

> @@ +414,5 @@
> >    classID: Components.ID("{c40b1c70-00fb-11e2-a21f-0800200c9a66}"),
> >    contractID: "@mozilla.org/settingsManager;1",
> >    QueryInterface: XPCOMUtils.generateQI([Ci.nsISupports,
> > +                                         Ci.nsIDOMGlobalPropertyInitializer,
> > +                                         Ci.nsIObserver,
> 
> I don't understand why you added Ci.nsIObserver.

This is something that should have been there before.  This object adds itself as an observer with the observer service, but relies on XPConnect type coercion rather than just declaring that it is an observer.

Comment 3

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/08aa13c6feac