1361604 - Coverity report:nsMathMLSelectedFrame::​nsMathMLSelectedFrame(nsStyleContext *): A pointer field is not initialized in the constructor

Status: RESOLVED FIXED

(Core :: MathML, defect, P4)

Description

[Mats Palmgren (inactive)]

Coverity CID 1123704 Uninitialized pointer field

The pointer field will point to an arbitrary memory location, any attempt to write may cause corruption.

In nsMathMLSelectedFrame::​nsMathMLSelectedFrame(nsStyleContext *): A pointer field is not initialized in the constructor



56  explicit nsMathMLSelectedFrame(nsStyleContext* aContext) :
  2. uninit_member: Non-static class member mSelectedFrame is not initialized in this constructor nor in any functions that it calls.
  CID 1123704 (#1 of 1): Uninitialized pointer field (UNINIT_CTOR)4. uninit_member: Non-static class member mInvalidMarkup is not initialized in this constructor nor in any functions that it calls.
57    nsMathMLContainerFrame(aContext) {}
58  virtual ~nsMathMLSelectedFrame();
59  
60  virtual nsIFrame* GetSelectedFrame() = 0;
  1. member_decl: Class member declaration for mSelectedFrame.
61  nsIFrame*       mSelectedFrame;
62
  3. member_decl: Class member declaration for mInvalidMarkup.
63  bool            mInvalidMarkup;

Comment 1

[Mats Palmgren (inactive)]

We're probably OK since we initialize these members in Init:
http://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/layout/mathml/nsMathMLSelectedFrame.cpp#21-22
but we should init them in the ctor instead and then remove the Init method
since it serves no purpose after that.

Comment 2

[Frédéric Wang (:fredw)]

(In reply to Mats Palmgren (:mats) from comment #1)
> We're probably OK since we initialize these members in Init:
> http://searchfox.org/mozilla-central/rev/
> abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/layout/mathml/nsMathMLSelectedFrame.
> cpp#21-22
> but we should init them in the ctor instead and then remove the Init method
> since it serves no purpose after that.

Note that Init is an inherited member and it's used again in derived classes. I don't remember why we de these in the Init function, but I guess it was for a good reason :-) So I'm not sure we can remove it so easily...

Regarding all these Coverity reports, I wonder whether it would help to move to C++11 initialization of class members i.e. just

nsIFrame*       mSelectedFrame = nullptr;
bool            mInvalidMarkup = false;

Are there are any plans to do that in Mozilla?

Comment 3

[Mats Palmgren (inactive)]

> Note that Init is an inherited member and it's used again in derived classes.

Right, it's virtual, so removing this method will make all calls go to its
base class' method instead, so having an Init method that just calls its
base class Init method is redundant and should be removed.

> nsIFrame*       mSelectedFrame = nullptr;

I tend to think that for all but the most trivial structs we should use a ctor
and initialize all members there.  It just more readable that way IMO.
Frame classes tend to be non-trivial, and using the same init convention for
all classes is less confusing than using a mix.

Comment 4

[Robert Longson [:longsonr]]

Attachment: patch

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d4a586ccc42
initialise nsMathMLSelectedFrame members in constructor rather than ::Init r=mats

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/5d4a586ccc42