Closed
Bug 1453795
Opened 6 years ago
Closed 6 years ago
uninitialized class members: Review of fixes by 525063 per module
Categories
(Developer Infrastructure :: Source Code Analysis, enhancement)
Developer Infrastructure
Source Code Analysis
Tracking
(firefox63 fixed)
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: tristanbourvon, Assigned: andi)
References
Details
(Keywords: csectype-uninitialized, sec-audit, Whiteboard: [adv-main63-])
Attachments
(1 file, 110 obsolete files)
29.77 KB,
patch
|
nical
:
review+
|
Details | Diff | Splinter Review |
The fixes landed with 525063 were not individually reviewed by each module owner and have some quality issues. These include: - Stray FIXME comments that require hand-written initialization to be written - Poor formatting caused by clang-format being confused by #ifdefs - Variables getting initialized while a comment explicitly mentions they should NOT be initialized Module owners may also have additional specific comments to add. See these commits for reference: Main commit: https://hg.mozilla.org/mozilla-central/rev/d7d2f08e051c Fix webrender assertions: https://hg.mozilla.org/mozilla-central/rev/bf13e4103150 Backing out of js/src/: https://hg.mozilla.org/mozilla-central/rev/6ff8aaef2866 Backing out of js/public/: https://hg.mozilla.org/mozilla-central/rev/516c4fb1e4b862b78a40c472e4d61dea79d51890
Reporter | ||
Updated•6 years ago
|
Summary: Review of fixes landed with 525063 per module → Review of fixes by 525063 per module
Reporter | ||
Comment 1•6 years ago
|
||
The patch has since been backed out, but it still needs individual review per module before being re-landed.
Reporter | ||
Comment 2•6 years 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•6 years ago
|
||
I've written a patch for clang-format that fixes the ifdef issue, here is the result.
Flags: needinfo?(bpostelnicu)
Assignee | ||
Comment 4•6 years ago
|
||
Reporter | ||
Comment 5•6 years 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•6 years ago
|
||
Reporter | ||
Comment 7•6 years ago
|
||
Reporter | ||
Comment 8•6 years ago
|
||
Reporter | ||
Comment 9•6 years ago
|
||
Reporter | ||
Comment 10•6 years ago
|
||
Reporter | ||
Comment 11•6 years ago
|
||
Reporter | ||
Comment 12•6 years ago
|
||
Reporter | ||
Comment 13•6 years ago
|
||
Reporter | ||
Comment 14•6 years ago
|
||
Reporter | ||
Comment 15•6 years ago
|
||
Reporter | ||
Comment 16•6 years ago
|
||
Reporter | ||
Comment 17•6 years ago
|
||
Reporter | ||
Comment 18•6 years ago
|
||
Reporter | ||
Comment 19•6 years ago
|
||
Reporter | ||
Comment 20•6 years ago
|
||
Reporter | ||
Comment 21•6 years ago
|
||
Reporter | ||
Comment 22•6 years ago
|
||
Reporter | ||
Comment 23•6 years ago
|
||
Reporter | ||
Comment 24•6 years ago
|
||
Reporter | ||
Comment 25•6 years ago
|
||
Reporter | ||
Comment 26•6 years ago
|
||
Reporter | ||
Comment 27•6 years ago
|
||
Reporter | ||
Comment 28•6 years ago
|
||
Reporter | ||
Comment 29•6 years ago
|
||
Reporter | ||
Comment 30•6 years ago
|
||
Reporter | ||
Comment 31•6 years ago
|
||
Reporter | ||
Comment 32•6 years ago
|
||
Reporter | ||
Comment 33•6 years ago
|
||
Reporter | ||
Comment 34•6 years ago
|
||
Reporter | ||
Comment 35•6 years ago
|
||
Reporter | ||
Comment 36•6 years ago
|
||
Reporter | ||
Comment 37•6 years ago
|
||
Reporter | ||
Comment 38•6 years ago
|
||
Reporter | ||
Comment 39•6 years ago
|
||
Reporter | ||
Comment 40•6 years ago
|
||
Reporter | ||
Comment 41•6 years ago
|
||
Reporter | ||
Comment 42•6 years ago
|
||
Reporter | ||
Comment 43•6 years ago
|
||
Reporter | ||
Comment 44•6 years ago
|
||
Reporter | ||
Comment 45•6 years ago
|
||
Reporter | ||
Comment 46•6 years ago
|
||
Reporter | ||
Comment 47•6 years ago
|
||
Reporter | ||
Comment 48•6 years ago
|
||
Reporter | ||
Comment 49•6 years ago
|
||
Reporter | ||
Comment 50•6 years ago
|
||
Reporter | ||
Comment 51•6 years ago
|
||
Reporter | ||
Comment 52•6 years ago
|
||
Reporter | ||
Comment 53•6 years ago
|
||
Reporter | ||
Comment 54•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Attachment #8982678 -
Flags: review?(bbirtles)
Reporter | ||
Updated•6 years ago
|
Attachment #8982679 -
Flags: review?(ckerschb)
Reporter | ||
Updated•6 years ago
|
Attachment #8982680 -
Flags: review?(josh)
Reporter | ||
Updated•6 years ago
|
Attachment #8982681 -
Flags: review?(continuation)
Reporter | ||
Updated•6 years ago
|
Attachment #8982682 -
Flags: review?(bzbarsky)
Reporter | ||
Updated•6 years ago
|
Attachment #8982682 -
Flags: review?(bzbarsky)
Reporter | ||
Updated•6 years ago
|
Attachment #8982683 -
Flags: review?(bzbarsky)
Reporter | ||
Updated•6 years ago
|
Attachment #8982684 -
Flags: review?(peterv)
Reporter | ||
Updated•6 years ago
|
Attachment #8982685 -
Flags: review?(amarchesini)
Reporter | ||
Updated•6 years ago
|
Attachment #8982686 -
Flags: review?(masayuki)
Reporter | ||
Updated•6 years ago
|
Attachment #8982687 -
Flags: review?(bugs)
Comment 55•6 years ago
|
||
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•6 years ago
|
Attachment #8982688 -
Flags: review?(jmuizelaar)
Reporter | ||
Updated•6 years ago
|
Attachment #8982690 -
Flags: review?(mrbkap)
Reporter | ||
Updated•6 years ago
|
Attachment #8982682 -
Flags: review?(pbrosset)
Reporter | ||
Updated•6 years ago
|
Attachment #8982691 -
Flags: review?(tnikkel)
Reporter | ||
Updated•6 years ago
|
Attachment #8982692 -
Flags: review?(jfkthame)
Reporter | ||
Updated•6 years ago
|
Attachment #8982693 -
Flags: review?(jmathies)
Reporter | ||
Updated•6 years ago
|
Attachment #8982694 -
Flags: review?(jorendorff)
Comment 56•6 years ago
|
||
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•6 years ago
|
Attachment #8982695 -
Flags: review?(jdemooij)
Reporter | ||
Updated•6 years ago
|
Attachment #8982696 -
Flags: review?(dbaron)
Reporter | ||
Updated•6 years ago
|
Attachment #8982697 -
Flags: review?(aklotz)
Reporter | ||
Updated•6 years ago
|
Attachment #8982698 -
Flags: review?(karlt)
Reporter | ||
Updated•6 years ago
|
Attachment #8982700 -
Flags: review?(jyavenard)
Reporter | ||
Updated•6 years ago
|
Attachment #8982701 -
Flags: review?(jwalden+bmo)
Reporter | ||
Updated•6 years ago
|
Attachment #8982702 -
Flags: review?(mcmanus)
Reporter | ||
Updated•6 years ago
|
Attachment #8982703 -
Flags: review?(jmathies)
Reporter | ||
Updated•6 years ago
|
Attachment #8982704 -
Flags: review?(mstange)
Reporter | ||
Updated•6 years ago
|
Attachment #8982705 -
Flags: review?(l10n)
Reporter | ||
Updated•6 years ago
|
Attachment #8982706 -
Flags: review?(kaie)
Reporter | ||
Updated•6 years ago
|
Attachment #8982707 -
Flags: review?(dkeeler)
Reporter | ||
Updated•6 years ago
|
Attachment #8982708 -
Flags: review?(haftandilian)
Reporter | ||
Updated•6 years ago
|
Attachment #8982709 -
Flags: review?(mak77)
Reporter | ||
Updated•6 years ago
|
Attachment #8982710 -
Flags: review?(dbaron)
Reporter | ||
Updated•6 years ago
|
Attachment #8982712 -
Flags: review?(cam)
Reporter | ||
Updated•6 years ago
|
Attachment #8982713 -
Flags: review?(jwatt)
Reporter | ||
Updated•6 years ago
|
Attachment #8982714 -
Flags: review?(jmaher)
Updated•6 years ago
|
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+
Updated•6 years ago
|
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 61•6 years ago
|
||
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-
Updated•6 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 62•6 years ago
|
||
Comment on attachment 8982705 [details] [diff] [review] RDF RDF is gone.
Attachment #8982705 -
Flags: review?(l10n) → review-
Comment 63•6 years ago
|
||
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 64•6 years ago
|
||
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 65•6 years ago
|
||
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 66•6 years ago
|
||
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 67•6 years ago
|
||
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 68•6 years ago
|
||
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+
Comment 69•6 years ago
|
||
(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.)
Comment 70•6 years ago
|
||
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.
Updated•6 years ago
|
Attachment #8982678 -
Flags: review?(bbirtles) → review+
Comment 71•6 years ago
|
||
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 72•6 years ago
|
||
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+
Comment 73•6 years ago
|
||
(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 74•6 years ago
|
||
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 75•6 years ago
|
||
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 76•6 years ago
|
||
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 77•6 years ago
|
||
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-
Comment 78•6 years ago
|
||
FWIW, I'd also prefer () over {}, but the XPCOM patch was so tiny I didn't care enough to say anything.
Comment 79•6 years ago
|
||
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 80•6 years ago
|
||
Comment on attachment 8982708 [details] [diff] [review] Sandboxing - OSX Please use parens instead of braces for consistency.
Attachment #8982708 -
Flags: review?(haftandilian) → review+
Updated•6 years ago
|
Attachment #8982703 -
Flags: review?(jmathies) → review+
Updated•6 years ago
|
Attachment #8982693 -
Flags: review?(jmathies) → review+
Updated•6 years ago
|
Group: firefox-build-security → core-security
Updated•6 years ago
|
Keywords: csectype-uninitialized
Comment 81•6 years ago
|
||
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 82•6 years ago
|
||
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 83•6 years ago
|
||
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+
Comment 84•6 years ago
|
||
(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.
Updated•6 years ago
|
Attachment #8982698 -
Flags: review?(karlt) → review+
Comment 85•6 years ago
|
||
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•6 years ago
|
Attachment #8982680 -
Flags: review?(josh) → review+
Assignee | ||
Comment 86•6 years ago
|
||
Comment on attachment 8982681 [details] [diff] [review] Cycle Collector https://hg.mozilla.org/integration/mozilla-inbound/rev/b75ad30847fe46f49fcd93975de49609bd820248
Comment 87•6 years ago
|
||
Please initialize using () and not {} in CC, if possible.
Assignee | ||
Comment 88•6 years 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 | ||
Comment 89•6 years ago
|
||
Comment on attachment 8982704 [details] [diff] [review] Gecko Profiler https://hg.mozilla.org/integration/mozilla-inbound/rev/73a994c463a0b1f6cab41b8c48f04cb99a1c0008
Assignee | ||
Updated•6 years ago
|
Attachment #8982681 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Attachment #8982704 -
Attachment is obsolete: true
Assignee | ||
Comment 90•6 years ago
|
||
Comment on attachment 8982686 [details] [diff] [review] Editor https://hg.mozilla.org/integration/mozilla-inbound/rev/93588b7e9bc82cb381b8580d0172585118db7506
Attachment #8982686 -
Attachment is obsolete: true
Assignee | ||
Comment 91•6 years ago
|
||
Comment on attachment 8982678 [details] [diff] [review] Animation https://hg.mozilla.org/integration/mozilla-inbound/rev/a9ec5b194001c83e987e4b1f41d911d329379607
Attachment #8982678 -
Attachment is obsolete: true
Assignee | ||
Comment 92•6 years ago
|
||
Comment on attachment 8982679 [details] [diff] [review] Content Security https://hg.mozilla.org/integration/mozilla-inbound/rev/3b89a0f2abe5ae65cf2ec2970e1b0ac754f3f282
Attachment #8982679 -
Attachment is obsolete: true
Assignee | ||
Comment 93•6 years ago
|
||
Comment on attachment 8982680 [details] [diff] [review] Cookies and Permissions https://hg.mozilla.org/integration/mozilla-inbound/rev/c06e65cb2745f2d62f9cb797731b253fc466a8d1
Attachment #8982680 -
Attachment is obsolete: true
Assignee | ||
Comment 94•6 years ago
|
||
Comment on attachment 8982690 [details] [diff] [review] Legacy HTML Parser https://hg.mozilla.org/integration/mozilla-inbound/rev/c22b89e6f83a7aeb9f030691703c3e25dff0a113
Attachment #8982690 -
Attachment is obsolete: true
Assignee | ||
Comment 95•6 years ago
|
||
Comment on attachment 8982691 [details] [diff] [review] ImageLib https://hg.mozilla.org/integration/mozilla-inbound/rev/74487424ec36bf15f041e99dd57dd4c85052ebba
Attachment #8982691 -
Attachment is obsolete: true
Assignee | ||
Comment 96•6 years ago
|
||
Comment on attachment 8982703 [details] [diff] [review] Plugins https://hg.mozilla.org/integration/mozilla-inbound/rev/00e35d8446bd1dbead1a69fb3d2e3c0d7f278cf3
Attachment #8982703 -
Attachment is obsolete: true
Assignee | ||
Comment 97•6 years ago
|
||
Comment on attachment 8982693 [details] [diff] [review] IPC https://hg.mozilla.org/integration/mozilla-inbound/rev/99917d1409c200ca6a1fa35894895d89340deb09
Attachment #8982693 -
Attachment is obsolete: true
Assignee | ||
Comment 98•6 years ago
|
||
Comment on attachment 8982698 [details] [diff] [review] MathML https://hg.mozilla.org/integration/mozilla-inbound/rev/01fb8307daafc8160fd2eb2a0ce1d44df64ad5e0
Attachment #8982698 -
Attachment is obsolete: true
Assignee | ||
Comment 99•6 years ago
|
||
Comment on attachment 8982702 [details] [diff] [review] Necko https://hg.mozilla.org/integration/mozilla-inbound/rev/287bdf729c7985a5995900b8cee7a7a7db4f98d9
Attachment #8982702 -
Attachment is obsolete: true
Assignee | ||
Comment 100•6 years ago
|
||
Comment on attachment 8982707 [details] [diff] [review] Security - Mozilla PSM Glue https://hg.mozilla.org/integration/mozilla-inbound/rev/44fc7d8706800cdc44612805123d60c01d5ac616
Attachment #8982707 -
Attachment is obsolete: true
Assignee | ||
Comment 101•6 years ago
|
||
Comment on attachment 8982689 [details] [diff] [review] HTML Parser https://hg.mozilla.org/integration/mozilla-inbound/rev/b84f61d9893a5bcc8cc8d6acbca131f288afc494
Attachment #8982689 -
Attachment is obsolete: true
Assignee | ||
Comment 102•6 years ago
|
||
Comment on attachment 8982708 [details] [diff] [review] Sandboxing - OSX https://hg.mozilla.org/integration/mozilla-inbound/rev/24142d07f73edf9124ddef3cb01ba7a17617d9be
Attachment #8982708 -
Attachment is obsolete: true
Assignee | ||
Comment 103•6 years ago
|
||
Comment on attachment 8982705 [details] [diff] [review] RDF RDF is gone
Attachment #8982705 -
Attachment is obsolete: true
Assignee | ||
Comment 104•6 years ago
|
||
Comment on attachment 8982712 [details] [diff] [review] Style System https://hg.mozilla.org/integration/mozilla-inbound/rev/ebe097110cd2b4c04d19aa7c50a3a58a159b34e8 remote:
Attachment #8982712 -
Attachment is obsolete: true
Assignee | ||
Comment 105•6 years 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•6 years ago
|
Attachment #8982710 -
Attachment is obsolete: true
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → bpostelnicu
Assignee | ||
Comment 106•6 years ago
|
||
Attachment #8985326 -
Flags: review?(dbaron)
Assignee | ||
Comment 107•6 years ago
|
||
Attachment #8982696 -
Attachment is obsolete: true
Attachment #8985457 -
Flags: review?(dbaron)
Assignee | ||
Comment 108•6 years ago
|
||
Attachment #8982685 -
Attachment is obsolete: true
Attachment #8985465 -
Flags: review?(amarchesini)
Assignee | ||
Comment 109•6 years ago
|
||
Attachment #8982687 -
Attachment is obsolete: true
Attachment #8985467 -
Flags: review?(bugs)
Assignee | ||
Comment 110•6 years ago
|
||
Attachment #8982692 -
Attachment is obsolete: true
Attachment #8985469 -
Flags: review?(jfkthame)
Comment 111•6 years ago
|
||
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•6 years ago
|
||
Attachment #8982695 -
Attachment is obsolete: true
Attachment #8985483 -
Flags: review?(jdemooij)
Updated•6 years ago
|
Attachment #8985467 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 113•6 years ago
|
||
Comment on attachment 8985465 [details] [diff] [review] dom_filesystem.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/65bf931f5806d5a18577c3eba088dbb41bf7e453
Attachment #8985465 -
Attachment is obsolete: true
Assignee | ||
Comment 114•6 years ago
|
||
Comment on attachment 8985467 [details] [diff] [review] dom_events.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/43f5aa859a2fc1999c6d2b2b5cbb62726c061fca
Attachment #8985467 -
Attachment is obsolete: true
Assignee | ||
Comment 115•6 years ago
|
||
Attachment #8982700 -
Attachment is obsolete: true
Attachment #8985566 -
Flags: review?(jyavenard)
Assignee | ||
Comment 116•6 years ago
|
||
Attachment #8982709 -
Attachment is obsolete: true
Attachment #8985567 -
Flags: review?(mak77)
Assignee | ||
Comment 117•6 years ago
|
||
Attachment #8982701 -
Attachment is obsolete: true
Attachment #8985578 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 118•6 years ago
|
||
Attachment #8982729 -
Attachment is obsolete: true
Attachment #8985579 -
Flags: review?(peterv)
Assignee | ||
Comment 119•6 years ago
|
||
Attachment #8982728 -
Attachment is obsolete: true
Attachment #8985584 -
Flags: review?(jvarga)
Assignee | ||
Comment 120•6 years ago
|
||
Attachment #8982727 -
Attachment is obsolete: true
Attachment #8985585 -
Flags: review?(peterv)
Assignee | ||
Comment 121•6 years ago
|
||
Attachment #8982726 -
Attachment is obsolete: true
Attachment #8985589 -
Flags: review?(nfroyd)
Updated•6 years ago
|
Attachment #8985567 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 122•6 years ago
|
||
Comment on attachment 8985567 [details] [diff] [review] storage.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/16a997fcfb0635f170f1858134ccca377a0b82d7
Attachment #8985567 -
Attachment is obsolete: true
Assignee | ||
Comment 123•6 years ago
|
||
Attachment #8982725 -
Attachment is obsolete: true
Attachment #8985601 -
Flags: review?(peterv)
Assignee | ||
Comment 124•6 years ago
|
||
Attachment #8982724 -
Attachment is obsolete: true
Attachment #8985602 -
Flags: review?(mrbkap)
Assignee | ||
Comment 125•6 years ago
|
||
Attachment #8982723 -
Attachment is obsolete: true
Attachment #8985610 -
Flags: review?(nfroyd)
Assignee | ||
Comment 126•6 years ago
|
||
Attachment #8982722 -
Attachment is obsolete: true
Attachment #8985617 -
Flags: review?(nfroyd)
Comment 127•6 years ago
|
||
FYI, Coverity is detecting the fixes "Defects eliminated: 68" bravo!
Assignee | ||
Comment 128•6 years ago
|
||
Attachment #8982720 -
Attachment is obsolete: true
Attachment #8985618 -
Flags: review?(nfroyd)
Assignee | ||
Comment 129•6 years ago
|
||
Attachment #8982719 -
Attachment is obsolete: true
Attachment #8985630 -
Flags: review?(kgilbert)
Assignee | ||
Comment 130•6 years ago
|
||
Attachment #8982718 -
Attachment is obsolete: true
Attachment #8985632 -
Flags: review?(rjesup)
Assignee | ||
Comment 131•6 years ago
|
||
Attachment #8982717 -
Attachment is obsolete: true
Attachment #8985635 -
Flags: review?(rjesup)
Assignee | ||
Comment 132•6 years ago
|
||
Attachment #8982716 -
Attachment is obsolete: true
Attachment #8985636 -
Flags: review?(rjesup)
Assignee | ||
Comment 133•6 years ago
|
||
Attachment #8982715 -
Attachment is obsolete: true
Attachment #8985640 -
Flags: review?(mstange)
Comment 134•6 years ago
|
||
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 135•6 years ago
|
||
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-
Updated•6 years ago
|
Attachment #8985635 -
Flags: review?(rjesup) → review+
Comment 136•6 years ago
|
||
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•6 years ago
|
||
Attachment #8985632 -
Attachment is obsolete: true
Attachment #8985651 -
Flags: review?(rjesup)
Comment 138•6 years ago
|
||
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+
Updated•6 years ago
|
Attachment #8985651 -
Flags: review?(rjesup) → review+
Assignee | ||
Comment 139•6 years 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;
Assignee | ||
Comment 140•6 years ago
|
||
Comment on attachment 8985651 [details] [diff] [review] webrtc-signaling.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/485b3efd9ea89580be685757a783730e2589c0d4
Attachment #8985651 -
Attachment is obsolete: true
Assignee | ||
Comment 141•6 years ago
|
||
Comment on attachment 8985635 [details] [diff] [review] webrtc-media.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/3bd8839fdafacfdc8c1d2c6c4b01a1722817ff2f
Attachment #8985635 -
Attachment is obsolete: true
Assignee | ||
Comment 142•6 years ago
|
||
Comment on attachment 8985636 [details] [diff] [review] webrtc.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/837f246ed35243176c24b8756c0b1deac548a182
Attachment #8985636 -
Attachment is obsolete: true
Comment 143•6 years ago
|
||
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•6 years 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.
Assignee | ||
Comment 145•6 years ago
|
||
Comment on attachment 8985630 [details] [diff] [review] webvr.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/489451bfba8c129635a5686077ba3ecfc320aa81
Attachment #8985630 -
Attachment is obsolete: true
Comment 146•6 years ago
|
||
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+
Comment 147•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/b75ad30847fe https://hg.mozilla.org/mozilla-central/rev/73a994c463a0 https://hg.mozilla.org/mozilla-central/rev/93588b7e9bc8 https://hg.mozilla.org/mozilla-central/rev/a9ec5b194001 https://hg.mozilla.org/mozilla-central/rev/3b89a0f2abe5 https://hg.mozilla.org/mozilla-central/rev/c06e65cb2745 https://hg.mozilla.org/mozilla-central/rev/c22b89e6f83a https://hg.mozilla.org/mozilla-central/rev/74487424ec36 https://hg.mozilla.org/mozilla-central/rev/00e35d8446bd https://hg.mozilla.org/mozilla-central/rev/99917d1409c2 https://hg.mozilla.org/mozilla-central/rev/01fb8307daaf https://hg.mozilla.org/mozilla-central/rev/287bdf729c79 https://hg.mozilla.org/mozilla-central/rev/44fc7d870680 https://hg.mozilla.org/mozilla-central/rev/b84f61d9893a https://hg.mozilla.org/mozilla-central/rev/24142d07f73e https://hg.mozilla.org/mozilla-central/rev/ebe097110cd2 https://hg.mozilla.org/mozilla-central/rev/65bf931f5806 https://hg.mozilla.org/mozilla-central/rev/43f5aa859a2f https://hg.mozilla.org/mozilla-central/rev/16a997fcfb06 https://hg.mozilla.org/mozilla-central/rev/485b3efd9ea8 https://hg.mozilla.org/mozilla-central/rev/3bd8839fdafa https://hg.mozilla.org/mozilla-central/rev/837f246ed352 https://hg.mozilla.org/mozilla-central/rev/489451bfba8c
Keywords: leave-open
Updated•6 years ago
|
Attachment #8985469 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 148•6 years ago
|
||
Comment on attachment 8985602 [details] [diff] [review] dom-xbl.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/3c49d41af70aa67f119b154c41187691c02d599c
Attachment #8985602 -
Attachment is obsolete: true
Assignee | ||
Comment 149•6 years ago
|
||
Comment on attachment 8985469 [details] [diff] [review] I18N.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/6e671db307db8527ed82b365f6050c41bc0e70e5
Attachment #8985469 -
Attachment is obsolete: true
Assignee | ||
Comment 150•6 years ago
|
||
Attachment #8982713 -
Attachment is obsolete: true
Attachment #8982713 -
Flags: review?(jwatt)
Attachment #8985771 -
Flags: review?(jwatt)
Assignee | ||
Comment 151•6 years ago
|
||
Attachment #8982697 -
Attachment is obsolete: true
Attachment #8982697 -
Flags: review?(aklotz)
Attachment #8985772 -
Flags: review?(aklotz)
Assignee | ||
Comment 152•6 years ago
|
||
Attachment #8982694 -
Attachment is obsolete: true
Attachment #8982694 -
Flags: review?(jorendorff)
Attachment #8985773 -
Flags: review?(jorendorff)
Assignee | ||
Comment 153•6 years ago
|
||
Attachment #8982682 -
Attachment is obsolete: true
Attachment #8982682 -
Flags: review?(jimb)
Attachment #8985774 -
Flags: review?(jimb)
Assignee | ||
Comment 154•6 years ago
|
||
Attachment #8982684 -
Attachment is obsolete: true
Attachment #8982684 -
Flags: review?(peterv)
Attachment #8985778 -
Flags: review?(peterv)
Assignee | ||
Comment 156•6 years ago
|
||
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)
Assignee | ||
Comment 157•6 years ago
|
||
Comment on attachment 8985640 [details] [diff] [review] web-painting.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/d15645bc44c7f4b7e35f39a290f67ca81b5e6f8d
Attachment #8985640 -
Attachment is obsolete: true
Comment 158•6 years ago
|
||
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 159•6 years ago
|
||
Comment on attachment 8985771 [details] [diff] [review] svg.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/ceec781c0e837ddfbb19a4979ffbb009dfc5edd4
Attachment #8985771 -
Attachment is obsolete: true
Assignee | ||
Comment 160•6 years ago
|
||
Attachment #8985578 -
Attachment is obsolete: true
Attachment #8985820 -
Flags: review?(jwalden+bmo)
Comment 161•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6e671db307db https://hg.mozilla.org/mozilla-central/rev/d15645bc44c7 https://hg.mozilla.org/mozilla-central/rev/ceec781c0e83
Assignee | ||
Comment 162•6 years ago
|
||
Attachment #8982683 -
Attachment is obsolete: true
Attachment #8985886 -
Flags: review?(bugs)
Assignee | ||
Comment 163•6 years 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 | ||
Comment 164•6 years 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 165•6 years ago
|
||
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 166•6 years ago
|
||
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 167•6 years ago
|
||
Comment on attachment 8985566 [details] [diff] [review] dom_media.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/d7079fb3a17993021fd7908af2b22e363d3143e3
Attachment #8985566 -
Attachment is obsolete: true
Assignee | ||
Comment 168•6 years ago
|
||
Comment on attachment 8985886 [details] [diff] [review] docshell.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/06a113a023aea085852f3ed19f97c8f412e038bf
Attachment #8985886 -
Attachment is obsolete: true
Assignee | ||
Comment 169•6 years ago
|
||
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)
Comment 170•6 years ago
|
||
> 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•6 years 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 172•6 years ago
|
||
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 173•6 years ago
|
||
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 174•6 years ago
|
||
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 175•6 years ago
|
||
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 176•6 years ago
|
||
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•6 years 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•6 years ago
|
Attachment #8985618 -
Attachment is obsolete: true
Assignee | ||
Comment 178•6 years ago
|
||
Attachment #8986012 -
Flags: review?(nfroyd)
Assignee | ||
Updated•6 years ago
|
Attachment #8985610 -
Attachment is obsolete: true
Assignee | ||
Comment 179•6 years ago
|
||
Attachment #8985617 -
Attachment is obsolete: true
Attachment #8986019 -
Flags: review?(nfroyd)
Reporter | ||
Comment 181•6 years 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•6 years ago
|
||
Attachment #8985589 -
Attachment is obsolete: true
Attachment #8986032 -
Flags: review?(nfroyd)
Assignee | ||
Comment 184•6 years ago
|
||
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 186•6 years ago
|
||
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+
Comment 187•6 years 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 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
Updated•6 years ago
|
Flags: needinfo?(botond)
Assignee | ||
Comment 188•6 years ago
|
||
Comment on attachment 8986043 [details] [diff] [review] psm-security.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/01b407d8a5afabe4fd3da80e3f586c26dc9bc901
Attachment #8986043 -
Attachment is obsolete: true
Assignee | ||
Comment 189•6 years ago
|
||
Comment on attachment 8985820 [details] [diff] [review] mfbt.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/e149fb41c712fb6e5236ca192d326451a41b0a82
Attachment #8985820 -
Attachment is obsolete: true
Assignee | ||
Comment 190•6 years ago
|
||
Attachment #8985772 -
Attachment is obsolete: true
Attachment #8985772 -
Flags: review?(aklotz)
Attachment #8986104 -
Flags: review?(aklotz)
Assignee | ||
Comment 191•6 years ago
|
||
Attachment #8985773 -
Attachment is obsolete: true
Attachment #8985773 -
Flags: review?(jwalden+bmo)
Attachment #8986127 -
Flags: review?(jorendorff)
Assignee | ||
Comment 192•6 years ago
|
||
Attachment #8985774 -
Attachment is obsolete: true
Attachment #8985774 -
Flags: review?(jimb)
Attachment #8986131 -
Flags: review?(jimb)
Assignee | ||
Comment 193•6 years ago
|
||
Attachment #8985778 -
Attachment is obsolete: true
Attachment #8985778 -
Flags: review?(peterv)
Attachment #8986169 -
Flags: review?(peterv)
Assignee | ||
Comment 194•6 years ago
|
||
Attachment #8985789 -
Attachment is obsolete: true
Attachment #8985789 -
Flags: review?(mstange)
Attachment #8986175 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 195•6 years ago
|
||
Attachment #8985483 -
Attachment is obsolete: true
Attachment #8985483 -
Flags: review?(jdemooij)
Attachment #8986193 -
Flags: review?(jdemooij)
Updated•6 years ago
|
Attachment #8986127 -
Flags: review?(jorendorff) → review?(jwalden+bmo)
Assignee | ||
Comment 196•6 years ago
|
||
Attachment #8985457 -
Attachment is obsolete: true
Attachment #8985457 -
Flags: review?(dbaron)
Attachment #8986200 -
Flags: review?(dbaron)
Updated•6 years ago
|
Attachment #8986032 -
Flags: review?(nfroyd) → review+
Comment 197•6 years ago
|
||
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 198•6 years ago
|
||
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 199•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e149fb41c712 https://hg.mozilla.org/mozilla-central/rev/908692fe7446
Comment 200•6 years ago
|
||
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 201•6 years ago
|
||
Comment on attachment 8986032 [details] [diff] [review] xpcom.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/00ae61d464066c27a4485e5f0dae02a7b78243ac
Attachment #8986032 -
Attachment is obsolete: true
Assignee | ||
Comment 202•6 years ago
|
||
Comment on attachment 8986012 [details] [diff] [review] widget_osx.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/85493d8b9dae872d8b594f145fc289fb49ab0a4e
Attachment #8986012 -
Attachment is obsolete: true
Assignee | ||
Comment 203•6 years 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•6 years ago
|
||
Attachment #8986019 -
Attachment is obsolete: true
Attachment #8986019 -
Flags: review?(jmathies)
Attachment #8986363 -
Flags: review?(jmathies)
Comment 205•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Attachment #8986449 -
Attachment is obsolete: true
Attachment #8986449 -
Flags: review?(jdemooij)
Attachment #8986467 -
Flags: review?(jdemooij)
Comment 208•6 years ago
|
||
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+
Comment 209•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #208) > Both searchfox and XDR claim it's unused.. Er, DXR :) Muscle memory..
Updated•6 years ago
|
Attachment #8986363 -
Flags: review?(jmathies) → review+
Assignee | ||
Comment 210•6 years ago
|
||
Comment on attachment 8986363 [details] [diff] [review] widget.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/a4f65ee3cba7d71084e92113ce1a3e2ffe428842
Attachment #8986363 -
Attachment is obsolete: true
Assignee | ||
Comment 211•6 years ago
|
||
Attachment #8985326 -
Attachment is obsolete: true
Attachment #8985326 -
Flags: review?(dbaron)
Attachment #8986528 -
Flags: review?(dbaron)
Assignee | ||
Updated•6 years ago
|
Attachment #8986528 -
Flags: review?(dbaron) → review?(nfroyd)
Comment 212•6 years 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•6 years ago
|
||
Attachment #8986131 -
Attachment is obsolete: true
Attachment #8986540 -
Flags: review?(jimb)
Comment 214•6 years 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+
Assignee | ||
Comment 215•6 years ago
|
||
Comment on attachment 8986540 [details] [diff] [review] devtools.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/72106854c71e6f19bf109ea967a0017662bd561b
Attachment #8986540 -
Attachment is obsolete: true
Assignee | ||
Comment 216•6 years ago
|
||
Comment on attachment 8986467 [details] [diff] [review] jit.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/aa34ca78ad4d508e4e1c7a8ba0f278a5f83658bc
Attachment #8986467 -
Attachment is obsolete: true
Comment 217•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/00ae61d46406 https://hg.mozilla.org/mozilla-central/rev/85493d8b9dae https://hg.mozilla.org/mozilla-central/rev/a4f65ee3cba7 https://hg.mozilla.org/mozilla-central/rev/72106854c71e https://hg.mozilla.org/mozilla-central/rev/aa34ca78ad4d
Updated•6 years ago
|
Attachment #8986104 -
Flags: review?(aklotz) → review+
Assignee | ||
Comment 218•6 years ago
|
||
Comment on attachment 8986104 [details] [diff] [review] libjar.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/a79434a2511b80f92554c15317e2d6271849ace9
Attachment #8986104 -
Attachment is obsolete: true
Assignee | ||
Comment 219•6 years ago
|
||
Attachment #8985601 -
Attachment is obsolete: true
Attachment #8985601 -
Flags: review?(peterv)
Attachment #8986683 -
Flags: review?(peterv)
Assignee | ||
Comment 220•6 years ago
|
||
Attachment #8985584 -
Attachment is obsolete: true
Attachment #8985584 -
Flags: review?(jvarga)
Attachment #8986686 -
Flags: review?(jvarga)
Updated•6 years ago
|
Attachment #8985579 -
Flags: review?(peterv) → review+
Comment 222•6 years ago
|
||
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+
Assignee | ||
Comment 223•6 years ago
|
||
Comment on attachment 8985579 [details] [diff] [review] xslt.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/668a0db86e1e596a946f7eb7bf90fa1341465efb
Attachment #8985579 -
Attachment is obsolete: true
Assignee | ||
Comment 224•6 years ago
|
||
Comment on attachment 8985585 [details] [diff] [review] xpconnect.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/32d9328cab02821a252de5a23f2f6de090d9e083
Attachment #8985585 -
Attachment is obsolete: true
Assignee | ||
Comment 225•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f18328bef6bb069e0b651c913281537b219b1dbd
Updated•6 years ago
|
Group: core-security → core-security-release
Comment 226•6 years ago
|
||
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+
Assignee | ||
Comment 227•6 years ago
|
||
Comment on attachment 8986528 [details] [diff] [review] XPCom-String.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/8fc73342ecd9e600923f210d19e458257626f007
Attachment #8986528 -
Attachment is obsolete: true
Comment 228•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/668a0db86e1e https://hg.mozilla.org/mozilla-central/rev/f18328bef6bb https://hg.mozilla.org/mozilla-central/rev/8fc73342ecd9
Comment 229•6 years ago
|
||
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•6 years ago
|
||
Attachment #8985773 -
Attachment is obsolete: true
Attachment #8987130 -
Flags: review?(jwalden+bmo)
Comment 231•6 years ago
|
||
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•6 years 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-
Assignee | ||
Comment 233•6 years ago
|
||
Comment on attachment 8987130 [details] [diff] [review] js.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/6a76baf321797f3831aeffaab4eb852e9383f213 Jeff thank you for all your help!
Attachment #8987130 -
Attachment is obsolete: true
Comment 235•6 years ago
|
||
Comment on attachment 8986686 [details] [diff] [review] xptoolkit.patch Review of attachment 8986686 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/xul/nsXULContentSink.h @@ +115,5 @@ > + Entry() > + : mChildren(8) > + , mState(eInProlog) > + , mNext(nullptr) > + {} I see only one place where this object is created: XULContentSinkImpl::ContextStack::Push I think the constructor should take node, state, next as arguments instead of initializing members here. For example mState(eInProlog) initial value doesn't make sense and there's also a tiny perf penalty since all 3 members are set to some values first and then immediately rewritten to real values. ::: layout/xul/nsImageBoxFrame.cpp @@ +906,4 @@ > NS_IMPL_ISUPPORTS(nsImageBoxListener, imgINotificationObserver) > > nsImageBoxListener::nsImageBoxListener() > + : mFrame(nullptr) This constructor should take aFrame as an argument, so nsImageBoxFrame::Init can create it without calling SetFrame afterwards. SetFrame should be renamed to ClearFrame, so nsImageBoxFrame::DestroyFrom can still use it. ::: layout/xul/nsMenuPopupFrame.h @@ +607,5 @@ > ReflowCallbackData() : > mPosted(false), > mAnchor(nullptr), > + mSizedToPopup(false), > + mIsOpenChanged(false) ok ::: layout/xul/nsSplitterFrame.cpp @@ +63,5 @@ > > explicit nsSplitterFrameInner(nsSplitterFrame* aSplitter) > + : mDidDrag(false) > + , mDragStart(0) > + , mCurrentPos(0) this attribute seems to be unused @@ +69,5 @@ > + , mChildInfosBeforeCount(0) > + , mChildInfosAfterCount(0) > + , mState(Open) > + , mSplitterPos(0) > + , mDragging(false) nsSplitterFrame::Init needs to be adjusted now since these attributes are now initialized to default values here. No need to do it again in nsSplitterFrame::Init ::: layout/xul/tree/nsTreeBodyFrame.h @@ +509,5 @@ > public: > + Slots() > + : mDropAllowed(false) > + , mIsDragging(false) > + , mDropRow(0) default value for this is -1 @@ +510,5 @@ > + Slots() > + : mDropAllowed(false) > + , mIsDragging(false) > + , mDropRow(0) > + , mDropOrient(0) default value for this is -1 ::: layout/xul/tree/nsTreeColumns.cpp @@ +23,4 @@ > nsTreeColumn::nsTreeColumn(nsTreeColumns* aColumns, dom::Element* aElement) > : mContent(aElement), > mColumns(aColumns), > + mIndex(0), ok
Attachment #8986686 -
Flags: review?(jvarga)
Comment 236•6 years ago
|
||
> regarding [1] m3 is of type 'uint8_t [8]' which we don't currently support. That is, the static analysis doesn't support? Feels like either we should initialize he whole uuid or not worry about it at all... Anyway, we can sort that out in bug 1469240.
Assignee | ||
Comment 237•6 years ago
|
||
Attachment #8986686 -
Attachment is obsolete: true
Attachment #8987612 -
Flags: review?(jvarga)
Comment 238•6 years ago
|
||
Comment on attachment 8987612 [details] [diff] [review] xptoolkit.patch Review of attachment 8987612 [details] [diff] [review]: ----------------------------------------------------------------- r=me if you address my additional comments ::: dom/xul/nsXULContentSink.h @@ +112,4 @@ > nsPrototypeArray mChildren; > State mState; > Entry* mNext; > + Entry(nsXULPrototypeNode* aNode, State aState, Entry* aNext) Fix the extra white space please ::: layout/xul/nsSplitterFrame.cpp @@ +273,3 @@ > > mInner->AddRef(); > mInner->mState = nsSplitterFrameInner::Open; I haven't tried to compile this, nor run any tests, but ... Isn't |mState| initialized to Open in the constructor ? I don't know why the original code had to set it twice to the same value. If you are unsure, I would rather keep the second |mInner->mState = nsSplitterFrameInner::Open;| which comes after nsBoxFrame::Init() call. You can also ask someone else who better understands this layout code.
Attachment #8987612 -
Flags: review?(jvarga) → review+
Assignee | ||
Comment 239•6 years ago
|
||
Comment on attachment 8987612 [details] [diff] [review] xptoolkit.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/71e4328d15abe687fdd7db20e4d0bcc4d821f407
Attachment #8987612 -
Attachment is obsolete: true
Assignee | ||
Comment 240•6 years ago
|
||
Hello,
I begin this by thanking you for the through review!
>>Please explain why the change to TextOverflow::TextOverflow actually
>>changes anything. I also don't understand why it's needed since mIStart
>>and mIEnd have their Init methods called later in the constructor. Or
>>is the issue that Marker::Init should initialize all the fields?
The problem here is that Marker::Init doesn't initialize all the fields. On the other hand we could add a ctor for Marker where we initialize everything, but doing so, Marker is no longer POD and I don't know if we want this.
Attachment #8986200 -
Attachment is obsolete: true
Attachment #8987825 -
Flags: review?(dbaron)
Comment 242•6 years ago
|
||
Discussing with Ehsan, we don't think we have to keep this bug closed. Dan, do you agree?
Flags: needinfo?(dveditz)
Assignee | ||
Updated•6 years ago
|
Attachment #8986175 -
Flags: review?(jmuizelaar) → review?(bugmail)
Comment on attachment 8986175 [details] [diff] [review] gfx.patch Review of attachment 8986175 [details] [diff] [review]: ----------------------------------------------------------------- Overall this looks fine to me. I have a couple of questions below that I'd like answers for before giving final r+. Also I'm going to flag Bas and jgilbert for feedback here just in case they have any objections - a lot of the code changes here are in Moz2D and WebGL code and they might have opinions on stuff. If they don't respond soon we can land this anyway. ::: dom/canvas/CanvasRenderingContext2D.cpp @@ +3999,5 @@ > : nsBidiPresUtils::BidiProcessor() > + , mCtx(nullptr) > + , mFontgrp(nullptr) > + , mAppUnitsPerDevPixel(0) > + , mOp(static_cast<CanvasRenderingContext2D::TextDrawOperation>(0)) So in other places (e.g. the change in WebGLTexture.cpp), the value used for initialization is a named member of the enum instead of a static_cast. i.e. the equivalent here would be CanvasRenderingContext2D::TextDrawOperation::FILL instead of static_cast<CanvasRenderingContext2D::TextDrawOperation>(0). Why is it that some places used the named member and other places use the static_cast? ::: gfx/2d/Matrix.h @@ +1878,4 @@ > Float a41, Float a42, Float a43, Float a44) > : Parent(a11, a12, a13, a14, a21, a22, a23, a24, > a31, a32, a33, a34, a41, a42, a43, a44) > + , mType{ MatrixType::Identity } Here the Analyze() call in the constructor body will always assign a value to mType, do we really need to use an initializer? Can the static analysis that detected this as an issue be enhanced to detect that the constructor body always ensures a value is assigned? Ditto for the other constructor just below this. ::: gfx/2d/RecordedEventImpl.h @@ +182,5 @@ > class RecordedFillRect : public RecordedDrawingEvent<RecordedFillRect> { > public: > RecordedFillRect(DrawTarget *aDT, const Rect &aRect, const Pattern &aPattern, const DrawOptions &aOptions) > + : RecordedDrawingEvent(FILLRECT, aDT), mRect(aRect), > + mPattern(), mOptions(aOptions) Same here, the StorePattern call in the constructor body populates mPattern, do we really need to add the mPattern() ? Ditto for most of the changes in this file. ::: gfx/layers/wr/WebRenderCommandBuilder.cpp @@ +206,4 @@ > struct Grouper > { > explicit Grouper(ClipManager& aClipManager) > + : mAppUnitsPerDevPixel(0) nit: trailing whitespace ::: gfx/src/nsRegion.h @@ +2181,5 @@ > explicit RectIterator(const nsRegion& aRegion) > : mRegion(aRegion) > , mCurrentBand(aRegion.mBands.begin()) > +#ifndef DEBUG > + , mCurrentStrip(nullptr) A little weird that this is in an ifndef DEBUG but presumably that's because StripArray is a different type in DEBUG and non-DEBUG.
Attachment #8986175 -
Flags: feedback?(jgilbert)
Attachment #8986175 -
Flags: feedback?(bas)
Assignee | ||
Comment 244•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #243) > Comment on attachment 8986175 [details] [diff] [review] > gfx.patch > > Review of attachment 8986175 [details] [diff] [review]: > ----------------------------------------------------------------- > > Overall this looks fine to me. I have a couple of questions below that I'd > like answers for before giving final r+. Also I'm going to flag Bas and > jgilbert for feedback here just in case they have any objections - a lot of > the code changes here are in Moz2D and WebGL code and they might have > opinions on stuff. If they don't respond soon we can land this anyway. > > ::: dom/canvas/CanvasRenderingContext2D.cpp > @@ +3999,5 @@ > > : nsBidiPresUtils::BidiProcessor() > > + , mCtx(nullptr) > > + , mFontgrp(nullptr) > > + , mAppUnitsPerDevPixel(0) > > + , mOp(static_cast<CanvasRenderingContext2D::TextDrawOperation>(0)) > > So in other places (e.g. the change in WebGLTexture.cpp), the value used for > initialization is a named member of the enum instead of a static_cast. i.e. > the equivalent here would be > CanvasRenderingContext2D::TextDrawOperation::FILL instead of > static_cast<CanvasRenderingContext2D::TextDrawOperation>(0). Why is it that > some places used the named member and other places use the static_cast? > This patch was mostly auto-generated and only parts of it were done manually, the parts with static_cast were auto-generated using an auto-fix option from our checker. > ::: gfx/2d/Matrix.h > @@ +1878,4 @@ > > Float a41, Float a42, Float a43, Float a44) > > : Parent(a11, a12, a13, a14, a21, a22, a23, a24, > > a31, a32, a33, a34, a41, a42, a43, a44) > > + , mType{ MatrixType::Identity } > > Here the Analyze() call in the constructor body will always assign a value > to mType, do we really need to use an initializer? Can the static analysis > that detected this as an issue be enhanced to detect that the constructor > body always ensures a value is assigned? > Yes but the assignment only take places of branches of ifstmt. The reason behind this was to do a separate analysis on the then branch and on the else branch and to compare the outcomes, if we find matches than those variables are marked as initialised. > Ditto for the other constructor just below this. > > ::: gfx/2d/RecordedEventImpl.h > @@ +182,5 @@ > > class RecordedFillRect : public RecordedDrawingEvent<RecordedFillRect> { > > public: > > RecordedFillRect(DrawTarget *aDT, const Rect &aRect, const Pattern &aPattern, const DrawOptions &aOptions) > > + : RecordedDrawingEvent(FILLRECT, aDT), mRect(aRect), > > + mPattern(), mOptions(aOptions) > partly same reason as above and the type of mPattern also has an union inside it. > Same here, the StorePattern call in the constructor body populates mPattern, > do we really need to add the mPattern() ? Ditto for most of the changes in > this file. > > ::: gfx/layers/wr/WebRenderCommandBuilder.cpp > @@ +206,4 @@ > > struct Grouper > > { > > explicit Grouper(ClipManager& aClipManager) > > + : mAppUnitsPerDevPixel(0) > > nit: trailing whitespace > > ::: gfx/src/nsRegion.h > @@ +2181,5 @@ > > explicit RectIterator(const nsRegion& aRegion) > > : mRegion(aRegion) > > , mCurrentBand(aRegion.mBands.begin()) > > +#ifndef DEBUG > > + , mCurrentStrip(nullptr) > > A little weird that this is in an ifndef DEBUG but presumably that's because > StripArray is a different type in DEBUG and non-DEBUG.
(In reply to Andi-Bogdan Postelnicu [:andi] from comment #244) > > Here the Analyze() call in the constructor body will always assign a value > > to mType, do we really need to use an initializer? Can the static analysis > > that detected this as an issue be enhanced to detect that the constructor > > body always ensures a value is assigned? > > > Yes but the assignment only take places of branches of ifstmt. The reason > behind this was to do a separate analysis on the then branch and on the else > branch and to compare the outcomes, if we find matches than those variables > are marked as initialised. Sorry, I don't follow this. Do you mean that all branches of the Analyze() function need to assign the same value into mType? Why is that? Looking at the Analyze function [1] it's pretty obvious that mType is assigned some value in all branches, and that should be sufficient for initialization purposes. [1] https://searchfox.org/mozilla-central/rev/6ef785903fee6c0b16a1eab79d722373d940fd78/gfx/2d/Matrix.h#2265
Assignee | ||
Comment 246•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #245) > (In reply to Andi-Bogdan Postelnicu [:andi] from comment #244) > > > Here the Analyze() call in the constructor body will always assign a value > > > to mType, do we really need to use an initializer? Can the static analysis > > > that detected this as an issue be enhanced to detect that the constructor > > > body always ensures a value is assigned? > > > > > Yes but the assignment only take places of branches of ifstmt. The reason > > behind this was to do a separate analysis on the then branch and on the else > > branch and to compare the outcomes, if we find matches than those variables > > are marked as initialised. > > Sorry, I don't follow this. Do you mean that all branches of the Analyze() > function need to assign the same value into mType? Why is that? Looking at > the Analyze function [1] it's pretty obvious that mType is assigned some > value in all branches, and that should be sufficient for initialization > purposes. > > [1] > https://searchfox.org/mozilla-central/rev/ > 6ef785903fee6c0b16a1eab79d722373d940fd78/gfx/2d/Matrix.h#2265 I've seen the final assignment. And yes we don't need to initialise it at start. This wa a bug in the analysis that has been solved recently. What I wanted to say is that when we have this trivial example: >>class Test { >>int b; >>Test() { >> if (something) >> b = 10; >>} >>}; In this case b will be triggered as uninitialised because for the ifstmt we have two branches, the then branch where it gets assigned and the else branch where it doesn't.
Ok. So then will the patch be updated to take this into account?
Assignee | ||
Comment 248•6 years ago
|
||
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #247) > Ok. So then will the patch be updated to take this into account? Of course will post a new version of the patch with this fix.
Assignee | ||
Comment 249•6 years ago
|
||
Attachment #8986175 -
Attachment is obsolete: true
Attachment #8986175 -
Flags: review?(bugmail)
Attachment #8986175 -
Flags: feedback?(jgilbert)
Attachment #8986175 -
Flags: feedback?(bas)
Attachment #8990085 -
Flags: review?(bugmail)
Attachment #8990085 -
Flags: feedback?(jgilbert)
Attachment #8990085 -
Flags: feedback?(bas)
Comment 250•6 years ago
|
||
Comment on attachment 8990085 [details] [diff] [review] gfx.patch Review of attachment 8990085 [details] [diff] [review]: ----------------------------------------------------------------- r+: dom/*, gfx/gl/* ::: dom/canvas/WebGLContext.cpp @@ +120,4 @@ > , mMaxPerfWarnings(gfxPrefs::WebGLMaxPerfWarnings()) > , mNumPerfWarnings(0) > , mMaxAcceptableFBStatusInvals(gfxPrefs::WebGLMaxAcceptableFBStatusInvals()) > + , mDepthTestEnabled(0) It'd be nicer to do initialization at declaration as much as possible.
Attachment #8990085 -
Flags: feedback?(jgilbert) → feedback+
Comment 251•6 years ago
|
||
(In reply to Sylvestre Ledru [:sylvestre] from comment #242) > Discussing with Ehsan, we don't think we have to keep this bug closed. Dan, do you agree? Yeah, we can unhide this. Probably Bug 525063 also.
Flags: needinfo?(dveditz)
Assignee | ||
Comment 253•6 years ago
|
||
I've split gfx patch into smaller pieces, since the dom/* and gfx/gl* has been r+ according to: https://bugzilla.mozilla.org/show_bug.cgi?id=1453795#c250 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d8492f777963593f21f86d955673293675c1be1
Comment 254•6 years ago
|
||
Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fe0a7d4b6daf GFX - Initialize member fields in classes/ structures. r=jgilbert
Assignee | ||
Comment 255•6 years ago
|
||
(In reply to Pulsebot from comment #254) > Pushed by bpostelnicu@mozilla.com: > https://hg.mozilla.org/integration/mozilla-inbound/rev/fe0a7d4b6daf > GFX - Initialize member fields in classes/ structures. r=jgilbert This is part of the patch that has bee r=jgilbert, dom/* and gfx/gl*
Assignee | ||
Comment 256•6 years ago
|
||
Attachment #8990085 -
Attachment is obsolete: true
Attachment #8990085 -
Flags: review?(bugmail)
Attachment #8990085 -
Flags: feedback?(bas)
Attachment #8990288 -
Flags: review?(bugmail)
Attachment #8990288 -
Flags: feedback?(bas)
Assignee | ||
Updated•6 years ago
|
Attachment #8986683 -
Flags: review?(peterv) → review?(bugs)
Updated•6 years ago
|
Attachment #8986683 -
Flags: review?(bugs) → review+
Comment 257•6 years ago
|
||
Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a985c7fbfd01 DOM/XML - Initialize member fields in classes/ structures. r=smaug
Assignee | ||
Comment 258•6 years ago
|
||
Comment on attachment 8986683 [details] [diff] [review] dom-xml.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/a985c7fbfd01
Attachment #8986683 -
Attachment is obsolete: true
Comment on attachment 8990288 [details] [diff] [review] gfx.patch Review of attachment 8990288 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/Logging.h @@ +271,1 @@ > , mLogIt(false) The Init function called in the constructor body unconditionally initializes mReason (as well as mOptions and mLogIt), so you shouldn't need to initialize mReason here. I thought you were going to regenerate the patch using the new analysis that corrected this class of problems?
Attachment #8990288 -
Flags: review?(bugmail)
Comment 260•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fe0a7d4b6daf https://hg.mozilla.org/mozilla-central/rev/a985c7fbfd01
Comment 261•6 years ago
|
||
Comment on attachment 8986169 [details] [diff] [review] dom.patch Review of attachment 8986169 [details] [diff] [review]: ----------------------------------------------------------------- Please get rbarnes or ttaubert to review dom/crypto/WebCryptoTask.cpp. ::: dom/base/DOMException.cpp @@ +176,4 @@ > , mResult(aResult) > , mName(aName) > , mData(aData) > + , mInitialized(false) Looks unused, remove it instead. ::: dom/base/EventSource.cpp @@ +1928,4 @@ > EventSource::EventSource(nsPIDOMWindowInner* aOwnerWindow, > bool aWithCredentials) > : DOMEventTargetHelper(aOwnerWindow) > + , mReadyState(0) The constructor already initializes this by calling SetReadyState(CONNECTING), so remove this. ::: dom/base/TextInputProcessor.cpp @@ +39,4 @@ > public: > explicit TextInputProcessorNotification(const char* aType) > : mType(aType) > + , mTextChangeData() You could instead add a constructor to the union, but I guess this is ok. ::: dom/base/nsContentList.h @@ +591,5 @@ > const nsAString& aString) : > nsContentList(aRootNode, aFunc, aDestroyFunc, nullptr), > +#ifdef DEBUG > + mType(static_cast<ContentListType>(0)), > +#endif This seems wrong. I think you should add a |mozilla::DebugOnly<ContentListType>| argument to the nsCacheableFuncStringContentList constructor and pass in the right value from the derived classes' constructors. ::: dom/base/nsContentPermissionHelper.cpp @@ +579,4 @@ > } > > nsContentPermissionRequestProxy::nsContentPermissionRequestProxy() > + : mParent(nullptr) Make the constructor take the right value, instead of passing it in through init. There's only one caller I think. ::: dom/base/nsFrameLoader.cpp @@ +182,4 @@ > , mClipSubdocument(true) > , mClampScrollPosition(true) > , mObservingOwnerContent(false) > + , mFreshProcess(false) Looks unused, remove it instead. ::: dom/base/nsJSEnvironment.cpp @@ +589,5 @@ > nsIScriptGlobalObject* aGlobalObject) > : mWindowProxy(nullptr) > , mGCOnDestruction(aGCOnDestruction) > + , mModalStateTime(0) > + , mModalStateDepth(0) Both look unused, remove them instead. ::: dom/base/nsPlainTextSerializer.cpp @@ +81,5 @@ > } > > nsPlainTextSerializer::nsPlainTextSerializer() > + : mFlags(0) > + , mFloatingLines(0) nsPlainTextSerializer::Init sets it to -1, so this should probably too. ::: dom/base/nsQueryContentEventResult.cpp @@ +58,2 @@ > , mSucceeded(false) > + , mReversed(false) I think it makes more sense to merge the code from nsQueryContentEventResult::SetEventResult into this constructor. ::: dom/base/nsXHTMLContentSerializer.cpp @@ +48,4 @@ > > nsXHTMLContentSerializer::nsXHTMLContentSerializer() > : mIsHTMLSerializer(false) > + , mDoHeader(false) Looks unused, remove it instead. ::: dom/bindings/BindingDeclarations.h @@ +425,4 @@ > { > public: > NonNull() > + : ptr(nullptr) This has an explicit comment: "ptr is left uninitialized for optimization purposes.", so remove this. ::: dom/bindings/ErrorResult.h @@ +129,4 @@ > , mUnionState(HasNothing) > #endif > { > + mExtra.mMessage = nullptr; Why is this needed? mExtra has a constructor. ::: dom/bindings/FakeString.h @@ +19,5 @@ > // for small strings and a nsStringBuffer for longer strings. > struct FakeString { > FakeString() : > + mData(nullptr), > + mLength(0), Both of these have comments about "left uninitialized for optimization purposes", so undo these changes. ::: dom/bindings/TypedArray.h @@ +221,4 @@ > public: > ArrayBufferView_base() > : Base() > + , mType(static_cast<js::Scalar::Type>(0)) Maybe use js::Scalar::MaxTypedArrayViewType? ::: dom/cache/SavedTypes.h @@ +27,5 @@ > + , mCacheId(0) > + { > + mBodyId.m0 = 0; > + mBodyId.m1 = 0; > + mBodyId.m2 = 0; If you're not initializing m3 then you shouldn't do the others either. @@ +44,5 @@ > + , mCacheId(0) > + { > + mBodyId.m0 = 0; > + mBodyId.m1 = 0; > + mBodyId.m2 = 0; Same here. ::: dom/fetch/InternalRequest.cpp @@ +124,4 @@ > const nsAString& aIntegrity) > : mMethod(aMethod) > , mHeaders(aHeaders) > + , mBodyLength(0) Why not InternalResponse::UNKNOWN_BODY_SIZE? @@ +185,4 @@ > , mURLList(aIPCRequest.urls()) > , mHeaders(new InternalHeaders(aIPCRequest.headers(), > aIPCRequest.headersGuard())) > + , mBodyLength(0) Same here. ::: dom/gamepad/GamepadHapticActuator.h @@ +23,4 @@ > GamepadHapticActuator(nsISupports* aParent, uint32_t aGamepadId, > uint32_t aIndex); > explicit GamepadHapticActuator(nsISupports* aParent) > + : mParent(aParent) This constructor looks unused, just remove it. ::: dom/ipc/FilePickerParent.h @@ +26,4 @@ > const int16_t& aMode) > : mTitle(aTitle) > , mMode(aMode) > + , mResult(0) Use nsIFilePicker::returnOK. ::: dom/ipc/TabChild.cpp @@ +405,4 @@ > , mActiveSuppressDisplayport(0) > , mLayersId{0} > , mBeforeUnloadListeners(0) > + , mLastBackgroundColor(0) Looks unused, remove it instead. ::: dom/messagechannel/MessagePort.cpp @@ +199,4 @@ > > MessagePort::MessagePort(nsIGlobalObject* aGlobal) > : DOMEventTargetHelper(aGlobal) > + , mState(static_cast<State>(0)) You should pass the right value from the two callers through the constructor instead of through Initialize. ::: dom/payments/PaymentActionResponse.cpp @@ +181,4 @@ > nsIPaymentShowActionResponse) > > PaymentShowActionResponse::PaymentShowActionResponse() > + : mAcceptStatus(0) nsIPaymentActionResponse::PAYMENT_REJECTED @@ +293,4 @@ > nsIPaymentAbortActionResponse) > > PaymentAbortActionResponse::PaymentAbortActionResponse() > + : mAbortStatus(0) nsIPaymentActionResponse::ABORT_FAILED @@ +330,4 @@ > nsIPaymentCompleteActionResponse) > > PaymentCompleteActionResponse::PaymentCompleteActionResponse() > + : mCompleteStatus(0) nsIPaymentActionResponse::COMPLETE_FAILED ::: dom/performance/Performance.cpp @@ +75,4 @@ > Performance::Performance() > : mResourceTimingBufferSize(kDefaultResourceTimingBufferSize) > , mPendingNotificationObserversTask(false) > + , mSystemPrincipal(false) Pass the right value through an argument for the constructor. @@ +84,4 @@ > : DOMEventTargetHelper(aWindow) > , mResourceTimingBufferSize(kDefaultResourceTimingBufferSize) > , mPendingNotificationObserversTask(false) > + , mSystemPrincipal(false) Pass the right value through an argument for the constructor. ::: dom/worklet/Worklet.cpp @@ +434,5 @@ > > WorkletLoadInfo::WorkletLoadInfo() > + : mOuterWindowID(0) > + , mInnerWindowID(0) > + , mDumpEnabled(false) Make Worklet::Worklet pass in aWindow and aPrincipal, set the right values here straight away and remove most of the code in Worklet::Worklet. And set mDumpEnabled to DOMPrefs::DumpEnabled().
Attachment #8986169 -
Flags: review?(peterv) → review-
Assignee | ||
Comment 262•6 years ago
|
||
Thank you very much for the detailed review, It helped a lot! I have a comment, regarding the way how the static analysis understands assignment on a particular case:
>>if (expr) {
>> mValue = 1;
>>}
In this case |mValue| will still be considered uninitialized because it is assigned a value only on one branch of the IfStmt, in this case the |then| branch, but on the else branch, that is missing, it isn't assigned, so the analysis collates the two results and see what it doesn't match.
I felt the need of saying this because I've encountered this issue in one particular place in the patch. If you still think that we shouldn't have this we can ignore the variable from analysis.
Attachment #8986169 -
Attachment is obsolete: true
Attachment #8991231 -
Flags: review?(peterv)
Assignee | ||
Updated•6 years ago
|
Attachment #8991231 -
Attachment description: gfx.patch → dom.patch
Attachment #8991231 -
Attachment filename: gfx.patch → dom.patch
Assignee | ||
Comment 263•6 years ago
|
||
Attachment #8991234 -
Flags: review?(bugmail)
Assignee | ||
Comment 264•6 years ago
|
||
Comment on attachment 8991231 [details] [diff] [review] dom.patch Also we don't regress with this patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=11b5cf7521969ef8ba2229facbfd3b4b4bc6f25c
Comment on attachment 8987825 [details] [diff] [review] layout.patch My previous review was in comment 232. In nsTextFrame.cpp where you're removing mSelectionStatus, please remove the line rather than replacing it with a blank line. In TextOverflow::TextOverflow, please replace your change by a change that initializes all the fields in Marker::Init: mIntrinsicISize to 0 and mHasOverflow and mActive to false. (I don't see why a ctor would be needed if fixing Marker::Init would fix it.) (This is in reply to comment 240.) r=dbaron with those changes
Attachment #8987825 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 266•6 years ago
|
||
For the layout patch let's make sure we don't regress: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5115c9f543ae09b497e978e442636163d292c9f
Updated•6 years ago
|
Summary: Review of fixes by 525063 per module → uninitialized class members: Review of fixes by 525063 per module
Comment 267•6 years ago
|
||
Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/56062a63fa8c Layout - Initialize member fields in classes/ structures. r=dbaron
Assignee | ||
Updated•6 years ago
|
Attachment #8987825 -
Attachment is obsolete: true
Comment 268•6 years ago
|
||
Comment on attachment 8991231 [details] [diff] [review] dom.patch Review of attachment 8991231 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsContentList.h @@ +588,4 @@ > nsContentListMatchFunc aFunc, > nsContentListDestroyFunc aDestroyFunc, > nsFuncStringContentListDataAllocator aDataAllocator, > + mozilla::DebugOnly<ContentListType> aType, I'd make this the last argument. ::: dom/bindings/ErrorResult.h @@ +129,4 @@ > , mUnionState(HasNothing) > #endif > { > + mExtra.mMessage = nullptr; I still don't think this is necessary, given the constructor in the union (which sets mMessage to nullptr already). ::: dom/performance/Performance.h @@ +118,4 @@ > void InsertResourceEntry(PerformanceEntry* aEntry); > > protected: > + Performance(bool aSystemPrincipal); Shouldn't this be explicit? @@ +118,5 @@ > void InsertResourceEntry(PerformanceEntry* aEntry); > > protected: > + Performance(bool aSystemPrincipal); > + explicit Performance(nsPIDOMWindowInner* aWindow, bool aSystemPrincipal); I think you can remove explicit.
Attachment #8991231 -
Flags: review?(peterv) → review+
Assignee | ||
Comment 269•6 years ago
|
||
let's make sure we don't regress with the dom patch: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5138cbef30cb0313b4dd418e7b329b1992995e80
Comment 270•6 years ago
|
||
Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/13efb4f58614 DOM - Initialize member fields in classes/ structures. r=peterv
Assignee | ||
Updated•6 years ago
|
Attachment #8991231 -
Attachment is obsolete: true
Comment 271•6 years ago
|
||
The initialization of mExtra.mMessage in ErrorResult is not needed or desired: that member is not examined until an error is thrown on an ErrorResult and doing that initializes it as needed. ErrorResult is meant to be as zero-cost as possible unless an exception is thrown; we shouldn't be wasting time initializing things that don't need to be initialized there. That means the Extra constructor should not initialize mMessage either.
Flags: needinfo?(bpostelnicu)
Comment 272•6 years ago
|
||
Comment on attachment 8991234 [details] [diff] [review] dom-crypto.patch Review of attachment 8991234 [details] [diff] [review]: ----------------------------------------------------------------- I don't think I'm the appropriate reviewer for this, but with :ttaubert not around anymore, I'm not sure who is. So redirecting to :mt as co-owner of security. (I acknowledge I reviewed the most recent patch to the file in question, but that was about DOM Worker code, not the actual crypto subject domain implementation.) Briefly looking at it myself, everything seems sane enough, but mMechansism of 0 is CKM_RSA_PKCS_KEY_PAIR_GEN which is a legit value, so that's unfortunate, but there is no explicit illegal/uninitialized value as a better alternative that I see in https://searchfox.org/mozilla-central/source/security/nss/lib/util/pkcs11t.h#565. When used in AESTask it is fine because DoCrypto() doesn't recognize it as a legal possibility... but it's also used in GenerateAsymmetricKeyTask where it is an acceptable value. And the CKM_ constants are defined in security/nss/lib/util/pkcs11t.h which I definitely can't make suggestions about. So, :mt?
Attachment #8991234 -
Flags: review?(bugmail) → review?(martin.thomson)
Comment 273•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/56062a63fa8c https://hg.mozilla.org/mozilla-central/rev/13efb4f58614
Comment 274•6 years ago
|
||
Comment on attachment 8990288 [details] [diff] [review] gfx.patch Review of attachment 8990288 [details] [diff] [review]: ----------------------------------------------------------------- In general, is there a reason we're not using C++ member initializers here? (i.e. uint8_t* mData = nullptr; instead of adding it to every initializer list)
Assignee | ||
Comment 275•6 years ago
|
||
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #271) > The initialization of mExtra.mMessage in ErrorResult is not needed or > desired: that member is not examined until an error is thrown on an > ErrorResult and doing that initializes it as needed. ErrorResult is meant > to be as zero-cost as possible unless an exception is thrown; we shouldn't > be wasting time initializing things that don't need to be initialized there. > That means the Extra constructor should not initialize mMessage either. I've created this followup bug 1475515. Thanks again for noting this!
Flags: needinfo?(bpostelnicu)
Assignee | ||
Comment 276•6 years ago
|
||
(In reply to Bas Schouten (:bas.schouten) from comment #274) > Comment on attachment 8990288 [details] [diff] [review] > gfx.patch > > Review of attachment 8990288 [details] [diff] [review]: > ----------------------------------------------------------------- > > In general, is there a reason we're not using C++ member initializers here? > (i.e. uint8_t* mData = nullptr; instead of adding it to every initializer > list) Most of the fixes have been done by implementing a fix-up for clang-tidy, so they were done automatically.
Comment 277•6 years ago
|
||
Comment on attachment 8991234 [details] [diff] [review] dom-crypto.patch Review of attachment 8991234 [details] [diff] [review]: ----------------------------------------------------------------- As bz said separately, this would be better as inline initialization, but this seems fine. ::: dom/crypto/WebCryptoTask.cpp @@ +485,4 @@ > public: > AesTask(JSContext* aCx, const ObjectOrString& aAlgorithm, > CryptoKey& aKey, bool aEncrypt) > + : mMechanism(0) Try CKM_INVALID_MECHANISM
Attachment #8991234 -
Flags: review?(martin.thomson) → review+
Comment 278•6 years ago
|
||
Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ddb66025724b DOM/Crypto - Initialize member fields in classes/ structures. r=mt
Assignee | ||
Comment 279•6 years ago
|
||
Comment on attachment 8991234 [details] [diff] [review] dom-crypto.patch https://hg.mozilla.org/integration/mozilla-inbound/rev/ddb66025724b2ebae3b0d8493c0093e6fcfce1af
Attachment #8991234 -
Attachment is obsolete: true
Comment 280•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ddb66025724b
Assignee | ||
Comment 281•6 years ago
|
||
Attachment #8990288 -
Attachment is obsolete: true
Attachment #8990288 -
Flags: feedback?(bas)
Attachment #8997781 -
Flags: review?(nical.bugzilla)
Comment 282•6 years ago
|
||
Comment on attachment 8997781 [details] [diff] [review] gfx.patch Review of attachment 8997781 [details] [diff] [review]: ----------------------------------------------------------------- Looks nice overall. I'm not found of the static_cast<EnumType>(0) pattern so I'd rather fix those before landing. ::: gfx/2d/SVGTurbulenceRenderer-inl.h @@ +102,5 @@ > : mBaseFrequency(aBaseFrequency) > , mNumOctaves(aNumOctaves) > + , mStitchInfo() > + , mStitchable(false) > + , mType(static_cast<TurbulenceType>(0)) Please use the constant value here (TURBULENCE_TYPE_TURBULENCE) instead of casting zero. ::: gfx/layers/ipc/ShadowLayers.cpp @@ +65,4 @@ > public: > Transaction() > : mTargetRotation(ROTATION_0) > + , mTargetOrientation(0) Please use eScreenOrientation_None from https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/dom/base/ScreenOrientation.h#25 instead of 0. ::: gfx/layers/mlgpu/RenderPassMLGPU.cpp @@ +569,4 @@ > > SingleTexturePass::SingleTexturePass(FrameBuilder* aBuilder, const ItemInfo& aItem) > : TexturedRenderPass(aBuilder, aItem), > + mSamplerMode(static_cast<SamplerMode>(0)), Please use SamplerMode::LinearClamp from https://searchfox.org/mozilla-central/source/gfx/layers/mlgpu/MLGDeviceTypes.h#49 @@ +678,5 @@ > > ComponentAlphaPass::ComponentAlphaPass(FrameBuilder* aBuilder, const ItemInfo& aItem) > : TexturedRenderPass(aBuilder, aItem), > + mOpacity(1.0f), > + mSamplerMode(static_cast<SamplerMode>(0)) Please use SamplerMode::LinearClamp from https://searchfox.org/mozilla-central/source/gfx/layers/mlgpu/MLGDeviceTypes.h#49 @@ +744,4 @@ > > VideoRenderPass::VideoRenderPass(FrameBuilder* aBuilder, const ItemInfo& aItem) > : TexturedRenderPass(aBuilder, aItem), > + mSamplerMode(static_cast<SamplerMode>(0)), Please use SamplerMode::LinearClamp (ad lib) ::: gfx/layers/mlgpu/TexturedLayerMLGPU.cpp @@ +171,5 @@ > > TempImageLayerMLGPU::TempImageLayerMLGPU(LayerManagerMLGPU* aManager) > : ImageLayer(aManager, static_cast<HostLayer*>(this)), > + TexturedLayerMLGPU(aManager), > + mFilter(static_cast<gfx::SamplingFilter>(0)), Please use gfx::SamplingFilter::GOOD from https://searchfox.org/mozilla-central/rev/3fdc491e118c5cdfbaf6e2d52f3466d2b27ad1de/gfx/2d/Types.h#249 ::: gfx/layers/opengl/TextureHostOGL.h @@ +83,4 @@ > { > public: > TextureSourceOGL() > + : mCachedSamplingFilter(static_cast<gfx::SamplingFilter>(0)) Same here about the sampling filter constant. ::: gfx/layers/protobuf/LayerScopePacket.pb.cc @@ +412,4 @@ > #endif // !defined(_MSC_VER) || _MSC_VER >= 1900 > > FramePacket::FramePacket() > + : ::google::protobuf::MessageLite(), _internal_metadata_(NULL) , scale_(0.0) { I am not sure whether we want to run this on the vendored protobuf code (in case we want to merge upstream changes in the future). Perhaps it would be best to separate this into another patch so that we can land the rest and decide what to do with this.
Assignee | ||
Comment 283•6 years ago
|
||
Updated patch that addresses your comments.
Attachment #8997781 -
Attachment is obsolete: true
Attachment #8997781 -
Flags: review?(nical.bugzilla)
Attachment #8997937 -
Flags: review?(nical.bugzilla)
Comment 284•6 years ago
|
||
Comment on attachment 8997937 [details] [diff] [review] gfx.patch Review of attachment 8997937 [details] [diff] [review]: ----------------------------------------------------------------- Oops, I missed one. With this static_cast removed r=me. ::: gfx/layers/apz/src/KeyboardMap.cpp @@ +15,5 @@ > + : mKeyCode(0) > + , mCharCode(0) > + , mModifiers(0) > + , mModifiersMask(0) > + , mEventType(static_cast<KeyboardInput::KeyboardEventType>(0)) Another static_cast I hadn't spotted the first time.
Assignee | ||
Comment 285•6 years ago
|
||
Attachment #8980418 -
Attachment is obsolete: true
Attachment #8997944 -
Flags: review?(nical.bugzilla)
Assignee | ||
Updated•6 years ago
|
Attachment #8997937 -
Attachment is obsolete: true
Attachment #8997937 -
Flags: review?(nical.bugzilla)
Comment 286•6 years ago
|
||
Comment on attachment 8997944 [details] [diff] [review] gfx.patch Review of attachment 8997944 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8997944 -
Flags: review?(nical.bugzilla) → review+
Assignee | ||
Comment 287•6 years ago
|
||
To be sure we don't regress: 1. Build - https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d1de533028226e7da8d0c99030ece8c6a1e43e8 2. Talos - https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=2d1de533028226e7da8d0c99030ece8c6a1e43e8
Comment 288•6 years ago
|
||
Pushed by bpostelnicu@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba5c08f10923 GFX - Initialize member fields in classes/ structures. r=nical
Assignee | ||
Updated•6 years ago
|
Keywords: leave-open
Comment 289•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ba5c08f10923
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Updated•6 years ago
|
Whiteboard: [adv-main63-]
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•