918176 - uninitialized variable warning for "type" in nsRuleNode::ComputeContentData

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

Attachment: patch

I used nsStyleContentType(0) since that's what nsStyleContentData::mType gets initialized to in its constructor.

Comment 1

[Daniel Holbert [:dholbert]]

Hmm... 0 isn't one of the enum's valid values, so it feels kinda bogus use that value. (even if we already do somewhere)

Comment 2

[Cameron McCormack (:heycam)]

OK, two other options then.  We could (a) explicitly handle these invalid values by sticking a new eStyleContent_String (with an empty string) into the nsStyleContentData object, or (b) define a new eStyleContentType_None (or some other name sounding less valid) and handle that explicitly here, and make sure other inspections of nsStyleContentData::mType would do something sensible with it.

Comment 3

[Daniel Holbert [:dholbert]]

I'd prefer (b), with the constructor's bogus "0" usage updated as well.  It's not great, but I think it's probably better than using an out-of-bounds-for-the-enum value and uninitialized variable usage  (albeit unlikely), like we've got now.

Maybe eStyleContentType_Uninitialized = 0 ?

Comment 4

[Cameron McCormack (:heycam)]

Sounds good, I'll do that.

Comment 5

[Cameron McCormack (:heycam)]

Attachment: patch v2

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 807072 [details] [diff] [review]
patch v2

>+++ b/layout/style/nsRuleNode.cpp
>-          nsStyleContentType type;
>+          nsStyleContentType type = nsStyleContentType(0);

You probably want to revert this, right? (both the unnecessary* initialization, and the "(0)" usage)

*unnecessary because I think (?) you're making all of the code paths set a value for "type" now?

>-          else if (type <= eStyleContentType_Attr) {
>+          else if (type <= eStyleContentType_Attr &&
>+                   type != eStyleContentType_Uninitialized) {
>             value.GetStringValue(buffer);
>             data.mContent.mString = NS_strdup(buffer.get());
>           }
>-          else if (type <= eStyleContentType_Counters) {
>+          else if (type <= eStyleContentType_Counters &&
>+                   type != eStyleContentType_Uninitialized) {

Maybe put eStyleContentType_Uninitialized at the end of the enum's list (so e.g. just don't give it a value, and list it after the last value), so that these != checks aren't needed? Then any _Uninitialized values would fall into this catch-all "else" case, without you having to change the "if" checks here.

>+++ b/layout/style/nsStyleStruct.cpp
>@@ -2568,17 +2568,17 @@ bool nsStyleContentData::operator==(cons
> }
> 
> void
> nsStyleContentData::TrackImage(nsPresContext* aContext)
> {
>   // Sanity
>   NS_ABORT_IF_FALSE(!mImageTracked, "Already tracking image!");
>   NS_ABORT_IF_FALSE(mType == eStyleContentType_Image,
>-                    "Tryingto do image tracking on non-image!");
>+                    "Trying to do image tracking on non-image!");

This whitespace fix is the only change in this file.

I guess it's fine here, but it'd probably be better to just push this change on its own, with "(no bug)" and DONTBUILD.

(Feel free to say rs=dholbert if you like; I don't think you need review for one-off whitespace fixes like this, though.)

>diff --git a/layout/style/nsStyleStruct.h b/layout/style/nsStyleStruct.h
> enum nsStyleContentType {
>+  eStyleContentType_Uninitialized = 0,
>   eStyleContentType_String        = 1,

(Per above, you might want to put this at the end of the list instead of at the beginning)

r=me with that.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> Comment on attachment 807072 [details] [diff] [review]
> patch v2
> 
> >+++ b/layout/style/nsRuleNode.cpp
> >-          nsStyleContentType type;
> >+          nsStyleContentType type = nsStyleContentType(0);
> 
> You probably want to revert this, right? (both the unnecessary*
> initialization, and the "(0)" usage)

er s/both the/both because of the/

Comment 8

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #6)
> >+++ b/layout/style/nsRuleNode.cpp
> >-          nsStyleContentType type;
> >+          nsStyleContentType type = nsStyleContentType(0);
> 
> You probably want to revert this, right? (both the unnecessary*
> initialization, and the "(0)" usage)
> 
> *unnecessary because I think (?) you're making all of the code paths set a
> value for "type" now?

Yes, forgot to take that out again.

> >-          else if (type <= eStyleContentType_Attr) {
> >+          else if (type <= eStyleContentType_Attr &&
> >+                   type != eStyleContentType_Uninitialized) {
> >             value.GetStringValue(buffer);
> >             data.mContent.mString = NS_strdup(buffer.get());
> >           }
> >-          else if (type <= eStyleContentType_Counters) {
> >+          else if (type <= eStyleContentType_Counters &&
> >+                   type != eStyleContentType_Uninitialized) {
> 
> Maybe put eStyleContentType_Uninitialized at the end of the enum's list (so
> e.g. just don't give it a value, and list it after the last value), so that
> these != checks aren't needed? Then any _Uninitialized values would fall
> into this catch-all "else" case, without you having to change the "if"
> checks here.

That works for me.

> >+++ b/layout/style/nsStyleStruct.cpp
> >@@ -2568,17 +2568,17 @@ bool nsStyleContentData::operator==(cons
> > }
> > 
> > void
> > nsStyleContentData::TrackImage(nsPresContext* aContext)
> > {
> >   // Sanity
> >   NS_ABORT_IF_FALSE(!mImageTracked, "Already tracking image!");
> >   NS_ABORT_IF_FALSE(mType == eStyleContentType_Image,
> >-                    "Tryingto do image tracking on non-image!");
> >+                    "Trying to do image tracking on non-image!");
> 
> This whitespace fix is the only change in this file.
> 
> I guess it's fine here, but it'd probably be better to just push this change
> on its own, with "(no bug)" and DONTBUILD.
> 
> (Feel free to say rs=dholbert if you like; I don't think you need review for
> one-off whitespace fixes like this, though.)

Will do.

Comment 9

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a895d95c88b

Comment 10

[Phil Ringnalda (:philor)]

Beaucoup "uninitialized content type: 'Not Reached', file ../../../layout/base/nsCSSFrameConstructor.cpp, line 1644" assertions across every mochitest hunk except maybe 4, and crashtests and reftests, and also the https://tbpl.mozilla.org/php/getParsedLog.php?id=28129686&tree=Mozilla-Inbound failures for the non-asserting set.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e1998769de9e

Comment 11

[Cameron McCormack (:heycam)]

> @@ -6989,32 +6989,35 @@ nsRuleNode::ComputeContentData(void* aSt
>              case NS_STYLE_CONTENT_CLOSE_QUOTE:
>                type = eStyleContentType_CloseQuote;    break;
>              case NS_STYLE_CONTENT_NO_OPEN_QUOTE:
>                type = eStyleContentType_NoOpenQuote;   break;
>              case NS_STYLE_CONTENT_NO_CLOSE_QUOTE:
>                type = eStyleContentType_NoCloseQuote;  break;
>              default:
>                NS_ERROR("bad content value");
> +              type = eStyleContentType_Uninitialized; break;
>              }
> -            break;
>            default:
>              NS_ERROR("bad content type");
> +            type = eStyleContentType_Uninitialized;
>            }

Moving the break into the inner switch was bogus.

Comment 12

[Cameron McCormack (:heycam)]

https://tbpl.mozilla.org/?tree=Try&rev=96f141980c6e

Comment 13

[Cameron McCormack (:heycam)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccfc5f4492f1

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ccfc5f4492f1