660770 - caps should use mozilla::Preferences

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v1.0

Does the patch need additional review due to security related code?

nsCodeBasePrefObserver doesn't need now because it can use Preferences::AddBoolVarCache().

And also Preferences::GetService() and Preferences::GetRootBranch() should initialize the static members.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 536256 [details] [diff] [review]
Patch v1.0

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

Hmm, you probably should get module owner review for this one and maybe some of the others.

Looks like another case of not releasing observers...

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 536256 [details] [diff] [review]
Patch v1.0

nsScriptSecurityManager adds pref observers but it doesn't release them...

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 536256 [details] [diff] [review]
Patch v1.0

Stop requesting review due to bug 660640.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Patch v2.0

Comment 5

[Boris Zbarsky [:bzbarsky]]

I'd really appreciate it if you could find someone else (jst?) to review this... I can do it otherwise, but it may be at least a week.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 538190 [details] [diff] [review]
Patch v2.0

Okay, Boris. jst, do you have much time for reviewing this?

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 538190 [details] [diff] [review]
Patch v2.0

This looks good to me, but I have one question:

-        nsXPIDLCString id;
-        if (NS_FAILED(mPrefBranch->GetCharPref(aPrefNames[c], getter_Copies(id))))
+        nsAdoptingCString id = Preferences::GetCString(aPrefNames[c]);
+        if (!id) {

One of the things that's different between nsXPIDL[C]String and our other strings is that nsXPIDL[C]String can hold a null value, and thus test false in a check like if (!str), but I'm not sure that that's the case with the string that's returned by Preferences::GetCString(). r=jst on this patch, but if my question leads to something that needs changed, then no. :)

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

nsAdopting[C]String is an inherited class of nsXPISL[C]String.

Preferences::Get[C]String() returns NULL contained string if the result of nsIPrefBranch::GetCharPref() failed. If it returned empty string, Preferences::Get[C]String().get() were not return but empty.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/a8e2f5953d1c

Comment 10

[Mounir Lamouri (:mounir)]

Merged:
http://hg.mozilla.org/mozilla-central/rev/a8e2f5953d1c

Comment 11

[[e-mail]]

Comment on attachment 538190 [details] [diff] [review]
Patch v2.0

> nsScriptSecurityManager::~nsScriptSecurityManager(void)
> {
>+    Preferences::RemoveObservers(this, kObservedPrefs);
Removing preferences in your destructor makes no sense. In particular, when you have added yourself as a strong observer, you can't be destroyed until your observer has been removed...

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to [e-mail] from comment #11)
> Comment on attachment 538190 [details] [diff] [review]
> Patch v2.0
> 
> > nsScriptSecurityManager::~nsScriptSecurityManager(void)
> > {
> >+    Preferences::RemoveObservers(this, kObservedPrefs);
> Removing preferences in your destructor makes no sense. In particular, when
> you have added yourself as a strong observer, you can't be destroyed until
> your observer has been removed...

Ah, you're right.