103883 - [MLK] Using Preference observers is prone to reference cycles

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect)

Description

[Brian Nesse (gone)]

As a number of people have discovered, I think I have cc'd you all, registering
as a prefs observer is prone to causing cyclical references with the preferences
branch. Based on some discussions that stemmed from bug 102332 I have worked up
a patch in an attempt to reduce these cycles.

The patch takes a 2 pronged approach. First, the nsPrefBranch object now listens
for the xpcom-shutdown message and releases all of the observers in its list.
This releases the observing object from having to do the same, which appears to
be the direction current patches are heading. Second, I have added a parameter
to the AddObserver call |aHoldWeak|. If the observing object supports weak
reference and passes in PR_TRUE for this parameter the nsPrefBranch object will
hold a weak reference to the object in its observer list.

Hopefully I will get the patch attached before Darin and crew add the
xpcom-shutdown fix to any more files in the netwerk library. ;)

Comment 1

[Brian Nesse (gone)]

Attachment: Proposed patch

Comment 2

[Darin Fisher]

i gave the patch a once over and it looks pretty good to me.  r=darin on the
libpref and necko changes.

Comment 3

[Mitchell Stoltz (not reading bugmail)]

Question: even if the observing object doesn't support weak references,
releasing the observers on xpcom-shutdown should break the reference cycle, right?

Comment 4

[Darin Fisher]

mstolz: yup.. however, waiting for xpcom-shutdown may not be appropriate for all
obsevers.  one could imagine a short lived object that Adds itself as a pref
observer in its constructor and removes itself in its destructor... code like
this would only work if the prefbranch supported weak references.

Comment 5

[Alec Flett]

Comment on attachment 52744 [details] [diff] [review]
Proposed patch

there have been debates about putting html markup in  comments..not that 
you've done that per say, but you've put <true> and <false> in brackets. true 
or |true| should be fine..

in freeObserverList, domain should be nsAutoCString.
Also, I moved nsCharsetMenu to xpfe/intl, though I think 
I haven't yet removed the one in intl/uconv/src it from the mac build.. 
any chance you can remove it from the project? :)
sr=alecf with those changes

Comment 6

[Brian Nesse (gone)]

I was wondering why there were 2 copies of nsCharsetMenu.cpp. I'd be more than
happy to remove it... I was very, very scared when I found the second one... :)

I'll kill the brackets... I was going to revisit the documentation anyway. I
still want to add a note about holding a weak reference to the prefBranch object
and notate the work covered by this patch.

Comment 7

[Brian Nesse (gone)]

Attachment: Patch reflecting alec's comments and better IDL documentation.

Comment 8

[Brian Nesse (gone)]

Mitch, could you please review the nsScriptSecurityManager changes?

Gordon, can you review the nsCacheService changes?

Seth, can you review the nsDBFolderInfo changes, or suggest someone on your team 
who can?

