uninitialized class members: Review of fixes by 525063 per module

RESOLVED FIXED in Firefox 63

Status

RESOLVED FIXED
11 months ago
a month ago

People

(Reporter: tristanbourvon, Assigned: andi)

Tracking

(Blocks: 1 bug, {csectype-uninitialized, sec-audit})

unspecified
mozilla63
csectype-uninitialized, sec-audit
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [adv-main63-])

Attachments

(1 attachment, 110 obsolete attachments)

29.77 KB, patch
nical
: review+
Details | Diff | Splinter Review
(Reporter)

Description

11 months ago
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
(Reporter)

Updated

11 months ago
Summary: Review of fixes landed with 525063 per module → Review of fixes by 525063 per module
(Reporter)

Comment 1

11 months ago
The patch has since been backed out, but it still needs individual review per module before being re-landed.
(Reporter)

Updated

11 months ago
Blocks: 525063
(Reporter)

Comment 2

11 months ago
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)
(Assignee)

Comment 3

10 months ago
I've written a patch for clang-format that fixes the ifdef issue, here is the result.
Flags: needinfo?(bpostelnicu)
(Assignee)

Comment 4

10 months ago
Posted patch unint_format.patch (obsolete) — Splinter Review
(Reporter)

Comment 5

10 months ago
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.
(Reporter)

Comment 6

10 months ago
Posted patch Animation (obsolete) — Splinter Review
(Reporter)

Comment 7

10 months ago
Posted patch Content Security (obsolete) — Splinter Review
(Reporter)

Comment 8

10 months ago
Posted patch Cookies and Permissions (obsolete) — Splinter Review
(Reporter)

Comment 9

10 months ago
Posted patch Cycle Collector (obsolete) — Splinter Review
(Reporter)

Comment 10

10 months ago
Posted patch DevTools (obsolete) — Splinter Review
(Reporter)

Comment 11

10 months ago
Posted patch docshell (obsolete) — Splinter Review
(Reporter)

Comment 12

10 months ago
Posted patch Document Object Model (obsolete) — Splinter Review
(Reporter)

Comment 13

10 months ago
Posted patch DOM File (obsolete) — Splinter Review
(Reporter)

Comment 14

10 months ago
Posted patch Editor (obsolete) — Splinter Review
(Reporter)

Comment 15

10 months ago
Posted patch Event Handling (obsolete) — Splinter Review
(Reporter)

Comment 16

10 months ago
Posted patch Graphics (obsolete) — Splinter Review
(Reporter)

Comment 17

10 months ago
Posted patch HTML Parser (obsolete) — Splinter Review
(Reporter)

Comment 18

10 months ago
Posted patch Legacy HTML Parser (obsolete) — Splinter Review
(Reporter)

Comment 19

10 months ago
Posted patch ImageLib (obsolete) — Splinter Review
(Reporter)

Comment 20

10 months ago
Posted patch I18N Library (obsolete) — Splinter Review
(Reporter)

Comment 21

10 months ago
Posted patch IPC (obsolete) — Splinter Review
(Reporter)

Comment 22

10 months ago
Posted patch JavaScript (obsolete) — Splinter Review
(Reporter)

Comment 23

10 months ago
Posted patch JavaScript JIT (obsolete) — Splinter Review
(Reporter)

Comment 24

10 months ago
Posted patch Layout Engine (obsolete) — Splinter Review
(Reporter)

Comment 25

10 months ago
Posted patch libjar (obsolete) — Splinter Review
(Reporter)

Comment 26

10 months ago
Posted patch MathML (obsolete) — Splinter Review
(Reporter)

Comment 27

10 months ago
Posted patch Media Playback (obsolete) — Splinter Review
(Reporter)

Comment 28

10 months ago
Posted patch mfbt (obsolete) — Splinter Review
(Reporter)

Comment 29

10 months ago
Posted patch Necko (obsolete) — Splinter Review
(Reporter)

Comment 30

10 months ago
Posted patch Plugins (obsolete) — Splinter Review
(Reporter)

Comment 31

10 months ago
Posted patch Gecko Profiler (obsolete) — Splinter Review
(Reporter)

Comment 32

10 months ago
Posted patch RDF (obsolete) — Splinter Review
(Reporter)

Comment 33

10 months ago
Posted patch security (obsolete) — Splinter Review
(Reporter)

Comment 34

10 months ago
Posted patch Security - Mozilla PSM Glue (obsolete) — Splinter Review
(Reporter)

Comment 35

