Closed Bug 1282408 Opened 4 years ago Closed 3 years ago

[Static Analysis][Clang-Plugin] Ignore class members initialisation check

Categories

(Firefox Build System :: Source Code Analysis, defect)

defect
Not set

Tracking

(firefox52 fixed)

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: sec-audit, Whiteboard: [post-critsmash-triage][adv-main52-])

Attachments

(19 files, 36 obsolete files)

1.04 KB, patch
paul.bignier
: review?
dcamp
Details | Diff | Splinter Review
882 bytes, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.71 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.69 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.49 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
860 bytes, patch
mccr8
: review+
Details | Diff | Splinter Review
790 bytes, patch
baku
: review+
Details | Diff | Splinter Review
1.13 KB, patch
dbaron
: review+
Details | Diff | Splinter Review
883 bytes, patch
jorendorff
: review+
Details | Diff | Splinter Review
1.25 KB, patch
bevis
: review+
Details | Diff | Splinter Review
1017 bytes, patch
jgilbert
: review+
Details | Diff | Splinter Review
791 bytes, patch
bevis
: review+
Details | Diff | Splinter Review
1022 bytes, patch
jorendorff
: review+
Details | Diff | Splinter Review
4.78 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
805 bytes, patch
valentin
: review+
Details | Diff | Splinter Review
1.44 KB, patch
jst
: review-
Details | Diff | Splinter Review
918 bytes, patch
jst
: review-
Details | Diff | Splinter Review
1023 bytes, patch
Details | Diff | Splinter Review
1.50 KB, patch
Details | Diff | Splinter Review
As for some member variables we want to ignore it's initialisation check performed by clang-plugin - https://bugzilla.mozilla.org/show_bug.cgi?id=525063
We've added a flag for the declaration of each variable that we want to be ignored.
On this bug there will be posted patches of this kind.
No longer blocks: coverity-analysis
Blocks: 525063
Group: core-security
MozReview-Commit-ID: BNA5ANQJjS1
Attachment #8765437 - Flags: review?(wmccloskey)
(In reply to Andi-Bogdan Postelnicu from comment #1)
> Created attachment 8765437 [details] [diff] [review]
> add ignore initialization check flag for variables from RunnableMethod
> 
> MozReview-Commit-ID: BNA5ANQJjS1

We don't need to initialise the variables in the default constructor since they will be overwritten in in Init function so we just need to add ignore flag in order to be skipped by clang-plugin.
Group: core-security → core-security-release
MozReview-Commit-ID: GpIcq2CNa8N
Attachment #8765842 - Flags: review?(jdemooij)
(In reply to Andi-Bogdan Postelnicu from comment #3)
> Created attachment 8765842 [details] [diff] [review]
> add ignore initialization check flag for variables from FunctionValidator
> 
> MozReview-Commit-ID: GpIcq2CNa8N

the add of this flag is for our static analysis tool - clang plugin in order to signal it to eliminate the variable from analysis.
MozReview-Commit-ID: 40yR3oLZhkH
Attachment #8765844 - Flags: review?(jdemooij)
Attachment #8765844 - Attachment description: add ignore initialization check flag for variables from InternalLink → Bug 1282408 - add ignore initialization check flag for variables from InternalLink
Attachment #8765437 - Attachment description: add ignore initialization check flag for variables from RunnableMethod → Bug 1282408 - add ignore initialization check flag for variables from RunnableMethod
Attachment #8765842 - Attachment description: add ignore initialization check flag for variables from FunctionValidator → Bug 1282408 - add ignore initialization check flag for variables from FunctionValidator
Comment on attachment 8765842 [details] [diff] [review]
Bug 1282408 - add ignore initialization check flag for variables from FunctionValidator

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

::: js/src/asmjs/AsmJS.cpp
@@ +2826,1 @@
>      ExprType          ret_;

MOZ_INITIALIZED_OUTSIDE_CONSTRUCTOR is quite verbose, let's just initialize this to ExprType::Limit in the constructor. Add ret_(ExprType::Limit) after hasAlreadyReturned_(false). r=me with that.
Attachment #8765842 - Flags: review?(jdemooij) → review+
Comment on attachment 8765844 [details] [diff] [review]
Bug 1282408 - add ignore initialization check flag for variables from InternalLink

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

::: js/src/asmjs/WasmModule.h
@@ +62,1 @@
>          uint32_t targetOffset;

Nit: slight preference for having these on the same line, I think:

MOZ_INITIALIZED_OUTSIDE_CONSTRUCTOR uint32_t patchAtOffset;
MOZ_INITIALIZED_OUTSIDE_CONSTRUCTOR uint32_t targetOffset;

Problem is that MOZ_INITIALIZED_OUTSIDE_CONSTRUCTOR is really long. Can't we change it to something like MOZ_INIT_OUTSIDE_CTOR?

Thanks for working on this! Static analysis is great.
Attachment #8765844 - Flags: review?(jdemooij) → review+
Comment on attachment 8765842 [details] [diff] [review]
Bug 1282408 - add ignore initialization check flag for variables from FunctionValidator

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab05556d326
Attachment #8765842 - Flags: review+
(In reply to Jan de Mooij [:jandem] from comment #7)
> Comment on attachment 8765844 [details] [diff] [review]
> Bug 1282408 - add ignore initialization check flag for variables from
> InternalLink
> 
> Review of attachment 8765844 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/asmjs/WasmModule.h
> @@ +62,1 @@
> >          uint32_t targetOffset;
> 
> Nit: slight preference for having these on the same line, I think:
> 
> MOZ_INITIALIZED_OUTSIDE_CONSTRUCTOR uint32_t patchAtOffset;
> MOZ_INITIALIZED_OUTSIDE_CONSTRUCTOR uint32_t targetOffset;
> 
> Problem is that MOZ_INITIALIZED_OUTSIDE_CONSTRUCTOR is really long. Can't we
> change it to something like MOZ_INIT_OUTSIDE_CTOR?
> 
Sure i can do this, i will have to re-land a patch for renaming the prefixer.

> Thanks for working on this! Static analysis is great.
MozReview-Commit-ID: 40yR3oLZhkH
Attachment #8765891 - Flags: review?(jdemooij)
Attachment #8765844 - Attachment is obsolete: true
MozReview-Commit-ID: BNA5ANQJjS1
Attachment #8765901 - Flags: review?(wmccloskey)
Attachment #8765437 - Attachment is obsolete: true
Attachment #8765437 - Flags: review?(wmccloskey)
Comment on attachment 8765891 [details] [diff] [review]
add ignore initialization check flag for variables from InternalLink

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

Thanks!

::: js/src/asmjs/WasmModule.h
@@ +55,4 @@
>              RawPointer,
>              CodeLabel,
>              InstructionImmediate
> +        };	

Nit: trailing whitespace here
Attachment #8765891 - Flags: review?(jdemooij) → review+
Comment on attachment 8765901 [details] [diff] [review]
add ignore initialization check flag for variables from RunnableMethod

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

Could you just get rid of the Init method and do the work in the constructor instead? I don't see any reason why that wouldn't work.
Attachment #8765901 - Flags: review?(wmccloskey)
Attached patch Bug 1282408_3.patch (obsolete) — Splinter Review
I would help for our static analysis clang based tool to have this variable marked as skipped since the initialisation is performed in Init function
Attachment #8765842 - Attachment is obsolete: true
Attachment #8765901 - Attachment is obsolete: true
Attachment #8766283 - Flags: review?(dbaron)
Attached patch Bug 1282408_4.patch (obsolete) — Splinter Review
We need to mark these variables to ignore list for our static analysis tool.
Attachment #8766288 - Flags: review?(jst)
Comment on attachment 8766283 [details] [diff] [review]
Bug 1282408_3.patch

>+    MOZ_INIT_OUTSIDE_CTOR
>     uint8_t       mOrigin;        // [reset] See nsStyleConsts.h

Please indent the "uint8_t" (but not the "mOrigin") by 2 more spaces so that it's clear this is one logical line.

r=dbaron with that
Attachment #8766283 - Flags: review?(dbaron) → review+
Attachment #8766288 - Flags: review?(jst) → review+
https://hg.mozilla.org/mozilla-central/rev/8ab05556d326
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
grr, sorry missed the leave-open flag
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8765891 [details] [diff] [review]
add ignore initialization check flag for variables from InternalLink

https://hg.mozilla.org/integration/mozilla-inbound/rev/4230f642c60c
Attachment #8765891 - Flags: review+
Blocks: clang-plugin
No longer blocks: 525063
Blocks: 525063
Blocks: 1287752
MozReview-Commit-ID: 1KAlxDtZyMe
Attachment #8774695 - Flags: review?(jmuizelaar)
Comment on attachment 8774695 [details] [diff] [review]
add ignore initialization check flag for variables from GlyphRunIterator

we can mark them to be ignored since they are initialised in gfxTextRun::GlyphRunIterator::NextRun(), and this gets called each time the object gets created.
MozReview-Commit-ID: BZC7SnvmOKl
Attachment #8774702 - Flags: review?(jmuizelaar)
MozReview-Commit-ID: 1jd0JbeVapq
Attachment #8774704 - Flags: review?(jorendorff)
Comment on attachment 8774704 [details] [diff] [review]
add ignore initialization check flag for variables from ElemSegment

The reason why default constructor is present is only to make this class compatible with Vector template so ignoring the initialisation check i think it's ok.
MozReview-Commit-ID: 3Ldof83eL0P
Attachment #8774727 - Flags: review?(bzbarsky)
Comment on attachment 8774727 [details] [diff] [review]
add ignore initialization check flag for mGenerateComputedGridInfo from SharedGridData

I think we could ignore this variable from member initialisation check since it's assigned a value here:
>>      sharedGridData->mAbsPosItems.SwapElements(gridReflowInput.mAbsPosItems);
>>
>>      sharedGridData->mGenerateComputedGridInfo =
>>          HasAnyStateBits(NS_STATE_GRID_GENERATE_COMPUTED_VALUES);
MozReview-Commit-ID: 71zQBOwH6tS
Attachment #8774738 - Flags: review?(jorendorff)
Comment on attachment 8774738 [details] [diff] [review]
add ignore initialization check flag for pod from FuncExport

I think we could ignore this sine the default ctor is only to make it compatible with Vector
Comment on attachment 8774727 [details] [diff] [review]
add ignore initialization check flag for mGenerateComputedGridInfo from SharedGridData

Brad, does this make sense?  You know this code way better than I do.  ;)
Attachment #8774727 - Flags: review?(bzbarsky) → review?(bwerth)
(In reply to Boris Zbarsky [:bz] from comment #32)
> Comment on attachment 8774727 [details] [diff] [review]
> add ignore initialization check flag for mGenerateComputedGridInfo from
> SharedGridData
> 
> Brad, does this make sense?  You know this code way better than I do.  ;)

That would work and is correct, but I think the intent will be clearer if the variable is given a default false value. I'll supply a patch that does that.
Depends on: 1289509
Comment on attachment 8774727 [details] [diff] [review]
add ignore initialization check flag for mGenerateComputedGridInfo from SharedGridData

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

mGenerateComputedGridInfo should be given a default value. Will fix in bug 1289509.
Attachment #8774727 - Flags: review?(bwerth) → review-
Comment on attachment 8774695 [details] [diff] [review]
add ignore initialization check flag for variables from GlyphRunIterator

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

Please have these on the same line as the variable they impact.
Attachment #8774695 - Flags: review?(jmuizelaar) → review-
Attachment #8774702 - Flags: review?(jmuizelaar) → review+
Attachment #8774727 - Flags: review-
MozReview-Commit-ID: 1KAlxDtZyMe
Attachment #8775045 - Flags: review?(jmuizelaar)
Attachment #8774695 - Attachment is obsolete: true
Attachment #8774702 - Attachment is obsolete: true
Attachment #8774727 - Attachment is obsolete: true
Attachment #8766288 - Attachment is obsolete: true
Attachment #8766283 - Attachment is obsolete: true
Attachment #8765891 - Attachment is obsolete: true
Only pushing this file because it's the only "Uninitialized scalar field" defect in the "embedding" module.
Attachment #8775107 - Flags: review?(bpostelnicu)
Comment on attachment 8775107 [details] [diff] [review]
0001-Coverity-749957-Add-ignore-initialization-check-flag.patch

Forwarding this to jst since he's the reviewer. But for what it matters LGTM.
Attachment #8775107 - Flags: review?(bpostelnicu) → review?(jst)
Only pushing this file because it's the only "Uninitialized scalar field" defect in the "content" module.
Attachment #8775117 - Flags: review?(bzbarsky)
(In reply to Paul Bignier from comment #39)
> Created attachment 8775117 [details] [diff] [review]
> 0001-Coverity-749902-Add-ignore-initialization-check-flag.patch
> 
> Only pushing this file because it's the only "Uninitialized scalar field"
> defect in the "content" module.

Sorry I had sorted the report wrong, this isn't the only "content" defect, I will update my patch!
MozReview-Commit-ID: L5ymS8HUczi
Attachment #8775121 - Flags: review?(jorendorff)
Now, these two files are the only "Uninitialized scalar field" defects in the "dom/base/" module that are fixed that way. The two other defects require ctor init so I will push them in a separate commit.
Attachment #8775117 - Attachment is obsolete: true
Attachment #8775117 - Flags: review?(bzbarsky)
Attachment #8775130 - Flags: review?(bzbarsky)
MozReview-Commit-ID: DW4whzEYdbl
Attachment #8775139 - Flags: review?(jmuizelaar)
Comment on attachment 8775139 [details] [diff] [review]
add ignore initialization check flag for mProcessToken from GPUProcessManager

This variable should be ignored since it's first assigned a value in OnProcessLaunchComplete and i don't think there is a change that NotifyRemoteActorDestroyed comes before it.
MozReview-Commit-ID: L5ymS8HUczi
Attachment #8775141 - Flags: review?(jorendorff)
Attachment #8775121 - Attachment is obsolete: true
Attachment #8775121 - Flags: review?(jorendorff)
Comment on attachment 8775141 [details] [diff] [review]
add ignore initialization check flag for alwaysBaseline from CompileArgs

I think |alwaysBaseline| can be ignored since initFromContext is always called after the object gets created and so it's assigned a value there.
Comment on attachment 8775130 [details] [diff] [review]
0001-Coverity-749902-749787-Add-ignore-initialization-che.patch

I guess this is OK.  The fact that lastWasFirst always gets initialized is not too obvious, but I guess it works out if InitializeSortState is never passed a null container...
Attachment #8775130 - Flags: review?(bzbarsky) → review+
Comment on attachment 8775107 [details] [diff] [review]
0001-Coverity-749957-Add-ignore-initialization-check-flag.patch

I'm fine with this change, but would it not be better to simply initialize these variables to zero in the nsWindowWatcher constructor? Seems unlikely that'll mean anything in terms of performance.
Attachment #8775045 - Flags: review?(jmuizelaar) → review+
Attachment #8775139 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8775045 [details] [diff] [review]
add ignore initialization check flag for variables from GlyphRunIterator

https://hg.mozilla.org/integration/mozilla-inbound/rev/b12ffedac685
Attachment #8775045 - Attachment is obsolete: true
Comment on attachment 8775139 [details] [diff] [review]
add ignore initialization check flag for mProcessToken from GPUProcessManager

https://hg.mozilla.org/integration/mozilla-inbound/rev/f6de256937ac
Attachment #8775139 - Attachment is obsolete: true
Comment on attachment 8775107 [details] [diff] [review]
0001-Coverity-749957-Add-ignore-initialization-check-flag.patch

https://bugzilla.mozilla.org/show_bug.cgi?id=1290015
Attachment #8775107 - Attachment is obsolete: true
Attachment #8775107 - Flags: review?(jst)
MozReview-Commit-ID: 6IDxHLbSkoJ
Attachment #8775519 - Flags: review?(dbaron)
Comment on attachment 8775519 [details] [diff] [review]
add ignore initialization check flag for bool variables from ScrollReflowInput

We could just ignore these variables for our static analysis checker since they are assigned values through reflow functionality.
Attachment #8775519 - Flags: review?(dbaron) → review+
Comment on attachment 8775519 [details] [diff] [review]
add ignore initialization check flag for bool variables from ScrollReflowInput

 https://hg.mozilla.org/integration/mozilla-inbound/rev/d8b267ae7f69
Attachment #8775519 - Attachment is obsolete: true
MozReview-Commit-ID: 22VI0j7QWbL
Attachment #8775560 - Flags: review?(dbaron)
Comment on attachment 8775560 [details] [diff] [review]
add ignore initialization check flag for some variables from ReflowInput

These variables are assigned in the init function.
Attachment #8775577 - Flags: review?(surkov.alexander)
Attachment #8775577 - Flags: review?(surkov.alexander) → review+
Comment on attachment 8775560 [details] [diff] [review]
add ignore initialization check flag for some variables from ReflowInput

r=dbaron, although I wonder if it would be more useful to have the ability to mark additional methods as part of the required initialization sequence of a class (so that they have to be called before other methods are)
Attachment #8775560 - Flags: review?(dbaron) → review+
Comment on attachment 8775560 [details] [diff] [review]
add ignore initialization check flag for some variables from ReflowInput

 https://hg.mozilla.org/integration/mozilla-inbound/rev/81d77e3cdd5f
Attachment #8775560 - Attachment is obsolete: true
(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #60)
> Comment on attachment 8775560 [details] [diff] [review]
> add ignore initialization check flag for some variables from ReflowInput
> 
> r=dbaron, although I wonder if it would be more useful to have the ability
> to mark additional methods as part of the required initialization sequence
> of a class (so that they have to be called before other methods are)

This is a great idea, by having the possibility of marking functions that are used for initialisation we can check them during analysis and see if they are initialised/ assigned values in those function as well.
I will have this implemented in https://bugzilla.mozilla.org/show_bug.cgi?id=525063
MozReview-Commit-ID: 2B0bPOi1Tde
Attachment #8783946 - Flags: review?(jdemooij)
MozReview-Commit-ID: 9AYyHN5a5Wf
Attachment #8783955 - Flags: review?(gpascutto)
MozReview-Commit-ID: 4DrIUUf4tuk
Attachment #8783956 - Flags: review?(nfroyd)
MozReview-Commit-ID: AqiQ8wrtWvQ
Attachment #8783963 - Flags: review?(rjesup)
Attachment #8783956 - Flags: review?(nfroyd) → review+
Attachment #8783963 - Flags: review?(rjesup) → review+
Comment on attachment 8783955 [details] [diff] [review]
add ignore initialization check flag for some variables in RemoveFolderTransaction

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

Wrong reviewer. My best guess would be mak but he's not open to reviews (despite being the storage component owner).
Attachment #8783955 - Flags: review?(gpascutto) → review?(bugmail)
Comment on attachment 8783955 [details] [diff] [review]
add ignore initialization check flag for some variables in RemoveFolderTransaction

This is Places code, which I guess isn't a module (anymore)?  Redirecting to :adw based on timezone and hg log for toolkit/components/places.
Attachment #8783955 - Flags: review?(bugmail) → review?(adw)
Attachment #8783955 - Flags: review?(adw) → review+
Comment on attachment 8783963 [details] [diff] [review]
add ignore initialisation check flag for mTrackID in MediaEngineWebRTCMicrophoneSource

https://hg.mozilla.org/integration/mozilla-inbound/rev/1def0962cf4c
Attachment #8783963 - Attachment is obsolete: true
Comment on attachment 8783956 [details] [diff] [review]
add ignore initialization check flag for mAnyMarked in FixWeakMappingGrayBitsTracer

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cdf880fc41c
Attachment #8783956 - Attachment is obsolete: true
Comment on attachment 8783955 [details] [diff] [review]
add ignore initialization check flag for some variables in RemoveFolderTransaction

https://hg.mozilla.org/integration/mozilla-inbound/rev/62332e479c4d
Attachment #8783955 - Attachment is obsolete: true
Comment on attachment 8783946 [details] [diff] [review]
add ignore initialization check flag for mPseudoStackHack in ImageBridgeThread

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

hg annotate suggests Tom is a better reviewer.
Attachment #8783946 - Flags: review?(jdemooij) → review?(ttromey)
Comment on attachment 8774704 [details] [diff] [review]
add ignore initialization check flag for variables from ElemSegment

These are all wasm code, Luke can review those.
Attachment #8774704 - Flags: review?(jorendorff) → review?(luke)
Attachment #8774738 - Flags: review?(jorendorff) → review?(luke)
Attachment #8775141 - Flags: review?(jorendorff) → review?(luke)
Attachment #8774704 - Flags: review?(luke) → review+
Attachment #8774738 - Flags: review?(luke) → review+
Attachment #8775141 - Flags: review?(luke) → review+
Comment on attachment 8783946 [details] [diff] [review]
add ignore initialization check flag for mPseudoStackHack in ImageBridgeThread

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

Thank you.  This patch is fine.

I do see that bug 1215265 was fixed.  The only purpose for mPseudoStackHack was to work around this bug.
So, another approach might be to just remove this code -- if it works, this would be preferable.
It's ok by me if you chose not to do this though.
Attachment #8783946 - Flags: review?(ttromey) → review+
Comment on attachment 8774738 [details] [diff] [review]
add ignore initialization check flag for pod from FuncExport

 https://hg.mozilla.org/integration/mozilla-inbound/rev/9ecc07164c2e
Attachment #8774738 - Attachment is obsolete: true
Comment on attachment 8783946 [details] [diff] [review]
add ignore initialization check flag for mPseudoStackHack in ImageBridgeThread

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0f066daf3e
Attachment #8783946 - Attachment is obsolete: true
Comment on attachment 8774704 [details] [diff] [review]
add ignore initialization check flag for variables from ElemSegment

https://hg.mozilla.org/integration/mozilla-inbound/rev/09b84bcc720b
Attachment #8774704 - Attachment is obsolete: true
MozReview-Commit-ID: KreWU3RBSnU
Attachment #8786330 - Flags: review?(jorendorff)
MozReview-Commit-ID: BfEWPfVU0dj
Attachment #8786345 - Flags: review?(jorendorff)
MozReview-Commit-ID: IzLeG9MueUt
Attachment #8787093 - Flags: review?(jorendorff)
Comment on attachment 8790295 [details] [diff] [review]
add ignore initialization check flag for mDashOffset mStrokeWidth from SVGContextPaint

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

This seems fine to me but I'm not really familiar with SVGContextPaint. Ideally jwatt would review but since he is away this month, perhaps Daniel can have a quick check (since I believe he reviewed the original patches that introduced this class).
Attachment #8790295 - Flags: review?(bbirtles) → review?(dholbert)
Attachment #8790304 - Flags: review?(bbirtles) → review?(dholbert)
MozReview-Commit-ID: GRWqLr1pW3q
Attachment #8790645 - Flags: review?(valentin.gosu)
Attachment #8790645 - Flags: review?(valentin.gosu) → review?(mcmanus)
Comment on attachment 8790295 [details] [diff] [review]
add ignore initialization check flag for mDashOffset mStrokeWidth from SVGContextPaint

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

::: layout/svg/SVGContextPaint.h
@@ +83,5 @@
>  
>  private:
>    FallibleTArray<gfxFloat> mDashes;
> +  MOZ_INIT_OUTSIDE_CTOR gfxFloat mDashOffset;
> +  MOZ_INIT_OUTSIDE_CTOR gfxFloat mStrokeWidth;

Please insert a comment just after "private", to hint at how/where these are initialized. e.g.:

 // Member-vars are populated/initialized in InitStrokeGeometry.
Attachment #8790295 - Flags: review?(dholbert) → review+
Comment on attachment 8790304 [details] [diff] [review]
add ignore initialization check flag for mFrame from Paint

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

Commit message nit:
> Bug 1282408 - add ignore initialization check flag for mFrame from Paint

s/from Paint/from SVGContextPaintImpl::Paint/

(It's nice to be specific about which of probably-many "Paint" things we're talking about.)

::: layout/svg/SVGContextPaint.h
@@ +166,5 @@
>        SVGContextPaint* mContextPaint;
>        nscolor mColor;
>      } mPaintDefinition;
>  
> +    MOZ_INIT_OUTSIDE_CTOR nsIFrame* mFrame;

As above, please add a comment to hint at why/where this is lazily initialized; something like:

 // Initialized (if needed) in SetPaintServer():

...or perhaps:

 // Only used if we have paint-type eStyleSVGPaintType_Server:
Attachment #8790304 - Flags: review?(dholbert) → review+
Attachment #8790295 - Attachment is obsolete: true
Attachment #8790304 - Attachment is obsolete: true
Comment on attachment 8790699 [details] [diff] [review]
add ignore initialization check flag for mDashOffset mStrokeWidth from SVGContextPaint

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

Thanks. r=me
Attachment #8790699 - Flags: review?(dholbert) → review+
Comment on attachment 8790702 [details] [diff] [review]
add ignore initialization check flag for mFrame from SVGContextPaintImpl::Paint

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

Looks good; r=me
Attachment #8790702 - Flags: review?(dholbert) → review+
Comment on attachment 8790645 [details] [diff] [review]
add ignore initialization check flag for variables in AltSvcMapping

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

why are nsCString members included here? They definitely should have their default c++ ctor run to make a empty string and the code expects that..
MozReview-Commit-ID: GRWqLr1pW3q
Attachment #8790731 - Flags: review?(mcmanus)
Attachment #8790645 - Attachment is obsolete: true
Attachment #8790645 - Flags: review?(mcmanus)
Attachment #8790731 - Flags: review?(mcmanus) → review+
Comment on attachment 8790731 [details] [diff] [review]
add ignore initialization check flag for variables in AltSvcMapping

https://hg.mozilla.org/integration/mozilla-inbound/rev/4a0dca1cb72d
Attachment #8790731 - Attachment is obsolete: true
Comment on attachment 8775141 [details] [diff] [review]
add ignore initialization check flag for alwaysBaseline from CompileArgs

https://hg.mozilla.org/integration/mozilla-inbound/rev/c02571f1396f
Attachment #8775141 - Attachment is obsolete: true
Comment on attachment 8790702 [details] [diff] [review]
add ignore initialization check flag for mFrame from SVGContextPaintImpl::Paint

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f69f6c8c9ca
Attachment #8790702 - Attachment is obsolete: true
Comment on attachment 8775577 [details] [diff] [review]
0001-Static-Analysis-Uninitialized-scalar-field-In-explic.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/497b1f1f2a39
Attachment #8775577 - Attachment is obsolete: true
Comment on attachment 8775130 [details] [diff] [review]
0001-Coverity-749902-749787-Add-ignore-initialization-che.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/b6884127a9bf
Attachment #8775130 - Attachment is obsolete: true
Comment on attachment 8790699 [details] [diff] [review]
add ignore initialization check flag for mDashOffset mStrokeWidth from SVGContextPaint

https://hg.mozilla.org/integration/mozilla-inbound/rev/54969d0d94e8
Attachment #8790699 - Attachment is obsolete: true
MozReview-Commit-ID: D9DQT52JfNd
Attachment #8791536 - Flags: review?(longsonr)
Comment on attachment 8791536 [details] [diff] [review]
add ignore initialization check for member variables from SVGContextPaint

r=me but hasn't this already been reviewed by dholbert and landed.
Attachment #8791536 - Flags: review?(longsonr) → review+
Comment on attachment 8791536 [details] [diff] [review]
add ignore initialization check for member variables from SVGContextPaint

indeed this was already landed.
Attachment #8791536 - Attachment is obsolete: true
Comment on attachment 8791622 [details] [diff] [review]
add ignore initialization check flag for mSourceGraphicIndex and mSourceAlphaIndex from nsSVGFilterInstance

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

r=me, just a few editorial nits:

Firstly, some commit-message nits:
> Bug 1282408 - add ignore initialization check flag for mSourceGraphicIndex and mSourceAlphaIndex from nsSVGFilterInstance

The beginning here (everything up to "for") feels clumsy & is kind of hard to visually parse, because nearly every word in there is (or could be read as) a verb, which makes it hard to figure out what's actually doing whta.

Several suggestions:
 (1) Use hyphens in "ignore-initialization-check", to make it clearer that's grouped as a single adjective phrase (describing "flag")
 (2) s/flag/annotation/, I think?  ("flag" is imprecise, because we call boolean variables, bits, about:config settings, #ifdefs, and many other things "flags".  "Annotation" is more precise and excludes all of those possibilities.)
 (3) s/for/to/ feels slightly better, grammatically

So, I think this should look like:
 Add ignore-initialization-check annotation to mSourceGraphicIndex and mSourceAlphaIndex from nsSVGFilterInstance

That's easier for me to read, at least.

::: layout/svg/nsSVGFilterInstance.h
@@ +254,5 @@
>  
>    /**
>     * The index of the FilterPrimitiveDescription that this SVG filter should use
>     * as its SourceGraphic, or the SourceGraphic keyword index if this is the
> +   * first filter in a chain. Initialized in BuildPrimitives

Thanks for mentioning where it's initialized!

Please add a period at the end of the new text, though.

@@ +261,5 @@
>  
>    /**
>     * The index of the FilterPrimitiveDescription that this SVG filter should use
>     * as its SourceAlpha, or the SourceAlpha keyword index if this is the first
> +   * filter in a chain. Initialized in BuildPrimitives

(and add a period here as well)
Attachment #8791622 - Flags: review?(dholbert) → review+
(if you agree with my commit message nits, you might want to apply them to the other patches here as well, since they all seem to use similar wording)
Also, it looks like maybe you're marking patches as obsolete after you land them?  (comment 8, comment 19, comment 103)

Please don't do that -- and please un-obsolete any such patches.

(Patches should be obsoleted when they're discarded or replaced with a newly-uploaded version, but not when they're landed.  It's useful to be able to go back to a bug and look at the bugzilla-hosted versions of its patches (the ones that were reviewed), and that's hard to do when they're auto-hidden in the "obsolete" section and mixed up with actually-legitimately-obsolete patches.)
MozReview-Commit-ID: FFgmluIyvbs
Attachment #8791937 - Flags: review?(continuation)
(In reply to Daniel Holbert [:dholbert] from comment #109)
> (if you agree with my commit message nits, you might want to apply them to
> the other patches here as well, since they all seem to use similar wording)

(In reply to Daniel Holbert [:dholbert] from comment #108)
> Comment on attachment 8791622 [details] [diff] [review]
> add ignore initialization check flag for mSourceGraphicIndex and
> mSourceAlphaIndex from nsSVGFilterInstance
> 
> Review of attachment 8791622 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, just a few editorial nits:
> 
> Firstly, some commit-message nits:
> > Bug 1282408 - add ignore initialization check flag for mSourceGraphicIndex and mSourceAlphaIndex from nsSVGFilterInstance
> 
> The beginning here (everything up to "for") feels clumsy & is kind of hard
> to visually parse, because nearly every word in there is (or could be read
> as) a verb, which makes it hard to figure out what's actually doing whta.
> 
> Several suggestions:
>  (1) Use hyphens in "ignore-initialization-check", to make it clearer that's
> grouped as a single adjective phrase (describing "flag")
>  (2) s/flag/annotation/, I think?  ("flag" is imprecise, because we call
> boolean variables, bits, about:config settings, #ifdefs, and many other
> things "flags".  "Annotation" is more precise and excludes all of those
> possibilities.)
>  (3) s/for/to/ feels slightly better, grammatically
> 
> So, I think this should look like:
>  Add ignore-initialization-check annotation to mSourceGraphicIndex and
> mSourceAlphaIndex from nsSVGFilterInstance
> 
Totally agree with this one! Future patches will follow this schema.
(In reply to Daniel Holbert [:dholbert] from comment #110)
> Also, it looks like maybe you're marking patches as obsolete after you land
> them?  (comment 8, comment 19, comment 103)
> 
> Please don't do that -- and please un-obsolete any such patches.
> 
> (Patches should be obsoleted when they're discarded or replaced with a
> newly-uploaded version, but not when they're landed.  It's useful to be able
> to go back to a bug and look at the bugzilla-hosted versions of its patches
> (the ones that were reviewed), and that's hard to do when they're
> auto-hidden in the "obsolete" section and mixed up with
> actually-legitimately-obsolete patches.)

yes i was marking the patches obsolete since it's way easier to keep track of the ones that have been pushed to m-c. Any suggestion is more than welcome!
(In reply to Andi-Bogdan Postelnicu from comment #113)
> yes i was marking the patches obsolete since it's way easier to keep track
> of the ones that have been pushed to m-c. Any suggestion is more than
> welcome!

Some people use the checkin flag for that, so after you push a patch you can mark it as checkin+.
great will do that!
Comment on attachment 8791937 [details] [diff] [review]
Add ignore-initialization-check annotation to mCurrPi from CCGraphBuilder

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

You could instead initialize mCurrPi to null in the ctor.
Attachment #8791937 - Flags: review?(continuation) → review+
Comment on attachment 8791937 [details] [diff] [review]
Add ignore-initialization-check annotation to mCurrPi from CCGraphBuilder

https://hg.mozilla.org/integration/mozilla-inbound/rev/40cb45881d37
Attachment #8791937 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/40cb45881d37
Status: REOPENED → RESOLVED
Closed: 4 years ago3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
MozReview-Commit-ID: EkreJsiAIZH
Attachment #8792801 - Flags: review?(amarchesini)
Attachment #8792801 - Flags: review?(amarchesini) → review+
Comment on attachment 8792801 [details] [diff] [review]
Add ignore-initialization-check annotation to mForcePaintEpoch from HangMonitorChild

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c5d58066ad
Attachment #8792801 - Flags: checkin+
Attachment #8794853 - Attachment is obsolete: true
Attachment #8794853 - Flags: review?(dbaron)
Why is this bug security-confidential?
This bug is a result of this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=525063 that is security-confidential so we've decided to mark this one as well.
What information is in this bug that isn't already public based on the patches having landed?
My ideea was that if patches pushed to this bugzilla are r- and the variables need to be initialised we don't want to expose this to the public. But if you think that we should make this bug public just let me know and i'll remove the security-confidential flag.
MozReview-Commit-ID: 7zCqBnVAD88
Attachment #8795228 - Flags: review?(jorendorff)
MozReview-Commit-ID: 5wmvRUVTRnq
Attachment #8795246 - Flags: review?(jmuizelaar)
Attachment #8795246 - Flags: review?(jmuizelaar) → review?(jgilbert)
(In reply to Andi-Bogdan Postelnicu from comment #127)
> My ideea was that if patches pushed to this bugzilla are r- and the
> variables need to be initialised we don't want to expose this to the public.

This is a reasonable concern -- but still, that would result in this bug just being hidden indefinitely (in its entirety), and that's not what we really want to have happen.

Are there any specific r-'d never-to-be-landed patches here that deal with dangerously-still-uninitialized variables?  (I can't easily tell, because this bug is so huge.)  If so, perhaps you could you mark those patches (and comments about them) as private, so we can open this up?

> But if you think that we should make this bug public just let me know and
> i'll remove the security-confidential flag.

The decision on whether this would be appropriate would entirely depend on an answer to my question above. (Would we be revealing known uninitialized-variable-usage bugs by opening this up?)

Also, a followup question: do you have a plan for when this bug can be closed?  If there are more patches still to come here, I'd strongly suggest that you avoid posting further patches here, and consider this bug as "wave 1", and file a subsequent bug (hidden if you like) for "wave 2" of these patches.
Flags: needinfo?(bpostelnicu)
Attachment #8794855 - Flags: review?(dbaron) → review+
Attachment #8795169 - Flags: review?(dbaron) → review?(xidorn+moz)
Comment on attachment 8795169 [details] [diff] [review]
Add ignore-initialization-check annotation to mSpeakAs from CustomCounterStyle

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

::: layout/style/CounterStyleManager.cpp
@@ +1145,5 @@
>    uint32_t mRuleGeneration;
>  
>    uint8_t mSystem;
> +  // mSpeakAs gets initialized in ComputeSpeakAs
> +  MOZ_INIT_OUTSIDE_CTOR uint8_t mSpeakAs;

It should probably say, GetSpeakAs would ensure this is initialized before used.
Attachment #8795169 - Flags: review?(xidorn+moz) → review+
Attachment #8795169 - Attachment is obsolete: true
Comment on attachment 8795623 [details] [diff] [review]
Add ignore-initialization-check annotation to mSpeakAs from CustomCounterStyle

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

Thanks.
Attachment #8795623 - Flags: review?(xidorn+moz) → review+
Comment on attachment 8795623 [details] [diff] [review]
Add ignore-initialization-check annotation to mSpeakAs from CustomCounterStyle

https://hg.mozilla.org/integration/mozilla-inbound/rev/0695f1dba9c0
Flags: needinfo?(bpostelnicu)
Attachment #8795623 - Flags: checkin+
Comment on attachment 8794855 [details] [diff] [review]
Add ignore-initialization-check annotation to mStatus from SheetLoadData

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f462877e3f8
Attachment #8794855 - Flags: checkin+
Comment on attachment 8791622 [details] [diff] [review]
add ignore initialization check flag for mSourceGraphicIndex and mSourceAlphaIndex from nsSVGFilterInstance

https://hg.mozilla.org/integration/mozilla-inbound/rev/cad53bb6997f
Attachment #8791622 - Flags: checkin+
(In reply to Daniel Holbert [:dholbert] from comment #134)
> (In reply to Andi-Bogdan Postelnicu from comment #127)
> > My ideea was that if patches pushed to this bugzilla are r- and the
> > variables need to be initialised we don't want to expose this to the public.
> 
> This is a reasonable concern -- but still, that would result in this bug
> just being hidden indefinitely (in its entirety), and that's not what we
> really want to have happen.
> 
> Are there any specific r-'d never-to-be-landed patches here that deal with
> dangerously-still-uninitialized variables?  (I can't easily tell, because
> this bug is so huge.)  If so, perhaps you could you mark those patches (and
> comments about them) as private, so we can open this up?
> 
For the moment i can't recall any issues that you mentioned. 

> > But if you think that we should make this bug public just let me know and
> > i'll remove the security-confidential flag.
> 
> The decision on whether this would be appropriate would entirely depend on
> an answer to my question above. (Would we be revealing known
> uninitialized-variable-usage bugs by opening this up?)
> 
> Also, a followup question: do you have a plan for when this bug can be
> closed?  If there are more patches still to come here, I'd strongly suggest
> that you avoid posting further patches here, and consider this bug as "wave
> 1", and file a subsequent bug (hidden if you like) for "wave 2" of these
> patches.

This bug will be closed when we will no longer have errors of uninitialised member variables detected by this clang-plugin patch: 525063
When our code will be up to it and no errors will be triggered, that patch will be landed to m-c and thus any further patches that introduce uninitialised members will be rejected since they will trigger try failures. 
Sure we can split this bug in two and close this instance and open a wave 2 patch.
Comment on attachment 8795642 [details] [diff] [review]
Add ignore-initialization-check annotation to mPendingReqVersion & mRemainingBodySize from HttpServer::Connection

I'm curious as to whether we wouldn't be better off simply initializing this variables in the ctor instead of adding these annotations. This code is not at all performance critical... r- to change the approach to initialize these variables instead of adding the annotations.
Attachment #8795642 - Flags: review?(jst) → review-
Comment on attachment 8795644 [details] [diff] [review]
Add ignore-initialization-check annotation to mPort & mHttps from HttpServer

Same here, let's just initialize these variables in the ctor instead of adding annotations...
Attachment #8795644 - Flags: review?(jst) → review-
Attachment #8795632 - Flags: review?(valentin.gosu) → review+
Comment on attachment 8795632 [details] [diff] [review]
Add ignore-initialization-check annotation to variables mPeakSizeID and mPeakCountID from Http2BaseCompressor

 https://hg.mozilla.org/integration/mozilla-inbound/rev/bba43e5b3633
Attachment #8795632 - Flags: checkin+
Comment on attachment 8791622 [details] [diff] [review]
add ignore initialization check flag for mSourceGraphicIndex and mSourceAlphaIndex from nsSVGFilterInstance

Please try to note the correct reviewer when landing patches.  This was reviewed by dholbert, but https://hg.mozilla.org/integration/mozilla-inbound/rev/cad53bb6997f says it was reviewed by me.
Attachment #8795246 - Flags: review?(jgilbert) → review+
Attachment #8795282 - Flags: review?(btseng) → review+
Attachment #8795236 - Flags: review?(btseng) → review+
Comment on attachment 8795236 [details] [diff] [review]
Add ignore-initialization-check annotation to mIsDisplayNetworkNameRequired & mIsDisplaySpnRequired from IccInfo

https://hg.mozilla.org/integration/mozilla-inbound/rev/3065442079d9
Attachment #8795236 - Flags: checkin+
Comment on attachment 8795246 [details] [diff] [review]
Add ignore-initialization-check annotation to variables from WebGLTransformFeedback

https://hg.mozilla.org/integration/mozilla-inbound/rev/0571f7f30f42
Attachment #8795246 - Flags: checkin+
Comment on attachment 8795282 [details] [diff] [review]
Add ignore-initialization-check annotation to mPrlVersion from CdmaIccInfo

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5f861ab495
Attachment #8795282 - Flags: checkin+
Comment on attachment 8795690 [details] [diff] [review]
Add ignore-initialization-check annotation to mState from TelephonyCallGroup

Apologies for the long delay here. I'm clearing this review request as I believe this code was since removed. If that's not true, then it will be removed, or we could land something like this until it is, if so desired. Regardless, this is unused code as far as Firefox goes.
Attachment #8795690 - Flags: review?(jst)
Attachment #8796067 - Flags: review?(jst)
Attachment #8796100 - Flags: review?(jst)
(In reply to Johnny Stenback  (:jst, jst@mozilla.com) from comment #156)
> Comment on attachment 8795690 [details] [diff] [review] [diff] [review]
> Add ignore-initialization-check annotation to mState from TelephonyCallGroup
>
> I'm clearing this review request as I believe this code was since removed.

It was indeed (in bug 1309719) -- though before that, we took a broader version of this same patch on bug 1308868:  https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0be6c3f68a

So for both of those reasons, the patch here isn't needed. :)

@Andi-Bogdan: as we approach 160 comments here, perhaps it's time to close out this bug as FIXED? (and perhaps spin off helper bugs for the still-open review requests, if you haven't already done so)
Flags: needinfo?(bpostelnicu)
Attachment #8795690 - Attachment is obsolete: true
(In reply to Daniel Holbert [:dholbert] from comment #157)
> (In reply to Johnny Stenback  (:jst, jst@mozilla.com) from comment #156)
> > Comment on attachment 8795690 [details] [diff] [review]
> > Add ignore-initialization-check annotation to mState from TelephonyCallGroup
> >
> > I'm clearing this review request as I believe this code was since removed.
> 
> It was indeed (in bug 1309719) -- though before that, we took a broader
> version of this same patch on bug 1308868: 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ba0be6c3f68a
> 
> So for both of those reasons, the patch here isn't needed. :)
> 
> @Andi-Bogdan: as we approach 160 comments here, perhaps it's time to close
> out this bug as FIXED? (and perhaps spin off helper bugs for the still-open
> review requests, if you haven't already done so)

Yes you are right, we already have this follow up bug where we track thins kind of issues: https://bugzilla.mozilla.org/show_bug.cgi?id=1308868

I haven't closed it because not all patches where reviewed, but i will do so now and continue to follow up the more recent one.
Flags: needinfo?(bpostelnicu)
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52-]
Attachment #8786345 - Flags: review?(jorendorff) → review+
Attachment #8786330 - Flags: review?(jorendorff) → review+
Comment on attachment 8786330 [details] [diff] [review]
add ignore initialization check flag for functionBodyEndPos from BytecodeEmitter

https://hg.mozilla.org/integration/mozilla-inbound/rev/ecd4dfc78f59ac42178938f4630a584e2b14e40b
Attachment #8786330 - Flags: checkin+
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa3ad46271ea6f3813cd7254c7b18fd06ac694b
Backed out changeset e11754955e76 (bug 1282408) for compilation failures. r=backout CLOSED TREE
(In reply to Mark Banner (:standard8) from comment #161)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> eaa3ad46271ea6f3813cd7254c7b18fd06ac694b
> Backed out changeset e11754955e76 (bug 1282408) for compilation failures.
> r=backout CLOSED TREE

Thanks for that, re-pushed it:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba819294022028176720e7880120af69c7a46222
Why are we landing patches under this bug number still?
Flags: needinfo?(bpostelnicu)
Keywords: leave-open
(In reply to Ryan VanderMeulen [:RyanVM] from comment #164)
> Why are we landing patches under this bug number still?

I think these are the last two that are gonna be landed under this bug, it can be closed. now. Thank you!
Flags: needinfo?(bpostelnicu)
Attachment #8787093 - Flags: review?(jorendorff) → review+
Comment on attachment 8787093 [details] [diff] [review]
add ignore initialization check flag for member variables from Ptr and AddPtr

https://hg.mozilla.org/integration/mozilla-inbound/rev/3396fc568d66310676d816454359222430ddc6af
Attachment #8787093 - Flags: checkin+
Attachment #8795228 - Flags: review?(jorendorff) → review+
Attachment #8795333 - Flags: review?(jorendorff) → review+
Comment on attachment 8795228 [details] [diff] [review]
Add ignore-initialization-check annotation to parents from NameResolver

 https://hg.mozilla.org/integration/mozilla-inbound/rev/cc8b3345c2e53e3181f2214a77b86de45b0d000c
Attachment #8795228 - Flags: checkin+
Group: core-security-release
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.