Uninitialised value use in nsMathMLChar::StretchInternal

RESOLVED FIXED in mozilla5

Status

()

defect
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: jseward, Assigned: fredw)

Tracking

({valgrind})

Trunk
mozilla5
x86
Windows XP
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

9 years ago
Whilst running crashtests on WinXP build of M-C of yesterday, on Wine:

layout/generic/crashtests/472587-1.xhtml

Conditional jump or move depends on uninitialised value(s)
   at 0x1093EF91: nsMathMLChar::StretchInternal (nsmathmlchar.cpp:1748)
   by 0x1093F83E: nsMathMLChar::GetMaxWidth (nsmathmlchar.cpp:1835)
   by 0x1092B757: nsMathMLmencloseFrame::PlaceInternal
                  (nsmathmlmencloseframe.cpp:530)
   by 0x1092AE1A: nsMathMLmencloseFrame::MeasureForWidth
                  (nsmathmlmencloseframe.cpp:338)
   by 0x10928E77: nsMathMLContainerFrame::GetIntrinsicWidth
                  (nsmathmlcontainerframe.cpp:1080)
   by 0x10928DA0: nsMathMLContainerFrame::GetMinWidth
                  (nsmathmlcontainerframe.cpp:1039)
   by 0x1043AF6D: nsLayoutUtils::IntrinsicForContainer (nslayoututils.cpp:2012)
   by 0x1048E7AF: nsFrame::AddInlineMinWidth (nsframe.cpp:3146)
   by 0x104A3993: nsBlockFrame::GetMinWidth (nsblockframe.cpp:743)
   by 0x104D17E4: nsColumnSetFrame::GetMinWidth (nscolumnsetframe.cpp:461)
   by 0x1043AF6D: nsLayoutUtils::IntrinsicForContainer (nslayoututils.cpp:2012)
   by 0x104A38B6: nsBlockFrame::GetMinWidth (nsblockframe.cpp:724)

 Uninitialised value was created by a heap allocation
   at 0x72184B2: RtlAllocateHeap (heap.c:205)
   by 0x78583A57: ???
   by 0x7F67107C: ??? (in /mnt/win32/MC-19-10-2010/ff-exp/dist/bin/mozalloc.dll)
   by 0x10CC85FC: NS_Alloc_P (nsmemoryimpl.cpp:210)
   by 0x10257A38: nsTArray_base::EnsureCapacity (nstarray.cpp:76)
   by 0x1092C5CF: nsTArray<nsMathMLChar>::AppendElements (nstarray.h:678)
   by 0x1092C550: nsTArray<nsMathMLChar>::AppendElement (nstarray.h:693)
   by 0x1092A130: nsMathMLmencloseFrame::AllocateMathMLChar
                  (nsmathmlmencloseframe.cpp:101)
   by 0x1092E9F5: nsMathMLmsqrtFrame::Init (nsmathmlmsqrtframe.cpp:81)
   by 0x1041DA51: nsCSSFrameConstructor::InitAndRestoreFrame
                  (nscssframeconstructor.cpp:4547)
   by 0x1041C286: nsCSSFrameConstructor::ConstructFrameFromItemInternal
                  (nscssframeconstructor.cpp:3760)
   by 0x1041FDB4: nsCSSFrameConstructor::ConstructFramesFromItem
                  (nscssframeconstructor.cpp:5475)
Reporter

Updated

9 years ago
OS: Linux → Windows XP
Hardware: x86_64 → x86
Fred, the stretch process avoids setting mGlyph when doing NS_STRETCH_MAXWIDTH.
I can't remember whether that is necessary or not (I think it was to avoid undoing a previous stretch if merely measuring widths),
but do we need some special max-width handling for the
"if (mGlyph.font == -1 && largeop)" block in StretchInternal?
Assignee

Comment 2

9 years ago
(In reply to comment #1)
> Fred, the stretch process avoids setting mGlyph when doing NS_STRETCH_MAXWIDTH.
> I can't remember whether that is necessary or not (I think it was to avoid
> undoing a previous stretch if merely measuring widths),
> but do we need some special max-width handling for the
> "if (mGlyph.font == -1 && largeop)" block in StretchInternal?

