Tristan Bourvon Assignee
	Description • 7 years ago

As part of the static analysis effort to ensure all class fields are initialized before usage (see https://bugzilla.mozilla.org/show_bug.cgi?id=525063), here are a few fixes to the security/manager module.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 2 • 7 years ago

Comment on attachment 8923165 [details] [diff] [review]
security_manager.patch

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

Hi Tristan - thanks for the patch. Please re-submit it with 8 lines of unified context and following the style guidelines at https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style (in particular, the "C++ classes" section, which illustrates how we format initialization lists). See also https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#Commit_Message_Conventions

::: security/manager/ssl/DataStorage.cpp
@@ +73,3 @@
>  DataStorage::DataStorage(const nsString& aFilename)
>    : mMutex("DataStorage::mMutex")
> +  , mTimerDelay{0}, mPendingWrite(false)

This should be sDataStorageDefaultTimerDelay. Also, let's use value initialization (i.e. "()" instead of "{}"). Additionally, these each need to be on their own line.

::: security/manager/ssl/ScopedNSSTypes.h
@@ +109,4 @@
>  {
>  public:
>    Digest()
> +  : mItemBuf() {

nit: '{' on its own line

	Tristan Bourvon Assignee
	Comment 3 • 7 years ago

Sorry about the previous low quality patch, we have such a huge amount of patches for this type of issue that I tried to rush things and forgot about code standards. Hope this is better.

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 4 • 7 years ago

Comment on attachment 8923762 [details] [diff] [review]
security_manager.patch

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

This is great - thanks!
(It would be best to have 8 lines of unified context, but in this case I imagine we should be fine.)
For posterity, I was intending my tone in comment 2 to be informative and straightforward, but perhaps I was too stern. I appreciate you taking the time to fix this bug.

	Tristan Bourvon Assignee
	Comment 5 • 7 years ago

(In reply to David Keeler [:keeler] (use needinfo?) from comment #4)
> Comment on attachment 8923762 [details] [diff] [review]
> security_manager.patch
> 
> Review of attachment 8923762 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is great - thanks!
> (It would be best to have 8 lines of unified context, but in this case I
> imagine we should be fine.)
> For posterity, I was intending my tone in comment 2 to be informative and
> straightforward, but perhaps I was too stern. I appreciate you taking the
> time to fix this bug.

Don't worry, I didn't take it as something else than a good review on a neutral tone. As for the unified context, I'm not sure I understand what's wrong... I see 4 lines of context before and after each change, doesn't that make 8 lines of context? Or maybe you meant 8 on both sides?

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 6 • 7 years ago

Yes - 8 on both sides. Your .hgrc (or equivalent) should have something like this:

[diff]
git = 1
showfunc = 1
unified = 8

[defaults]
diff = -p -U 8

(I don't actually know if that's redundant - they say almost the same thing.)

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 7 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=42eec9b379c13caef9650b2371faf5eb4985286c

	Dana Keeler (she/her) (use needinfo) (:keeler for reviews)
	Comment 8 • 7 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/202ff4e53892c94131974daf3b9637fac064e558

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 9 • 7 years ago

https://hg.mozilla.org/mozilla-central/rev/202ff4e53892

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 7 years ago

Please request Beta approval on this when you get a chance.

	Tristan Bourvon Assignee
	Comment 11 • 7 years ago

Comment on attachment 8923762 [details] [diff] [review]
security_manager.patch

Approval Request Comment
[Feature/Bug causing the regression]: Uninitialized members
[User impact if declined]: Possible (unconfirmed) security issues
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Builds and passes tests, no noticeable perf regression
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Only some specific code relying on UB would be affected by this change, and tests seem to indicate that no problems have been introduced
[String changes made/needed]: N/A

	Julien Cristau [:jcristau]
	Comment 12 • 7 years ago

Comment on attachment 8923762 [details] [diff] [review]
security_manager.patch

2 weeks in nightly, let's try this on beta.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 13 • 7 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/ec25006fd49d