Closed Bug 1453795 Opened 6 years ago Closed 6 years ago

uninitialized class members: Review of fixes by 525063 per module

Categories

(Developer Infrastructure :: Source Code Analysis, enhancement)

enhancement
Not set
normal

Tracking

(firefox63 fixed)

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: tristanbourvon, Assigned: andi)

References

Details

(Keywords: csectype-uninitialized, sec-audit, Whiteboard: [adv-main63-])

Attachments

(1 file, 110 obsolete files)

29.77 KB, patch
nical
: review+
Details | Diff | Splinter Review
The fixes landed with 525063 were not individually reviewed by each module owner and have some quality issues. These include:
 - Stray FIXME comments that require hand-written initialization to be written
 - Poor formatting caused by clang-format being confused by #ifdefs
 - Variables getting initialized while a comment explicitly mentions they should NOT be initialized

Module owners may also have additional specific comments to add.

See these commits for reference:

Main commit:
https://hg.mozilla.org/mozilla-central/rev/d7d2f08e051c

Fix webrender assertions:
https://hg.mozilla.org/mozilla-central/rev/bf13e4103150

Backing out of js/src/:
https://hg.mozilla.org/mozilla-central/rev/6ff8aaef2866

Backing out of js/public/:
https://hg.mozilla.org/mozilla-central/rev/516c4fb1e4b862b78a40c472e4d61dea79d51890
Summary: Review of fixes landed with 525063 per module → Review of fixes by 525063 per module
The patch has since been backed out, but it still needs individual review per module before being re-landed.
Blocks: 525063
Points 1 and 3 are now fixed. We still have to fix the poor formatting from clang-format with the #ifdefs, and also manually format the fixes for js/.

Andi, do you think you can help with this?
Flags: needinfo?(bpostelnicu)
I've written a patch for clang-format that fixes the ifdef issue, here is the result.
Flags: needinfo?(bpostelnicu)
Attached patch unint_format.patch (obsolete) — Splinter Review
IF YOU ARE BEING R? ON A PATCH HERE, PLEASE READ THIS:

This series of patch is the result of running a static-analysis autofix script on mozilla-central. Some issues have been solved by hands afterwards to make sure everything builds and runs nicely. Also, all the patches should be properly formatted.

Since this patch is globally huge and automatically generated, only a very minor sanity check has been made for each fix (checking that there are no explicit comments disallowing initialization, checking for proper formatting, etc). As a result, we ask each module owner/peer to review the patch concerning their respective module.

The performance analysis which has previously been made indicates no notable performance hit. Should any perf regression arise when landing the current patch as a whole, we will take a look at it and deal with the situation accordingly.

If you have any additional question, please feel free to ask it here.
Attached patch Animation (obsolete) — Splinter Review
Attached patch Content Security (obsolete) — Splinter Review
Attached patch Cookies and Permissions (obsolete) — Splinter Review
Attached patch Cycle Collector (obsolete) — Splinter Review
Attached patch DevTools (obsolete) — Splinter Review
Attached patch docshell (obsolete) — Splinter Review
Attached patch Document Object Model (obsolete) — Splinter Review
Attached patch DOM File (obsolete) — Splinter Review
Attached patch Editor (obsolete) — Splinter Review
Attached patch Event Handling (obsolete) — Splinter Review
Attached patch Graphics (obsolete) — Splinter Review
Attached patch HTML Parser (obsolete) — Splinter Review
Attached patch Legacy HTML Parser (obsolete) — Splinter Review
Attached patch ImageLib (obsolete) — Splinter Review
Attached patch I18N Library (obsolete) — Splinter Review
Attached patch IPC (obsolete) — Splinter Review
Attached patch JavaScript (obsolete) — Splinter Review
Attached patch JavaScript JIT (obsolete) — Splinter Review
Attached patch Layout Engine (obsolete) — Splinter Review
Attached patch libjar (obsolete) — Splinter Review
Attached patch MathML (obsolete) — Splinter Review
Attached patch Media Playback (obsolete) — Splinter Review
Attached patch mfbt (obsolete) — Splinter Review
Attached patch Necko (obsolete) — Splinter Review
Attached patch Plugins (obsolete) — Splinter Review
Attached patch Gecko Profiler (obsolete) — Splinter Review
Attached patch RDF (obsolete) — Splinter Review
Attached patch security (obsolete) — Splinter Review
Attached patch Security - Mozilla PSM Glue (obsolete) — Splinter Review
Attached patch Sandboxing - OSX (obsolete) — Splinter Review
Attached patch storage (obsolete) — Splinter Review
Attached patch String (obsolete) — Splinter Review
Attached patch Style System (obsolete) — Splinter Review
Attached patch SVG (obsolete) — Splinter Review
Attached patch Testing Infrastructure (obsolete) — Splinter Review
Attached patch Web Painting (obsolete) — Splinter Review
Attached patch WebRTC (obsolete) — Splinter Review
Attached patch WebRTC Media (obsolete) — Splinter Review
Attached patch WebRTC Signaling (obsolete) — Splinter Review
Attached patch WebVR (obsolete) — Splinter Review
Attached patch Web Workers (obsolete) — Splinter Review
Attached patch Widget (obsolete) — Splinter Review
Attached patch Widget - OS X (obsolete) — Splinter Review
Attached patch XBL (obsolete) — Splinter Review
Attached patch XML (obsolete) — Splinter Review
Attached patch XPCOM (obsolete) — Splinter Review
Attached patch XPConnect (obsolete) — Splinter Review
Attached patch XPToolkit (obsolete) — Splinter Review
Attached patch XSLT Processor (obsolete) — Splinter Review
Attachment #8982678 - Flags: review?(bbirtles)
Attachment #8982679 - Flags: review?(ckerschb)
Attachment #8982680 - Flags: review?(josh)
Attachment #8982681 - Flags: review?(continuation)
Attachment #8982682 - Flags: review?(bzbarsky)
Attachment #8982682 - Flags: review?(bzbarsky)
Attachment #8982683 - Flags: review?(bzbarsky)
Attachment #8982684 - Flags: review?(peterv)
Attachment #8982685 - Flags: review?(amarchesini)
Attachment #8982686 - Flags: review?(masayuki)
Attachment #8982687 - Flags: review?(bugs)
Comment on attachment 8982681 [details] [diff] [review]
Cycle Collector

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

I guess this is okay, though note that both of these methods crash the browser immediately if they are ever called. I'm not sure if there's a better way to do that nowadays. These data structures are a little weird.
Attachment #8982681 - Flags: review?(continuation) → review+
Attachment #8982688 - Flags: review?(jmuizelaar)
Attachment #8982690 - Flags: review?(mrbkap)
Attachment #8982682 - Flags: review?(pbrosset)
Attachment #8982691 - Flags: review?(tnikkel)
Attachment #8982692 - Flags: review?(jfkthame)
Attachment #8982693 - Flags: review?(jmathies)
Attachment #8982694 - Flags: review?(jorendorff)
Comment on attachment 8982687 [details] [diff] [review]
Event Handling

>diff -r 7ef845081069 dom/events/EventStateManager.cpp
>--- a/dom/events/EventStateManager.cpp	Tue May 01 00:57:17 2018 +0300
>+++ b/dom/events/EventStateManager.cpp	Thu May 31 20:52:24 2018 -0700
>@@ -294,8 +294,10 @@
>   : mLockCursor(0)
>   , mLastFrameConsumedSetCursor(false)
>   , mCurrentTarget(nullptr)
>-    // init d&d gesture state machine variables
>-  , mGestureDownPoint(0,0)
>+  // init d&d gesture state machine variables
>+  , mGestureDownPoint(0, 0)
>+  , mGestureModifiers{ 0 }
>+  , mGestureDownButtons{ 0 }

Use () syntax, not {}.
Same also elsewhere.
Attachment #8982687 - Flags: review?(bugs) → review-
Attachment #8982695 - Flags: review?(jdemooij)
Attachment #8982696 - Flags: review?(dbaron)
Attachment #8982697 - Flags: review?(aklotz)
Attachment #8982698 - Flags: review?(karlt)
Attachment #8982700 - Flags: review?(jyavenard)
Attachment #8982701 - Flags: review?(jwalden+bmo)
Attachment #8982702 - Flags: review?(mcmanus)
Attachment #8982703 - Flags: review?(jmathies)
Attachment #8982704 - Flags: review?(mstange)
Attachment #8982705 - Flags: review?(l10n)
Attachment #8982706 - Flags: review?(kaie)
Attachment #8982707 - Flags: review?(dkeeler)
Attachment #8982708 - Flags: review?(haftandilian)
Attachment #8982709 - Flags: review?(mak77)
Attachment #8982710 - Flags: review?(dbaron)
Attachment #8982712 - Flags: review?(cam)
Attachment #8982713 - Flags: review?(jwatt)
Attachment #8982714 - Flags: review?(jmaher)
Attachment #8982702 - Flags: review?(mcmanus) → review?(michal.novotny)
Comment on attachment 8982710 [details] [diff] [review]
String

While it's a style nit, for a patchset of this size, I'd prefer that you update the patches to follow our normal style and use () rather than {} before requesting review.
Attachment #8982710 - Flags: review?(dbaron)
Comment on attachment 8982707 [details] [diff] [review]
Security - Mozilla PSM Glue

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

Just minor style nits.

::: security/manager/ssl/nsCertOverrideService.cpp
@@ +88,4 @@
>  
>  nsCertOverrideService::nsCertOverrideService()
>    : mMutex("nsCertOverrideService.mutex")
> +  , mOidTagForStoringNewHashes{ SEC_OID_UNKNOWN }

Honestly this field isn't even necessary. Removing it is not completely trivial, though, so I can do it in a follow-up (probably bug 1461803, which has turned into a misc. PSM cleanup bug).

::: security/manager/ssl/nsNSSCallbacks.cpp
@@ -559,3 @@
>  }
>  
>  nsHTTPListener::nsHTTPListener()

This doesn't exist any more.

::: security/manager/ssl/nsNSSComponent.cpp
@@ +208,4 @@
>    , mLoadableRootsLoadedResult(NS_ERROR_FAILURE)
>    , mMutex("nsNSSComponent.mMutex")
>    , mNSSInitialized(false)
> +  , mMitmDetecionEnabled{ false }

style nit: we use ()
Attachment #8982707 - Flags: review?(dkeeler) → review+
Attachment #8982704 - Flags: review?(mstange) → review+
Comment on attachment 8982706 [details] [diff] [review]
security

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

Please either make the requested changes or file a follow-up to do them. (Also, for future reference, security/certverifier and security/pkix are part of PSM, module-wise.)

::: security/certverifier/CertVerifier.h
@@ +73,5 @@
>  class PinningTelemetryInfo
>  {
>  public:
> +  PinningTelemetryInfo()
> +    : certPinningResultHistogram{ static_cast<Telemetry::HistogramID>(0) }

I think this is probably not what we want to do. Short of adding a placeholder value for an uninitialized HistogramID, maybe we could convert this to a Maybe<HistogramID> type?

@@ +74,5 @@
>  {
>  public:
> +  PinningTelemetryInfo()
> +    : certPinningResultHistogram{ static_cast<Telemetry::HistogramID>(0) }
> +    , certPinningResultBucket{ 0 }

nit: we use ()

@@ +75,5 @@
>  public:
> +  PinningTelemetryInfo()
> +    : certPinningResultHistogram{ static_cast<Telemetry::HistogramID>(0) }
> +    , certPinningResultBucket{ 0 }
> +    , rootBucket{ 0 }

Might be best to use ROOT_CERTIFICATE_UNKNOWN here.

::: security/pkix/lib/pkixbuild.cpp
@@ +64,4 @@
>      , stapledOCSPResponse(aStapledOCSPResponse)
>      , subCACount(aSubCACount)
>      , deferredSubjectError(aDeferredSubjectError)
> +    , subjectSignaturePublicKeyAlg{ static_cast<der::PublicKeyAlgorithm>(0) }

If this is something we're going to enforce programmatically, then we need a different solution here. It would probably be best to declare a variant 'Uninitialized' in PublicKeyAlgorithm, initialize this to that, and assert that we never actually encounter it. If this isn't something you have the time/interest to do, please file a bug in Core :: Security: PSM to follow up on this.

::: security/pkix/lib/pkixutil.h
@@ +47,5 @@
>             const BackCert* aChildCert)
>      : der(aCertDER)
>      , endEntityOrCA(aEndEntityOrCA)
>      , childCert(aChildCert)
> +    , version{ static_cast<der::Version>(0) }

Same idea as with PublicKeyAlgorithm.
Attachment #8982706 - Flags: review?(kaie) → review-
Comment on attachment 8982696 [details] [diff] [review]
Layout Engine

Please put reformatting and substantive changes in separate patches, and please follow local style and use () rather than {}.
Attachment #8982696 - Flags: review?(dbaron) → review-
Comment on attachment 8982683 [details] [diff] [review]
docshell

>+++ b/docshell/base/SerializedLoadContext.cpp	Thu May 31 20:45:09 2018 -0700
>+  : mIsContent{ false }

A few things:

1)  Please use "mIsContent(false)" or init it to false at the declaration point.
2)  Why are we adding an initializer for mIsContent but not mIsNotNull or  mIsPrivateBitValid?  I guess because we statically know those get set before the constructor returns (via Init())?

>+++ b/docshell/base/nsDocShell.cpp	Thu May 31 20:45:09 2018 -0700
>+  this->mHistoryID.m0 = { 0 };

Soo.. I understand why we're doing this; the GenerateUUIDInPlace call might technically fail to initialize the memory, so this seems like a real issue.  However:

1)  We're not initializing m3 anyway.
2)  Initializing to 0 doesn't seem like the right thing in the failure case: it'll pretty much guarantee that the uuid is not unique, which is bad for a uuid.

It's probably best to file a followup bug on figuring out what should happen here.  This change is maybe OK to quiet the static analysis for now, with a comment pointing to the followup.  Maybe we can make it clearer that uuid generation can't fail.

>+++ b/uriloader/base/nsDocLoader.h	Thu May 31 20:45:09 2018 -0700
>+          : mStatusCode{ NS_ERROR_NOT_INITIALIZED }

Again, (), not {}.

I think the right thing to do here is to have status and message args to the ctor and move the assignments at the callsite into the non-constructor path.

>+++ b/uriloader/base/nsURILoader.cpp	Thu May 31 20:45:09 2018 -0700
>+  : mFlags{ 0 }

(), not {}.

But actually, this constructor should just be removed.  The comment claims it's "needed for nsCOMPtr to work right", and maybe that was true 15 years ago... but at this point I doubt it's needed.  I have no idea why it was needed even back then... and yes, I know I added the comment.  I have no idea what it was trying to say.

