caps should use mozilla::Preferences

RESOLVED FIXED in mozilla7

Status

()

Core
Security: CAPS
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

(Depends on: 1 bug)

Trunk
mozilla7
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Created attachment 536256 [details] [diff] [review]
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.
Attachment #536256 - Flags: review?(roc)
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...
Attachment #536256 - Flags: review?(roc) → review+
Comment on attachment 536256 [details] [diff] [review]
Patch v1.0

nsScriptSecurityManager adds pref observers but it doesn't release them...
Attachment #536256 - Flags: review?(bzbarsky)
Comment on attachment 536256 [details] [diff] [review]
Patch v1.0

Stop requesting review due to bug 660640.
Attachment #536256 - Flags: review?(bzbarsky)
Created attachment 538190 [details] [diff] [review]
Patch v2.0
Attachment #536256 - Attachment is obsolete: true
Attachment #538190 - Flags: review?(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 on attachment 538190 [details] [diff] [review]
Patch v2.0

Okay, Boris. jst, do you have much time for reviewing this?
Attachment #538190 - Flags: review?(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. :)
Attachment #538190 - Flags: review?(jst) → review+
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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/a8e2f5953d1c
Whiteboard: [inbound]
Merged:
http://hg.mozilla.org/mozilla-central/rev/a8e2f5953d1c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Attachment #538190 - Flags: review?(bzbarsky)
Depends on: 731122
Depends on: 699529
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...
(In reply to neil@parkwaycc.co.uk 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.
You need to log in before you can comment on or make changes to this bug.