Closed
Bug 1282408
Opened 8 years ago
Closed 7 years ago
[Static Analysis][Clang-Plugin] Ignore class members initialisation check
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
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
|
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.
Assignee | ||
Updated•8 years ago
|
No longer blocks: coverity-analysis
Assignee | ||
Updated•8 years ago
|
Group: core-security
Assignee | ||
Comment 1•8 years ago
|
||
MozReview-Commit-ID: BNA5ANQJjS1
Attachment #8765437 -
Flags: review?(wmccloskey)
Assignee | ||
Comment 2•8 years ago
|
||
(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.
Updated•8 years ago
|
Group: core-security → core-security-release
Assignee | ||
Comment 3•8 years ago
|
||
MozReview-Commit-ID: GpIcq2CNa8N
Attachment #8765842 -
Flags: review?(jdemooij)
Assignee | ||
Comment 4•8 years ago
|
||
(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.
Assignee | ||
Comment 5•8 years ago
|
||
MozReview-Commit-ID: 40yR3oLZhkH
Attachment #8765844 -
Flags: review?(jdemooij)
Assignee | ||
Updated•8 years ago
|
Attachment #8765844 -
Attachment description: add ignore initialization check flag for variables from InternalLink → Bug 1282408 - add ignore initialization check flag for variables from InternalLink
Assignee | ||
Updated•8 years ago
|
Attachment #8765437 -
Attachment description: add ignore initialization check flag for variables from RunnableMethod → Bug 1282408 - add ignore initialization check flag for variables from RunnableMethod
Assignee | ||
Updated•8 years ago
|
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 6•8 years ago
|
||
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 7•8 years ago
|
||
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+
Assignee | ||
Comment 8•8 years ago
|
||
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+
Assignee | ||
Comment 9•8 years ago
|
||
(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.
Assignee | ||
Comment 10•8 years ago
|
||
MozReview-Commit-ID: 40yR3oLZhkH
Attachment #8765891 -
Flags: review?(jdemooij)
Assignee | ||
Updated•8 years ago
|
Attachment #8765844 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
MozReview-Commit-ID: BNA5ANQJjS1
Attachment #8765901 -
Flags: review?(wmccloskey)
Assignee | ||
Updated•8 years ago
|
Attachment #8765437 -
Attachment is obsolete: true
Attachment #8765437 -
Flags: review?(wmccloskey)
Comment 12•8 years ago
|
||
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)
Assignee | ||
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 15•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8766288 -
Flags: review?(jst) → review+
Comment 17•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8ab05556d326
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 18•8 years ago
|
||
grr, sorry missed the leave-open flag
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 19•8 years ago
|
||
Comment on attachment 8766288 [details] [diff] [review] Bug 1282408_4.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/3a604cba63e9
Attachment #8766288 -
Flags: review+
Assignee | ||
Comment 20•8 years ago
|
||
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+
Assignee | ||
Comment 21•8 years ago
|
||
Comment on attachment 8766283 [details] [diff] [review] Bug 1282408_3.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/5db4165bdfa0
Attachment #8766283 -
Flags: review+
Comment 22•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3a604cba63e9 https://hg.mozilla.org/mozilla-central/rev/4230f642c60c https://hg.mozilla.org/mozilla-central/rev/5db4165bdfa0
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 23•8 years ago
|
||
MozReview-Commit-ID: 1KAlxDtZyMe
Attachment #8774695 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 24•8 years ago
|
||
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.
Assignee | ||
Comment 25•8 years ago
|
||
MozReview-Commit-ID: BZC7SnvmOKl
Attachment #8774702 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 26•8 years ago
|
||
MozReview-Commit-ID: 1jd0JbeVapq
Attachment #8774704 -
Flags: review?(jorendorff)
Assignee | ||
Comment 27•8 years ago
|
||
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.
Assignee | ||
Comment 28•8 years ago
|
||
MozReview-Commit-ID: 3Ldof83eL0P
Attachment #8774727 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 29•8 years ago
|
||
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);
Assignee | ||
Comment 30•8 years ago
|
||
MozReview-Commit-ID: 71zQBOwH6tS
Attachment #8774738 -
Flags: review?(jorendorff)
Assignee | ||
Comment 31•8 years ago
|
||
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 32•8 years ago
|
||
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)
Comment 33•8 years ago
|
||
(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.
Comment 34•8 years ago
|
||
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 35•8 years ago
|
||
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-
Updated•8 years ago
|
Attachment #8774702 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Updated•8 years ago
|
Attachment #8774727 -
Flags: review-
Assignee | ||
Comment 36•8 years ago
|
||
MozReview-Commit-ID: 1KAlxDtZyMe
Attachment #8775045 -
Flags: review?(jmuizelaar)
Assignee | ||
Updated•8 years ago
|
Attachment #8774695 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8774702 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8774727 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8766288 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8766283 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Attachment #8765891 -
Attachment is obsolete: true
Comment 37•8 years ago
|
||
Only pushing this file because it's the only "Uninitialized scalar field" defect in the "embedding" module.
Attachment #8775107 -
Flags: review?(bpostelnicu)
Assignee | ||
Comment 38•8 years ago
|
||
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)
Comment 39•8 years ago
|
||
Only pushing this file because it's the only "Uninitialized scalar field" defect in the "content" module.
Attachment #8775117 -
Flags: review?(bzbarsky)
Comment 40•8 years ago
|
||
(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!
Assignee | ||
Comment 41•8 years ago
|
||
MozReview-Commit-ID: L5ymS8HUczi
Attachment #8775121 -
Flags: review?(jorendorff)
Comment 42•8 years ago
|
||
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)
Assignee | ||
Comment 43•8 years ago
|
||
MozReview-Commit-ID: DW4whzEYdbl
Attachment #8775139 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 44•8 years ago
|
||
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.
Assignee | ||
Comment 45•8 years ago
|
||
MozReview-Commit-ID: L5ymS8HUczi
Attachment #8775141 -
Flags: review?(jorendorff)
Assignee | ||
Updated•8 years ago
|
Attachment #8775121 -
Attachment is obsolete: true
Attachment #8775121 -
Flags: review?(jorendorff)
Assignee | ||
Comment 46•8 years ago
|
||
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 47•8 years ago
|
||
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 49•8 years ago
|
||
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.
Updated•8 years ago
|
Attachment #8775045 -
Flags: review?(jmuizelaar) → review+
Updated•8 years ago
|
Attachment #8775139 -
Flags: review?(jmuizelaar) → review+
Assignee | ||
Comment 50•8 years ago
|
||
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
Assignee | ||
Comment 51•8 years ago
|
||
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 52•8 years ago
|
||
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)
Assignee | ||
Comment 53•8 years ago
|
||
MozReview-Commit-ID: 6IDxHLbSkoJ
Attachment #8775519 -
Flags: review?(dbaron)
Assignee | ||
Comment 54•8 years ago
|
||
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+
Assignee | ||
Comment 55•8 years ago
|
||
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
Assignee | ||
Comment 56•8 years ago
|
||
MozReview-Commit-ID: 22VI0j7QWbL
Attachment #8775560 -
Flags: review?(dbaron)
Assignee | ||
Comment 57•8 years ago
|
||
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.
Comment 58•8 years ago
|
||
Attachment #8775577 -
Flags: review?(surkov.alexander)
Comment 59•8 years ago
|
||
Attachment #8775599 -
Flags: review?(dcamp)
Updated•8 years ago
|
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+
Assignee | ||
Comment 61•8 years ago
|
||
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
Assignee | ||
Comment 62•8 years ago
|
||
(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
Assignee | ||
Comment 63•8 years ago
|
||
MozReview-Commit-ID: 2B0bPOi1Tde
Attachment #8783946 -
Flags: review?(jdemooij)
Assignee | ||
Comment 64•8 years ago
|
||
MozReview-Commit-ID: 9AYyHN5a5Wf
Attachment #8783955 -
Flags: review?(gpascutto)
Assignee | ||
Comment 65•8 years ago
|
||
MozReview-Commit-ID: 4DrIUUf4tuk
Attachment #8783956 -
Flags: review?(nfroyd)
Assignee | ||
Comment 66•8 years ago
|
||
MozReview-Commit-ID: AqiQ8wrtWvQ
Attachment #8783963 -
Flags: review?(rjesup)
Updated•8 years ago
|
Attachment #8783956 -
Flags: review?(nfroyd) → review+
Updated•8 years ago
|
Attachment #8783963 -
Flags: review?(rjesup) → review+
Comment 67•8 years ago
|
||
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 68•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8783955 -
Flags: review?(adw) → review+
Assignee | ||
Comment 69•8 years ago
|
||
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
Assignee | ||
Comment 70•8 years ago
|
||
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
Assignee | ||
Comment 71•8 years ago
|
||
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 72•8 years ago
|
||
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 73•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8774738 -
Flags: review?(jorendorff) → review?(luke)
Updated•8 years ago
|
Attachment #8775141 -
Flags: review?(jorendorff) → review?(luke)
Updated•8 years ago
|
Attachment #8774704 -
Flags: review?(luke) → review+
Updated•8 years ago
|
Attachment #8774738 -
Flags: review?(luke) → review+
Updated•8 years ago
|
Attachment #8775141 -
Flags: review?(luke) → review+
Comment 74•8 years ago
|
||
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+
Assignee | ||
Comment 75•8 years ago
|
||
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
Assignee | ||
Comment 76•8 years ago
|
||
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
Assignee | ||
Comment 77•8 years ago
|
||
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
Comment 78•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/81d77e3cdd5f
status-firefox50:
fixed → ---
Target Milestone: mozilla50 → ---
Comment 80•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5cdf880fc41c https://hg.mozilla.org/mozilla-central/rev/62332e479c4d
Comment 82•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ba0f066daf3e https://hg.mozilla.org/mozilla-central/rev/09b84bcc720b
Assignee | ||
Comment 83•8 years ago
|
||
MozReview-Commit-ID: KreWU3RBSnU
Attachment #8786330 -
Flags: review?(jorendorff)
Assignee | ||
Comment 84•8 years ago
|
||
MozReview-Commit-ID: BfEWPfVU0dj
Attachment #8786345 -
Flags: review?(jorendorff)
Assignee | ||
Comment 85•8 years ago
|
||
MozReview-Commit-ID: IzLeG9MueUt
Attachment #8787093 -
Flags: review?(jorendorff)
Comment 86•8 years ago
|
||
Attachment #8790295 -
Flags: review?(bbirtles)
Comment 87•8 years ago
|
||
Attachment #8790304 -
Flags: review?(bbirtles)
Comment 88•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8790304 -
Flags: review?(bbirtles) → review?(dholbert)
Assignee | ||
Comment 89•8 years ago
|
||
MozReview-Commit-ID: GRWqLr1pW3q
Attachment #8790645 -
Flags: review?(valentin.gosu)
Updated•8 years ago
|
Attachment #8790645 -
Flags: review?(valentin.gosu) → review?(mcmanus)
Comment 90•8 years ago
|
||
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 91•8 years ago
|
||
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+
Comment 92•8 years ago
|
||
Attachment #8790699 -
Flags: review?(dholbert)
Updated•8 years ago
|
Attachment #8790295 -
Attachment is obsolete: true
Comment 93•8 years ago
|
||
Attachment #8790702 -
Flags: review?(dholbert)
Updated•8 years ago
|
Attachment #8790304 -
Attachment is obsolete: true
Comment 94•8 years ago
|
||
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 95•8 years ago
|
||
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 96•8 years ago
|
||
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..
Assignee | ||
Comment 97•8 years ago
|
||
MozReview-Commit-ID: GRWqLr1pW3q
Attachment #8790731 -
Flags: review?(mcmanus)
Assignee | ||
Updated•8 years ago
|
Attachment #8790645 -
Attachment is obsolete: true
Attachment #8790645 -
Flags: review?(mcmanus)
Updated•8 years ago
|
Attachment #8790731 -
Flags: review?(mcmanus) → review+
Assignee | ||
Comment 98•8 years ago
|
||
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
Assignee | ||
Comment 99•8 years ago
|
||
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
Assignee | ||
Comment 100•8 years ago
|
||
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
Assignee | ||
Comment 101•8 years ago
|
||
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
Assignee | ||
Comment 102•8 years ago
|
||
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
Assignee | ||
Comment 103•8 years ago
|
||
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
Assignee | ||
Comment 104•8 years ago
|
||
MozReview-Commit-ID: D9DQT52JfNd
Attachment #8791536 -
Flags: review?(longsonr)
Comment 105•8 years ago
|
||
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+
Assignee | ||
Comment 106•8 years ago
|
||
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 107•8 years ago
|
||
Attachment #8791622 -
Flags: review?(dholbert)
Comment 108•8 years ago
|
||
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+
Comment 109•8 years ago
|
||
(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)
Comment 110•8 years ago
|
||
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.)
Assignee | ||
Comment 111•8 years ago
|
||
MozReview-Commit-ID: FFgmluIyvbs
Attachment #8791937 -
Flags: review?(continuation)
Assignee | ||
Comment 112•8 years ago
|
||
(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.
Assignee | ||
Comment 113•8 years ago
|
||
(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!
Comment 114•8 years ago
|
||
(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+.
Assignee | ||
Comment 115•8 years ago
|
||
great will do that!
Comment 116•8 years ago
|
||
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+
Assignee | ||
Comment 117•8 years ago
|
||
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: 8 years ago → 8 years ago
status-firefox52:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Assignee | ||
Updated•8 years ago
|
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 119•8 years ago
|
||
MozReview-Commit-ID: EkreJsiAIZH
Attachment #8792801 -
Flags: review?(amarchesini)
Updated•8 years ago
|
Attachment #8792801 -
Flags: review?(amarchesini) → review+
Assignee | ||
Comment 120•8 years ago
|
||
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+
Comment 122•8 years ago
|
||
Attachment #8794853 -
Flags: review?(dbaron)
Comment 123•8 years ago
|
||
Attachment #8794855 -
Flags: review?(dbaron)
Updated•8 years ago
|
Attachment #8794853 -
Attachment is obsolete: true
Attachment #8794853 -
Flags: review?(dbaron)
Why is this bug security-confidential?
Assignee | ||
Comment 125•8 years ago
|
||
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?
Assignee | ||
Comment 127•8 years ago
|
||
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.
Comment 128•8 years ago
|
||
Attachment #8795169 -
Flags: review?(dbaron)
Assignee | ||
Comment 129•8 years ago
|
||
MozReview-Commit-ID: 7zCqBnVAD88
Attachment #8795228 -
Flags: review?(jorendorff)
Comment 130•8 years ago
|
||
Attachment #8795236 -
Flags: review?(btseng)
Assignee | ||
Comment 131•8 years ago
|
||
MozReview-Commit-ID: 5wmvRUVTRnq
Attachment #8795246 -
Flags: review?(jmuizelaar)
Comment 132•8 years ago
|
||
Attachment #8795282 -
Flags: review?(btseng)
Updated•8 years ago
|
Attachment #8795246 -
Flags: review?(jmuizelaar) → review?(jgilbert)
Comment 133•8 years ago
|
||
Attachment #8795333 -
Flags: review?(jorendorff)
Comment 134•8 years ago
|
||
(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 135•8 years ago
|
||
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+
Comment 136•8 years ago
|
||
Attachment #8795623 -
Flags: review?(xidorn+moz)
Updated•8 years ago
|
Attachment #8795169 -
Attachment is obsolete: true
Comment 137•8 years ago
|
||
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+
Assignee | ||
Comment 138•8 years ago
|
||
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+
Assignee | ||
Comment 139•8 years ago
|
||
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+
Assignee | ||
Comment 140•8 years ago
|
||
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+
Assignee | ||
Comment 141•8 years ago
|
||
(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.
Assignee | ||
Comment 142•8 years ago
|
||
MozReview-Commit-ID: H5DsehXu2nr
Attachment #8795632 -
Flags: review?(valentin.gosu)
Comment 143•8 years ago
|
||
Attachment #8795642 -
Flags: review?(jst)
Comment 144•8 years ago
|
||
Attachment #8795644 -
Flags: review?(jst)
Comment 145•8 years ago
|
||
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 146•8 years ago
|
||
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-
Updated•8 years ago
|
Attachment #8795632 -
Flags: review?(valentin.gosu) → review+
Assignee | ||
Comment 147•8 years ago
|
||
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 148•8 years ago
|
||
Attachment #8795690 -
Flags: review?(jst)
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.
Updated•8 years ago
|
Attachment #8795246 -
Flags: review?(jgilbert) → review+
Updated•8 years ago
|
Attachment #8795282 -
Flags: review?(btseng) → review+
Updated•8 years ago
|
Attachment #8795236 -
Flags: review?(btseng) → review+
Assignee | ||
Comment 150•8 years ago
|
||
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+
Assignee | ||
Comment 151•8 years ago
|
||
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+
Assignee | ||
Comment 152•8 years ago
|
||
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 153•8 years ago
|
||
Attachment #8796067 -
Flags: review?(jst)
Comment 154•8 years ago
|
||
Attachment #8796100 -
Flags: review?(jst)
Comment 155•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0695f1dba9c0 https://hg.mozilla.org/mozilla-central/rev/4f462877e3f8 https://hg.mozilla.org/mozilla-central/rev/cad53bb6997f https://hg.mozilla.org/mozilla-central/rev/bba43e5b3633 https://hg.mozilla.org/mozilla-central/rev/3065442079d9 https://hg.mozilla.org/mozilla-central/rev/0571f7f30f42 https://hg.mozilla.org/mozilla-central/rev/5f5f861ab495
Comment 156•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8796067 -
Flags: review?(jst)
Updated•7 years ago
|
Attachment #8796100 -
Flags: review?(jst)
Comment 157•7 years ago
|
||
(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)
Updated•7 years ago
|
Attachment #8795690 -
Attachment is obsolete: true
Assignee | ||
Comment 158•7 years ago
|
||
(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)
Assignee | ||
Updated•7 years ago
|
Status: REOPENED → RESOLVED
Closed: 8 years ago → 7 years ago
Resolution: --- → FIXED
Updated•7 years ago
|
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]
Updated•7 years ago
|
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main52-]
Updated•7 years ago
|
Attachment #8786345 -
Flags: review?(jorendorff) → review+
Updated•7 years ago
|
Attachment #8786330 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 159•7 years ago
|
||
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+
Assignee | ||
Comment 160•7 years ago
|
||
Comment on attachment 8786345 [details] [diff] [review] add ignore initialization check flag for variables from BindingIter https://hg.mozilla.org/integration/mozilla-inbound/rev/e11754955e760666b46ad6631333d3b1d97df34d
Attachment #8786345 -
Flags: checkin+
Comment 161•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/eaa3ad46271ea6f3813cd7254c7b18fd06ac694b Backed out changeset e11754955e76 (bug 1282408) for compilation failures. r=backout CLOSED TREE
Assignee | ||
Comment 162•7 years ago
|
||
(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
Comment 163•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ecd4dfc78f59 https://hg.mozilla.org/mozilla-central/rev/ba8192940220
Comment 164•7 years ago
|
||
Why are we landing patches under this bug number still?
Flags: needinfo?(bpostelnicu)
Keywords: leave-open
Assignee | ||
Comment 165•7 years ago
|
||
(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)
Updated•7 years ago
|
Attachment #8787093 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 166•7 years ago
|
||
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
Assignee | ||
Updated•7 years ago
|
Attachment #8787093 -
Flags: checkin+
Updated•7 years ago
|
Attachment #8795228 -
Flags: review?(jorendorff) → review+
Updated•7 years ago
|
Attachment #8795333 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 167•7 years ago
|
||
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+
Assignee | ||
Comment 168•7 years ago
|
||
Comment on attachment 8795333 [details] [diff] [review] Add ignore-initialization-check annotation to which_ from CalleeDesc https://hg.mozilla.org/integration/mozilla-inbound/rev/9fbd30932290df8363fdeb6c1923fed39dba9cbc
Attachment #8795333 -
Flags: checkin+
Comment 169•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/3396fc568d66 https://hg.mozilla.org/mozilla-central/rev/cc8b3345c2e5 https://hg.mozilla.org/mozilla-central/rev/9fbd30932290
Updated•7 years ago
|
Group: core-security-release
Updated•6 years ago
|
Product: Core → Firefox Build System
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•