>+++ b/uriloader/exthandler/ExternalHelperAppParent.cpp	Thu May 31 20:45:09 2018 -0700
>+ExternalHelperAppParent::ExternalHelperAppParent(const OptionalURIParams& uri,

Please undo the whitespace changes, especially since they go over the 80 char limit.

>+  , mContentDisposition{ 0 }

What should happen here instead is that the mContentDispositionHeader/mContentDispositionFilename/mContentDisposition bits should all move to the constructor (and the arguments to Init() that are used for them should become constructor arguments).

>+++ b/uriloader/exthandler/HandlerServiceParent.cpp	Thu May 31 20:45:09 2018 -0700
>+  , mPrefAction{ 0 }

(), and this should be initialized to nsIHandlerInfo::alwaysAsk, I would think.

>+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp	Thu May 31 20:45:09 2018 -0700
>+  , mKeepRequestAlive{ false }

This member is unused, looks like; we can just delete it.

The others look fine, with () instead of {}.
Attachment #8982683 - Flags: review?(bzbarsky) → review-
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment on attachment 8982705 [details] [diff] [review]
RDF

RDF is gone.
Attachment #8982705 - Flags: review?(l10n) → review-
Comment on attachment 8982700 [details] [diff] [review]
Media Playback

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

Inconsistency between list initialization.
I'd prefer C++11 simple initialization, however that would mean more change.

::: dom/media/mp4/MP4Metadata.cpp
@@ +26,4 @@
>  
>  IndiceWrapper::IndiceWrapper(Mp4parseByteData& aIndice)
>  {
> +  this->mIndice.data = { nullptr };

mIndice.data keep consistency

::: dom/media/ogg/OggWriter.cpp
@@ +18,2 @@
>  {
> +  this->mOggStreamState.body_data = { nullptr };

why the use of this dereferencing? (this->)

in any case, use PodZero on mOggStreamState (it's a plain C-class) or zero initialization above

::: dom/media/ogg/OpusParser.cpp
@@ +29,2 @@
>  #ifdef MOZ_SAMPLE_TYPE_FLOAT32
> +  mGain(1.0f)

, mGain(...)

why the , on a single line

@@ +32,2 @@
>  #else
> +  mGain_Q16(65536)

same

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm
@@ +109,4 @@
>                                         const nsTArray<size_t>& aOffsets)
>    : mTask(aTask)
>    , mSpeechSynthesizer(aSynth)
> +  , mCurrentIndex{0}, mOffsets(aOffsets)

new line
Attachment #8982700 - Flags: review?(jyavenard) → review-
Comment on attachment 8982692 [details] [diff] [review]
I18N Library

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

::: intl/chardet/nsCyrillicDetector.cpp
@@ +130,5 @@
> +  uint8_t aItems,
> +  const uint8_t** aCyrillicClass,
> +  const char** aCharsets)
> +  : nsCyrillicDetector(aItems, aCyrillicClass, aCharsets)
> +  , mResult{ nullptr }

This doesn't need to be initialized here. It's a private field which is only read in one place, towards the end of the DoIt() method; and DoIt() starts out by initializing it to nullptr.

If we have to do this just to quiet the analysis, I guess it's harmless enough, but please use () rather than {} to fit in with the style of (ancient) i18n-module code.
Attachment #8982692 - Flags: review?(jfkthame) → review-
Comment on attachment 8982695 [details] [diff] [review]
JavaScript JIT

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

Thanks for doing this. Some style nits below that affect most of the patch, it will be easier to review once these have been addressed.

::: js/src/jit/IonBuilder.cpp
@@ +131,5 @@
> +  , backgroundCodegen_(nullptr)
> +  , actionableAbortScript_(nullptr)
> +  , actionableAbortPc_(nullptr)
> +  , actionableAbortMessage_(nullptr)
> +  , rootList_(nullptr)

SpiderMonkey style is to use trailing comma, please don't change that here and elsewhere in the patch (it's fine to change if it involves #ifdefs and putting it at the end is more annoying).

::: js/src/jit/x64/Bailouts-x64.cpp
@@ +49,2 @@
>    : machine_(bailout->machineState())
> +  , activation_{ nullptr }

activation_(nullptr) and put the comma at the end of the previous line here too.

Please use foo_(x) instead of foo_{ x } elsewhere too.
Attachment #8982695 - Flags: review?(jdemooij)
Comment on attachment 8982685 [details] [diff] [review]
DOM File

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

::: dom/filesystem/GetFilesHelper.h
@@ +160,5 @@
>      : GetFilesHelper(aGlobal, aRecursiveFlag)
>      , mPendingOperation(false)
> +  {
> +    this->mUUID.m0 = { 0 };
> +    this->mUUID.m1 = { 0 };

This should go in the CTOR of nsID here:
https://searchfox.org/mozilla-central/source/xpcom/base/nsID.h#20
Attachment #8982685 - Flags: review?(amarchesini) → review-
Comment on attachment 8982685 [details] [diff] [review]
DOM File

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

::: dom/filesystem/GetFilesHelper.h
@@ +161,5 @@
>      , mPendingOperation(false)
> +  {
> +    this->mUUID.m0 = { 0 };
> +    this->mUUID.m1 = { 0 };
> +    this->mUUID.m2 = { 0 };

A couple of things:
1. Don't use 'this->'.
2. a UUID with all zeros is a valid UUID. I don't see the point of setting these values to 0.
Comment on attachment 8982714 [details] [diff] [review]
Testing Infrastructure

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

I am not familiar with this code or changes- after looking around this looks safe.  This code itself is imported from a third party and there is no development that has happened on this code here at Mozilla.

Call this a rubber stamp from me.
Attachment #8982714 - Flags: review?(jmaher) → review+
(In reply to Joel Maher ( :jmaher ) (UTC-4) from comment #68)
> Comment on attachment 8982714 [details] [diff] [review]
> Testing Infrastructure
> 
> Review of attachment 8982714 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not familiar with this code or changes- after looking around this looks
> safe.  This code itself is imported from a third party and there is no
> development that has happened on this code here at Mozilla.

Given it is third-party code that we aren't maintaining separately in mozilla, wouldn't it be preferable to leave it untouched, for easier comparison against upstream? Especially if we ever want to cherry-pick/backport anything, as opposed to simply taking a new code drop.

The changes here look unnecessary, AFAICS. In all three cases, the fields being initialized are going to be overwritten immediately by the initialization method that the constructor calls, so there is no risk of them being used before initialization. (I'm a bit surprised the analysis didn't figure that out for itself and leave them alone.)
I do agree that third party code should be exempt and this is just test only code.  There is some security in knowing we can run a tool on 100% of our code base and have no issues with it.  I think if we do change this we should consider adding a .diff file alongside so we know the changes we have made since importing.
Attachment #8982678 - Flags: review?(bbirtles) → review+
Comment on attachment 8982691 [details] [diff] [review]
ImageLib

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

::: image/RasterImage.cpp
@@ +70,5 @@
>  
>  //******************************************************************************
> +RasterImage::RasterImage(ImageURL* aURI /* = nullptr */)
> +  : ImageResource(aURI)
> +  , // invoke superclass's constructor

ImageResource is the superclass so this comment should go above the ImageResource constructor call.

::: image/VectorImage.cpp
@@ +372,5 @@
>  // Constructor / Destructor
>  
> +VectorImage::VectorImage(ImageURL* aURI /* = nullptr */)
> +  : ImageResource(aURI)
> +  , // invoke superclass's constructor

Same thing with this comment.
Attachment #8982691 - Flags: review?(tnikkel) → review+
Comment on attachment 8982689 [details] [diff] [review]
HTML Parser

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

This is OK to land, but nsHtml5StackNode.cpp and nsHtml5Tokenizer.cpp are generated files that will be overwritten when regenerated, so a follow-up bug to update the corresponding Java files in the https://hg.mozilla.org/projects/htmlparser/ repo is needed.
Attachment #8982689 - Flags: review+
Depends on: 1466449
(In reply to Henri Sivonen (:hsivonen) from comment #72)
> This is OK to land, but nsHtml5StackNode.cpp and nsHtml5Tokenizer.cpp are
> generated files that will be overwritten when regenerated, so a follow-up
> bug to update the corresponding Java files in the
> https://hg.mozilla.org/projects/htmlparser/ repo is needed.

Filed as bug 1466449.
Comment on attachment 8982686 [details] [diff] [review]
Editor

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

::: editor/libeditor/SelectionState.cpp
@@ +639,5 @@
>   ******************************************************************************/
>  
>  RangeItem::RangeItem()
> +  : mStartOffset{ 0 }
> +  , mEndOffset{ 0 }

Use (0) instead of { 0 }.

::: editor/spellchecker/nsFilteredContentIterator.h
@@ +49,4 @@
>    void         ClearDidSkip() {  mDidSkip = false; }
>  
>  protected:
> +  nsFilteredContentIterator()

Looks like that this constructor is used by nobody. So, please mark this construct to delete if it's possible.

@@ +51,5 @@
>  protected:
> +  nsFilteredContentIterator()
> +    : mDidSkip(false)
> +    , mIsOutOfRange(false)
> +    , mDirection{ eDirNotSet }

Otherwise, use (eDirNotSet) here instead of {}.
Attachment #8982686 - Flags: review?(masayuki) → review+
Comment on attachment 8982682 [details] [diff] [review]
DevTools

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

Transferring to Jim who knows this code.
Attachment #8982682 - Flags: review?(pbrosset) → review?(jimb)
Comment on attachment 8982679 [details] [diff] [review]
Content Security

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

I am fine with those changes; thanks; r=me
Attachment #8982679 - Flags: review?(ckerschb) → review+
Comment on attachment 8982709 [details] [diff] [review]
storage

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

::: storage/StorageBaseStatementInternal.cpp
@@ +111,4 @@
>  //// StorageBaseStatementInternal
>  
>  StorageBaseStatementInternal::StorageBaseStatementInternal()
> +  : mNativeConnection{ nullptr }

nit: Not changing indentation would probably preserve more blame

As others said, please use (nullptr) instead of { nullptr }

::: storage/mozStorageStatement.cpp
@@ +110,5 @@
>  
>  Statement::Statement()
> +  : StorageBaseStatementInternal()
> +  , mDBStatement(nullptr)
> +  , mParamCount{ 0 }

ditto
Attachment #8982709 - Flags: review?(mak77) → review-
FWIW, I'd also prefer () over {}, but the XPCOM patch was so tiny I didn't care enough to say anything.
Comment on attachment 8982690 [details] [diff] [review]
Legacy HTML Parser

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

As pointed out by everybody, the added initializers should use () instead of {}

r=me with that fixed.
Attachment #8982690 - Flags: review?(mrbkap) → review+
Comment on attachment 8982708 [details] [diff] [review]
Sandboxing - OSX

Please use parens instead of braces for consistency.
Attachment #8982708 - Flags: review?(haftandilian) → review+
Attachment #8982703 - Flags: review?(jmathies) → review+
Attachment #8982693 - Flags: review?(jmathies) → review+
Group: firefox-build-security → core-security
Keywords: sec-audit
Comment on attachment 8982702 [details] [diff] [review]
Necko

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

Keep the style of files you're changing, i.e. for initialization use () instead of {} and don't change indentation. r+ with these 2 things fixed.
Attachment #8982702 - Flags: review?(michal.novotny) → review+
Comment on attachment 8982701 [details] [diff] [review]
mfbt

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

Please (as so many others have said here already, for their respective parts) use () for member-list initializing unless list-initialization is specifically desired.

Once that's done, only the EnumSet concern about the static analysis seemingly not understanding member initializers *requires* a change/response.  The rest are nits I can fix after you land, or before if time permits.

::: mfbt/EnumSet.h
@@ +31,5 @@
>  
>    EnumSet()
>      : mBitField(0)
> +#ifdef DEBUG
> +    , mVersion{}

|mVersion| has a member-initializer at the declaration that zeroes this, so the presence or absence of this should have (wait for it) zero impact on initialization behavior.

Is the static analysis just broken about recognizing member-initializers as actually contributing initialization to a field in all constructors that don't override that initialization?  If so, IMO that's pretty bad in a world where we are specifically encouraging http://whereswalden.com/2018/05/21/psa-stop-using-mozillapodzero-and-mozillapodarrayzero/ member-initialization.  And a static analysis that doesn't understand member-initializers, really needs to be fixed before we go ahead with this.

::: mfbt/SegmentedVector.h
@@ +59,5 @@
>    {
> +    SegmentImpl()
> +      : mLength(0)
> +    {
> +      this->mStorage.mAlign = {};

This zeroing is unnecessary, and it will actively hide any case where we use uninitialized ("uninitialized", with this change) bytes within a SegmentedVector field.

Is there a way I can affirmatively indicate "I choose not to initialize this"?  Segments are big, so this has some perf cost to it, and that it hides bugs makes it worse.  I'd prefer if you at least told me how to do this so I can go through and clean up after you, if this patch-series lands any time soon.  (Alternatively I could easily just fix it myself in an unrelated bug that shouldn't draw any eyes.)

::: mfbt/SmallPointerArray.h
@@ +35,5 @@
>  {
>  public:
>    SmallPointerArray()
> +    : mPadding{ nullptr }
> +    , mArray{ nullptr }

This is not particularly desirable, IMO, but really this class needs some extra love given to it to not violate C++ union-arm access rules, so okay for now.  (These get immediately overwritten by the nullptr-sets below, so any compiler with any vague intelligence should eliminate these inits anyway.)
Attachment #8982701 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8982712 [details] [diff] [review]
Style System

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

r=me with these comments addressed.

Per others, please use ()s instead of {}s for initialization.

::: layout/style/StyleAnimationValue.h
@@ +44,4 @@
>  struct AnimationValue
>  {
>    explicit AnimationValue(const RefPtr<RawServoAnimationValue>& aValue)
> +    : mGecko{ 0 }

mGecko was removed a month ago, so this isn't needed.

::: layout/style/nsStyleCoord.h
@@ +435,4 @@
>  
>  inline nsStyleCoord::nsStyleCoord(const nsStyleUnion& aValue, nsStyleUnit aUnit)
>    : mUnit(eStyleUnit_Null)
> +  , mValue{}

(This shouldn't be necessary since the InitWithValue call will initialize mValue.  It's curious that the static analysis picked out this constructor, but not the previous one, which is very similar and also doesn't initialize mValue before calling InitWithValue.)

::: layout/style/nsStyleStruct.cpp
@@ +311,5 @@
>    , mBoxDecorationBreak(StyleBoxDecorationBreak::Slice)
> +  , mBorderTopColor{}
> +  , mBorderRightColor{}
> +  , mBorderBottomColor{}
> +  , mBorderLeftColor{}

These four are initialized in the body of the constructor (in the NS_FOR_CSS_SIDES loop), so we shouldn't need this.  If it's imperative to initialize them here to satisfy the static analysis, please remove the initialization below, and change these to explicitly initialize them like

  , mBorderTopColor(StyleComplexColor::CurrentColor())
  ...

::: layout/style/nsStyleStruct.h
@@ +899,5 @@
> +    : mXOffset{ 0 }
> +    , mYOffset{ 0 }
> +    , mRadius{ 0 }
> +    , mSpread{ 0 }
> +    , mColor{ 0 }

Use NS_RGB(0, 0, 0) for mColor's value.
Attachment #8982712 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #83)
> ::: layout/style/nsStyleStruct.cpp
> @@ +311,5 @@
> >    , mBoxDecorationBreak(StyleBoxDecorationBreak::Slice)
> > +  , mBorderTopColor{}
> > +  , mBorderRightColor{}
> > +  , mBorderBottomColor{}
> > +  , mBorderLeftColor{}
> 
> These four are initialized in the body of the constructor (in the
> NS_FOR_CSS_SIDES loop), so we shouldn't need this.  If it's imperative to
> initialize them here to satisfy the static analysis, please remove the
> initialization below, and change these to explicitly initialize them like
> 
>   , mBorderTopColor(StyleComplexColor::CurrentColor())
>   ...

This is coincidentally already happening in the bug 1465307 part 2 patch.
Attachment #8982698 - Flags: review?(karlt) → review+
Is there a description of the analysis algorithm as discussed in
https://bugzilla.mozilla.org/show_bug.cgi?id=525063#c185, please?

Particularly, what false positives should be expected, and what cases are not
analysed?
Attachment #8982680 - Flags: review?(josh) → review+
Please initialize using () and not {} in CC, if possible.
(In reply to Olli Pettay [:smaug] from comment #87)
> Please initialize using () and not {} in CC, if possible.
Sure will do that for the rest of the patches that are r+.
Attachment #8982681 - Attachment is obsolete: true
Attachment #8982704 - Attachment is obsolete: true
Comment on attachment 8982705 [details] [diff] [review]
RDF

RDF is gone
Attachment #8982705 - Attachment is obsolete: true
Comment on attachment 8982714 [details] [diff] [review]
Testing Infrastructure

I don't think that this should be pushed, It's a third party and also is marked in our static analysis to be skipped:

https://dxr.mozilla.org/mozilla-central/source/tools/rewriting/ThirdPartyPaths.txt#80
Attachment #8982714 - Attachment is obsolete: true
Attachment #8982710 - Attachment is obsolete: true
Assignee: nobody → bpostelnicu
Attached patch XPCom-String.patch (obsolete) — Splinter Review
Attachment #8985326 - Flags: review?(dbaron)
Attached patch Layout.patch (obsolete) — Splinter Review
Attachment #8982696 - Attachment is obsolete: true
Attachment #8985457 - Flags: review?(dbaron)
Attached patch dom_filesystem.patch (obsolete) — Splinter Review
Attachment #8982685 - Attachment is obsolete: true
Attachment #8985465 - Flags: review?(amarchesini)
Attached patch dom_events.patch (obsolete) — Splinter Review
Attachment #8982687 - Attachment is obsolete: true
Attachment #8985467 - Flags: review?(bugs)
Attached patch I18N.patch (obsolete) — Splinter Review
Attachment #8982692 - Attachment is obsolete: true
Attachment #8985469 - Flags: review?(jfkthame)
Comment on attachment 8985465 [details] [diff] [review]
dom_filesystem.patch

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

::: dom/filesystem/FileSystemTaskBase.h
@@ +188,4 @@
>  public:
>    FileSystemTaskParentBase()
>      : Runnable("FileSystemTaskParentBase")
> +    , mErrorValue(NS_ERROR_NOT_INITIALIZED) {}

{} must be in a new line.
Attachment #8985465 - Flags: review?(amarchesini) → review+
Attached patch jit.patch (obsolete) — Splinter Review
Attachment #8982695 - Attachment is obsolete: true
Attachment #8985483 - Flags: review?(jdemooij)
Attachment #8985467 - Flags: review?(bugs) → review+
Attached patch dom_media.patch (obsolete) — Splinter Review
Attachment #8982700 - Attachment is obsolete: true
Attachment #8985566 - Flags: review?(jyavenard)
Attached patch storage.patch (obsolete) — Splinter Review
Attachment #8982709 - Attachment is obsolete: true
Attachment #8985567 - Flags: review?(mak77)
Attached patch mftb.patch (obsolete) — Splinter Review
Attachment #8982701 - Attachment is obsolete: true
Attachment #8985578 - Flags: review?(jwalden+bmo)
Attached patch xslt.patch (obsolete) — Splinter Review
Attachment #8982729 - Attachment is obsolete: true
Attachment #8985579 - Flags: review?(peterv)
Attached patch xptoolkit.patch (obsolete) — Splinter Review
Attachment #8982728 - Attachment is obsolete: true
Attachment #8985584 - Flags: review?(jvarga)
Attached patch xpconnect.patch (obsolete) — Splinter Review
Attachment #8982727 - Attachment is obsolete: true
Attachment #8985585 - Flags: review?(peterv)
Attached patch xpcom.patch (obsolete) — Splinter Review
Attachment #8982726 - Attachment is obsolete: true
Attachment #8985589 - Flags: review?(nfroyd)
Attachment #8985567 - Flags: review?(mak77) → review+
Attached patch dom-xml.patch (obsolete) — Splinter Review
Attachment #8982725 - Attachment is obsolete: true
Attachment #8985601 - Flags: review?(peterv)
Attached patch dom-xbl.patch (obsolete) — Splinter Review
Attachment #8982724 - Attachment is obsolete: true
Attachment #8985602 - Flags: review?(mrbkap)
Attached patch widget_osx.patch (obsolete) — Splinter Review
Attachment #8982723 - Attachment is obsolete: true
Attachment #8985610 - Flags: review?(nfroyd)
Attached patch widget.patch (obsolete) — Splinter Review
Attachment #8982722 - Attachment is obsolete: true
Attachment #8985617 - Flags: review?(nfroyd)
FYI, Coverity is detecting the fixes "Defects eliminated: 68"
bravo!
Attached patch dom-workers.patch (obsolete) — Splinter Review
Attachment #8982720 - Attachment is obsolete: true
Attachment #8985618 - Flags: review?(nfroyd)
Attached patch webvr.patch (obsolete) — Splinter Review
Attachment #8982719 - Attachment is obsolete: true
Attachment #8985630 - Flags: review?(kgilbert)
Attached patch webrtc-signaling.patch (obsolete) — Splinter Review
Attachment #8982718 - Attachment is obsolete: true
Attachment #8985632 - Flags: review?(rjesup)
Attached patch webrtc-media.patch (obsolete) — Splinter Review
Attachment #8982717 - Attachment is obsolete: true
Attachment #8985635 - Flags: review?(rjesup)
Attached patch webrtc.patch (obsolete) — Splinter Review
Attachment #8982716 - Attachment is obsolete: true
Attachment #8985636 - Flags: review?(rjesup)
Attached patch web-painting.patch (obsolete) — Splinter Review
Attachment #8982715 - Attachment is obsolete: true
Attachment #8985640 - Flags: review?(mstange)
Comment on attachment 8985578 [details] [diff] [review]
mftb.patch

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

::: mfbt/SegmentedVector.h
@@ +59,5 @@
>    {
> +    SegmentImpl()
> +      : mLength(0)
> +    {
> +      mStorage.mAlign = {};

This is the same initialize-to-all-zeroes thing I had a problem with in the previous iteration of this patch.  Still waiting for an answer as to how to mark the field as initialized-late or something similar.

::: mfbt/SmallPointerArray.h
@@ +38,5 @@
>  {
>  public:
>    SmallPointerArray()
> +    : mPadding(nullptr)
> +    , mArray(nullptr)

Neither of these fields exists on trunk any more.  And indeed, judging by the "List-initialization would be nicer" comment inside this constructor body, you've generated this patch against that selfsame trunk code.  So this patch straightforwardly does not compile.  What gives?
Attachment #8985578 - Flags: review?(jwalden+bmo) → review-
Comment on attachment 8985632 [details] [diff] [review]
webrtc-signaling.patch

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

Otherwise ok

::: media/webrtc/signaling/src/jsep/JsepCodecDescription.h
@@ +236,5 @@
>          mTmmbrEnabled(false),
>          mRembEnabled(false),
>          mFECEnabled(false),
> +        mREDPayloadType('\0'),
> +        mULPFECPayloadType('\0'),

these are uint8_t's; they should be initialized to 0 not '\0'
Attachment #8985632 - Flags: review?(rjesup) → review-
Attachment #8985635 - Flags: review?(rjesup) → review+
Comment on attachment 8985640 [details] [diff] [review]
web-painting.patch

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

::: layout/painting/DisplayItemClipChain.h
@@ +72,3 @@
>    {}
>  
>    DisplayItemClipChain()

I think this constructor is unused. So it should probably be removed entirely.

::: layout/painting/nsCSSRendering.cpp
@@ +77,5 @@
>  struct InlineBackgroundData
>  {
>    InlineBackgroundData()
> +      : mFrame(nullptr), mLineContainer(nullptr),
> +        mContinuationPoint(0), mUnbrokenMeasure(0), 

end-of-line whitespace
Attachment #8985640 - Flags: review?(mstange) → review+
Attached patch webrtc-signaling.patch (obsolete) — Splinter Review
Attachment #8985632 - Attachment is obsolete: true
Attachment #8985651 - Flags: review?(rjesup)
Comment on attachment 8985636 [details] [diff] [review]
webrtc.patch

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

::: netwerk/sctp/datachannel/DataChannel.h
@@ +71,5 @@
> +    : mLength(0)
> +    , mData(nullptr)
> +    , mInfo(nullptr)
> +    , mPos(0) 
> +  {}; 

remove trailing spaces from this line and the previous one
This does have some very minor side-effects (not using ==default makes it non-trivial, non-pod, etc -- if it was before).  However, this doesn't matter here
Attachment #8985636 - Flags: review?(rjesup) → review+
Attachment #8985651 - Flags: review?(rjesup) → review+
(In reply to Jeff Walden [:Waldo] from comment #134)
> Comment on attachment 8985578 [details] [diff] [review]
> mftb.patch
> 
> Review of attachment 8985578 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/SegmentedVector.h
> @@ +59,5 @@
> >    {
> > +    SegmentImpl()
> > +      : mLength(0)
> > +    {
> > +      mStorage.mAlign = {};
> 
> This is the same initialize-to-all-zeroes thing I had a problem with in the
> previous iteration of this patch.  Still waiting for an answer as to how to
> mark the field as initialized-late or something similar.
> 
> ::: mfbt/SmallPointerArray.h
> @@ +38,5 @@
> >  {
> >  public:
> >    SmallPointerArray()
> > +    : mPadding(nullptr)
> > +    , mArray(nullptr)
> 
> Neither of these fields exists on trunk any more.  And indeed, judging by
> the "List-initialization would be nicer" comment inside this constructor
> body, you've generated this patch against that selfsame trunk code.  So this
> patch straightforwardly does not compile.  What gives?

Sorry for the bad patch, It seems I've based this patch on an earlier revision, will update it shortly. If you feel that you want to exclude a variable for the initialization check, you can declare it with the annotation:

>>MOZ_INIT_OUTSIDE_CTOR type a;
Comment on attachment 8985630 [details] [diff] [review]
webvr.patch

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

This LGTM, thanks!
Attachment #8985630 - Flags: review?(kgilbert) → review+
(In reply to Markus Stange [:mstange] from comment #136)
> Comment on attachment 8985640 [details] [diff] [review]
> web-painting.patch
> 
> Review of attachment 8985640 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/painting/DisplayItemClipChain.h
> @@ +72,3 @@
> >    {}
> >  
> >    DisplayItemClipChain()
> 
> I think this constructor is unused. So it should probably be removed
> entirely.
> 

Hm....I'm not so sure because of this:

>>class DisplayListClipState::AutoClipMultiple : public AutoSaveRestore {
>>public:
>>  explicit AutoClipMultiple(nsDisplayListBuilder* aBuilder)
>>    : AutoSaveRestore(aBuilder)
>>#ifdef DEBUG
>>    , mExtraClipUsed(false)
>>#endif
>>  {}

and DisplayListClipState has this member:
>>DisplayItemClipChain mExtraClipChain;

And it will try to use the default constructor that doesn't exist any more.
Comment on attachment 8985602 [details] [diff] [review]
dom-xbl.patch

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

::: dom/xbl/nsXBLPrototypeHandler.cpp
@@ +122,5 @@
>    : mHandlerText(nullptr),
>      mLineNumber(0),
> +    mPhase('\0'),
> +    mType('\0'),
> +    mMisc('\0'),

These three members are uint_8, not char, so should be initialized to 0.
Attachment #8985602 - Flags: review?(mrbkap) → review+
Attachment #8985469 - Flags: review?(jfkthame) → review+
Attached patch svg.patch (obsolete) — Splinter Review
Attachment #8982713 - Attachment is obsolete: true
Attachment #8982713 - Flags: review?(jwatt)
Attachment #8985771 - Flags: review?(jwatt)
Attached patch libjar.patch (obsolete) — Splinter Review
Attachment #8982697 - Attachment is obsolete: true
Attachment #8982697 - Flags: review?(aklotz)
Attachment #8985772 - Flags: review?(aklotz)
Attached patch js.patch (obsolete) — Splinter Review
Attachment #8982694 - Attachment is obsolete: true
Attachment #8982694 - Flags: review?(jorendorff)
Attachment #8985773 - Flags: review?(jorendorff)
Attached patch devtools.patch (obsolete) — Splinter Review
Attachment #8982682 - Attachment is obsolete: true
Attachment #8982682 - Flags: review?(jimb)
Attachment #8985774 - Flags: review?(jimb)
Attached patch dom.patch (obsolete) — Splinter Review
Attachment #8982684 - Attachment is obsolete: true
Attachment #8982684 - Flags: review?(peterv)
Attachment #8985778 - Flags: review?(peterv)
Attached patch gfx.patch (obsolete) — Splinter Review
Since Jeff is in parental leave, do you think you can review this Markus? If you think that someone else is more fit please feel free to forward this review.
Attachment #8982688 - Attachment is obsolete: true
Attachment #8982688 - Flags: review?(jmuizelaar)
Attachment #8985789 - Flags: review?(mstange)
Comment on attachment 8985771 [details] [diff] [review]
svg.patch

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

r+ with comments addressed.

::: dom/svg/DOMSVGPathSegList.h
@@ +233,5 @@
> +    ItemProxy()
> +      : mItem(nullptr)
> +      , mInternalDataIndex(0)
> +    {
> +    }

Keep the opening and closing curly bracket on the same line please.

::: dom/svg/SVGMotionSMILType.cpp
@@ +65,5 @@
>    // other fields uninitialized (since client is presumably about to set them)
>    MotionSegment()
> +    : mRotateType(static_cast<RotateType>(0)),
> +      mRotateAngle(0.0), mSegmentType(eSegmentType_Translation),
> +      mU{}

Fix the formatting to the following, please:

  MotionSegment()
    : mRotateType(eRotateType_Auto)
    , mRotateAngle(0.0)
    , mSegmentType(eSegmentType_Translation)
    , mU
  {}

And don't use static_cast<RotateType>(0), use eRotateType_Auto.

::: layout/svg/SVGContextPaint.h
@@ +192,5 @@
>    struct Paint {
> +    Paint()
> +      : mPaintDefinition{}
> +      , mPaintType(eStyleSVGPaintType_None) {
> +    }

Bring the opening curly bracket down onto the same line as the closing curly bracket.
Attachment #8985771 - Flags: review?(jwatt) → review+
Attached patch mfbt.patch (obsolete) — Splinter Review
Attachment #8985578 - Attachment is obsolete: true
Attachment #8985820 - Flags: review?(jwalden+bmo)
Attached patch docshell.patch (obsolete) — Splinter Review
Attachment #8982683 - Attachment is obsolete: true
Attachment #8985886 - Flags: review?(bugs)
(In reply to Boris Zbarsky [:bz] (Vacation Jun 16-24) (no decent commit message means r-) from comment #61)
> Comment on attachment 8982683 [details] [diff] [review]
> 
> >+++ b/docshell/base/nsDocShell.cpp	Thu May 31 20:45:09 2018 -0700
> >+  this->mHistoryID.m0 = { 0 };
> 
> Soo.. I understand why we're doing this; the GenerateUUIDInPlace call might
> technically fail to initialize the memory, so this seems like a real issue. 
> However:
> 
> 1)  We're not initializing m3 anyway.
> 2)  Initializing to 0 doesn't seem like the right thing in the failure case:
> it'll pretty much guarantee that the uuid is not unique, which is bad for a
> uuid.
> 
> It's probably best to file a followup bug on figuring out what should happen
> here.  This change is maybe OK to quiet the static analysis for now, with a
> comment pointing to the followup.  Maybe we can make it clearer that uuid
> generation can't fail.
> 
First off let me thank you for the detailed review! regarding [1] m3 is of type 'uint8_t [8]' which we don't currently support.
See Also: → 1469240
Comment on attachment 8985886 [details] [diff] [review]
docshell.patch

I've also created a followup bud in case this patch gets accepted.
https://bugzilla.mozilla.org/show_bug.cgi?id=1469240
Comment on attachment 8985566 [details] [diff] [review]
dom_media.patch

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

r=me with comments addressed, a few bracket initializers got missed.

::: dom/media/BufferReader.h
@@ +21,5 @@
>  public:
> +  BufferReader()
> +    : mPtr(nullptr)
> +    , mRemaining(0)
> +    , mLength{ 0 }

use of { }

::: dom/media/MediaCache.h
@@ +419,5 @@
> +      explicit Entry(KeyTypePointer aKey)
> +        : nsUint32HashKey(aKey)
> +        , mNextBlock(0)
> +        , mPrevBlock(0)
> +      { }

{ } on new line

::: dom/media/StreamTracks.h
@@ +250,5 @@
>       * Iterate through the tracks of aBuffer in order of ID.
>       */
>      explicit TrackIter(const StreamTracks& aBuffer) :
> +      mBuffer(&aBuffer.mTracks), mIndex(0),
> +      mType(static_cast<MediaSegment::Type>(0)), mMatchType(false) {}

may as well correct the style while at it, one initialization per line

::: dom/media/ogg/OggWriter.cpp
@@ +18,2 @@
>  {
> +  mOggStreamState.body_data = nullptr;

Can just do mOggStreamState()
no need to manually initialize all members.

::: dom/media/platforms/apple/AppleATDecoder.cpp
@@ +28,4 @@
>    , mFileStreamError(false)
>    , mTaskQueue(aTaskQueue)
>    , mConverter(nullptr)
> +  , mOutputFormat{}

don't use brackets

::: dom/media/webspeech/synth/cocoa/OSXSpeechSynthesizerService.mm
@@ +109,4 @@
>                                         const nsTArray<size_t>& aOffsets)
>    : mTask(aTask)
>    , mSpeechSynthesizer(aSynth)
> +  , mCurrentIndex(0), mOffsets(aOffsets)

on new line

::: dom/media/webspeech/synth/ipc/SpeechSynthesisParent.h
@@ +80,4 @@
>    friend class SpeechSynthesisRequestParent;
>  public:
>    SpeechTaskParent(float aVolume, const nsAString& aUtterance, bool aIsChrome)
> +    : nsSpeechTask(aVolume, aUtterance, aIsChrome), mActor(nullptr) {}

on new line

::: media/gmp-clearkey/0.1/RefCounted.h
@@ +62,1 @@
>      Set(aPtr);

this is unnecessary, that's what the call to Set does in the constructor!
Attachment #8985566 - Flags: review?(jyavenard) → review+
Comment on attachment 8985886 [details] [diff] [review]
docshell.patch

>+++ b/dom/ipc/ContentParent.cpp
>@@ -3505,14 +3505,16 @@ ContentParent::AllocPExternalHelperAppPa
>                                              const OptionalURIParams& aReferrer,
>                                              PBrowserParent* aBrowser)
> {
>-  ExternalHelperAppParent *parent =
>-    new ExternalHelperAppParent(uri, aContentLength, aWasFileChannel);
>+  ExternalHelperAppParent* parent =
>+    new ExternalHelperAppParent(uri,
>+                                aContentLength,
>+                                aWasFileChannel,
>+                                aContentDisposition,
>+                                aContentDispositionHint,
>+                                aContentDispositionFilename);
>   parent->AddRef();
>   parent->Init(this,
>                aMimeContentType,
>-               aContentDisposition,
>-               aContentDispositionHint,
>-               aContentDispositionFilename,
>                aForceSave,
>                aReferrer,
>                aBrowser);
aha, params which are used to initialize member variables are moved

>+  mContentDispositionHeader = aContentDispositionHeader;
>+  if (!mContentDispositionHeader.IsEmpty()) {
>+    NS_GetFilenameFromDisposition(mContentDispositionFilename,
>+                                  mContentDispositionHeader,
>+                                  mURI);
>+    mContentDisposition =
>+      NS_GetContentDispositionFromHeader(mContentDispositionHeader, this);
>+  }
>+  else {
Nit, should be } else {
Attachment #8985886 - Flags: review?(bugs) → review+
Attached patch psm-security.patch (obsolete) — Splinter Review
I don't find where ROOT_CERTIFICATE_UNKNOWN, that's why I didn't change it.
Attachment #8982706 - Attachment is obsolete: true
Attachment #8985945 - Flags: review?(dkeeler)
> Is there a way I can affirmatively indicate "I choose not to initialize this"?

I asked Botond to ask the C++ standard committee for this, I think a year or two ago.

It is a weird thing to ask for; IIRC nothing came of it, but let's get that on the record. ==> ni?botond
Flags: needinfo?(botond)
(In reply to Jason Orendorff [:jorendorff] from comment #170)
> > Is there a way I can affirmatively indicate "I choose not to initialize this"?
> 
> I asked Botond to ask the C++ standard committee for this, I think a year or
> two ago.
> 
> It is a weird thing to ask for; IIRC nothing came of it, but let's get that
> on the record. ==> ni?botond

For our static-analysis you can use MOZ_INIT_OUTSIDE_CTOR in order to instruct the analysis not to track that member.
Comment on attachment 8985773 [details] [diff] [review]
js.patch

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

Forwarding the js/src review to Jeff.
Attachment #8985773 - Flags: review?(jorendorff) → review?(jwalden+bmo)
Comment on attachment 8985618 [details] [diff] [review]
dom-workers.patch

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

::: dom/workers/WorkerScope.cpp
@@ +745,5 @@
>    explicit ReportFetchListenerWarningRunnable(const nsString& aScope)
>      : mozilla::Runnable("ReportFetchListenerWarningRunnable")
>      , mScope(NS_ConvertUTF16toUTF8(aScope))
> +    , mLine(0)
> +    , mColumn(0)

This seems pretty useless; we're just going to write to `mLine` and `mColumn` a little further down in the constructor.  Does the analysis not handle cases like this?
Attachment #8985618 - Flags: review?(nfroyd) → review-
Comment on attachment 8985617 [details] [diff] [review]
widget.patch

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

::: widget/IMEData.h
@@ +527,4 @@
>  {
>    IMENotification()
>      : mMessage(NOTIFY_IME_OF_NOTHING)
> +    , mSelectionChangeData{}

I'm unsure if there was consensus on dev-platform, but until such is achieved, I think we want () instead of {}.

@@ +544,4 @@
>  
>    MOZ_IMPLICIT IMENotification(IMEMessage aMessage)
>      : mMessage(aMessage)
> +    , mSelectionChangeData{}

Same here.

::: widget/InputData.cpp
@@ +94,4 @@
>  
>  MultiTouchInput::MultiTouchInput()
>    : InputData(MULTITOUCH_INPUT)
> +  , mType{ static_cast<MultiTouchType>(0) }

Several things here:

1) You use {} for initializing mType here, and () below.  Please be consistent, and choose the latter.
2) mType is a proper enum, with a value at 0 (MULTITOUCH_START), so we should be using that instead, rather than blindly casting 0 and expecting that to Just Work.

Ideally, we wouldn't need this constructor at all; we'd have some static function or a separate constructor to produce a properly-initialized object.  Maybe that would be good for a followup bug?

@@ +110,4 @@
>  MultiTouchInput::MultiTouchInput(const WidgetTouchEvent& aTouchEvent)
>    : InputData(MULTITOUCH_INPUT, aTouchEvent.mTime, aTouchEvent.mTimeStamp,
>                aTouchEvent.mModifiers)
> +  , mType(static_cast<MultiTouchType>(0))

Same comment about using a proper value of the enumeration applies here.

@@ +496,4 @@
>  
>  PanGestureInput::PanGestureInput()
>    : InputData(PANGESTURE_INPUT)
> +  , mType(static_cast<PanGestureType>(0))

Different enum type, but the same comment applies here as well.

@@ +597,4 @@
>  
>  PinchGestureInput::PinchGestureInput()
>    : InputData(PINCHGESTURE_INPUT)
> +  , mType(static_cast<PinchGestureType>(0))

And here.

@@ +647,4 @@
>  
>  TapGestureInput::TapGestureInput()
>    : InputData(TAPGESTURE_INPUT)
> +  , mType(static_cast<TapGestureType>(0))

And here.

@@ +684,5 @@
>  
>  ScrollWheelInput::ScrollWheelInput()
>    : InputData(SCROLLWHEEL_INPUT)
> +  , mDeltaType(static_cast<ScrollDeltaType>(0))
> +  , mScrollMode(static_cast<ScrollMode>(0))

And here.

@@ +874,4 @@
>  
>  KeyboardInput::KeyboardInput()
>    : InputData(KEYBOARD_INPUT)
> +  , mType(static_cast<KeyboardEventType>(0))

And here.
Attachment #8985617 - Flags: review?(nfroyd) → review-
Comment on attachment 8985610 [details] [diff] [review]
widget_osx.patch

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

::: widget/cocoa/TextInputHandler.mm
@@ +4446,4 @@
>    , mPendingMethods(0)
>    , mIMECompositionString(nullptr)
>    , mIMECompositionStart(UINT32_MAX)
> +  , mRangeForWritingMode{}

() instead of {}, please.

::: widget/cocoa/nsLookAndFeel.mm
@@ +38,4 @@
>   , mUseOverlayScrollbarsCached(false)
>   , mAllowOverlayScrollbarsOverlap(-1)
>   , mAllowOverlayScrollbarsOverlapCached(false)
> + , mColorTextSelectBackground(0)

Bleh, a whole bunch of boilerplate simply because nscolor isn't an actual struct (for good reasons, I think).

::: widget/cocoa/nsMacDockSupport.mm
@@ +14,4 @@
>  nsMacDockSupport::nsMacDockSupport()
>  : mAppIcon(nil)
>  , mProgressBackground(nil)
> +, mProgressBounds{}

() instead of {}, please.
Attachment #8985610 - Flags: review?(nfroyd) → review-
Comment on attachment 8985589 [details] [diff] [review]
xpcom.patch

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

r- given the PrioritizedEventQueue.cpp and Queue.h comments.  Didn't check everything as a result.

::: xpcom/base/nsTraceRefcnt.cpp
@@ +306,4 @@
>  public:
>    BloatEntry(const char* aClassName, uint32_t aClassSize)
>      : mClassSize(aClassSize)
> +    , mStats{}

() instead of {}, please.

::: xpcom/ds/nsTArray.h
@@ +2448,4 @@
>    typedef typename base_type::elem_type elem_type;
>  
>    AutoTArray()
> +    : mAlign{}

() instead of {}, please, all throughout this file.

::: xpcom/io/nsInputStreamTee.cpp
@@ +135,5 @@
>  };
>  
> +nsInputStreamTee::nsInputStreamTee()
> +  : mWriter(nullptr)
> +  , mClosure(nullptr)

Please file a followup for moving these two members out of nsInputStreamTee.

::: xpcom/threads/PrioritizedEventQueue.cpp
@@ +29,3 @@
>    , mIdlePeriod(aIdlePeriod)
> +  , mHasPendingEventsPromisedIdleEvent(false)
> +  , mInputQueueState(STATE_DISABLED)

These are all initialized in the class definition.  If the analysis can't handle such initializers, then the analysis needs to be fixed, rather than duplicating code like this or moving the initializers to the constructor.

::: xpcom/threads/Queue.h
@@ +24,5 @@
> +  Queue()
> +    : mHead(nullptr)
> +    , mTail(nullptr)
> +    , mOffsetHead(0)
> +    , mOffsetTail(0)

Same here.
Attachment #8985589 - Flags: review?(nfroyd) → review-
(In reply to Nathan Froyd [:froydnj] from comment #173)
> Comment on attachment 8985618 [details] [diff] [review]
> dom-workers.patch
> 
> Review of attachment 8985618 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/workers/WorkerScope.cpp
> @@ +745,5 @@
> >    explicit ReportFetchListenerWarningRunnable(const nsString& aScope)
> >      : mozilla::Runnable("ReportFetchListenerWarningRunnable")
> >      , mScope(NS_ConvertUTF16toUTF8(aScope))
> > +    , mLine(0)
> > +    , mColumn(0)
> 
> This seems pretty useless; we're just going to write to `mLine` and
> `mColumn` a little further down in the constructor.  Does the analysis not
> handle cases like this?

I agree with you, it's because of this: https://dxr.mozilla.org/mozilla-central/source/js/src/jsapi.cpp?q=%2Bfunction%3A%22JS%3A%3ADescribeScriptedCaller%28JSContext+%2A%2C+JS%3A%3AAutoFilename+%2A%2C+unsigned+int+%2A%2C+unsigned+int+%2A%29%22&redirect_type=single#7494

The way how the analysis works now if it encounters an ifStmt it builds a map with the variables that are initialized on the two branches and compares the maps. If the maps don't correlate than it marks the variables that are not present in the both maps as uninit. In this particular case the static analysis it's pretty stupid, I admit. I will discard the patch. Thanks for noticing it.
Attachment #8985618 - Attachment is obsolete: true
Attached patch widget_osx.patch (obsolete) — Splinter Review
Attachment #8986012 - Flags: review?(nfroyd)
Attachment #8985610 - Attachment is obsolete: true
Attached patch widget.patch (obsolete) — Splinter Review
Attachment #8985617 - Attachment is obsolete: true
Attachment #8986019 - Flags: review?(nfroyd)
See Also: → 1469365
Nathan, thank you for noticing that in-class initializers are not always taken into account. They are supposed to (and we have a working test case for that), since we go through all the constructor initializers and Clang is supposed to convert in-class inits into implicit constructor initializers.

However, it apparently doesn't do it for every single in-class init (not sure whether that's a bug or if it's on purpose for reasons I ignore). Anyway, I'm going to add an explicit check when going through the class fields so that we're certain this problem doesn't come up anymore.
Comment on attachment 8985945 [details] [diff] [review]
psm-security.patch

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

Please remember to include 8 lines of context - it's harder to review with only 3.
In any case, r=me with comments addressed.

::: security/certverifier/CertVerifier.h
@@ +79,5 @@
>  {
>  public:
> +  PinningTelemetryInfo()
> +    : certPinningResultBucket(0)
> +    , rootBucket(0)

ROOT_CERTIFICATE_UNKNOWN is defined here: https://searchfox.org/mozilla-central/rev/285da1fd7dcf67448b9175741fa330158edcff73/security/manager/ssl/RootCertificateTelemetryUtils.h#18

::: security/pkix/lib/pkixder.h
@@ +457,4 @@
>  
>  // x.509 and OCSP both use this same version numbering scheme, though OCSP
>  // only supports v1.
> +enum class Version { v1 = 0, v2 = 1, v3 = 2, v4 = 3, Uninitialized = 4 };

Just in case we ever get an x509 version 5 (heaven forbid), we should make Uninitialized something like 255.

::: security/pkix/lib/pkixverify.cpp
@@ +79,5 @@
>      case der::PublicKeyAlgorithm::RSA_PKCS1:
>        return trustDomain.VerifyRSAPKCS1SignedDigest(signedDigest,
>                                                      signerSubjectPublicKeyInfo);
> +    case der::PublicKeyAlgorithm::Uninitialized:
> +      // We should never reach this

This is a small nit, but I think comments like this should either be removed entirely or they should spell out exactly why we think we should never hit this case (in this situation it's because the value in question will get set in SignatureAlgorithmIdentifierValue because we call DigestSignedData first).

@@ +80,5 @@
>        return trustDomain.VerifyRSAPKCS1SignedDigest(signedDigest,
>                                                      signerSubjectPublicKeyInfo);
> +    case der::PublicKeyAlgorithm::Uninitialized:
> +      // We should never reach this
> +      assert(false);

Let's add a `return Result::FATAL_ERROR_LIBRARY_FAILURE;` line after this (so we do the safe thing in NDEBUG builds).
Attachment #8985945 - Flags: review?(dkeeler) → review+
Attached patch xpcom.patch (obsolete) — Splinter Review
Attachment #8985589 - Attachment is obsolete: true
Attachment #8986032 - Flags: review?(nfroyd)
Attached patch psm-security.patch (obsolete) — Splinter Review
I'm resubmitting this for review since besides the suggested modifications I've also added a new case der::PublicKeyAlgorithm::Uninitialized in CheckSignatureAlgorithm.
Attachment #8985945 - Attachment is obsolete: true
Attachment #8986043 - Flags: review?(dkeeler)
Comment on attachment 8986043 [details] [diff] [review]
psm-security.patch

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

Great - thanks!

::: security/pkix/lib/pkixcheck.cpp
@@ +117,5 @@
>        // for any curve that we support, the chances of us encountering a curve
>        // during path building is too low to be worth bothering with.
>        break;
> +    case der::PublicKeyAlgorithm::Uninitialized:
> +    {

nit: braces shouldn't be necessary here
Attachment #8986043 - Flags: review?(dkeeler) → review+
Comment on attachment 8985820 [details] [diff] [review]
mfbt.patch

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

::: mfbt/SegmentedVector.h
@@ +106,1 @@
>      union Storage

I just changed how this storage is implemented in bug 1469003, so you shouldn't need to touch this file at all now.  (No need for a look at the revised patch 'cause you're just removing changes.)
Attachment #8985820 - Flags: review?(jwalden+bmo) → review+
(In reply to Jason Orendorff [:jorendorff] from comment #170)
> > Is there a way I can affirmatively indicate "I choose not to initialize this"?
> 
> I asked Botond to ask the C++ standard committee for this, I think a year or
> two ago.
> 
> It is a weird thing to ask for; IIRC nothing came of it, but let's get that
> on the record. ==> ni?botond

It came up in the context of a paper submitted last year called "Proposal of [[uninitialized]] attribute" [1]. When the committee's Evolution Working Group reviewed the paper last November, people felt it wasn't sufficiently motivated because compilers these days are pretty good about only warning about _uses_ (reads) of uninitialized memory, rather than the mere declaration / creation of it.

[1] http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0632r0.html
Flags: needinfo?(botond)
Attached patch libjar.patch (obsolete) — Splinter Review
Attachment #8985772 - Attachment is obsolete: true
Attachment #8985772 - Flags: review?(aklotz)
Attachment #8986104 - Flags: review?(aklotz)
Attached patch js.patch (obsolete) — Splinter Review
Attachment #8985773 - Attachment is obsolete: true
Attachment #8985773 - Flags: review?(jwalden+bmo)
Attachment #8986127 - Flags: review?(jorendorff)
Attached patch devtools.patch (obsolete) — Splinter Review
Attachment #8985774 - Attachment is obsolete: true
Attachment #8985774 - Flags: review?(jimb)
Attachment #8986131 - Flags: review?(jimb)
Attached patch dom.patch (obsolete) — Splinter Review
Attachment #8985778 - Attachment is obsolete: true
Attachment #8985778 - Flags: review?(peterv)
Attachment #8986169 - Flags: review?(peterv)
Attached patch gfx.patch (obsolete) — Splinter Review
Attachment #8985789 - Attachment is obsolete: true
Attachment #8985789 - Flags: review?(mstange)
Attachment #8986175 - Flags: review?(jmuizelaar)
Attached patch jit.patch (obsolete) — Splinter Review
Attachment #8985483 - Attachment is obsolete: true
Attachment #8985483 - Flags: review?(jdemooij)
Attachment #8986193 - Flags: review?(jdemooij)
Attachment #8986127 - Flags: review?(jorendorff) → review?(jwalden+bmo)
Attached patch Layout.patch (obsolete) — Splinter Review
Attachment #8985457 - Attachment is obsolete: true
Attachment #8985457 - Flags: review?(dbaron)
Attachment #8986200 - Flags: review?(dbaron)
Attachment #8986032 - Flags: review?(nfroyd) → review+
Comment on attachment 8986012 [details] [diff] [review]
widget_osx.patch

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

Markus is probably a better reviewer here than I am.

::: widget/cocoa/nsMacDockSupport.mm
@@ -108,4 @@
>    }
>  
>    if (mProgressState == STATE_NORMAL || mProgressState == STATE_INDETERMINATE) {
> -    int perSecond = 8; // Empirically determined, see bug 848792 

Please revert this change.
Attachment #8986012 - Flags: review?(nfroyd) → review?(mstange)
Comment on attachment 8986019 [details] [diff] [review]
widget.patch

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

I'm not a widget peer, and looking over this, I'm not sure that the better solution in a lot of cases is to just rewrite the code so that we always create fully initialized things, rather than initializing things with bogus state, only to be overwritten later.  :jimm might have an opinion on this?

::: widget/TextEvents.h
@@ +1142,4 @@
>      , mExpandToClusterBoundary(true)
>      , mSucceeded(false)
>      , mUseNativeLineBreak(true)
> +    , mReason(0)

This looks like it should be initialized with nsISelectionListener::NO_REASON, but I am not enough of a widget expert to say whether this is definitely the case.
Attachment #8986019 - Flags: review?(nfroyd) → review?(jmathies)
Comment on attachment 8986012 [details] [diff] [review]
widget_osx.patch

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

r+ with Nathan's comment 197 addressed.
Attachment #8986012 - Flags: review?(mstange) → review+
(In reply to Nathan Froyd [:froydnj] from comment #198)
> Comment on attachment 8986019 [details] [diff] [review]
> widget.patch
> 
> Review of attachment 8986019 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm not a widget peer, and looking over this, I'm not sure that the better
> solution in a lot of cases is to just rewrite the code so that we always
> create fully initialized things, rather than initializing things with bogus
> state, only to be overwritten later.  :jimm might have an opinion on this?
> 
> ::: widget/TextEvents.h
> @@ +1142,4 @@
> >      , mExpandToClusterBoundary(true)
> >      , mSucceeded(false)
> >      , mUseNativeLineBreak(true)
> > +    , mReason(0)
> 
> This looks like it should be initialized with
> nsISelectionListener::NO_REASON, but I am not enough of a widget expert to
> say whether this is definitely the case.

Thanks for forwarding this to the right person! regarding |mReason| I think you're right since looking down the road into TestEvents.h we have:


>>  WidgetSelectionEvent(bool aIsTrusted, EventMessage aMessage,
>>                       nsIWidget* aWidget)
>>    : WidgetGUIEvent(aIsTrusted, aMessage, aWidget, eSelectionEventClass)
>>    , mOffset(0)
>>    , mLength(0)
>>    , mReversed(false)
>>    , mExpandToClusterBoundary(true)
>>    , mSucceeded(false)
>>    , mUseNativeLineBreak(true)
>>    , mReason(nsISelectionListener::NO_REASON)
>>  {
>>  }

The problem here lies with the fact that the 'fixes' for these uninit variables are done automatically by this patch [1]. It statically determines what's the best value depending on it's underlying type. So there is no contextual coherence when choosing the value besides it's type. And I don't think we can improve on this much, but at least during the review phase we can have this sort of iteration to the initial patch.

https://bug525063.bmoattachments.org/attachment.cgi?id=8986035&t=0Tr1orSdun52zqeA4e0LTU
Attached patch widget.patch (obsolete) — Splinter Review
Attachment #8986019 - Attachment is obsolete: true
Attachment #8986019 - Flags: review?(jmathies)
Attachment #8986363 - Flags: review?(jmathies)
Comment on attachment 8986193 [details] [diff] [review]
jit.patch

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

Looks pretty good, some nits/questions below.

::: js/src/jit/BaselineFrameInfo.h
@@ +61,2 @@
>          // In debug builds, assert Kind is initialized.
>          , Uninitialized

Instead of this we should annotate this one IMO. We use this as an expression stack and there's no need to initialize all the slots in opt builds.

::: js/src/jit/IonCode.h
@@ +70,2 @@
>  
>      JitCode()

This constructor is unused I think - we only use the one below. Can you verify/remove?

::: js/src/jit/IonOptimizationLevels.h
@@ +150,4 @@
>      uint32_t inliningRecompileThresholdFactor_;
>  
>      OptimizationInfo()
> +      : level_(OptimizationLevel::Normal),

Hm mark this constructor constexpr? I think this is allocated statically so it would be good to make sure no extra static constructors are introduced. Also probably good to check number of static constructors on Windows because MSVC messes this up sometimes.

::: js/src/jit/MoveResolver.h
@@ +46,3 @@
>  
>    public:
>      MoveOperand()

Is this constructor dead? I see one use in Trampoline-x64.cpp on searchfox but that variable is unused?

@@ +211,3 @@
>  
>    public:
>      MoveOp()

Same here.

::: js/src/jit/RangeAnalysis.cpp
@@ +585,4 @@
>  }
>  
>  Range::Range(const MDefinition* def)
> +  : lower_(0),

These are initialized in the code below, right? Is there a part that confuses the analysis? Maybe we can fix it some other way.

::: js/src/jit/RegisterSets.h
@@ +247,4 @@
>    public:
>  
>      ConstantOrRegister()
> +      : constant_(false)

Could we remove this constructor and use mozilla::Maybe<ConstantOrRegister> where we use it?
Attachment #8986193 - Flags: review?(jdemooij)
Attached patch jit.patch (obsolete) — Splinter Review
As per our discussion on IRC I'm attaching this patch.
Attachment #8986193 - Attachment is obsolete: true
Attachment #8986449 - Flags: review?(jdemooij)
Attached patch jit.patch (obsolete) — Splinter Review
Attachment #8986449 - Attachment is obsolete: true
Attachment #8986449 - Flags: review?(jdemooij)
Attachment #8986467 - Flags: review?(jdemooij)
Comment on attachment 8986467 [details] [diff] [review]
jit.patch

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

Excellent, thanks! I'm glad we could |= delete;| a bunch of constructors. r=me with comments addressed.

::: js/src/jit/JitFrames.h
@@ +231,4 @@
>  
>    public:
>      FrameSizeClass()
> +      : class_(0)

Nit: can we = delete this constructor as well? Both searchfox and XDR claim it's unused.. Follow-up patch/bug is fine.

::: js/src/jit/RangeAnalysis.cpp
@@ +590,5 @@
> +    hasInt32LowerBound_(false),
> +    hasInt32UpperBound_(false),
> +    canHaveFractionalPart_(static_cast<FractionalPartFlag>(false)),
> +    canBeNegativeZero_(static_cast<NegativeZeroFlag>(false)),
> +    max_exponent_(0),

The extra initialization here should be removed because of the annotations we added + the follow-up issue for *this, right?
Attachment #8986467 - Flags: review?(jdemooij) → review+
(In reply to Jan de Mooij [:jandem] from comment #208)
> Both searchfox and XDR claim it's unused..

Er, DXR :) Muscle memory..
Attachment #8986363 - Flags: review?(jmathies) → review+
Attached patch XPCom-String.patch (obsolete) — Splinter Review
Attachment #8985326 - Attachment is obsolete: true
Attachment #8985326 - Flags: review?(dbaron)
Attachment #8986528 - Flags: review?(dbaron)
Attachment #8986528 - Flags: review?(dbaron) → review?(nfroyd)
Comment on attachment 8986131 [details] [diff] [review]
devtools.patch

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

The changes to AutoMemMap.h and HeapSnapshot.cpp look fine.

But CoreDump.pb.h is an automatically generated file, produced by the protobuf compiler from CoreDump.proto. Any changes made to CoreDump.pb.h will be overwritten the next time it's regenerated. So it's pointless to change this file.
Attachment #8986131 - Flags: review?(jimb)
Attached patch devtools.patch (obsolete) — Splinter Review
Attachment #8986131 - Attachment is obsolete: true
Attachment #8986540 - Flags: review?(jimb)
Comment on attachment 8986540 [details] [diff] [review]
devtools.patch

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

Looks good, thanks.
Attachment #8986540 - Flags: review?(jimb) → review+
Attachment #8986104 - Flags: review?(aklotz) → review+
Attached patch dom-xml.patch (obsolete) — Splinter Review
Attachment #8985601 - Attachment is obsolete: true
Attachment #8985601 - Flags: review?(peterv)
Attachment #8986683 - Flags: review?(peterv)
Attached patch xptoolkit.patch (obsolete) — Splinter Review
Attachment #8985584 - Attachment is obsolete: true
Attachment #8985584 - Flags: review?(jvarga)
Attachment #8986686 - Flags: review?(jvarga)
Attachment #8985579 - Flags: review?(peterv) → review+
Comment on attachment 8985585 [details] [diff] [review]
xpconnect.patch

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

::: js/xpconnect/src/BackstagePass.h
@@ +41,4 @@
>    void SetGlobalObject(JSObject* global);
>  
>    explicit BackstagePass(nsIPrincipal* prin) :
> +    mPrincipal(prin), mWrapper(nullptr)

Please put mWrapper on a separate line.

::: js/xpconnect/src/xpcprivate.h
@@ +298,2 @@
>          mNext = nullptr;
>          mSelfp = nullptr;

Convert these to member initializers.
Attachment #8985585 - Flags: review?(peterv) → review+
Group: core-security → core-security-release
Comment on attachment 8986528 [details] [diff] [review]
XPCom-String.patch

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

Reluctant r=me.  What we should really be doing here is:

a) Make an nsTStringRepr::BeginReading$BLAH (resp. other functions) which returns a fully-initialized one of these, and rewriting callsites to use such; or
b) rationalizing our string iteration story.

But I think compilers should be able to optimizing out these extra writes, so whatever.
Attachment #8986528 - Flags: review?(nfroyd) → review+
Comment on attachment 8985773 [details] [diff] [review]
js.patch

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

Slightly too many complaints to just stamp on the assumption you'll fix when landing, so let's have another tilt.  :-|

::: js/src/gc/Nursery.cpp
@@ +135,4 @@
>    , canAllocateStrings_(false)
>    , reportTenurings_(0)
>    , minorGCTriggerReason_(JS::gcreason::NO_REASON)
> +  , previousGC{}

I think I'd prefer if you brace-initialized directly on the field declaration, if we're going to do this.  That declaration has a comment by it:

    /*
     * This data is initialised only if the nursery is enabled and after at
     * least one call to Nursery::collect()
     */
    struct {
        JS::gcreason::Reason reason;
        size_t nurseryCapacity;
        size_t nurseryLazyCapacity;
        size_t nurseryUsedBytes;
        size_t tenuredBytes;
    } previousGC;

that should be removed when you do this, so you'd just have

    struct {
        JS::gcreason::Reason reason;
        size_t nurseryCapacity;
        size_t nurseryLazyCapacity;
        size_t nurseryUsedBytes;
        size_t tenuredBytes;
    } previousGC = {};

::: js/src/irregexp/RegExpEngine.h
@@ +686,4 @@
>  
>      ActionNode(ActionType action_type, RegExpNode* on_success)
>        : SeqRegExpNode(on_success),
> +        data_{},

I mean, okay, but this initialization is basically facile.  |data_| is a union, this "initializes" an essentially arbitrary arm of the union, and it is basically a false security to think that zeroing out (I think) the first arm really does anything meaningful for correctness.

But it's not like I have a proposal for anything better, so whatever.

::: js/src/jsapi-tests/tests.h
@@ +481,5 @@
>    public:
> +    explicit AutoLeaveZeal(JSContext* cx)
> +      : cx_(cx),
> +        zealBits_{},
> +        frequency_{} {

(0) for these, and put the body-opening '{' on its own line aligned with the body-closing '}'.

::: js/src/jsfriendapi.h
@@ +1434,5 @@
>  
>    public:
>      explicit AutoStableStringChars(JSContext* cx)
> +      : s_(cx),
> +        twoByteChars_(nullptr),

This too is a false security, IMO.  Tack a MOZ_INIT_OUTSIDE_CTOR on the union that stores twoByteChars_ instead.

::: js/src/vm/Caches.h
@@ +86,3 @@
>      RootedLinearString str;
>      RootedScript callerScript;
>      jsbytecode* pc;

I'd prefer tacking a MOZ_INIT_OUTSIDE_CTOR on this.

::: js/src/vm/Compression.cpp
@@ +50,5 @@
> +    zs.msg = nullptr;
> +    zs.state = nullptr;
> +    zs.data_type = 0;
> +    zs.adler = 0;
> +    zs.reserved = 0;

Ugh.  Big structs in third-party headers are the worst.  I'm tempted to suggest |zs{}| in the constructor's member-list, but for a big struct we don't control with a possibly-varying-across-time set of fields, it is probably preferable to write them all out like this.

::: js/src/vm/Shape.h
@@ +1534,5 @@
>      immutableFlags(other.immutableFlags),
>      attrs(other.attrs),
>      mutableFlags(other.mutableFlags),
> +    parent(nullptr),
> +    listp(nullptr)

(writing comments in opposite order for this file)  This is not really correct either.  If |other.immutableFlags & SHAPE_IN_DICTIONARY|, then this is correct, but based on searchfoxing I'm not convinced it *definitely* is correct -- I think this may be callable for dictionaries.

But fixing this properly looks a bit too messy to change in this patch only, so okay, I guess.

@@ +1569,5 @@
>      immutableFlags(SHAPE_INVALID_SLOT | (nfixed << FIXED_SLOTS_SHIFT)),
>      attrs(0),
>      mutableFlags(0),
> +    parent(nullptr),
> +    listp(nullptr)

This is not actually correct -- you should be initializing the kids field instead -- but then you'd have to make KidsPointer non-POD or something, or give it a constructor to initialize it by setNull().  And that opens up a can of worms about the C++ object model, that is not trivial to describe, nor wholly trivial to fix, unless you understand that model *and* this code.  Which only us SpiderMonkey hackers do (or rather a subset of them).  So I guess okay for now.

::: js/src/vm/Stack.cpp
@@ +1583,5 @@
>      lastProfilingFrame_(nullptr),
>      lastProfilingCallSite_(nullptr)
> +#ifdef CHECK_OSIPOINT_REGISTERS
> +  , checkRegs_{}
> +#endif

Add a = 0 to the member declaration rather than touch this.

::: js/src/wasm/AsmJS.cpp
@@ +1525,4 @@
>              // non-trivial constructor and therefore MUST be placement-new'd
>              // into existence.
>              MOZ_PUSH_DISABLE_NONTRIVIAL_UNION_WARNINGS
> +            U(): funcDefIndex_(0) {}

Space before ':'.

@@ +1616,4 @@
>              AsmJSMathBuiltinFunction func;
>          } u;
>  
> +        MathBuiltin(): kind(Kind(-1)), u{} {}

Also space before ':'.

::: js/src/wasm/WasmFrameIter.h
@@ +100,5 @@
>  {
>      uint32_t payload_;
>  
> +    ExitReason()
> +      : payload_(0)

ExitReason() : ExitReason(Fixed::None) {}

is IMO better.  (Although the enum-class is going to have to move above this constructor to do it, but that's not a big deal.)

::: js/src/wasm/WasmTypes.h
@@ +773,4 @@
>              uint32_t index_;
>              ValType type_;
>          } global;
> +        U(): global{} {}

These union-inits are just as silly as the ones I criticized before, but oh well.
Attachment #8985773 - Attachment is obsolete: false
Attached patch js.patch (obsolete) — Splinter Review
Attachment #8985773 - Attachment is obsolete: true
Attachment #8987130 - Flags: review?(jwalden+bmo)
Comment on attachment 8987130 [details] [diff] [review]
js.patch

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

::: js/src/builtin/Array.h
@@ +276,1 @@
>          reset();

This change is probably no longer necessary now that I've landed bug 1469444 -- state_ is initialized at its declaration, and the other fields are init-outside-ctor.  If something really does have to be done here, I'd like to know -- I tried to eliminate that need here in that bug.

::: js/src/frontend/Parser.cpp
@@ +858,5 @@
>                                                   ScriptSourceObject* sourceObject,
>                                                   ParseGoal parseGoal)
>    : ParserBase(cx, alloc, options, foldConstants, usedNames, sourceObject, parseGoal),
> +    handler(cx, alloc, lazyOuterFunction),
> +    internalSyntaxParser_(nullptr)

Bug 1469469 should mean this change isn't needed.

::: js/src/gc/Heap.h
@@ +488,5 @@
>  {
>      // Construct a Nursery ChunkTrailer.
>      ChunkTrailer(JSRuntime* rt, StoreBuffer* sb)
> +      : location(ChunkLocation::Nursery),
> +        padding(0),

I also made this field an anonymous bit-field, so this change and the one below should be unnecessary after bug 1469593.

::: js/src/wasm/AsmJS.cpp
@@ +1525,4 @@
>              // non-trivial constructor and therefore MUST be placement-new'd
>              // into existence.
>              MOZ_PUSH_DISABLE_NONTRIVIAL_UNION_WARNINGS
> +            U( ): funcDefIndex_(0) {}

U() : funcDefIndex_(0) {}

::: js/src/wasm/WasmFrameIter.h
@@ +109,4 @@
>          Trap,            // call to trap handler
>          DebugTrap        // call to debug trap handler
>      };
> +      

Don't add the trailing WS.

::: js/src/wasm/WasmTypes.h
@@ +797,4 @@
>              uint32_t index_;
>              ValType type_;
>          } global;
> +        U(): global{} {}

U() : global{} {}

@@ +920,4 @@
>                      ValType type_;
>                      uint32_t index_;
>                  } import;
> +                U(): import{} {}

U() : import{} {}

@@ +1618,4 @@
>      uint32_t returnAddressOffset_;
>  
>    public:
> +    CallSite(): returnAddressOffset_(0) {}

CallSite() : returnAddressOffset_(0) {}

@@ +1996,4 @@
>      // which_ shall be initialized in the static constructors
>      MOZ_INIT_OUTSIDE_CTOR Which which_;
>      union U {
> +        U(): funcIndex_(0) {}

U() : funcIndex_(0) {}
Attachment #8987130 - Flags: review?(jwalden+bmo) → review+
Attachment #8986127 - Attachment is obsolete: true
Attachment #8986127 - Flags: review?(jwalden+bmo)
Comment on attachment 8986200 [details] [diff] [review]
Layout.patch

The following are no longer used and should be removed instead of initialized:

  nsIPresShell::mFontSizeInflationEnabledIsDirty
  TextPaintStyle::mSelectionStatus
  BCCornerInfo::unused (please replace with a comment that says 1 bit is unused)





In AutoMaybeDisableFontInflation::AutoMaybeDisableFontInflation please add the
initialization only inside the else in the body of the constructor.  (It's
obviously unneeded, though.)

In nsPresContext's constructor, please *move* the:
  mLangService = nsLanguageAtomService::GetService();
out of Init into the initializer that you've added in the constructor,
instead of having both.

In nsPresContext's constructor, please initialize mPostedPrefChangedRunnable
to false rather than 0.

Remove the change to TextDrawTarget::mWRGlyphFlags; it's initialized already
at its declaration.  (I've forgotten the name of that new-ish C++ feature...)

Please explain why the change to TextOverflow::TextOverflow actually
changes anything.  I also don't understand why it's needed since mIStart
and mIEnd have their Init methods called later in the constructor.  Or
is the issue that Marker::Init should initialize all the fields?

For BulletRenderer::mColor, please initialize to NS_RGBA(0, 0, 0, 0)
instead of just 0.

For FlexboxAxisTracker::mMainAxis, please use eAxis_LR rather than using
static_cast.

For FlexboxAxisTracker::mIsRowOriented, please initialize to true rather
than false so the unused initial state is consistent.

For nsFloatManager::FloatInfo::mLeftBEnd and mRightBEnd, please initialize
to nscoord_MIN rather than 0, since that's consistent with what AddFloat
does.

In order to make your change to SelectionDetails affect the code we ship,
please move the constructor out of #ifdef NS_BUILD_REFCNT_LOGGING.  (The
one macro in it has behavior already controlled by the #ifdef.)

In ScrollFrameHelper::mScrollParentID, please initialize to NULL_SCROLL_ID
instead of a literal 0.

In nsLineList_iterator and nsLineList_reverse_iterator and
nsLineList_const_iterator and nsLineList_const_reverse_iterator, please
add to all the methods that assert about mListLink that mListLink is not
null (i.e., assert that the list is initialized).  Otherwise your change
reduces the power of the assertions, since today those assertions will
catch uninitialized use *most* of the time, and your change makes them
catch it never.

In nsLineLayout, please initialize mSpansAllocated, mSpansFreed,
mFramesAllocated, and mFramesFreed to 0 rather than ().

In BuildTextRunsScanner, please initialize mStartOfLine to true rather
than false, so the unused uninitialized state is consistent.

nsTextPaintStyle, please initialize mSelectionTextColor,
mSelectionBGColor, mFrameBackgroundColor, mSystemFieldForegroundColor,
and mSystemFieldBackgroundColor to NS_RGBA(0, 0, 0, 0) rather than
explicit 0.

Please initialize ClusterIterator::mHaveWordBreak to false rather than 0.

Please initialize nsSkipCharsRunIterator::mSkipped to false rather than 0.

Please initialize inDeepTreeWalker::mCurrentIndex to -1 rather than 0.

Please initialize BCInlineDirSeg::mOwner to eTableOwner rather
than using '\0'.

Please initialize BCInlineDirSeg::mIEndBevelSide to eLogicalSideBStart rather
than using static_cast.
Attachment #8986200 - Flags: review?(dbaron) → review-
Comment on attachment 8986686 [details] [diff] [review]
xptoolkit.patch

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

::: dom/xul/nsXULContentSink.h
@@ +115,5 @@
> +            Entry()
> +                : mChildren(8)
> +                , mState(eInProlog)
> +                , mNext(nullptr)
> +            {}

I see only one place where this object is created:
XULContentSinkImpl::ContextStack::Push
I think the constructor should take node, state, next as arguments instead of initializing members here. For example mState(eInProlog) initial value doesn't make sense and there's also a tiny perf penalty since all 3 members are set to some values first and then immediately rewritten to real values.

::: layout/xul/nsImageBoxFrame.cpp
@@ +906,4 @@
>  NS_IMPL_ISUPPORTS(nsImageBoxListener, imgINotificationObserver)
>  
>  nsImageBoxListener::nsImageBoxListener()
> +  : mFrame(nullptr)

This constructor should take aFrame as an argument, so nsImageBoxFrame::Init can create it without calling SetFrame afterwards.
SetFrame should be renamed to ClearFrame, so nsImageBoxFrame::DestroyFrom can still use it.

::: layout/xul/nsMenuPopupFrame.h
@@ +607,5 @@
>      ReflowCallbackData() :
>        mPosted(false),
>        mAnchor(nullptr),
> +      mSizedToPopup(false),
> +      mIsOpenChanged(false)

ok

::: layout/xul/nsSplitterFrame.cpp
@@ +63,5 @@
>  
>    explicit nsSplitterFrameInner(nsSplitterFrame* aSplitter)
> +    : mDidDrag(false)
> +    , mDragStart(0)
> +    , mCurrentPos(0)

this attribute seems to be unused

@@ +69,5 @@
> +    , mChildInfosBeforeCount(0)
> +    , mChildInfosAfterCount(0)
> +    , mState(Open)
> +    , mSplitterPos(0)
> +    , mDragging(false)

nsSplitterFrame::Init needs to be adjusted now since these attributes are now initialized to default values here. No need to do it again in nsSplitterFrame::Init

::: layout/xul/tree/nsTreeBodyFrame.h
@@ +509,5 @@
>      public:
> +      Slots()
> +        : mDropAllowed(false)
> +        , mIsDragging(false)
> +        , mDropRow(0)

default value for this is -1

@@ +510,5 @@
> +      Slots()
> +        : mDropAllowed(false)
> +        , mIsDragging(false)
> +        , mDropRow(0)
> +        , mDropOrient(0)

default value for this is -1

::: layout/xul/tree/nsTreeColumns.cpp
@@ +23,4 @@
>  nsTreeColumn::nsTreeColumn(nsTreeColumns* aColumns, dom::Element* aElement)
>    : mContent(aElement),
>      mColumns(aColumns),
> +    mIndex(0),

ok
Attachment #8986686 - Flags: review?(jvarga)
> regarding [1] m3 is of type 'uint8_t [8]' which we don't currently support.

That is, the static analysis doesn't support?

Feels like either we should initialize he whole uuid or not worry about it at all...  Anyway, we can sort that out in bug 1469240.
Attached patch xptoolkit.patch (obsolete) — Splinter Review
Attachment #8986686 - Attachment is obsolete: true
Attachment #8987612 - Flags: review?(jvarga)
Comment on attachment 8987612 [details] [diff] [review]
xptoolkit.patch

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

r=me if you address my additional comments

::: dom/xul/nsXULContentSink.h
@@ +112,4 @@
>              nsPrototypeArray    mChildren;
>              State               mState;
>              Entry*              mNext;
> +            Entry(nsXULPrototypeNode* aNode, State aState, Entry*  aNext)

Fix the extra white space please

::: layout/xul/nsSplitterFrame.cpp
@@ +273,3 @@
>  
>    mInner->AddRef();
>    mInner->mState = nsSplitterFrameInner::Open;

I haven't tried to compile this, nor run any tests, but ...
Isn't |mState| initialized to Open in the constructor ?

I don't know why the original code had to set it twice to the same value.

If you are unsure, I would rather keep the second |mInner->mState = nsSplitterFrameInner::Open;| which comes after nsBoxFrame::Init() call.

You can also ask someone else who better understands this layout code.
Attachment #8987612 - Flags: review?(jvarga) → review+
Attached patch layout.patch (obsolete) — Splinter Review
Hello,

I begin this by thanking you for the through review!

>>Please explain why the change to TextOverflow::TextOverflow actually
>>changes anything.  I also don't understand why it's needed since mIStart
>>and mIEnd have their Init methods called later in the constructor.  Or
>>is the issue that Marker::Init should initialize all the fields?
The problem here is that Marker::Init doesn't initialize all the fields. On the other hand we could add a ctor for Marker where we initialize everything, but doing so, Marker is no longer POD and I don't know if we want this.
Attachment #8986200 - Attachment is obsolete: true
Attachment #8987825 - Flags: review?(dbaron)
Discussing with Ehsan, we don't think we have to keep this bug closed.
Dan, do you agree?
Flags: needinfo?(dveditz)
Attachment #8986175 - Flags: review?(jmuizelaar) → review?(bugmail)
Comment on attachment 8986175 [details] [diff] [review]
gfx.patch

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

Overall this looks fine to me. I have a couple of questions below that I'd like answers for before giving final r+. Also I'm going to flag Bas and jgilbert for feedback here just in case they have any objections - a lot of the code changes here are in Moz2D and WebGL code and they might have opinions on stuff. If they don't respond soon we can land this anyway.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +3999,5 @@
>      : nsBidiPresUtils::BidiProcessor()
> +    , mCtx(nullptr)
> +    , mFontgrp(nullptr)
> +    , mAppUnitsPerDevPixel(0)
> +    , mOp(static_cast<CanvasRenderingContext2D::TextDrawOperation>(0))

So in other places (e.g. the change in WebGLTexture.cpp), the value used for initialization is a named member of the enum instead of a static_cast. i.e. the equivalent here would be CanvasRenderingContext2D::TextDrawOperation::FILL instead of static_cast<CanvasRenderingContext2D::TextDrawOperation>(0). Why is it that some places used the named member and other places use the static_cast?

::: gfx/2d/Matrix.h
@@ +1878,4 @@
>                          Float a41, Float a42, Float a43, Float a44)
>      : Parent(a11, a12, a13, a14, a21, a22, a23, a24,
>               a31, a32, a33, a34, a41, a42, a43, a44)
> +    , mType{ MatrixType::Identity }

Here the Analyze() call in the constructor body will always assign a value to mType, do we really need to use an initializer? Can the static analysis that detected this as an issue be enhanced to detect that the constructor body always ensures a value is assigned?

Ditto for the other constructor just below this.

::: gfx/2d/RecordedEventImpl.h
@@ +182,5 @@
>  class RecordedFillRect : public RecordedDrawingEvent<RecordedFillRect> {
>  public:
>    RecordedFillRect(DrawTarget *aDT, const Rect &aRect, const Pattern &aPattern, const DrawOptions &aOptions)
> +    : RecordedDrawingEvent(FILLRECT, aDT), mRect(aRect),
> +      mPattern(), mOptions(aOptions)

Same here, the StorePattern call in the constructor body populates mPattern, do we really need to add the mPattern() ? Ditto for most of the changes in this file.

::: gfx/layers/wr/WebRenderCommandBuilder.cpp
@@ +206,4 @@
>  struct Grouper
>  {
>    explicit Grouper(ClipManager& aClipManager)
> +   : mAppUnitsPerDevPixel(0) 

nit: trailing whitespace

::: gfx/src/nsRegion.h
@@ +2181,5 @@
>      explicit RectIterator(const nsRegion& aRegion)
>        : mRegion(aRegion)
>        , mCurrentBand(aRegion.mBands.begin())
> +#ifndef DEBUG
> +      , mCurrentStrip(nullptr)

A little weird that this is in an ifndef DEBUG but presumably that's because StripArray is a different type in DEBUG and non-DEBUG.
Attachment #8986175 - Flags: feedback?(jgilbert)
Attachment #8986175 - Flags: feedback?(bas)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #243)
> Comment on attachment 8986175 [details] [diff] [review]
> gfx.patch
> 
> Review of attachment 8986175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Overall this looks fine to me. I have a couple of questions below that I'd
> like answers for before giving final r+. Also I'm going to flag Bas and
> jgilbert for feedback here just in case they have any objections - a lot of
> the code changes here are in Moz2D and WebGL code and they might have
> opinions on stuff. If they don't respond soon we can land this anyway.
> 
> ::: dom/canvas/CanvasRenderingContext2D.cpp
> @@ +3999,5 @@
> >      : nsBidiPresUtils::BidiProcessor()
> > +    , mCtx(nullptr)
> > +    , mFontgrp(nullptr)
> > +    , mAppUnitsPerDevPixel(0)
> > +    , mOp(static_cast<CanvasRenderingContext2D::TextDrawOperation>(0))
> 
> So in other places (e.g. the change in WebGLTexture.cpp), the value used for
> initialization is a named member of the enum instead of a static_cast. i.e.
> the equivalent here would be
> CanvasRenderingContext2D::TextDrawOperation::FILL instead of
> static_cast<CanvasRenderingContext2D::TextDrawOperation>(0). Why is it that
> some places used the named member and other places use the static_cast?
> 
This patch was mostly auto-generated and only parts of it were done manually, the parts with static_cast were auto-generated using an auto-fix option from our checker.
> ::: gfx/2d/Matrix.h
> @@ +1878,4 @@
> >                          Float a41, Float a42, Float a43, Float a44)
> >      : Parent(a11, a12, a13, a14, a21, a22, a23, a24,
> >               a31, a32, a33, a34, a41, a42, a43, a44)
> > +    , mType{ MatrixType::Identity }
> 
> Here the Analyze() call in the constructor body will always assign a value
> to mType, do we really need to use an initializer? Can the static analysis
> that detected this as an issue be enhanced to detect that the constructor
> body always ensures a value is assigned?
> 
Yes but the assignment only take places of branches of ifstmt. The reason behind this was to do a separate analysis on the then branch and on the else branch and to compare the outcomes, if we find matches than those variables are marked as initialised. 
> Ditto for the other constructor just below this.
> 
> ::: gfx/2d/RecordedEventImpl.h
> @@ +182,5 @@
> >  class RecordedFillRect : public RecordedDrawingEvent<RecordedFillRect> {
> >  public:
> >    RecordedFillRect(DrawTarget *aDT, const Rect &aRect, const Pattern &aPattern, const DrawOptions &aOptions)
> > +    : RecordedDrawingEvent(FILLRECT, aDT), mRect(aRect),
> > +      mPattern(), mOptions(aOptions)
> 
partly same reason as above and the type of mPattern also has an union inside it.

> Same here, the StorePattern call in the constructor body populates mPattern,
> do we really need to add the mPattern() ? Ditto for most of the changes in
> this file.
> 
> ::: gfx/layers/wr/WebRenderCommandBuilder.cpp
> @@ +206,4 @@
> >  struct Grouper
> >  {
> >    explicit Grouper(ClipManager& aClipManager)
> > +   : mAppUnitsPerDevPixel(0) 
> 
> nit: trailing whitespace
> 
> ::: gfx/src/nsRegion.h
> @@ +2181,5 @@
> >      explicit RectIterator(const nsRegion& aRegion)
> >        : mRegion(aRegion)
> >        , mCurrentBand(aRegion.mBands.begin())
> > +#ifndef DEBUG
> > +      , mCurrentStrip(nullptr)
> 
> A little weird that this is in an ifndef DEBUG but presumably that's because
> StripArray is a different type in DEBUG and non-DEBUG.
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #244)
> > Here the Analyze() call in the constructor body will always assign a value
> > to mType, do we really need to use an initializer? Can the static analysis
> > that detected this as an issue be enhanced to detect that the constructor
> > body always ensures a value is assigned?
> > 
> Yes but the assignment only take places of branches of ifstmt. The reason
> behind this was to do a separate analysis on the then branch and on the else
> branch and to compare the outcomes, if we find matches than those variables
> are marked as initialised. 

Sorry, I don't follow this. Do you mean that all branches of the Analyze() function need to assign the same value into mType? Why is that? Looking at the Analyze function [1] it's pretty obvious that mType is assigned some value in all branches, and that should be sufficient for initialization purposes.

[1] https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/gfx/2d/Matrix.h#2265
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #245)
> (In reply to Andi-Bogdan Postelnicu [:andi] from comment #244)
> > > Here the Analyze() call in the constructor body will always assign a value
> > > to mType, do we really need to use an initializer? Can the static analysis
> > > that detected this as an issue be enhanced to detect that the constructor
> > > body always ensures a value is assigned?
> > > 
> > Yes but the assignment only take places of branches of ifstmt. The reason
> > behind this was to do a separate analysis on the then branch and on the else
> > branch and to compare the outcomes, if we find matches than those variables
> > are marked as initialised. 
> 
> Sorry, I don't follow this. Do you mean that all branches of the Analyze()
> function need to assign the same value into mType? Why is that? Looking at
> the Analyze function [1] it's pretty obvious that mType is assigned some
> value in all branches, and that should be sufficient for initialization
> purposes.
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 6ef785903fee6c0b16a1eab79d722373d940fd78/gfx/2d/Matrix.h#2265

I've seen the final assignment. And yes we don't need to initialise it at start. This wa a bug in the analysis that has been solved recently.

What I wanted to say is that when we have this trivial example:

>>class Test {
>>int b;
>>Test() {
>>  if (something)
>>    b = 10;
>>}
>>};

In this case b will be triggered as uninitialised because for the ifstmt we have two branches, the then branch where it gets assigned and the else branch where it doesn't.
Ok. So then will the patch be updated to take this into account?
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #247)
> Ok. So then will the patch be updated to take this into account?

Of course will post a new version of the patch with this fix.
Attached patch gfx.patch (obsolete) — Splinter Review
Attachment #8986175 - Attachment is obsolete: true
Attachment #8986175 - Flags: review?(bugmail)
Attachment #8986175 - Flags: feedback?(jgilbert)
Attachment #8986175 - Flags: feedback?(bas)
Attachment #8990085 - Flags: review?(bugmail)
Attachment #8990085 - Flags: feedback?(jgilbert)
Attachment #8990085 - Flags: feedback?(bas)
Comment on attachment 8990085 [details] [diff] [review]
gfx.patch

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

r+: dom/*, gfx/gl/*

::: dom/canvas/WebGLContext.cpp
@@ +120,4 @@
>      , mMaxPerfWarnings(gfxPrefs::WebGLMaxPerfWarnings())
>      , mNumPerfWarnings(0)
>      , mMaxAcceptableFBStatusInvals(gfxPrefs::WebGLMaxAcceptableFBStatusInvals())
> +    , mDepthTestEnabled(0)

It'd be nicer to do initialization at declaration as much as possible.
Attachment #8990085 - Flags: feedback?(jgilbert) → feedback+
(In reply to Sylvestre Ledru [:sylvestre] from comment #242)
> Discussing with Ehsan, we don't think we have to keep this bug closed. Dan, do you agree?

Yeah, we can unhide this. Probably Bug 525063 also.
Flags: needinfo?(dveditz)
yeah! Thanks :)
Group: core-security-release
I've split gfx patch into smaller pieces, since the dom/* and gfx/gl* has been r+ according to: https://bugzilla.mozilla.org/show_bug.cgi?id=1453795#c250
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d8492f777963593f21f86d955673293675c1be1
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe0a7d4b6daf
GFX - Initialize member fields in classes/ structures. r=jgilbert
(In reply to Pulsebot from comment #254)
> Pushed by bpostelnicu@mozilla.com:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/fe0a7d4b6daf
> GFX - Initialize member fields in classes/ structures. r=jgilbert

This is part of the patch that has bee r=jgilbert, dom/* and gfx/gl*
Attached patch gfx.patch (obsolete) — Splinter Review
Attachment #8990085 - Attachment is obsolete: true
Attachment #8990085 - Flags: review?(bugmail)
Attachment #8990085 - Flags: feedback?(bas)
Attachment #8990288 - Flags: review?(bugmail)
Attachment #8990288 - Flags: feedback?(bas)
Attachment #8986683 - Flags: review?(peterv) → review?(bugs)
Attachment #8986683 - Flags: review?(bugs) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a985c7fbfd01
DOM/XML - Initialize member fields in classes/ structures. r=smaug
Comment on attachment 8990288 [details] [diff] [review]
gfx.patch

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

::: gfx/2d/Logging.h
@@ +271,1 @@
>    , mLogIt(false)

The Init function called in the constructor body unconditionally initializes mReason (as well as mOptions and mLogIt), so you shouldn't need to initialize mReason here. I thought you were going to regenerate the patch using the new analysis that corrected this class of problems?
Attachment #8990288 - Flags: review?(bugmail)
Comment on attachment 8986169 [details] [diff] [review]
dom.patch

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

Please get rbarnes or ttaubert to review dom/crypto/WebCryptoTask.cpp.

::: dom/base/DOMException.cpp
@@ +176,4 @@
>    , mResult(aResult)
>    , mName(aName)
>    , mData(aData)
> +  , mInitialized(false)

Looks unused, remove it instead.

::: dom/base/EventSource.cpp
@@ +1928,4 @@
>  EventSource::EventSource(nsPIDOMWindowInner* aOwnerWindow,
>                           bool aWithCredentials)
>    : DOMEventTargetHelper(aOwnerWindow)
> +  , mReadyState(0)

The constructor already initializes this by calling SetReadyState(CONNECTING), so remove this.

::: dom/base/TextInputProcessor.cpp
@@ +39,4 @@
>  public:
>    explicit TextInputProcessorNotification(const char* aType)
>      : mType(aType)
> +    , mTextChangeData()

You could instead add a constructor to the union, but I guess this is ok.

::: dom/base/nsContentList.h
@@ +591,5 @@
>                                     const nsAString& aString) :
>      nsContentList(aRootNode, aFunc, aDestroyFunc, nullptr),
> +#ifdef DEBUG
> +    mType(static_cast<ContentListType>(0)),
> +#endif

This seems wrong. I think you should add a |mozilla::DebugOnly<ContentListType>| argument to the nsCacheableFuncStringContentList constructor and pass in the right value from the derived classes' constructors.

::: dom/base/nsContentPermissionHelper.cpp
@@ +579,4 @@
>  }
>  
>  nsContentPermissionRequestProxy::nsContentPermissionRequestProxy()
> +  : mParent(nullptr)

Make the constructor take the right value, instead of passing it in through init. There's only one caller I think.

::: dom/base/nsFrameLoader.cpp
@@ +182,4 @@
>    , mClipSubdocument(true)
>    , mClampScrollPosition(true)
>    , mObservingOwnerContent(false)
> +  , mFreshProcess(false)

Looks unused, remove it instead.

::: dom/base/nsJSEnvironment.cpp
@@ +589,5 @@
>                           nsIScriptGlobalObject* aGlobalObject)
>    : mWindowProxy(nullptr)
>    , mGCOnDestruction(aGCOnDestruction)
> +  , mModalStateTime(0)
> +  , mModalStateDepth(0)

Both look unused, remove them instead.

::: dom/base/nsPlainTextSerializer.cpp
@@ +81,5 @@
>  }
>  
>  nsPlainTextSerializer::nsPlainTextSerializer()
> +  : mFlags(0)
> +  , mFloatingLines(0)

nsPlainTextSerializer::Init sets it to -1, so this should probably too.

::: dom/base/nsQueryContentEventResult.cpp
@@ +58,2 @@
>    , mSucceeded(false)
> +  , mReversed(false)

I think it makes more sense to merge the code from nsQueryContentEventResult::SetEventResult into this constructor.

::: dom/base/nsXHTMLContentSerializer.cpp
@@ +48,4 @@
>  
>  nsXHTMLContentSerializer::nsXHTMLContentSerializer()
>    : mIsHTMLSerializer(false)
> +  , mDoHeader(false)

Looks unused, remove it instead.

::: dom/bindings/BindingDeclarations.h
@@ +425,4 @@
>  {
>  public:
>    NonNull()
> +    : ptr(nullptr)

This has an explicit comment: "ptr is left uninitialized for optimization purposes.", so remove this.

::: dom/bindings/ErrorResult.h
@@ +129,4 @@
>      , mUnionState(HasNothing)
>  #endif
>    {
> +    mExtra.mMessage = nullptr;

Why is this needed? mExtra has a constructor.

::: dom/bindings/FakeString.h
@@ +19,5 @@
>  // for small strings and a nsStringBuffer for longer strings.
>  struct FakeString {
>    FakeString() :
> +    mData(nullptr),
> +    mLength(0),

Both of these have comments about "left uninitialized for optimization purposes", so undo these changes.

::: dom/bindings/TypedArray.h
@@ +221,4 @@
>  public:
>    ArrayBufferView_base()
>      : Base()
> +    , mType(static_cast<js::Scalar::Type>(0))

Maybe use js::Scalar::MaxTypedArrayViewType?

::: dom/cache/SavedTypes.h
@@ +27,5 @@
> +    , mCacheId(0)
> +  {
> +    mBodyId.m0 = 0;
> +    mBodyId.m1 = 0;
> +    mBodyId.m2 = 0;

If you're not initializing m3 then you shouldn't do the others either.

@@ +44,5 @@
> +    , mCacheId(0)
> +  {
> +    mBodyId.m0 = 0;
> +    mBodyId.m1 = 0;
> +    mBodyId.m2 = 0;

Same here.

::: dom/fetch/InternalRequest.cpp
@@ +124,4 @@
>                                   const nsAString& aIntegrity)
>    : mMethod(aMethod)
>    , mHeaders(aHeaders)
> +  , mBodyLength(0)

Why not InternalResponse::UNKNOWN_BODY_SIZE?

@@ +185,4 @@
>    , mURLList(aIPCRequest.urls())
>    , mHeaders(new InternalHeaders(aIPCRequest.headers(),
>                                   aIPCRequest.headersGuard()))
> +  , mBodyLength(0)

Same here.

::: dom/gamepad/GamepadHapticActuator.h
@@ +23,4 @@
>    GamepadHapticActuator(nsISupports* aParent, uint32_t aGamepadId,
>                          uint32_t aIndex);
>    explicit GamepadHapticActuator(nsISupports* aParent)
> +    : mParent(aParent)

This constructor looks unused, just remove it.

::: dom/ipc/FilePickerParent.h
@@ +26,4 @@
>                     const int16_t& aMode)
>    : mTitle(aTitle)
>    , mMode(aMode)
> +  , mResult(0)

Use nsIFilePicker::returnOK.

::: dom/ipc/TabChild.cpp
@@ +405,4 @@
>    , mActiveSuppressDisplayport(0)
>    , mLayersId{0}
>    , mBeforeUnloadListeners(0)
> +  , mLastBackgroundColor(0)

Looks unused, remove it instead.

::: dom/messagechannel/MessagePort.cpp
@@ +199,4 @@
>  
>  MessagePort::MessagePort(nsIGlobalObject* aGlobal)
>    : DOMEventTargetHelper(aGlobal)
> +  , mState(static_cast<State>(0))

You should pass the right value from the two callers through the constructor instead of through Initialize.

::: dom/payments/PaymentActionResponse.cpp
@@ +181,4 @@
>                              nsIPaymentShowActionResponse)
>  
>  PaymentShowActionResponse::PaymentShowActionResponse()
> +  : mAcceptStatus(0)

nsIPaymentActionResponse::PAYMENT_REJECTED

@@ +293,4 @@
>                              nsIPaymentAbortActionResponse)
>  
>  PaymentAbortActionResponse::PaymentAbortActionResponse()
> +  : mAbortStatus(0)

nsIPaymentActionResponse::ABORT_FAILED

@@ +330,4 @@
>                              nsIPaymentCompleteActionResponse)
>  
>  PaymentCompleteActionResponse::PaymentCompleteActionResponse()
> +  : mCompleteStatus(0)

nsIPaymentActionResponse::COMPLETE_FAILED

::: dom/performance/Performance.cpp
@@ +75,4 @@
>  Performance::Performance()
>    : mResourceTimingBufferSize(kDefaultResourceTimingBufferSize)
>    , mPendingNotificationObserversTask(false)
> +  , mSystemPrincipal(false)

Pass the right value through an argument for the constructor.

@@ +84,4 @@
>    : DOMEventTargetHelper(aWindow)
>    , mResourceTimingBufferSize(kDefaultResourceTimingBufferSize)
>    , mPendingNotificationObserversTask(false)
> +  , mSystemPrincipal(false)

Pass the right value through an argument for the constructor.

::: dom/worklet/Worklet.cpp
@@ +434,5 @@
>  
>  WorkletLoadInfo::WorkletLoadInfo()
> +  : mOuterWindowID(0)
> +  , mInnerWindowID(0)
> +  , mDumpEnabled(false)

Make Worklet::Worklet pass in aWindow and aPrincipal, set the right values here straight away and remove most of the code in Worklet::Worklet. And set mDumpEnabled to DOMPrefs::DumpEnabled().
Attachment #8986169 - Flags: review?(peterv) → review-
Attached patch dom.patch (obsolete) — Splinter Review
Thank you very much for the detailed review, It helped a lot! I have a comment, regarding the way how the static analysis understands assignment on a particular case:

>>if (expr) {
>> mValue = 1;
>>}

In this case |mValue| will still be considered uninitialized because it is assigned a value only on one branch of the IfStmt, in this case the |then| branch, but on the else branch, that is missing, it isn't assigned, so the analysis collates the two results and see what it doesn't match.

I felt the need of saying this because I've encountered this issue in one particular place in the patch. If you still think that we shouldn't have this we can ignore the variable from analysis.
Attachment #8986169 - Attachment is obsolete: true
Attachment #8991231 - Flags: review?(peterv)
Attachment #8991231 - Attachment description: gfx.patch → dom.patch
Attachment #8991231 - Attachment filename: gfx.patch → dom.patch
Attached patch dom-crypto.patch (obsolete) — Splinter Review
Attachment #8991234 - Flags: review?(bugmail)
Comment on attachment 8987825 [details] [diff] [review]
layout.patch

My previous review was in comment 232.

In nsTextFrame.cpp where you're removing mSelectionStatus, please remove
the line rather than replacing it with a blank line.

In TextOverflow::TextOverflow, please replace your change by a change
that initializes all the fields in Marker::Init:  mIntrinsicISize to 0
and mHasOverflow and mActive to false.  (I don't see why a ctor would be
needed if fixing Marker::Init would fix it.)  (This is in reply to
comment 240.)

r=dbaron with those changes
Attachment #8987825 - Flags: review?(dbaron) → review+
For the layout patch let's make sure we don't regress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5115c9f543ae09b497e978e442636163d292c9f
Summary: Review of fixes by 525063 per module → uninitialized class members: Review of fixes by 525063 per module
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/56062a63fa8c
Layout - Initialize member fields in classes/ structures. r=dbaron
Attachment #8987825 - Attachment is obsolete: true
Comment on attachment 8991231 [details] [diff] [review]
dom.patch

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

::: dom/base/nsContentList.h
@@ +588,4 @@
>                                     nsContentListMatchFunc aFunc,
>                                     nsContentListDestroyFunc aDestroyFunc,
>                                     nsFuncStringContentListDataAllocator aDataAllocator,
> +                                   mozilla::DebugOnly<ContentListType> aType,

I'd make this the last argument.

::: dom/bindings/ErrorResult.h
@@ +129,4 @@
>      , mUnionState(HasNothing)
>  #endif
>    {
> +    mExtra.mMessage = nullptr;

I still don't think this is necessary, given the constructor in the union (which sets mMessage to nullptr already).

::: dom/performance/Performance.h
@@ +118,4 @@
>    void InsertResourceEntry(PerformanceEntry* aEntry);
>  
>  protected:
> +  Performance(bool aSystemPrincipal);

Shouldn't this be explicit?

@@ +118,5 @@
>    void InsertResourceEntry(PerformanceEntry* aEntry);
>  
>  protected:
> +  Performance(bool aSystemPrincipal);
> +  explicit Performance(nsPIDOMWindowInner* aWindow, bool aSystemPrincipal);

I think you can remove explicit.
Attachment #8991231 - Flags: review?(peterv) → review+
let's make sure we don't regress with the dom patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5138cbef30cb0313b4dd418e7b329b1992995e80
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/13efb4f58614
DOM - Initialize member fields in classes/ structures. r=peterv
Attachment #8991231 - Attachment is obsolete: true
The initialization of mExtra.mMessage in ErrorResult is not needed or desired: that member is not examined until an error is thrown on an ErrorResult and doing that initializes it as needed.  ErrorResult is meant to be as zero-cost as possible unless an exception is thrown; we shouldn't be wasting time initializing things that don't need to be initialized there.  That means the Extra constructor should not initialize mMessage either.
Flags: needinfo?(bpostelnicu)
Comment on attachment 8991234 [details] [diff] [review]
dom-crypto.patch

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

I don't think I'm the appropriate reviewer for this, but with :ttaubert not around anymore, I'm not sure who is.  So redirecting to :mt as co-owner of security.  (I acknowledge I reviewed the most recent patch to the file in question, but that was about DOM Worker code, not the actual crypto subject domain implementation.)

Briefly looking at it myself, everything seems sane enough, but mMechansism of 0 is CKM_RSA_PKCS_KEY_PAIR_GEN which is a legit value, so that's unfortunate, but there is no explicit illegal/uninitialized value as a better alternative that I see in https://searchfox.org/mozilla-central/source/security/nss/lib/util/pkcs11t.h#565.  When used in AESTask it is fine because DoCrypto() doesn't recognize it as a legal possibility... but it's also used in GenerateAsymmetricKeyTask where it is an acceptable value.  And the CKM_ constants are defined in security/nss/lib/util/pkcs11t.h which I definitely can't make suggestions about.  So, :mt?
Attachment #8991234 - Flags: review?(bugmail) → review?(martin.thomson)
Comment on attachment 8990288 [details] [diff] [review]
gfx.patch

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

In general, is there a reason we're not using C++ member initializers here? (i.e. uint8_t* mData = nullptr; instead of adding it to every initializer list)
See Also: → 1475515
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #271)
> The initialization of mExtra.mMessage in ErrorResult is not needed or
> desired: that member is not examined until an error is thrown on an
> ErrorResult and doing that initializes it as needed.  ErrorResult is meant
> to be as zero-cost as possible unless an exception is thrown; we shouldn't
> be wasting time initializing things that don't need to be initialized there.
> That means the Extra constructor should not initialize mMessage either.

I've created this followup bug 1475515. Thanks again for noting this!
Flags: needinfo?(bpostelnicu)
(In reply to Bas Schouten (:bas.schouten) from comment #274)
> Comment on attachment 8990288 [details] [diff] [review]
> gfx.patch
> 
> Review of attachment 8990288 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> In general, is there a reason we're not using C++ member initializers here?
> (i.e. uint8_t* mData = nullptr; instead of adding it to every initializer
> list)

Most of the fixes have been done by implementing a fix-up for clang-tidy, so they were done automatically.
Comment on attachment 8991234 [details] [diff] [review]
dom-crypto.patch

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

As bz said separately, this would be better as inline initialization, but this seems fine.

::: dom/crypto/WebCryptoTask.cpp
@@ +485,4 @@
>  public:
>    AesTask(JSContext* aCx, const ObjectOrString& aAlgorithm,
>            CryptoKey& aKey, bool aEncrypt)
> +    : mMechanism(0)

Try CKM_INVALID_MECHANISM
Attachment #8991234 - Flags: review?(martin.thomson) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddb66025724b
DOM/Crypto - Initialize member fields in classes/ structures. r=mt
Attached patch gfx.patch (obsolete) — Splinter Review
Attachment #8990288 - Attachment is obsolete: true
Attachment #8990288 - Flags: feedback?(bas)
Attachment #8997781 - Flags: review?(nical.bugzilla)
Comment on attachment 8997781 [details] [diff] [review]
gfx.patch

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

Looks nice overall. I'm not found of the static_cast<EnumType>(0) pattern so I'd rather fix those before landing.

::: gfx/2d/SVGTurbulenceRenderer-inl.h
@@ +102,5 @@
>   : mBaseFrequency(aBaseFrequency)
>   , mNumOctaves(aNumOctaves)
> + , mStitchInfo()
> + , mStitchable(false)
> + , mType(static_cast<TurbulenceType>(0))

Please use the constant value here (TURBULENCE_TYPE_TURBULENCE) instead of casting zero.

::: gfx/layers/ipc/ShadowLayers.cpp
@@ +65,4 @@
>  public:
>    Transaction()
>      : mTargetRotation(ROTATION_0)
> +    , mTargetOrientation(0)

Please use eScreenOrientation_None from https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/dom/base/ScreenOrientation.h#25 instead of 0.

::: gfx/layers/mlgpu/RenderPassMLGPU.cpp
@@ +569,4 @@
>  
>  SingleTexturePass::SingleTexturePass(FrameBuilder* aBuilder, const ItemInfo& aItem)
>   : TexturedRenderPass(aBuilder, aItem),
> +   mSamplerMode(static_cast<SamplerMode>(0)),

Please use SamplerMode::LinearClamp from https://searchfox.org/mozilla-central/source/gfx/layers/mlgpu/MLGDeviceTypes.h#49

@@ +678,5 @@
>  
>  ComponentAlphaPass::ComponentAlphaPass(FrameBuilder* aBuilder, const ItemInfo& aItem)
>   : TexturedRenderPass(aBuilder, aItem),
> +   mOpacity(1.0f),
> +   mSamplerMode(static_cast<SamplerMode>(0))

Please use SamplerMode::LinearClamp from https://searchfox.org/mozilla-central/source/gfx/layers/mlgpu/MLGDeviceTypes.h#49

@@ +744,4 @@
>  
>  VideoRenderPass::VideoRenderPass(FrameBuilder* aBuilder, const ItemInfo& aItem)
>   : TexturedRenderPass(aBuilder, aItem),
> +   mSamplerMode(static_cast<SamplerMode>(0)),

Please use SamplerMode::LinearClamp (ad lib)

::: gfx/layers/mlgpu/TexturedLayerMLGPU.cpp
@@ +171,5 @@
>  
>  TempImageLayerMLGPU::TempImageLayerMLGPU(LayerManagerMLGPU* aManager)
>   : ImageLayer(aManager, static_cast<HostLayer*>(this)),
> +   TexturedLayerMLGPU(aManager),
> +   mFilter(static_cast<gfx::SamplingFilter>(0)),

Please use gfx::SamplingFilter::GOOD from https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/gfx/2d/Types.h#249

::: gfx/layers/opengl/TextureHostOGL.h
@@ +83,4 @@
>  {
>  public:
>    TextureSourceOGL()
> +    : mCachedSamplingFilter(static_cast<gfx::SamplingFilter>(0))

Same here about the sampling filter constant.

::: gfx/layers/protobuf/LayerScopePacket.pb.cc
@@ +412,4 @@
>  #endif  // !defined(_MSC_VER) || _MSC_VER >= 1900
>  
>  FramePacket::FramePacket()
> +  : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) , scale_(0.0) {

I am not sure whether we want to run this on the vendored protobuf code (in case we want to merge upstream changes in the future).
Perhaps it would be best to separate this into another patch so that we can land the rest and decide what to do with this.
Attached patch gfx.patch (obsolete) — Splinter Review
Updated patch that addresses your comments.
Attachment #8997781 - Attachment is obsolete: true
Attachment #8997781 - Flags: review?(nical.bugzilla)
Attachment #8997937 - Flags: review?(nical.bugzilla)
Comment on attachment 8997937 [details] [diff] [review]
gfx.patch

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

Oops, I missed one. With this static_cast removed r=me.

::: gfx/layers/apz/src/KeyboardMap.cpp
@@ +15,5 @@
> +  : mKeyCode(0)
> +  , mCharCode(0)
> +  , mModifiers(0)
> +  , mModifiersMask(0)
> +  , mEventType(static_cast<KeyboardInput::KeyboardEventType>(0))

Another static_cast I hadn't spotted the first time.
Attached patch gfx.patchSplinter Review
Attachment #8980418 - Attachment is obsolete: true
Attachment #8997944 - Flags: review?(nical.bugzilla)
Attachment #8997937 - Attachment is obsolete: true
Attachment #8997937 - Flags: review?(nical.bugzilla)
Comment on attachment 8997944 [details] [diff] [review]
gfx.patch

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

Thanks!
Attachment #8997944 - Flags: review?(nical.bugzilla) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5c08f10923
GFX - Initialize member fields in classes/ structures. r=nical
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/ba5c08f10923
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Whiteboard: [adv-main63-]
Product: Firefox Build System → Developer Infrastructure
You need to log in before you can comment on or make changes to this bug.