I think this block is still necessary when doing NS_STRETCH_MAXWIDTH, because it updates the width. This "mGlyph.font == -1" (and the one a bit above, protected by !maxWidth) basically means that no glyph has been found in our StretchInternal function. Maybe we can replace this by a variable glyphFound in StretchInternal that we update after the each call to font.EnumerateFamilies (maybe we need to send this variable to the StretchEnumContext routine). Then, I guess setting mGlyph.font = 1 will no longer be necessary in the Stretch function.
(In reply to comment #2)
> Maybe we can replace this by a variable glyphFound in
> StretchInternal that we update after the each call to font.EnumerateFamilies
> (maybe we need to send this variable to the StretchEnumContext routine).

That might be the best option.

I thought about recording the original size and replacing "if (mGlyph.font == -1 && largeop)" with a check on whether the size had changed, but it looks like that check might be complicated or not quite right with maxWidth.
Assignee

Comment 4

9 years ago
If we want to prevent undoing a previous stretch with maxWidth, then mUnscaledAscent, mDrawNormal, mScaleX, mScaleY should not be modified. This is not currently the case for mScaleX and mScaleY.

It is not really possible from the StretchEnumContext to know where the computed max width comes from (it is the max of all the metrics of "variants" and "parts"). So maybe a simpler way to solve this bug is to always execute the block when maxWidth+largeop i.e. replacing the condition by

if ((maxWidth || mGlyph.font == -1) && largeop) {
(In reply to comment #4)
> If we want to prevent undoing a previous stretch with maxWidth, then
> mUnscaledAscent, mDrawNormal, mScaleX, mScaleY should not be modified. This is
> not currently the case for mScaleX and mScaleY.

They should probably not be modified.

> It is not really possible from the StretchEnumContext to know where the
> computed max width comes from (it is the max of all the metrics of "variants"
> and "parts"). So maybe a simpler way to solve this bug is to always execute the
> block when maxWidth+largeop i.e. replacing the condition by
> 
> if ((maxWidth || mGlyph.font == -1) && largeop) {

For the largeoponly case, it should be possible to get the exact width, and it would be preferable to have the exact width where possible.

I expect your earlier suggestion, with tracking whether a variant was found in the StretchEnumContext, would also give a better width for non-largeoponly largeops.
Assignee

Comment 6

9 years ago
Posted patch Patch V1 (obsolete) — Splinter Review
Assignee: nobody → fred.wang
Status: NEW → ASSIGNED
Attachment #488692 - Flags: review?(karlt)
Comment on attachment 488692 [details] [diff] [review]
Patch V1

>         if (maxWidth) {
>+          mGlyphFound = PR_TRUE;
>+

>         else {
>           mBoundingMetrics = bm;
>           haveBetter = PR_TRUE;
>           bestSize = charSize;
>           mChar->mGlyphTable = aGlyphTable;
>           mChar->mGlyph = ch;
>+          mGlyphFound = PR_TRUE;
>           mChar->mFamily = font.name;

Set mGlyphFound in only one place before the conditional blocks.

>   if (maxWidth)
>     return PR_FALSE; // Continue to check other sizes
> 
>   // reset
>   mChar->mGlyph = kNullGlyph; // this will tell paint to build by parts
>+  mGlyphFound = PR_TRUE;

For consistency of the maxWidth/normal-stretch cases, mGlyphFound should be set
before the "if (maxWidth)" early return, right?  (Because at that point we know
that the parts would have been used if it were a stretch.)

>-  // This will be updated if a better match than the base character is found
>-  mGlyph.font = -1;
>-

It looks like we need to set mDrawNormal, perhaps here, for the case when
StretchInternal returns early because the normal character is now appropriate
(but was not appropriate in a previous stretch).

>-  PRInt32 glue, bottom;
>+  PRInt32 glue = 0, bottom = 0;

>-  PRInt32 glue, right;
>+  PRInt32 glue = 0, right = 0;

I'd prefer not to initialize these to incorrect values just for the sake of
silencing a compiler that does understand the code, because it comes at the
cost of preventing valgrind from detecting if they don't get set
appropriately.
i mean "a compiler that does *not* understand the code".
Assignee

Comment 9

9 years ago
> 
> >   if (maxWidth)
> >     return PR_FALSE; // Continue to check other sizes
> > 
> >   // reset
> >   mChar->mGlyph = kNullGlyph; // this will tell paint to build by parts
> >+  mGlyphFound = PR_TRUE;
> 
> For consistency of the maxWidth/normal-stretch cases, mGlyphFound should be set
> before the "if (maxWidth)" early return, right?  (Because at that point we know
> that the parts would have been used if it were a stretch.)
> 

OK, that's not what was doing the old code (mGlyph.font is no longer -1 only after we set mGlyph = kNullGlyph) but maybe is was a mistake. However I'm not sure it changes anything to also move mChar->mGlyph = kNullGlyph before the "if (maxWidth)", now we rely on mGlyphFound?
Assignee

Comment 10

9 years ago
> OK, that's not what was doing the old code (mGlyph.font is no longer -1 only
> after we set mGlyph = kNullGlyph) but maybe is was a mistake.

Anyway, It was not the case either here:

>         if (maxWidth) {
>+          mGlyphFound = PR_TRUE;
>+

>         else {
>           mBoundingMetrics = bm;
>           haveBetter = PR_TRUE;
>           bestSize = charSize;
>           mChar->mGlyphTable = aGlyphTable;
>           mChar->mGlyph = ch;
>+          mGlyphFound = PR_TRUE;
>           mChar->mFamily = font.name;
(In reply to comment #9)
> OK, that's not what was doing the old code (mGlyph.font is no longer -1 only
> after we set mGlyph = kNullGlyph) but maybe is was a mistake.

Yes, being as consistent as possible between GetMaxWidth/Stretch is the goal.

> However I'm not
> sure it changes anything to also move mChar->mGlyph = kNullGlyph before the "if
> (maxWidth)", now we rely on mGlyphFound?

I'm not sure I completely understand your point here, so let me know if I'm not addressing your question.
I think mChar->mGlyph = kNullGlyph should stay where it is (because we don't want to mess up a previous stretch when we do a measure);
only the mGlyphFound = PR_TRUE should move.
Assignee

Comment 12

9 years ago
> I'm not sure I completely understand your point here, so let me know if I'm not
> addressing your question.
> I think mChar->mGlyph = kNullGlyph should stay where it is (because we don't
> want to mess up a previous stretch when we do a measure);
> only the mGlyphFound = PR_TRUE should move.

Yes, I meant that it is not necessary to move mChar->mGlyph = kNullGlyph and as you say it will even be incorrect. So I agree that only mGlyphFound = PR_TRUE should move.
Assignee

Comment 13

9 years ago
Posted patch Patch V2 (obsolete) — Splinter Review
Attachment #488692 - Attachment is obsolete: true
Attachment #489600 - Flags: review?(karlt)
Attachment #488692 - Flags: review?(karlt)
Comment on attachment 489600 [details] [diff] [review]
Patch V2

This looks very similar to the first patch.
Wrong patch or forgot to qref?
Attachment #489600 - Flags: review?(karlt)
Assignee

Comment 15

9 years ago
Posted patch Patch V2Splinter Review
oops, sorry...
Attachment #489600 - Attachment is obsolete: true
Attachment #489757 - Flags: review?(karlt)
Comment on attachment 489757 [details] [diff] [review]
Patch V2

No problem.  Looks good, thanks.
Attachment #489757 - Flags: review?(karlt) → review+
I assume layout/generic/crashtests/472587-1.xhtml doesn't show any visual differences due to the uninitialized variable, but I wonder whether some similar set-up perhaps with tables might do.

Frédéric, would you mind having a look to see if a reftest (perhaps a dynamic test similar to 472587-1.xhtml) could be written to test this, please?
Some effort should be made to write a test before requesting approval2.0, but, if you don't come up with anything, we'll just have to rely on valgrind.
Reporter

Comment 18

9 years ago
(In reply to comment #15)
> Created attachment 489757 [details] [diff] [review]
> Patch V2

I verified that this stops Valgrind complaining.
Assignee

Comment 19

9 years ago
(In reply to comment #17)
> I assume layout/generic/crashtests/472587-1.xhtml doesn't show any visual
> differences due to the uninitialized variable, but I wonder whether some
> similar set-up perhaps with tables might do.
> 
> Frédéric, would you mind having a look to see if a reftest (perhaps a dynamic
> test similar to 472587-1.xhtml) could be written to test this, please?
> Some effort should be made to write a test before requesting approval2.0, but,
> if you don't come up with anything, we'll just have to rely on valgrind.

Sorry for the delay, I've just seen your comment today (for some reason I did not receive bugspam for this). Unfortunately, I don't think I'll have time to contribute until next January so I fear it is too late for Gecko 2.0...
Assignee

Comment 20

9 years ago
I'm no longer able to get Valgrind's warning from the crashtest...
Assignee

Comment 21

8 years ago
> Frédéric, would you mind having a look to see if a reftest (perhaps a dynamic
> test similar to 472587-1.xhtml) could be written to test this, please?
> Some effort should be made to write a test before requesting approval2.0, but,
> if you don't come up with anything, we'll just have to rely on valgrind.

Should we add this bug to post2.0 now?
Depends on: post2.0
No, wasn't this patch.
Depends on: post2.0
Assignee

Comment 24

8 years ago
I think it's been a while since scale-stretchy-3 is failing (can't remember when it happened). The purpose of the test is to check that the width of the arrow does not exceed the one of the rectangle. However, the failure is because the blue arrow and red mspace are not positioned at the same height. This can be fixed by adding some depth to the first yellow mspace, so that the blue arrow will be slightly lower.
Traced the reftest failure to bug 534970 comment 62.
http://hg.mozilla.org/mozilla-central/rev/014fb62e0905
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
No longer depends on: post2.0
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
Blocks: 656873
You need to log in before you can comment on or make changes to this bug.