Coverity report: nsSVGMarkerFrame::​nsSVGMarkerFrame(nsStyleContext *): A scalar field is not initialized by the constructor

RESOLVED FIXED in Firefox 55

Status

()

defect
P4
minor
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mats, Assigned: longsonr)

Tracking

(Blocks 1 bug, {coverity, good-first-bug, regression})

53 Branch
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox53 wontfix, firefox54 wontfix, firefox55 fixed)

Details

(Whiteboard: [CID 750338], )

Attachments

(1 attachment)

Coverity CID 750338 Uninitialized scalar field

The field will contain an arbitrary value left over from earlier computations.

In nsSVGMarkerFrame::​nsSVGMarkerFrame(nsStyleContext *): A scalar field is not initialized by the constructor 

 35  explicit nsSVGMarkerFrame(nsStyleContext* aContext)
 36    : nsSVGContainerFrame(aContext, mozilla::FrameType::SVGMarker)
 37    , mMarkedFrame(nullptr)
 38    , mInUse(false)
 39    , mInUse2(false)
 40  {
 41    AddStateBits(NS_FRAME_IS_NONDISPLAY);
   2. uninit_member: Non-static class member mStrokeWidth is not initialized in this constructor nor in any functions that it calls.
   4. uninit_member: Non-static class member mX is not initialized in this constructor nor in any functions that it calls.
   6. uninit_member: Non-static class member mY is not initialized in this constructor nor in any functions that it calls.
   8. uninit_member: Non-static class member mAutoAngle is not initialized in this constructor nor in any functions that it calls.
   CID 750338 (#1 of 1): Uninitialized scalar field (UNINIT_CTOR)10. uninit_member: Non-static class member mIsStart is not initialized in this constructor nor in any functions that it calls.
 42  }


http://searchfox.org/mozilla-central/rev/abe68d5dad139e376d5521ca1d4b7892e1e7f1ba/layout/svg/nsSVGMarkerFrame.h#35
false positive, the methods in nsSVGFrame.cpp always initialise them before use
nsSVGMarkerFrame.cpp I mean.
Whiteboard: [CID 750338]
Posted patch patchSplinter Review
Assignee: nobody → longsonr
Attachment #8866427 - Flags: review?(jwatt)
The very big assumption in this patch is that nsSVGMarkerFrame::GetCanvasTM and nsSVGMarkerFrame::GetMarkBBoxContribution are are only called under nsSVGMarkerFrame::PaintMark (and only after that method has set mMarkerTM).

I *think* we've now killed off enough GetCanvasTM calls that this is valid for nsSVGMarkerFrame::GetCanvasTM (although it's quite an unexpected invariant to introduce). However, it seems GetMarkBBoxContribution breaks the invariant since nsSVGUtils::GetBBox can be called and passed eBBoxIncludeMarkers, in which case we will not be under PaintMark.
Flags: needinfo?(longsonr)
I got that a bit muddled. It's GetMarkBBoxContribution that sets mMarkerTM, so the invariant would be that GetMarkBBoxContribution must have been called for a specific marked element directly before calling nsSVGMarkerFrame::GetCanvasTM or nsSVGMarkerFrame::PaintMark for that element. I don't think we call GetMarkBBoxContribution for an element directly before PaintMark is called for it though.
Previously nsSVGMarkerFrame::PaintMark and nsSVGMarkerFrame::GetMarkBBoxContribution both use to set 

-  mStrokeWidth = aStrokeWidth;
-  mX = aMark->x;
-  mY = aMark->y;
-  mAutoAngle = aMark->angle;
-  mIsStart = aMark->type == nsSVGMark::eStart;

Now, instead they both set

+  mMarkerTM = content->GetMarkerTransform(aStrokeWidth, aMark);

instead. 

nsSVGMarkerFrame::GetCanvasTM is was only valid before if called as a callback from nsSVGMarkerFrame::PaintMark and nsSVGMarkerFrame::GetMarkBBoxContribution and that's still true now.

There's no functional change here other than we're depending on a class (mMarkerTM) to hold the data rather than various scalar values.
Flags: needinfo?(longsonr)
Should've gone to Specsavers.
Attachment #8866427 - Flags: review?(jwatt) → review+
Pushed by longsonr@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd72dc86c01d
Simplify nsSVGMarkerFrame by storing the transform matrix rather than all the parameters necessary to calculate it. r=jwatt
https://hg.mozilla.org/mozilla-central/rev/fd72dc86c01d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
We're seeing some strange SVG icon artifacts in the identity popup on Windows 8 in mozscreenshots (our visual regression tool):

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=f223e1fd2044a026c740434df95f37a7f7accf48&newProject=mozilla-central&newRev=6491fb29e7fcc9e02cc179ae2856f426a3552385&filter=%5Ewindows8.*controlcenter

The pushlog is: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f223e1fd2044a026c740434df95f37a7f7accf48&tochange=6491fb29e7fcc9e02cc179ae2856f426a3552385

This was the only SVG-related bug I could find on a quick glance in there.

I can't reproduce this problem on Windows 10 and haven't tried Windows 8 yet (due to lack of a Windows 8 installation). Robert, can you take a look at this?
Flags: needinfo?(longsonr)
This change should not have any functional effect. Feel free to do a local backout to prove it. 

I've no access to any windows PCs and I don't understand your steps to reproduce this issue. 

What do I do when I goto that page, it's got no SVG in it?

What's an identity popup, does it contain any marker elements, if not then this bug can't be the cause?
Flags: needinfo?(longsonr) → needinfo?(jhofmann)
Almost certainly an issue with the patches for bug 1359527. I've asked Johann on IRC to file a separate bug to track the artifacts issue.
Flags: needinfo?(jhofmann)
Thanks both of you! Sorry for the confusion, I filed bug 1372577. :)
You need to log in before you can comment on or make changes to this bug.