Closed
Bug 660770
Opened 13 years ago
Closed 13 years ago
caps should use mozilla::Preferences
Categories
(Core :: Security: CAPS, defect)
Core
Security: CAPS
Tracking
()
RESOLVED
FIXED
mozilla7
People
(Reporter: masayuki, Assigned: masayuki)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
23.47 KB,
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
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+
Assignee | ||
Comment 2•13 years ago
|
||
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)
Assignee | ||
Comment 3•13 years ago
|
||
Comment on attachment 536256 [details] [diff] [review] Patch v1.0 Stop requesting review due to bug 660640.
Attachment #536256 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 4•13 years ago
|
||
Attachment #536256 -
Attachment is obsolete: true
Attachment #538190 -
Flags: review?(bzbarsky)
Comment 5•13 years ago
|
||
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.
Assignee | ||
Comment 6•13 years ago
|
||
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 7•13 years ago
|
||
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+
Assignee | ||
Comment 8•13 years ago
|
||
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.
Assignee | ||
Comment 9•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/a8e2f5953d1c
Whiteboard: [inbound]
Comment 10•13 years ago
|
||
Merged: http://hg.mozilla.org/mozilla-central/rev/a8e2f5953d1c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → mozilla7
Assignee | ||
Updated•13 years ago
|
Attachment #538190 -
Flags: review?(bzbarsky)
Comment 11•12 years ago
|
||
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...
Assignee | ||
Comment 12•12 years ago
|
||
(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.
Description
•