And, finally, can someone (there's no clear owner) review the nsCharSetMenu 
changes? Thanks.

Comment 9

[gordon]

r=gordon for the changes to nsDNSService.cpp and nsCacheService.cpp.

Comment 10

[Brian Nesse (gone)]

Attachment: Fixed one bug in RemoveObserver() as per email with Darin.

Comment 11

[Conrad Carlen (not reading bugmail)]

I'm so glad this is being done :-)
r=ccarlen on changes to nsCharsetMenu.cpp

One thing, though not added in this patch, there are quite a few occurences:
Shouldn't NS_PREFBRANCH_PREFCHANGE_OBSERVER_ID be called
NS_PREFBRANCH_PREFCHANGE_TOPIC_ID? That would be more readable as well as more
accurate.

Comment 12

[Brian Nesse (gone)]

That's a good suggestion. Since I just added the define, this would be the time
to change it. I'm going to have to regenerate the patch anyway... I just
discovered some JS exceptions being thrown. :( I'm not sure why I didn't notice
them before...

Comment 13

[Alec Flett]

coolness. I'm with conrad, and I'm glad we caught it early..

Comment 14

[Mitchell Stoltz (not reading bugmail)]

r=mstoltz on the change to nsScriptSecurityManager.

Comment 15

[Alec Flett]

Comment on attachment 52968 [details] [diff] [review]
Fixed one bug in RemoveObserver() as per email with Darin.

actually marking sr=alecf

Comment 16

[(not reading, please use [e-mail] instead)]

the mailnews changes seem ok.

instead of :

if (pls != NULL) {
}

why not:

if (pls) {
}

Comment 17

[(not reading, please use [e-mail] instead)]

also, do we want to check the return value from

+      rv = prefBranch->GetComplexValue(kMAILNEWS_VIEW_DEFAULT_CHARSET,
+                      NS_GET_IID(nsIPrefLocalizedString), getter_AddRefs(pls));

or is checking if pls is non null the right thing to do?

I'd think we'd check rv, something like

if (NS_SUCCEEDED(rv)) {
}

or

if (NS_SUCCEEDED(rv) && pls) {
|

Comment 18

[Alec Flett]

bnesse, I think we need to make sure that getComplexValue() follows the same
semantics as QueryInterface - i.e. a success always means that the result has a
value... then we should be checking if (NS_SUCCEEDED(rv))

Comment 19

[Brian Nesse (gone)]

Seth, thanks for the input, I will clean up accordingly. Alec, yes, success ==
valid value... I'll double check that an empty preference is returned as a 0
length string, but I'm pretty sure it is.

Comment 20

[Brian Nesse (gone)]

Attachment: New patch reflecting review input and JavaScript changes.

Comment 21

[Joe Hewitt (gone)]

r=hewitt for the changes to PrefUtils.js, InspectorSidebar.js, and
urlbarBindings.xml.  Although, I should note that InspectorSidebar.js is a dead
file that isn't used.  I should just cvs remove it...

Comment 22

[Alec Flett]

cc'ing dougt who is doing this change for the observer service... we should make
sure the semantics of this are the same. I have no strong feelings one way or
the other for "holdweak" vs. "holdstrong"

Comment 23

[Brian Nesse (gone)]

Agreed. I went with holdweak because the "default" functionality for the prefs
service is to "hold strong", because that's the only kind of hold it supported.

Unfortunately the default (existing) functionality of ObserverService is to
"hold weak" if the object supports it...

Personally, I feel that holdweak is better because it singles out what I
consider to be the special case condition (i.e. I support weak reference and I
wish to be held that way).

Comment 24

[Srilatha Moturi]

In MsgComposeCommands.js:
   How about declaring a global pbi and initilaizing it in ComposeLoad() instead 
of declaring it in several functions.
+  var pbi = prefs.QueryInterface(Components.interfaces.nsIPrefBranchInternal);

Other than this, changes in MsgComposeCommands.js, navigator.js, fastnav.js 
looks ok to me.

Comment 25

[Doug Turner (:dougt)]

brian, I am with you.  I will be adding a new parameter to
nsIObserverService::AddObserver called holdWeak to that we will have a
consistant looking api.

Comment 26

[Alec Flett]

regarding srilatha's comment: the convention is to say:
const nsIPrefBranchInternal = Components.interfaces.nsIPrefBranchInternal

that way when you're reading code that later uses this, it is obvious that it's
a reference to an interface. (Also note the use of "const")

Comment 27

[Darin Fisher]

i vote for "holdweak" :-)

Comment 28

[Doug Turner (:dougt)]

what darin said. :-)

Comment 29

[Brian Nesse (gone)]

Attachment: Final patch reflecting srilatha's comments

Comment 30

[Alec Flett]

Comment on attachment 54211 [details] [diff] [review]
Final patch reflecting srilatha's comments

cool, looks good.. thanks for switching these files over to nsIPrefBranch, 
too. Make sure to test the heck out of compose, on the off chance that there 
are not other .js files in this window using the global "prefs" variable.

Comment 31

[Srilatha Moturi]

r=srilatha on the changes in MsgComposeCommands.js, fastnav.js and navigator.js

Comment 32

[Brian Nesse (gone)]

Fix is in. Hopefully we'll see a drop in leaks. :)

Comment 33

[jag (Peter Annema)]

Comment on attachment 54211 [details] [diff] [review]
Final patch reflecting srilatha's comments

>Index: mozilla/xpfe/browser/resources/content/navigator.js
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/navigator.js,v
>retrieving revision 1.380
>diff -u -2 -r1.380 navigator.js
>--- navigator.js	2001/10/13 02:26:30	1.380
>+++ navigator.js	2001/10/18 19:36:24
>@@ -205,5 +205,7 @@
> {
>   try {
>-    pref.addObserver(this.domain, this);
>+    var pbi = pref.QueryInterface(Components.interfaces.nsIPrefBranchInternal);
>+    if (pbi)
>+      pbi.addObserver(this.domain, this, false);

No need for the |if (pbi)|, the QI will throw an exception when it fails.

>   } catch(ex) {
>     dump("Failed to observe prefs: " + ex + "\n");
>@@ -276,4 +278,11 @@
>       throw "couldn't create a browser instance";
> 
>+    // Get the preferences service
>+    var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                .getService(Components.interfaces.nsIPrefService);
>+    if (!prefService)
>+      throw "couldn't create a preferences service";

No need to throw here manually, .getService() will throw when it fails.

>+    pref = prefService.getBranch(null);
>+
>     webNavigation = getWebNavigation();
>     if (!webNavigation)
>@@ -448,6 +457,8 @@
> 
>   // unregister us as a pref listener
>-  pref.removeObserver(window.buttonPrefListener.domain,
>-                      window.buttonPrefListener);
>+  var pbi = pref.QueryInterface(Components.interfaces.nsIPrefBranchInternal);
>+  if (pbi)
>+    pbi.removeObserver(window.buttonPrefListener.domain,
>+                       window.buttonPrefListener);

No need for the |if (pbi)|, the QI will throw an exception when it fails.

>@@ -950,5 +962,5 @@
>     BrowserViewSourceOfURL(url.replace(/^view-source:/, ""), null);
>   } else {
>-    if (pref && pref.GetBoolPref("browser.tabs.opentabfor.urlbar") && getBrowser().localName == "tabbrowser") {
>+    if (pref && pref.getBoolPref("browser.tabs.opentabfor.urlbar") && getBrowser().localName == "tabbrowser") {
>       var t = getBrowser().addTab(getShortcutOrURI(url)); // open link in new tab
>       getBrowser().selectedTab = t;
>@@ -1368,5 +1380,7 @@
>       chromeRegistry.uninstallSkin( themeName.getAttribute("name"), true );
>       // XXX - this sucks and should only be temporary.
>-      pref.SetUnicharPref("general.skins.removelist." + themeName.getAttribute("name"), true);
>+      pref.setComplexValue("general.skins.removelist." + themeName.getAttribute("name"),
>+                           Components.interfaces.nsISupportsWString,
>+                           true);
>       
>       if (inUse)
>@@ -1380,5 +1394,7 @@
>   // we STILL haven't fixed editor skin switch problems
>   // hacking around it yet again
>-  pref.SetUnicharPref("general.skins.selectedSkin", themeName.getAttribute("name"));
>+  pref.setComplexValue("general.skins.selectedSkin",
>+                       Components.interfaces.nsISupportsWString,
>+                       themeName.getAttribute("name"));
> 
>   var observerService = Components.classes["@mozilla.org/observer-service;1"].getService(Components.interfaces.nsIObserverService);

For some reason this code doesn't work. When trying to switch to another
theme from the view menu, I got this error:

JavaScript error:
 line 0: uncaught exception: [Exception... "Could not convert JavaScript
argument arg 2 [nsIPrefBranch.setComplexValue]"  nsresult: "0x80570009
(NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame ::
chrome://navigator/content/navigator.js :: applyTheme :: line 1393"  data: no]

Comment 34

[Brian Nesse (gone)]

jag, see bug 106159. Could this could be the reverse of that? nsIPref global
stomping on nsIPrefBranch somehow...

Reopening until we get a resolution...

Comment 35

[Alec Flett]

I just realized that the reason the last bit doesn't work (the setComplexValue)
is that we need to actually create an nsISupportsWString and set the data in it,
and pass THAT into setComplexValue - XPConnect isn't smart enough (and it won't
ever be :)) to make that conversion...

Comment 36

[Brian Nesse (gone)]

Attachment: Patch which eliminates error, addresses jag's comments, and fixes theme switching.

Comment 37

[R.K.Aa.]

could this be the reason for bug 106309 as well?

Comment 38

[Brian Nesse (gone)]

I doubt it. I don't really see what the connection would be bug 106309 doesn't
appear to have any preference related issues.

Comment 39

[Brian Nesse (gone)]

Can I get r/sr on attachment 54737 [details] [diff] [review] so I can land the fix for the theme switch
bug? Thanks.

Comment 40

[Alec Flett]

Comment on attachment 54737 [details] [diff] [review]
Patch which eliminates error, addresses jag's comments, and fixes theme switching.

looks almost right, but you're going to cause some JS warnings because you're not declaring "str"

try turning on JS warnings in the "Debug" panel - you'll be amazed at what you see :)

Comment 41

[Brian Nesse (gone)]

Crap... I've had this little voice in the back of my head all day telling me I
forgot to put a "var" somewhere.... these are the only files I didn't double
check. :(

Comment 42

[Brian Nesse (gone)]

Attachment: Better patch with no JS warnings :)

Comment 43

[Alec Flett]

Comment on attachment 54787 [details] [diff] [review]
Better patch with no JS warnings :)

sr=alecf

Comment 44

[jag (Peter Annema)]

Comment on attachment 54787 [details] [diff] [review]
Better patch with no JS warnings :)

r=jag

Comment 45

[Brian Nesse (gone)]

Fix checked in.