The new preference to override default port blocking network.security.ports.banned.override (bug 85601) works when inserted into the default all.js file - for example pref("network.security.ports.banned.override", "1080");. This setting lets me seeing any http://foo:1080 URL However setting this pref in the profile (either prefs.js or user.js) as user_pref("network.security.ports.banned.override", "1080"); does not have any effect. That is a warning about banned port pops up. The code to read this pref is in /mozilla/netwerk/base/src/nsIOService.cpp. I cannot guess why it works only with all.js. Do prefs from the profile files need registering?

Setting severity to major as prefs in all.js are subject to overwriting with every Mozilla installation and adding mozilla0.9.3 as it seems very easy to fix (if someone understands the prefs code).

->security?

Why security? What is the reason of a setting to override the default port blocking if an user cannot use them to see a page that has a "banned" port number? Actually there should be an UI setting for this but it is the focus of bug 85601. I believe therefore that this bug is blocking 85601 - marking accordingly. If someone disagrees, plese clear the block.

Over to dougt.

this is kind of pref problem... or you are not using the correct syntax in your user.js file. Try using "user_pref" instead of "pref"

Dough, please read the bug :-( One uses user_pref(); in prefs.js or user.js and pref(); in /defaults/pref/all.js. Please notice the correct use of both setting in my original comment. I used both exactly to avoid suggestions like that... In meantme I asked off-page Darin Fisher who saw similar behavior in another bug. He suggests that some necko components do not register themselves as profile/pref change observers and therefore have no way of detecting differences between prefs.js and all.js if the component were loaded before a profile is selected.

Samir, why does this work only in all.js? What am I doing wrong?

dougt - based on the comments from darin, I suspect you need to register yourself as a pref observer, with a callback. See the code in nsHttpHandler::Init()

ugh. you are right... the profilemanager reads these in AFTER the ioserver has started.... crap. This probably wont be fixed until after 0.9.3 unless someone submits a patch.

relnote added to commercial release notes, and I'll add one to the 0.9.4 relnote bug when one is created.

people are annoyed that they can't do this. moving to 9.6

Patch registers nsIOService with prefs. Following Darin's lead in bug 102332, it also listens for xpcom-shutdown so that the prefs observers can be removed -- otherwise everything leaks.

Comment on attachment 52120 [details] [diff] [review] patch >Index: nsIOService.cpp >@@ -205,50 +208,23 @@ >+ // Further modifications to the port list come from prefs >+ nsCOMPtr<nsIPrefBranch> prefBranch; >+ GetPrefBranch(getter_AddRefs(prefBranch)); >+ if (prefBranch) { >+ nsCOMPtr<nsIPrefBranchInternal> pbi = do_QueryInterface(prefBranch); >+ if (pbi) >+ pbi->AddObserver(NETWORK_PORT_PREFS, this); >+ PrefsChanged(prefBranch); > } > looks like you have an indentation problem here. >+void >+nsIOService::PrefsChanged(nsIPrefBranch *prefs, const char *pref) >+{ >+ if (!prefs) return; >+ >+ nsresult rv = NS_OK; >+ nsXPIDLCString portList; >+ >+ // Look for extra ports to block >+ if ((pref == nsnull) || PL_strcmp(pref, "network.security.ports.banned") == 0) { >+ rv = prefs->GetCharPref("network.security.ports.banned", >+ getter_Copies(portList)); >+ if (portList) { >+ char* tokp; >+ char* currentPos = (char *)portList.get(); >+ while ( (tokp = nsCRT::strtok(currentPos, ",", &currentPos)) != nsnull ) >+ { >+ nsCAutoString tmp(tokp); >+ tmp.StripWhitespace(); >+ >+ PRInt32 aErrorCode; >+ PRInt32 value = tmp.ToInteger(&aErrorCode); >+ mRestrictedPortList.AppendElement((void*)value); >+ } >+ >+ } >+ } >+ // ...as well as previous blocks to remove. >+ if ((pref == nsnull) || PL_strcmp(pref, "network.security.ports.banned.override") == 0) { >+ rv = prefs->GetCharPref("network.security.ports.banned.override", >+ getter_Copies(portList)); >+ if (portList) { >+ char* tokp; >+ char* currentPos = (char *)portList.get(); >+ while ( (tokp = nsCRT::strtok(currentPos, ",", &currentPos)) != nsnull ) >+ { >+ nsCAutoString tmp(tokp); >+ tmp.StripWhitespace(); >+ >+ PRInt32 aErrorCode; >+ PRInt32 value = tmp.ToInteger(&aErrorCode); >+ mRestrictedPortList.RemoveElement((void*)value); >+ } >+ >+ } >+ } >+} this routine needs to be cleaned up... do not duplicate code like this. either use a loop or a helper function. >+// nsIObserver interface >+NS_IMETHODIMP >+nsIOService::Observe(nsISupports *subject, >+ const PRUnichar *topic, >+ const PRUnichar *data) >+{ >+ if (!nsCRT::strcmp(topic, NS_LITERAL_STRING("nsPref:changed").get())) { >+ nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(subject); >+ if (prefBranch) >+ PrefsChanged(prefBranch, NS_ConvertUCS2toUTF8(data).get()); >+ } >+ else if (!nsCRT::strcmp(topic, NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID).get())) { >+ // Clean up the observers >+ nsCOMPtr<nsIPrefBranch> prefBranch; >+ GetPrefBranch(getter_AddRefs(prefBranch)); >+ if (prefBranch) { >+ nsCOMPtr<nsIPrefBranchInternal> pbi = do_QueryInterface(prefBranch); >+ if (pbi) >+ pbi->RemoveObserver(NETWORK_PORT_PREFS, this); >+ } >+ >+ nsCOMPtr<nsIObserverService> observerService = >+ do_GetService(NS_OBSERVERSERVICE_CONTRACTID); >+ if (observerService) >+ observerService->RemoveObserver(this, NS_LITERAL_STRING(NS_XPCOM_SHUTDOWN_OBSERVER_ID).get()); I recall there being a bug that you can't remove a nsIObserver from the observer service during its Observe method. HTTP implements nsISupportsWeakReference, and the observer service is implemented such that it'll actually hold only a weak reference to the observer if it supports weak referencing. This is why HTTP does not remove itself from the observer service at xpcom-shutdown time. >+ } >+ return NS_OK; >+} >Index: nsIOService.h >=================================================================== >RCS file: /cvsroot/mozilla/netwerk/base/src/nsIOService.h,v >retrieving revision 1.35 >diff -u -r1.35 nsIOService.h >--- nsIOService.h 2001/09/28 20:08:59 1.35 >+++ nsIOService.h 2001/10/04 22:47:15 >@@ -51,12 +51,16 @@ > #include "nsIEventQueueService.h" > #include "nsIURLParser.h" > #include "nsSupportsArray.h" >+#include "nsIObserver.h" > > #define NS_N(x) (sizeof(x)/sizeof(*x)) > > static const char *gScheme[] = {"chrome", "resource", "file", "http"}; > >+class nsIPrefBranch; >+ > class nsIOService : public nsIIOService >+ , public nsIObserver > { > public: > NS_DECL_ISUPPORTS >@@ -64,6 +68,9 @@ > // nsIIOService methods: > NS_DECL_NSIIOSERVICE > >+ // nsIObserver methods: >+ NS_DECL_NSIOBSERVER >+ > // nsIOService methods: > nsIOService(); > virtual ~nsIOService(); >@@ -88,6 +95,10 @@ > > NS_METHOD CacheURLParser(const char *scheme, > nsIURLParser* hdlr); >+ >+ // Prefs >+ void PrefsChanged(nsIPrefBranch *prefs, const char *pref = nsnull); >+ void GetPrefBranch(nsIPrefBranch **); > > protected: > PRBool mOffline;

> looks like you have an indentation problem here. oops. > this routine needs to be cleaned up... This is the original code, so I wasn't going to muck with it much. However, this would be a good opportunity to tighten it up. > I recall there being a bug that you can't remove a nsIObserver from the > observer service during its Observe method. It looks like bug 94349 and bug 68200 mention this. I'll add weak reference support.

Comment on attachment 52211 [details] [diff] [review] v2 sr=darin (good job cleaning this up!)

Thanks for the quick reviews. When the trunk unfreezes, I'll need someone to check this in for me.

Checking in nsIOService.cpp; /cvsroot/mozilla/netwerk/base/src/nsIOService.cpp,v <-- nsIOService.cpp new revision: 1.115; previous revision: 1.114 done Checking in nsIOService.h; /cvsroot/mozilla/netwerk/base/src/nsIOService.h,v <-- nsIOService.h new revision: 1.36; previous revision: 1.35 done Thanks alot for fixing this problem!

Marking verified as per above developer comments.