Closed
Bug 1361593
Opened 8 years ago
Closed 7 years ago
Coverity report: nsSVGMarkerFrame::nsSVGMarkerFrame(nsStyleContext *): A scalar field is not initialized by the constructor
Categories
(Core :: SVG, defect, P4)
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox53 | --- | wontfix |
firefox54 | --- | wontfix |
firefox55 | --- | fixed |
People
(Reporter: MatsPalmgren_bugz, Assigned: longsonr)
References
(Blocks 1 open bug, )
Details
(Keywords: coverity, good-first-bug, regression, Whiteboard: [CID 750338])
Attachments
(1 file)
8.39 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
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
Assignee | ||
Comment 1•8 years ago
|
||
false positive, the methods in nsSVGFrame.cpp always initialise them before use
Assignee | ||
Comment 2•8 years ago
|
||
nsSVGMarkerFrame.cpp I mean.
Updated•8 years ago
|
Blocks: coverity-analysis
Whiteboard: [CID 750338]
Assignee | ||
Comment 3•8 years ago
|
||
Assignee: nobody → longsonr
Attachment #8866427 -
Flags: review?(jwatt)
Comment 4•8 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.
Flags: needinfo?(longsonr)
Comment 5•8 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.
Assignee | ||
Comment 6•8 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.
Flags: needinfo?(longsonr)
Comment 7•8 years ago
|
||
Should've gone to Specsavers.
Updated•8 years ago
|
Attachment #8866427 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 8•7 years ago
|
||
Assignee | ||
Comment 9•7 years ago
|
||
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
Comment 11•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•7 years ago
|
status-firefox53:
--- → wontfix
status-firefox54:
--- → wontfix
status-firefox-esr52:
--- → unaffected
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?
Flags: needinfo?(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?
Flags: needinfo?(longsonr) → needinfo?(jhofmann)
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.
Flags: needinfo?(jhofmann)
Comment 15•7 years ago
|
||
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.
Description
•