10 months ago
Posted patch Sandboxing - OSX (obsolete) — Splinter Review
(Reporter)

Comment 36

10 months ago
Posted patch storage (obsolete) — Splinter Review
(Reporter)

Comment 37

10 months ago
Posted patch String (obsolete) — Splinter Review
(Reporter)

Comment 38

10 months ago
Posted patch Style System (obsolete) — Splinter Review
(Reporter)

Comment 39

10 months ago
Posted patch SVG (obsolete) — Splinter Review
(Reporter)

Comment 40

10 months ago
Posted patch Testing Infrastructure (obsolete) — Splinter Review
(Reporter)

Comment 41

10 months ago
Posted patch Web Painting (obsolete) — Splinter Review
(Reporter)

Comment 42

10 months ago
Posted patch WebRTC (obsolete) — Splinter Review
(Reporter)

Comment 43

10 months ago
Posted patch WebRTC Media (obsolete) — Splinter Review
(Reporter)

Comment 44

10 months ago
Posted patch WebRTC Signaling (obsolete) — Splinter Review
(Reporter)

Comment 45

10 months ago
Posted patch WebVR (obsolete) — Splinter Review
(Reporter)

Comment 46

10 months ago
Posted patch Web Workers (obsolete) — Splinter Review
(Reporter)

Comment 47

10 months ago
Posted patch Widget (obsolete) — Splinter Review
(Reporter)

Comment 48

10 months ago
Posted patch Widget - OS X (obsolete) — Splinter Review
(Reporter)

Comment 49

10 months ago
Posted patch XBL (obsolete) — Splinter Review
(Reporter)

Comment 50

10 months ago
Posted patch XML (obsolete) — Splinter Review
(Reporter)

Comment 51

10 months ago
Posted patch XPCOM (obsolete) — Splinter Review
(Reporter)

Comment 52

10 months ago
Posted patch XPConnect (obsolete) — Splinter Review
(Reporter)

Comment 53

10 months ago
Posted patch XPToolkit (obsolete) — Splinter Review
(Reporter)

Comment 54

10 months ago
Posted patch XSLT Processor (obsolete) — Splinter Review
(Reporter)

Updated

10 months ago
Attachment #8982678 - Flags: review?(bbirtles)
(Reporter)

Updated

10 months ago
Attachment #8982679 - Flags: review?(ckerschb)
(Reporter)

Updated

10 months ago
Attachment #8982680 - Flags: review?(josh)
(Reporter)

Updated

10 months ago
Attachment #8982681 - Flags: review?(continuation)
(Reporter)

Updated

10 months ago
Attachment #8982682 - Flags: review?(bzbarsky)
(Reporter)

Updated

10 months ago
Attachment #8982682 - Flags: review?(bzbarsky)
(Reporter)

Updated

10 months ago
Attachment #8982683 - Flags: review?(bzbarsky)
(Reporter)

Updated

10 months ago
Attachment #8982684 - Flags: review?(peterv)
(Reporter)

Updated

10 months ago
Attachment #8982685 - Flags: review?(amarchesini)
(Reporter)

Updated

10 months ago
Attachment #8982686 - Flags: review?(masayuki)
(Reporter)

Updated

10 months ago
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+
(Reporter)

Updated

10 months ago
Attachment #8982688 - Flags: review?(jmuizelaar)
(Reporter)

Updated

10 months ago
Attachment #8982690 - Flags: review?(mrbkap)
(Reporter)

Updated

10 months ago
Attachment #8982682 - Flags: review?(pbrosset)
(Reporter)

Updated

10 months ago
Attachment #8982691 - Flags: review?(tnikkel)
(Reporter)

Updated

10 months ago
Attachment #8982692 - Flags: review?(jfkthame)
(Reporter)

Updated

10 months ago
Attachment #8982693 - Flags: review?(jmathies)
(Reporter)

Updated

10 months ago
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-
(Reporter)

Updated

10 months ago
Attachment #8982695 - Flags: review?(jdemooij)
(Reporter)

Updated

10 months ago
Attachment #8982696 - Flags: review?(dbaron)
(Reporter)

Updated

10 months ago
Attachment #8982697 - Flags: review?(aklotz)
(Reporter)

Updated

10 months ago
Attachment #8982698 - Flags: review?(karlt)
(Reporter)

Updated

10 months ago
Attachment #8982700 - Flags: review?(jyavenard)
(Reporter)

Updated

10 months ago
Attachment #8982701 - Flags: review?(jwalden+bmo)
(Reporter)

