212598 - netwerk/ should not use nsIPref

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Darin Fisher]

netwerk/ should not use the deprecated nsIPref.  this blocks bug 175193.

Comment 1

[Darin Fisher]

Attachment: v1 patch

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 127678 [details] [diff] [review]
v1 patch


what part about this isn't thread safe? +  // XXX not threadsafe

I wonder what would happen if we ever had unsolicited notifications:

+nsProtocolProxyService::Observe(nsISupports	 *aSubject,
+				 const char	 *aTopic,
+				 const PRUnichar *aData)
+{
+    PrefsChanged(aTopic);


assuming it works...  r=dougt

Comment 3

[Darin Fisher]

doug: operator++ is not threadsafe.  we'd need to use PR_AtomicIncrement to make
that code threadsafe.  i don't think it currently matters any since the dir
index parser is only run on the UI thread, but the NS_IMPL_THREADSAFE_xxx macro
seems to suggest otherwise.

can there be unsolicited notifications?  that would just be a major bug in the
observer service, no?

Comment 4

[Darin Fisher]

Attachment: v1.1 patch

revised per comments from caillon over IRC:

1- use NS_PREFSERVICE_CONTRACTID instead of NS_PREFSERVICE_CID
2- make ns{FTP,Gopher}Channel::SetListFormat reject invalid values in prefs

Comment 5

[Darin Fisher]

Comment on attachment 127689 [details] [diff] [review]
v1.1 patch

minor changes over v1 patch... please see previous comments in bug for details.

Comment 6

[Doug Turner (:dougt)]

Comment on attachment 127689 [details] [diff] [review]
v1.1 patch

on unsolisted notifications -- there is nothing specifically ruling them out --
but clearly this would be a bigger problem than this bug.  

I don't see any reason to check for correctness on these pref values.

Comment 7

[Darin Fisher]

yeah, strictly checking for valid pref values shouldn't be a requirement;
however, it is easy to do in this case without costing us anything.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 127689 [details] [diff] [review]
v1.1 patch

>Index: base/src/nsProtocolProxyService.cpp
>+            prefsInt->AddObserver("network.proxy", this, PR_FALSE);
>Index: base/src/nsProtocolProxyService.h
>+    nsCOMPtr<nsIPrefBranch> mPrefs;

This gives you a reference cycle.

>Index: streamconv/converters/nsDirIndexParser.cpp
>+  if (!defCharset.IsEmpty())
>+    mEncoding.Assign(NS_ConvertUCS2toUTF8(defCharset).get());

CopyUTF16toUTF8(defCharset, mEncoding);

sr=me with those fixed.

Comment 9

[Darin Fisher]

fixed-on-trunk

Comment 10

[Andrew Schultz]

the checkin here added a printf to nsProtocolProxyService::PrefsChanged (I hit
it every time).  the patch to nsProtocolProxyService.cpp seems to do a lot of
stuff not contained in attachment 127689 [details] [diff] [review]...

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/netwerk/base/src&command=DIFF_FRAMESET&file=nsProtocolProxyService.cpp&rev1=1.47&rev2=1.48&root=/cvsroot

Comment 11

[Darin Fisher]

Andrew: the diffs between the current patch and the one that got checked in are
based on bz's comments (we discussed the diffs over IRC).  sorry about the
errant printf!