Last Comment Bug 660770 - caps should use mozilla::Preferences
: caps should use mozilla::Preferences
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Masayuki Nakano [:masayuki] (Mozilla Japan)
:
: Selena Deckelmann :selenamarie :selena use ni?
Mentors:
Depends on: 731122 656826 699529
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-31 02:05 PDT by Masayuki Nakano [:masayuki] (Mozilla Japan)
Modified: 2012-05-07 18:19 PDT (History)
5 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1.0 (15.82 KB, patch)
2011-05-31 02:05 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
roc: review+
Details | Diff | Splinter Review
Patch v2.0 (23.47 KB, patch)
2011-06-08 23:34 PDT, Masayuki Nakano [:masayuki] (Mozilla Japan)
jst: review+
Details | Diff | Splinter Review

Description Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-05-31 02:05:20 PDT
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.
Comment 1 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-31 04:10:56 PDT
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] (Mozilla Japan) 2011-05-31 16:48:02 PDT
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] (Mozilla Japan) 2011-05-31 19:26:22 PDT
Comment on attachment 536256 [details] [diff] [review]
Patch v1.0

Stop requesting review due to bug 660640.
Comment 4 Masayuki Nakano [:masayuki] (Mozilla Japan) 2011-06-08 23:34:09 PDT
Created attachment 538190 [details] [diff] [review]
Patch v2.0
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2011-06-09 22:39:56 PDT
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] (Mozilla Japan) 2011-06-09 23:07:00 PDT
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, jst@mozilla.com) 2011-06-17 15:04:52 PDT
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] (Mozilla Japan) 2011-06-19 08:07:00 PDT
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] (Mozilla Japan) 2011-06-19 20:02:11 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/a8e2f5953d1c
Comment 10 Mounir Lamouri (:mounir) 2011-06-20 08:52:49 PDT
Merged:
http://hg.mozilla.org/mozilla-central/rev/a8e2f5953d1c
Comment 11 neil@parkwaycc.co.uk 2012-05-07 15:39:59 PDT
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] (Mozilla Japan) 2012-05-07 18:19:10 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.