netwerk/ should not use nsIPref

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
15 years ago
15 years ago

People

(Reporter: Darin Fisher, Assigned: Darin Fisher)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

15 years ago
netwerk/ should not use the deprecated nsIPref.  this blocks bug 175193.
(Assignee)

Comment 1

15 years ago
Created attachment 127678 [details] [diff] [review]
v1 patch
(Assignee)

Updated

15 years ago
Attachment #127678 - Flags: review?(dougt)
(Assignee)

Updated

15 years ago
Attachment #127678 - Flags: superreview?(bzbarsky)

Comment 2

15 years ago
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
Attachment #127678 - Flags: review?(dougt) → review+
(Assignee)

Comment 3

15 years ago
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?
(Assignee)

Comment 4

15 years ago
Created attachment 127689 [details] [diff] [review]
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
Attachment #127678 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #127678 - Flags: superreview?(bzbarsky)
(Assignee)

Comment 5

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

minor changes over v1 patch... please see previous comments in bug for details.
Attachment #127689 - Flags: superreview?(bzbarsky)
Attachment #127689 - Flags: review?(dougt)

Comment 6

15 years ago
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.
Attachment #127689 - Flags: review?(dougt) → review+
(Assignee)

Comment 7

15 years ago
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 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.
Attachment #127689 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 9

15 years ago
fixed-on-trunk
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 10

15 years ago
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
(Assignee)

Comment 11

15 years ago
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!
You need to log in before you can comment on or make changes to this bug.