ClassInfoData constructor does not initialize mFlags (initialized lazily later)

RESOLVED FIXED in Firefox 53

Status

()

Core
Security: CAPS
RESOLVED FIXED
5 months ago
4 months ago

People

(Reporter: jbonnafo, Assigned: jbonnafo)

Tracking

50 Branch
mozilla53
x86
Windows 10
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [lang=c++])

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
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.
(Assignee)

Updated

5 months ago
Whiteboard: [lang=c++]
(Assignee)

Updated

5 months ago
OS: Unspecified → Windows 10
Hardware: Unspecified → x86

Updated

5 months ago
Component: Untriaged → Security: CAPS
Product: Firefox → Core
(Assignee)

Comment 1

5 months ago
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,
(Assignee)

Updated

5 months ago
Flags: needinfo?(dveditz)
(Assignee)

Comment 2

5 months ago
Hello,

Could you please assign me to this issue ?

Any comments on this member variable not initialized in constructor ?

Thanks,
(Assignee)

Comment 3

4 months ago
Hello,

How could we move forward on this case ?

Thanks,
That field is initialized lazi
(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.
(Assignee)

Comment 6

4 months ago
Created attachment 8819854 [details] [diff] [review]
initialize mFlags variable in constructor
(Assignee)

Updated

4 months ago
Attachment #8819854 - Flags: review?(emilio+bugs)
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! :)
Attachment #8819854 - Flags: review?(emilio+bugs)
Assignee: nobody → jeanluc.bonnafoux
Flags: needinfo?(dveditz)
Summary: ClassInfoData constructor does not initialize mFlags → ClassInfoData constructor does not initialize mFlags (initialized lazily later)
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
Attachment #8819854 - Flags: review+
(Assignee)

Updated

4 months ago
Keywords: checkin-needed

Comment 9

4 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cad5f47a08b7
initialize mFlags variable in constructor. r=dveditz
Keywords: checkin-needed

Comment 10

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/cad5f47a08b7
Status: UNCONFIRMED → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.