1361749 - Initialize a bunch of variables that are left uninitialized on construction after bug 1316556

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Emilio Cobos Álvarez [:emilio]]

I ran the analysis in bug 525063 (with a custom patch posted there, and another one to skip stuff out of layout, and other offenders like nsDisplayList.h and nsStyleStruct.h, which do this on purpose).

It caught a few things. Some were already caught by coverity, others weren't I think.

In any case, I've been patching as I read diagnostics, so I'm going to start posting patches soon.

Flagging ss just in case.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0001-Bug-1361749-Remove-unused-empty-constructor-of-Scrol.patch

Comment 2

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0002-Bug-1361749-Initialize-all-the-fields-in-nsFrameCont.patch

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0003-Initialize-nsGridContainerFrame-mDidPushItemsBitMayL.patch

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0004-Initialize-nsPlaceholderFrame-mOutOfFlowFrame.patch

Comment 5

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0005-Initialize-on-construction-HTMLColorRule.-r-mats.patch

This one is not exactly a regression, but feels nicer.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0006-Initialize-nsSVGPatternFrame-mSource.patch

Comment 7

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0007-Initialize-nsBoxFrame-mFlex-mAscent-.-r-mats.patch

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0008-Initialize-nsMenuPopupFrame-mXPos-mYPos-.-r-mats.patch

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0009-Initialize-nsScrollbarFrame-mIncrement-mSmoothScroll.patch

Comment 10

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0010-Initialize-nsSliderFrame-mDragStart-mThumbStart.patch

Comment 11

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0011-Initialize-nsInlineFrame-mBaseline.patch

Comment 12

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0012-Initialize-BRFrame-mAscent.patch

Comment 13

[Emilio Cobos Álvarez [:emilio]]

Attachment: 0013-Initialize-nsTextboxFrame-mAscent.patch

Comment 14

[Emilio Cobos Álvarez [:emilio]]

I think those are all. Others are more tricky, like the nsDisplayList.h ones, which use an empty constructor so protected classes can initialize their fields (they should use delegating constructors instead I think). But those aren't nothing new nor affected by bug 1316556, so...

Comment 15

[Mats Palmgren (inactive)]

FYI, I filed individual bugs for most of these I think, see recent bugs filed by me tagged 'coverity'.
I'll try to indicate which one matches each patch so you can resolve those as fixed, and/or perhaps
change the commit message to indicate that bug instead?  I think most of these are not security
sensitive, except perhaps the nsSVGPatternFrame::mSource one, but I suspect it's sec-low at worst.

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 8864204 [details] [diff] [review]
0001-Bug-1361749-Remove-unused-empty-constructor-of-Scrol.patch

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

r=mats, nit:
I can't really tell what the commit message will end up being from that
mail header but if the "1 file changed, 1 deletion" stuff is included,
could you get rid of that bit if possible?  I don't think we usually
include commit stats like that and it seems mostly spam to me.
(same nit applies to all patches I assume)

Comment 17

[Mats Palmgren (inactive)]

Comment on attachment 8864205 [details] [diff] [review]
0002-Bug-1361749-Initialize-all-the-fields-in-nsFrameCont.patch

I'm not a fan of individual member inits, but fine if it's allowed by the coding style guide I guess.

Comment 18

[Mats Palmgren (inactive)]

Comment on attachment 8864207 [details] [diff] [review]
0003-Initialize-nsGridContainerFrame-mDidPushItemsBitMayL.patch

FYI, this one is bug 1361656.

Comment 19

[Mats Palmgren (inactive)]

Comment on attachment 8864208 [details] [diff] [review]
0004-Initialize-nsPlaceholderFrame-mOutOfFlowFrame.patch

FYI, this one is bug 1361612.

Comment 20

[Mats Palmgren (inactive)]

Comment on attachment 8864213 [details] [diff] [review]
0006-Initialize-nsSVGPatternFrame-mSource.patch

FYI, this one is bug 1361596.

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 8864214 [details] [diff] [review]
0007-Initialize-nsBoxFrame-mFlex-mAscent-.-r-mats.patch

FYI, this one is bug 1361517.

Comment 22

[Mats Palmgren (inactive)]

Comment on attachment 8864218 [details] [diff] [review]
0011-Initialize-nsInlineFrame-mBaseline.patch

FYI, this one is bug 1361513.  Could initialize it to NS_INTRINSIC_WIDTH_UNKNOWN instead as I suggested in that bug?  That way we will likely catch any misuse of GetLogicalBaseline() before mBaseline have been properly calculated (during Reflow).

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 8864219 [details] [diff] [review]
0012-Initialize-BRFrame-mAscent.patch

FYI, this one is bug 1361659.  Ditto about using NS_INTRINSIC_WIDTH_UNKNOWN instead.

Comment 24

[Mats Palmgren (inactive)]

See bug 1361509 comment 1 and bug 1361612 comment 1 for the analysis of those two
issues.  The rest seems unlikely to cause security issues.

Comment 25

[Emilio Cobos Álvarez [:emilio]]

For the record, the stats of the commit are an artifact of how git exports commits, but don't end up in the final commit. Will land patches with bugs filled with the relevant bug number, and use NS_INTRINSIC_WIDTH_UNKNOWN as suggested.

Thanks for the reviews mats! :)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab36c381d34f
https://hg.mozilla.org/mozilla-central/rev/35c7efc47b1f
https://hg.mozilla.org/mozilla-central/rev/d7a57f76f0ef
https://hg.mozilla.org/mozilla-central/rev/1a826ca2c7c0
https://hg.mozilla.org/mozilla-central/rev/d7f069fa65d7
https://hg.mozilla.org/mozilla-central/rev/14990dfd94dd
https://hg.mozilla.org/mozilla-central/rev/6aa9b5172e88