Mats Palmgren (inactive) Reporter
	Description • 7 years ago

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

	Robert Longson [:longsonr] Assignee
	Comment 1 • 7 years ago

false positive, the methods in nsSVGFrame.cpp always initialise them before use

	Robert Longson [:longsonr] Assignee
	Comment 2 • 7 years ago

nsSVGMarkerFrame.cpp I mean.

	Jonathan Watt [:jwatt]
	Comment 4 • 7 years ago

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.

	Jonathan Watt [:jwatt]
	Comment 5 • 7 years ago

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.

	Robert Longson [:longsonr] Assignee
	Comment 6 • 7 years ago

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.

	Jonathan Watt [:jwatt]
	Comment 7 • 7 years ago

Should've gone to Specsavers.

	Robert Longson [:longsonr] Assignee
	Comment 8 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=feb34ad8f87170b217f8bc009feb3d1391bbe072

	Robert Longson [:longsonr] Assignee
	Comment 9 • 7 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7294081abfeedcf0709f9d393ba761b16a71de1

	Pulsebot
	Comment 10 • 7 years ago

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

	Carsten Book [:Tomcat]
	Comment 11 • 7 years ago
bugherder

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

	Johann Hofmann [:johannh]
	Comment 12 • 7 years ago

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?

	Robert Longson [:longsonr] Assignee
	Comment 13 • 7 years ago

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?

	Jonathan Watt [:jwatt]
	Comment 14 • 7 years ago

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.

	Johann Hofmann [:johannh]
	Comment 15 • 7 years ago

Thanks both of you! Sorry for the confusion, I filed bug 1372577. :)