Updated

10 months ago
Attachment #8982702 - Flags: review?(mcmanus)
(Reporter)

Updated

10 months ago
Attachment #8982703 - Flags: review?(jmathies)
(Reporter)

Updated

10 months ago
Attachment #8982704 - Flags: review?(mstange)
(Reporter)

Updated

10 months ago
Attachment #8982705 - Flags: review?(l10n)
(Reporter)

Updated

10 months ago
Attachment #8982706 - Flags: review?(kaie)
(Reporter)

Updated

10 months ago
Attachment #8982707 - Flags: review?(dkeeler)
(Reporter)

Updated

10 months ago
Attachment #8982708 - Flags: review?(haftandilian)
(Reporter)

Updated

10 months ago
Attachment #8982709 - Flags: review?(mak77)
(Reporter)

Updated

10 months ago
Attachment #8982710 - Flags: review?(dbaron)
(Reporter)

Updated

10 months ago
Attachment #8982712 - Flags: review?(cam)
(Reporter)

Updated

10 months ago
Attachment #8982713 - Flags: review?(jwatt)
(Reporter)

Updated

10 months ago
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 62

10 months ago
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+

Updated

10 months ago
Attachment #8982703 - Flags: review?(jmathies) → review+

Updated

10 months ago
Attachment #8982693 - Flags: review?(jmathies) → review+
Group: firefox-build-security → core-security
Keywords: sec-audit
Keywords: csectype-uninitialized
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?

Updated

10 months ago
Attachment #8982680 - Flags: review?(josh) → review+
Please initialize using () and not {} in CC, if possible.
(Assignee)

Comment 88

