Closed
Bug 10606
Opened 25 years ago
Closed 12 years ago
Uninitialized memory error caused by nsMargin
Categories
(Core :: Layout: Block and Inline, defect)
Core
Layout: Block and Inline
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | - |
People
(Reporter: eric, Unassigned)
References
Details
Attachments
(1 file)
23.79 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
Is there a reason why nsMargin's default constructor does not initialize its members to 0? struct nsMargin { nscoord left, top, right, bottom; // Constructors nsMargin() {} nsMargin(const nsMargin& aMargin) {*this = aMargin;} nsMargin(nscoord aLeft, nscoord aTop, nscoord aRight, nscoord aBottom) {left = aLeft; top = aTop; ... I have found numerous places in gecko where margins are constructed and then: nsMargin margin; spacing->GetMargin(margin); is done. But if you look as nsStyleContext GetMargin can fail and just not set the margin. PRBool nsStyleSpacing::GetPadding(nsMargin& aPadding) const { if (mHasCachedPadding) { aPadding = mCachedPadding; return PR_TRUE; } return PR_FALSE; } This is causing some nasty uninitialized memory bugs. It seems most people expect this: nsMargin margin; spacing->GetMargin(margin); to just work. Is there a reason nsMargin does not initialize is children in the default constructor? And is there a reason nsStyleContext does not set the margin's values to 0 if GetPadding fails? I would be happy to fix this I just want to make sure there isn't a reason for it. -E
nsMargin's constructor does not initialize the memory to 0 for performance reasons. I would prefer that we do not change this
Updated•25 years ago
|
Status: NEW → RESOLVED
Closed: 25 years ago
Resolution: --- → WONTFIX
Comment 2•25 years ago
|
||
Anyone expecting nsStyleSpacing's GetMargin (& GetPadding, GetBorder, GetOutline) to always work is flat out wrong. If that method returns PR_FALSE it does not mean the the margin is 0, it means that the margin could not be computed by the style code. The raw margin nsStyleCoords need to then be evaluated and "inherit" and/or percentage values must be computed by the consumer. Please file bugs for any call sites that do not repect the return status of GetMargin (et al). Having the nsMargin uninitialized just helps catch these errors, so I second the motion to leave it alone.
Reporter | ||
Updated•25 years ago
|
Status: RESOLVED → REOPENED
Reporter | ||
Comment 3•25 years ago
|
||
Agreed here are the files that I found doing a grep for GetBorder. But that doesn't include GetBorderPadding, GetMargin or any others. I fixed the ones on boxes and it fixed a lot of the strange behavior going on there. This may be causing strange things to happen in layout as well. nsAbsoluteContainingBlock RootFrame nsHTMLReflowState nsScrollFrame nsFieldSetFrame nsCSSRendering nsTableFrame This should probably be fixed ASAP.
The discussion was about GetMargin() and not about GetBorder(). Peter, at least in CSS border values can't be percentage based and so if we're sticking with that model then GetBorder() should never fail and we should probably make it return "void" instead of "PRBool" I know Kipp and I have been counting on that behavior and that's why we intentionally ignore the return from GetBorder(). Ignoring the return from GetMargin() is bad, of course
Updated•25 years ago
|
Assignee: peterl → troy
Status: REOPENED → NEW
Comment 5•25 years ago
|
||
I left GetBorder returning a PRBool because percentage values may become legal there in the future, we should at least test for it and assert on the PR_FALSE return, something like: nsMargin border; if (! spacing->GetBorder(border)) { NS_NOTYETIMPLEMENTED("percentage border"); } Note that GetBorderPadding can return PR_FALSE now (due to percentage padding). Fixing this is also a good time to expunge the use of the deprecated CalcMargin/Border/Padding/BorderPaddingFor methods, this should all be calculated in the reflow state...
Yes I know GetBorderPadding() can return PR_FALSE. That's never called outside of the HTML reflow state code. And we don't usually check the return there because we've already pre-initialized the value to {0, 0, 0, 0}
I'm accepting this bug mostly to fix any remaining calls to the CalcMargin/Border/Padding/BorderPaddingFor methods which as Peter indicated need to be eliminated.
Updated•25 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 25 years ago → 25 years ago
Resolution: --- → REMIND
Updated•25 years ago
|
Status: RESOLVED → VERIFIED
Comment 8•22 years ago
|
||
REMIND is deprecated per bug 35839.
Status: VERIFIED → REOPENED
Resolution: REMIND → ---
Comment 10•22 years ago
|
||
dbaron, roc, what do we want to do here? I just took a look at some callers (see http://lxr.mozilla.org/seamonkey/search?string=GetMargin\( ) and nsImageDocument, nsGfxScrollFrame (and maybe others; I only looked at the first 5-6 items) seem to break on percent margins....
Not sure ... can we pass in a frame to be used as the base for the % calculation, and then always return a valid margin?
Comment 12•22 years ago
|
||
Some places that are calling GetMargin() don't have a frame to pass in (nsImageDocument in particular).
Comment 13•21 years ago
|
||
->B&I
Assignee: attinasi → block-and-inline
Component: Layout → Layout: Block & Inline
QA Contact: petersen → ian
Comment 14•21 years ago
|
||
If you reassign, please remove obsolete priorities and milestones, ok?
Priority: P3 → --
Target Milestone: M12 → ---
Comment 15•21 years ago
|
||
requested in 1999. Please see if this can be fixed for 2003. Thanks.
Flags: blocking1.6b?
While some further analysis should be done here, there are no known symptoms, so it's not exactly high priority, and certainly not a release blocker. Priority of bugs should be evaluated independently of when they were filed -- there's no reason to assume that the bugs filed earliest are the most important.
Flags: blocking1.6b? → blocking1.6b-
Comment 17•19 years ago
|
||
Attachment #194911 -
Flags: superreview?(dbaron)
Attachment #194911 -
Flags: review?(dbaron)
Comment on attachment 194911 [details] [diff] [review] GetMargin() fixes, rev. 1 In nsImageDocument, you should use the if() pattern because there are multiple calls, and there could still be a bug if one of the later ones returns false. With that, r+sr=dbaron. Sorry for the delay.
Attachment #194911 -
Flags: superreview?(dbaron)
Attachment #194911 -
Flags: superreview+
Attachment #194911 -
Flags: review?(dbaron)
Attachment #194911 -
Flags: review+
Comment 19•18 years ago
|
||
Taking bug, I completely forgot about this one, sorry. I'll try to make an updated patch when time allows.
Assignee: layout.block-and-inline → mats.palmgren
OS: Windows NT → All
Are you planning to land this?
Comment 21•17 years ago
|
||
Mats, is this still on the radar for 1.9?
Updated•15 years ago
|
QA Contact: ian → layout.block-and-inline
Updated•15 years ago
|
Flags: wanted1.9.2?
Flags: wanted1.8.1.x?
Flags: wanted-fennec1.0?
Flags: in-testsuite+
Flags: in-litmus+
Flags: blocking1.9.2?
Flags: blocking1.9.0.17?
Flags: blocking1.9.0.16?
Flags: blocking1.8.1.next?
Flags: wanted1.9.2?
Flags: wanted1.9.2-
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Updated•15 years ago
|
Flags: wanted1.8.1.x?
Flags: blocking1.9.0.17?
Flags: blocking1.9.0.16?
Flags: blocking1.9.0.16-
Flags: blocking1.8.1.next?
Flags: blocking1.8.1.next-
Updated•15 years ago
|
Flags: wanted-fennec1.0?
Flags: in-testsuite+
Flags: in-litmus+
Comment 22•14 years ago
|
||
Happy birthday, bug 10606! Happy 11 years! Happy 11 years with importance = CRITICAL, affecting ALL operating systems on ALL platforms!
Updated•14 years ago
|
blocking2.0: --- → ?
Severity: critical → minor
While we might want to change the code design here, I don't think there are any real bugs caused by this problem, so it doesn't block.
blocking2.0: ? → -
Comment 24•12 years ago
|
||
This was fixed some time ago by making the ctor initialize all the fields to zero.
Assignee: matspal → nobody
Status: NEW → RESOLVED
Closed: 25 years ago → 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•