1322146 - ClassInfoData constructor does not initialize mFlags (initialized lazily later)

Status: RESOLVED FIXED

(Core :: Security: CAPS, defect)

Description

[jbonnafo]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161129173726

Steps to reproduce:

In file nsScriptsSecurityManager.cpp, line 164 the constructor:
 
ClassInfoData(nsIClassInfo *aClassInfo, const char *aName)
        : mClassInfo(aClassInfo),
          mName(const_cast<char *>(aName)),
          mDidGetFlags(false),
          mMustFreeName(false)
    {
    }

does not initialize the member variable: mFlags



Expected results:

This member viariable should be initialized at constructor level in my opinion.

Comment 1

[jbonnafo]

Hello,

I don't think codes generates an issue.

ClassInfoData is used only once here in nsScriptsSecurityManager.cpp, line 1195

ClassInfoData objClassInfo = ClassInfoData(aClassInfo, nullptr);
if (objClassInfo.IsDOMClass())
{
        return NS_OK;
}

where IsDOMClass() will trigger call to GetFlags() method.
Since constructor of ClassInfoData sets data member mDidGetFlags to false, the mFlags data member will be set by the call to:
nsresult rv = mClassInfo->GetFlags(&mFlags); line 182

IMHO, it is a good thing to initialize all data members in constructors.
I am also wondering why we a a full 'helper class' in a .cpp file.

Thanks,

Comment 2

[jbonnafo]

Hello,

Could you please assign me to this issue ?

Any comments on this member variable not initialized in constructor ?

Thanks,

Comment 3

[jbonnafo]

Hello,

How could we move forward on this case ?

Thanks,

Comment 4

[Emilio Cobos Álvarez (:emilio)]

That field is initialized lazi

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #4)
> That field is initialized lazi

Sorry (too much context-switching).

I meant... As discussed on IRC, that field is initialized lazily and not used without initialization, so it's not a bug. The reporter would like to submit a patch to simplify that code though, so I guess we can leave this open until that happens.

Comment 6

[jbonnafo]

Attachment: initialize mFlags variable in constructor

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8819854 [details] [diff] [review]
initialize mFlags variable in constructor

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

The patch looks ok to me, but I'd also take time to simplify GetFlags, otherwise the patch doesn't make as much sense I think.

Also, the patch can't technically land just with my review I think, but I'll find someone to redirect the review when the patch is updated.

Clearing the review flag for now. Feel free to flag me again when the patch is updated.

Thanks! :)

Comment 8

[Daniel Veditz [:dveditz]]

Comment on attachment 8819854 [details] [diff] [review]
initialize mFlags variable in constructor

Can't hurt. Currently we don't need it because it's initialized before being used, but this could prevent future problems if that ever changes.

r=dveditz

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cad5f47a08b7
initialize mFlags variable in constructor. r=dveditz

Comment 10

[Carsten Book [:Tomcat]]

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