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

RESOLVED FIXED in mozilla27

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: heycam, Assigned: heycam)

Tracking

(Blocks 1 bug)

Trunk
mozilla27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Posted patch patch (obsolete) — Splinter Review
I used nsStyleContentType(0) since that's what nsStyleContentData::mType gets initialized to in its constructor.
Attachment #807030 - Flags: review?(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)
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.
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 ?
Sounds good, I'll do that.
Posted patch patch v2Splinter Review
Attachment #807030 - Attachment is obsolete: true
Attachment #807030 - Flags: review?(dholbert)
Attachment #807072 - Flags: review?(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.
Attachment #807072 - Flags: review?(dholbert) → review+
(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/
Version: unspecified → Trunk
(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.
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
> @@ -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.
https://hg.mozilla.org/mozilla-central/rev/ccfc5f4492f1
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Duplicate of this bug: 846552
You need to log in before you can comment on or make changes to this bug.