458473 - ReflowInput::InitConstraints potential use of uninitialized value

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Zack Weinberg (:zwol)]

gcc sez:

layout/generic/nsHTMLReflowState.cpp: In member function 
    ‘void nsHTMLReflowState::InitConstraints(...)’:
layout/generic/nsHTMLReflowState.cpp:1647: warning: 
    ‘fType’ may be used uninitialized in this function

The variable ‘fType’ is set conditionally ...

   if (NS_AUTOHEIGHT == aContainingBlockHeight) {
      if (cbrs->parentReflowState) {
        fType = cbrs->frame->GetType();

... and used conditionally ...

    if (eStyleUnit_Percent == heightUnit) {
      if (NS_AUTOHEIGHT == aContainingBlockHeight) {
        if (NS_FRAME_REPLACED(NS_CSS_FRAME_TYPE_INLINE) == mFrameType ||
            NS_FRAME_REPLACED_CONTAINS_BLOCK(
                NS_CSS_FRAME_TYPE_INLINE) == mFrameType) {
          if (eCompatibility_NavQuirks == aPresContext->CompatibilityMode()) {
            if (!IS_TABLE_CELL(fType)) {

... but ‘cbrs->parentReflowState’ doesn't appear in the second chain of ifs, and as best I can tell, none of them imply it, either.  So I conclude that this is a true positive for the warning.

The simplest fix would be to default-initialize fType to some frame type constant, but - should that frame type constant be a table cell or not?  It is being used to decide whether or not to apply a quirk to the containing block's height; not knowing how we could get here with cbrs->parentReflowState null but all the other conditions true, I don't know whether the quirk should apply.

Comment 1

[jbonnafo]

Hello,

I don't think nsHTMLReflowState.cpp is still part of the code base.
Should this ticket be closed?

Thanks,

Comment 2

[Emilio Cobos Álvarez (:emilio)]

It was renamed to ReflowInput.cpp. Here's the code:

  https://searchfox.org/mozilla-central/rev/d4ef4e9747133aa2914aca2a15cf9df1e42a6aa0/layout/generic/ReflowInput.cpp#2263

Comment 3

[jbonnafo]

Hello,

I am not sure that the warning is still there and relevant because:
Since the variable type of local variable ftype is an enum class of uint8_t, the default init called will initialize the ftype variable with the 0 value.

This would solve the issue.

Thanks,

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(In reply to jbonnafo from comment #3)
> Hello,
> 
> I am not sure that the warning is still there and relevant because:
> Since the variable type of local variable ftype is an enum class of uint8_t,
> the default init called will initialize the ftype variable with the 0 value.

I don't think enum classes are default-initialized to zero.

Comment 5

[jbonnafo]

Attachment: Bug 458473 - initialize local variable fType r?emilio

Bug 458473 - initialize local variable fType

Comment 6

[jbonnafo]

Hello,

I have updated patch proposal following comments in phabricator page.

Thanks,

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 9007625 [details]
Bug 458473 - initialize local variable fType r?emilio

David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 has approved the revision.

Comment 8

[jbonnafo]

Hello,
Could you please help me to land this patch?
It does not seem i have rights to use 'lando'.
Thanks,

Comment 9

[jbonnafo]

Hello,
Should i create a new bug ticket is order to have IS_TABLE_CELL renamed to to use capital letters since it is not a macro anymore?
Thanks,

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to jbonnafo from comment #8)
> Hello,
> Could you please help me to land this patch?
> It does not seem i have rights to use 'lando'.
> Thanks,

You can add the checkin-needed keyword to the bug, fwiw :)

(In reply to jbonnafo from comment #9)
> Hello,
> Should i create a new bug ticket is order to have IS_TABLE_CELL renamed to
> to use capital letters since it is not a macro anymore?
> Thanks,

Yes, I think that'd be great :)

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6ef8cd81d405
initialize local variable fType r=dbaron

Comment 12

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/6ef8cd81d405