Last Comment Bug 10606 - Uninitialized memory error caused by nsMargin
: Uninitialized memory error caused by nsMargin
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: Trunk
: All All
: -- minor with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
Depends on:
Blocks: 201983
  Show dependency treegraph
 
Reported: 1999-07-27 13:28 PDT by Eric Vaughan
Modified: 2012-01-28 07:39 PST (History)
10 users (show)
dbaron: blocking1.6b-
dbaron: blocking1.9.2-
dbaron: wanted1.9.2-
dveditz: blocking1.9.0.16-
dveditz: blocking1.8.1.next-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
GetMargin() fixes, rev. 1 (23.79 KB, patch)
2005-09-05 06:13 PDT, Mats Palmgren (vacation)
dbaron: review+
dbaron: superreview+
Details | Diff | Splinter Review

Description Eric Vaughan 1999-07-27 13:28:31 PDT
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
Comment 1 troy 1999-07-27 14:17:59 PDT
nsMargin's constructor does not initialize the memory to 0 for performance
reasons. I would prefer that we do not change this
Comment 2 Peter Linss 1999-07-27 16:05:59 PDT
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.
Comment 3 Eric Vaughan 1999-07-27 19:51:59 PDT
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.
Comment 4 troy 1999-07-27 19:59:59 PDT
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
Comment 5 Peter Linss 1999-07-27 20:07:59 PDT
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...
Comment 6 troy 1999-07-27 20:29:59 PDT
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}
Comment 7 troy 1999-07-27 20:31:59 PDT
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.
Comment 8 Christopher Hoess (gone) 2002-04-29 14:03:08 PDT
REMIND is deprecated per bug 35839.
Comment 9 Christopher Hoess (gone) 2002-07-12 22:42:17 PDT
->default
Comment 10 Boris Zbarsky [:bz] 2003-02-16 10:04:11 PST
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....
Comment 11 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2003-02-16 11:26:15 PST
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 Boris Zbarsky [:bz] 2003-02-16 11:33:17 PST
Some places that are calling GetMargin() don't have a frame to pass in
(nsImageDocument in particular).
Comment 13 Christopher Hoess (gone) 2003-06-07 17:17:54 PDT
->B&I
Comment 14 Boris Zbarsky [:bz] 2003-06-07 18:42:24 PDT
If you reassign, please remove obsolete priorities and milestones, ok?
Comment 15 Peter Kroll 2003-11-09 11:01:41 PST
requested in 1999. Please see if this can be fixed for 2003. Thanks.
Comment 16 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2003-11-09 11:43:41 PST
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.
Comment 17 Mats Palmgren (vacation) 2005-09-05 06:13:46 PDT
Created attachment 194911 [details] [diff] [review]
GetMargin() fixes, rev. 1
Comment 18 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2005-09-28 17:16:11 PDT
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.
Comment 19 Mats Palmgren (vacation) 2006-09-12 04:23:54 PDT
Taking bug, I completely forgot about this one, sorry.
I'll try to make an updated patch when time allows.
Comment 20 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2007-07-18 11:49:53 PDT
Are you planning to land this?
Comment 21 Ryan VanderMeulen [:RyanVM] 2008-01-06 20:13:57 PST
Mats, is this still on the radar for 1.9?
Comment 22 nidaisfree 2010-07-27 09:07:55 PDT
Happy birthday, bug 10606!

Happy 11 years!

Happy 11 years with importance = CRITICAL, affecting ALL operating systems on ALL platforms!
Comment 23 David Baron :dbaron: ⌚️UTC+2 (review requests must explain patch) 2010-08-01 19:47:09 PDT
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.
Comment 24 Mats Palmgren (vacation) 2012-01-28 07:39:07 PST
This was fixed some time ago by making the ctor initialize all the fields to zero.

Note You need to log in before you can comment on or make changes to this bug.