valgrind: nsMenuPopupFrame::AdjustPositionForAnchorAlign creates undefined nsRects

RESOLVED FIXED in mozilla1.9.3a3

Status

()

Core
XP Toolkit/Widgets: Menus
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jseward, Assigned: jseward)

Tracking

(Blocks: 1 bug, {valgrind})

Trunk
mozilla1.9.3a3
valgrind
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

8 years ago
There is a path through layout/xul/base/source/nsMenuPopupFrame.cpp
function nsMenuPopupFrame::AdjustPositionForAnchorAlign which causes
it to return an undefined nsPoint (stack-allocated garbage).  It's
actually very simple.  We have:

783:  nsPoint pnt;
784:  switch (popupAnchor) {
785:    case POPUPALIGNMENT_TOPLEFT:
786:      pnt = anchorRect.TopLeft();
787:      break;
788:    case POPUPALIGNMENT_TOPRIGHT:
789:      pnt = anchorRect.TopRight();
790:      break;
791:    case POPUPALIGNMENT_BOTTOMLEFT:
792:      pnt = anchorRect.BottomLeft();
793:      break;
794:    case POPUPALIGNMENT_BOTTOMRIGHT:
795:      pnt = anchorRect.BottomRight();
796:      break;
797:  }

It then goes on to possibly add/subtract various offsets to pnt,
and then returns it.  That's all well and good, but the problem
is the switch doesn't handle the case POPUPALIGNMENT_NONE, in
which case garbage will be returned.  Adding the case, and a
printf, shows this case does indeed occur.

The bogus nsPoint is returned to the caller, nsMenuPopupFrame::SetPopupPosition,
which eventually hands it onwards via a call to nsMenuPopupFrame::FlipOrResize,
and, I think, various other parts of the graphics system.  Valgrind
tracks it as causing an immense number of errors, 49762 errors from 497 contexts
(different stack tracebacks) in this test case.  These are shown in
the attached file.

The flaw is simple once you see it, but tracking it down from the
presented information proved very difficult.


HOW TO REPRO: run layout/base/tests/test_bug420499.xul
on Memcheck as described in bug 549224.
(Assignee)

Updated

8 years ago
Blocks: 549224
Keywords: valgrind
Component: Graphics → XP Toolkit/Widgets: Menus
QA Contact: thebes → xptoolkit.menus
(Assignee)

Comment 1

8 years ago
Created attachment 429889 [details]
list of resulting valgrind errors

Note that the origin of the undefined value is erroneously listed
as being from 
nsMenuPopupFrame::SetPopupPosition(nsIFrame*, int) (nsMenuPopupFrame.cpp:908)
whereas it is really from 
nsMenuPopupFrame::AdjustPositionForAnchorAlign(const nsRect& anchorRect,
                                               PRBool& aHFlip, PRBool& aVFlip)
as discussed above.  I think this happens because the latter got inlined
into the former.
(Assignee)

Comment 2

8 years ago
Created attachment 429890 [details] [diff] [review]
proposed fix

Proposed fix.  Obviously the minimal safe thing to do is give pnt
an initial value of (0,0) so its safe regardless of whatever else
happens.  I took the liberty of adding do-nothing cases for
POPUPALIGNMENT_NONE to show that they are deliberately no-ops
rather than were merely forgotten about.
You can request review from enndeakin.
(Assignee)

Updated

8 years ago
Attachment #429890 - Flags: review?(enndeakin)

Comment 4

8 years ago
Actually this is fixed by the patch in bug 524545, which has some problems with the test on Mac which I've never figured out. Two options here:

- check in the patch in bug 524545 with the test disabled on Mac
- check in just something like the patch in this bug, however POPUPALIGNMENT_NONE should behave the same as POPUPALIGNMENT_TOPLEFT. The patch in bug 524545 uses a switch default for this.
Two thumbs up for always including 'default' cases in switch statements!
(Assignee)

Comment 6

8 years ago
Created attachment 431498 [details] [diff] [review]
patch v2

patch revised as per comment 5
Attachment #429890 - Attachment is obsolete: true
Attachment #429890 - Flags: review?(enndeakin)
(Assignee)

Comment 7

8 years ago
So much for guessing comment numbers.  That should have been "patch
revised as per comment 4."
(Assignee)

Updated

8 years ago
Attachment #431498 - Flags: review?(enndeakin)

Comment 8

8 years ago
The intent was to just add a 'default' case, and rely on it for the none value. Really, just the first part of https://bug524545.bugzilla.mozilla.org/attachment.cgi?id=408843
(Assignee)

Comment 9

8 years ago
Created attachment 431710 [details] [diff] [review]
patch v3, in accordance with comment 8
Attachment #431498 - Attachment is obsolete: true
Attachment #431710 - Flags: review?(enndeakin)
Attachment #431498 - Flags: review?(enndeakin)

Updated

8 years ago
Attachment #431710 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed

Updated

8 years ago
Assignee: nobody → jseward
http://hg.mozilla.org/mozilla-central/rev/581260c2e0d7
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a3
You need to log in before you can comment on or make changes to this bug.