9 months ago
(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+.
(Assignee)

Updated

9 months ago
Attachment #8982681 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Attachment #8982704 - Attachment is obsolete: true
(Assignee)

Comment 103

9 months ago
Comment on attachment 8982705 [details] [diff] [review]
RDF

RDF is gone
Attachment #8982705 - Attachment is obsolete: true
(Assignee)

Comment 105

9 months ago
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
(Assignee)

Updated

9 months ago
Attachment #8982710 - Attachment is obsolete: true
(Assignee)

Updated

9 months ago
Assignee: nobody → bpostelnicu
(Assignee)

Comment 106

9 months ago
Posted patch XPCom-String.patch (obsolete) — Splinter Review
Attachment #8985326 - Flags: review?(dbaron)
(Assignee)

Comment 107

9 months ago
Posted patch Layout.patch (obsolete) — Splinter Review
Attachment #8982696 - Attachment is obsolete: true
Attachment #8985457 - Flags: review?(dbaron)
(Assignee)

Comment 108

9 months ago
Posted patch dom_filesystem.patch (obsolete) — Splinter Review
Attachment #8982685 - Attachment is obsolete: true
Attachment #8985465 - Flags: review?(amarchesini)
(Assignee)

Comment 109

9 months ago
Posted patch dom_events.patch (obsolete) — Splinter Review
Attachment #8982687 - Attachment is obsolete: true
Attachment #8985467 - Flags: review?(bugs)
(Assignee)

Comment 110

9 months ago
Posted 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+
(Assignee)

Comment 112

9 months ago
Posted patch jit.patch (obsolete) — Splinter Review
Attachment #8982695 - Attachment is obsolete: true
Attachment #8985483 - Flags: review?(jdemooij)

Updated

9 months ago
Attachment #8985467 - Flags: review?(bugs) → review+
(Assignee)

Comment 115

9 months ago
Posted patch dom_media.patch (obsolete) — Splinter Review
Attachment #8982700 - Attachment is obsolete: true
Attachment #8985566 - Flags: review?(jyavenard)
(Assignee)

Comment 116

9 months ago
Posted patch storage.patch (obsolete) — Splinter Review
Attachment #8982709 - Attachment is obsolete: true
Attachment #8985567 - Flags: review?(mak77)
(Assignee)

Comment 117

9 months ago
Posted patch mftb.patch (obsolete) — Splinter Review
Attachment #8982701 - Attachment is obsolete: true
Attachment #8985578 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 118

9 months ago
Posted patch xslt.patch (obsolete) — Splinter Review
Attachment #8982729 - Attachment is obsolete: true
Attachment #8985579 - Flags: review?(peterv)
(Assignee)

Comment 119

9 months ago
Posted patch xptoolkit.patch (obsolete) — Splinter Review
Attachment #8982728 - Attachment is obsolete: true
Attachment #8985584 - Flags: review?(jvarga)
(Assignee)

Comment 120

9 months ago
Posted patch xpconnect.patch (obsolete) — Splinter Review
Attachment #8982727 - Attachment is obsolete: true
Attachment #8985585 - Flags: review?(peterv)
(Assignee)

Comment 121

9 months ago
Posted patch xpcom.patch (obsolete) — Splinter Review
Attachment #8982726 - Attachment is obsolete: true
Attachment #8985589 - Flags: review?(nfroyd)
Attachment #8985567 - Flags: review?(mak77) → review+
(Assignee)

Comment 123

9 months ago
Posted patch dom-xml.patch (obsolete) — Splinter Review
Attachment #8982725 - Attachment is obsolete: true
Attachment #8985601 - Flags: review?(peterv)
(Assignee)

Comment 124

9 months ago
Posted patch dom-xbl.patch (obsolete) — Splinter Review
Attachment #8982724 - Attachment is obsolete: true
Attachment #8985602 - Flags: review?(mrbkap)
(Assignee)

Comment 125

9 months ago
Posted patch widget_osx.patch (obsolete) — Splinter Review
Attachment #8982723 - Attachment is obsolete: true
Attachment #8985610 - Flags: review?(nfroyd)
(Assignee)

Comment 126

9 months ago
Posted 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!
(Assignee)

Comment 128

9 months ago
Posted patch dom-workers.patch (obsolete) — Splinter Review
Attachment #8982720 - Attachment is obsolete: true
Attachment #8985618 - Flags: review?(nfroyd)
(Assignee)

Comment 129

9 months ago
Posted patch webvr.patch (obsolete) — Splinter Review
Attachment #8982719 - Attachment is obsolete: true
Attachment #8985630 - Flags: review?(kgilbert)
(Assignee)

Comment 130

9 months ago
Posted patch webrtc-signaling.patch (obsolete) — Splinter Review
Attachment #8982718 - Attachment is obsolete: true
Attachment #8985632 - Flags: review?(rjesup)
(Assignee)

Comment 131

9 months ago
Posted patch webrtc-media.patch (obsolete) — Splinter Review
Attachment #8982717 - Attachment is obsolete: true
Attachment #8985635 - Flags: review?(rjesup)
(Assignee)

Comment 132

9 months ago
Posted patch webrtc.patch (obsolete) — Splinter Review
Attachment #8982716 - Attachment is obsolete: true
Attachment #8985636 - Flags: review?(rjesup)
(Assignee)

Comment 133

9 months ago
Posted 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+
(Assignee)

Comment 137

9 months ago
Posted 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+
(Assignee)

Comment 139

9 months ago
(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+
(Assignee)

Comment 144

9 months ago
(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+
(Assignee)

Comment 150

9 months ago
Posted patch svg.patch (obsolete) — Splinter Review
Attachment #8982713 - Attachment is obsolete: true
Attachment #8982713 - Flags: review?(jwatt)
Attachment #8985771 - Flags: review?(jwatt)
(Assignee)

Comment 151

9 months ago
Posted patch libjar.patch (obsolete) — Splinter Review
Attachment #8982697 - Attachment is obsolete: true
Attachment #8982697 - Flags: review?(aklotz)
Attachment #8985772 - Flags: review?(aklotz)
(Assignee)

Comment 152

9 months ago
Posted patch js.patch (obsolete) — Splinter Review
Attachment #8982694 - Attachment is obsolete: true
Attachment #8982694 - Flags: review?(jorendorff)
Attachment #8985773 - Flags: review?(jorendorff)
(Assignee)

Comment 153

9 months ago
Posted patch devtools.patch (obsolete) — Splinter Review
Attachment #8982682 - Attachment is obsolete: true
Attachment #8982682 - Flags: review?(jimb)
Attachment #8985774 - Flags: review?(jimb)
(Assignee)

Comment 154

9 months ago
Posted patch dom.patch (obsolete) — Splinter Review
Attachment #8982684 - Attachment is obsolete: true
Attachment #8982684 - Flags: review?(peterv)
Attachment #8985778 - Flags: review?(peterv)
(Assignee)

Comment 156

9 months ago
Posted 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+
(Assignee)

Comment 160

9 months ago
Posted patch mfbt.patch (obsolete) — Splinter Review
Attachment #8985578 - Attachment is obsolete: true
Attachment #8985820 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 162

9 months ago
Posted patch docshell.patch (obsolete) — Splinter Review
Attachment #8982683 - Attachment is obsolete: true
Attachment #8985886 - Flags: review?(bugs)
(Assignee)

Comment 163

9 months ago
(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.
(Assignee)

Updated

9 months ago
See Also: → bug 1469240
(Assignee)

Comment 164

9 months ago
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+
(Assignee)

Comment 169

9 months ago
Posted 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)
(Assignee)

Comment 171

9 months ago
(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-
(Assignee)

Comment 177

9 months ago
(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.
(Assignee)

Updated

9 months ago
Attachment #8985618 - Attachment is obsolete: true
(Assignee)

Comment 178

9 months ago
Posted patch widget_osx.patch (obsolete) — Splinter Review
Attachment #8986012 - Flags: review?(nfroyd)
(Assignee)

Updated

9 months ago
Attachment #8985610 - Attachment is obsolete: true
(Assignee)

Comment 179

9 months ago
Posted patch widget.patch (obsolete) — Splinter Review
Attachment #8985617 - Attachment is obsolete: true
Attachment #8986019 - Flags: review?(nfroyd)
(Assignee)

Updated

9 months ago
See Also: → bug 1469365
(Reporter)

Comment 181

9 months ago
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+
(Assignee)

Comment 183

9 months ago
Posted patch xpcom.patch (obsolete) — Splinter Review
Attachment #8985589 - Attachment is obsolete: true
Attachment #8986032 - Flags: review?(nfroyd)
(Assignee)

Comment 184

9 months ago
Posted 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)
(Assignee)

Comment 190

9 months ago
Posted patch libjar.patch (obsolete) — Splinter Review
Attachment #8985772 - Attachment is obsolete: true
Attachment #8985772 - Flags: review?(aklotz)
Attachment #8986104 - Flags: review?(aklotz)
(Assignee)

Comment 191

9 months ago
Posted patch js.patch (obsolete) — Splinter Review
Attachment #8985773 - Attachment is obsolete: true
Attachment #8985773 - Flags: review?(jwalden+bmo)
Attachment #8986127 - Flags: review?(jorendorff)
(Assignee)

Comment 192

9 months ago
Posted patch devtools.patch (obsolete) — Splinter Review
Attachment #8985774 - Attachment is obsolete: true
Attachment #8985774 - Flags: review?(jimb)
Attachment #8986131 - Flags: review?(jimb)
(Assignee)

Comment 193

9 months ago
Posted patch dom.patch (obsolete) — Splinter Review
Attachment #8985778 - Attachment is obsolete: true
Attachment #8985778 - Flags: review?(peterv)
Attachment #8986169 - Flags: review?(peterv)
(Assignee)

Comment 194

9 months ago
Posted patch gfx.patch (obsolete) — Splinter Review
Attachment #8985789 - Attachment is obsolete: true
Attachment #8985789 - Flags: review?(mstange)
Attachment #8986175 - Flags: review?(jmuizelaar)
(Assignee)

Comment 195

9 months ago
Posted 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)
(Assignee)

Comment 196

9 months ago
Posted 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+
(Assignee)

Comment 203

9 months ago
(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
(Assignee)

Comment 204

9 months ago
Posted 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)
(Assignee)

Comment 206

9 months ago
Posted 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)
(Assignee)

Comment 207

9 months ago
Posted 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..

Updated

9 months ago
Attachment #8986363 - Flags: review?(jmathies) → review+
(Assignee)

Comment 211

9 months ago
Posted patch XPCom-String.patch (obsolete) — Splinter Review
Attachment #8985326 - Attachment is obsolete: true
Attachment #8985326 - Flags: review?(dbaron)
Attachment #8986528 - Flags: review?(dbaron)
(Assignee)

Updated

9 months ago
Attachment #8986528 - Flags: review?(dbaron) → review?(nfroyd)

Comment 212

9 months ago
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)
(Assignee)

Comment 213

9 months ago
Posted patch devtools.patch (obsolete) — Splinter Review
Attachment #8986131 - Attachment is obsolete: true
Attachment #8986540 - Flags: review?(jimb)

Comment 214

9 months ago
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+
(Assignee)

Comment 219

9 months ago
Posted patch dom-xml.patch (obsolete) — Splinter Review
Attachment #8985601 - Attachment is obsolete: true
Attachment #8985601 - Flags: review?(peterv)
Attachment #8986683 - Flags: review?(peterv)
(Assignee)

Comment 220

9 months ago
Posted 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
(Assignee)

Comment 230

9 months ago
Posted 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+

Updated

9 months ago
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-